mviereck / x11docker

Run GUI applications and desktops in docker and podman containers. Focus on security.
MIT License
5.62k stars 378 forks source link

Opening confirmation window for docker pull fails #109

Closed eine closed 5 years ago

eine commented 5 years ago

While trying to use an image (which is not available locally) in interactive mode, opening a new window to ask for confirmation to docker pull fails. However, the image is downloaded and both the X server and the container are successfully created.

# ./x11docker -i ghdl/ext:vunit-gtkwave bash
x11docker note: Using X server option --vcxsrv

x11docker note: Per default x11docker stores its cache files on drive C:.
  docker setup may not allow to share files from drive C:.
  If startup fails with an 'access denied' error,
  please either allow access to drive C: or specify a custom folder for cache
  storage with option '--cachebasedir D:/some/cache/folder'.
  Same issue can occur with option '--home'.
  Use option '--homebasedir D:/some/home/folder' in that case.

x11docker note: Did not find container init system 'tini'.
  This is a bug in your distributions docker package.
  Normally, docker provides init system tini as '/usr/bin/docker-init'.

  x11docker uses tini for clean process handling and fast container shutdown.
  To provide tini yourself, please download tini-static:
    https://github.com/krallin/tini/releases/download/v0.18.0/tini-static
  Store it in one of:
    /home/eine/.local/share/x11docker/
    /usr/local/share/x11docker/

USERDOMAIN=DESKTOP-E1ER43H
OS=Windows_NT
COMMONPROGRAMFILES=C:\Program Files\Common Files
PROCESSOR_LEVEL=6
PSModulePath=C:\Program Files\WindowsPowerShell\Modules;C:\WINDOWS\system32\WindowsPowerShell\v1.0\Modules
CommonProgramW6432=C:\Program Files\Common Files
CommonProgramFiles(x86)=C:\Program Files (x86)\Common Files
_=/usr/bin/env
MSYSCON=mintty.exe
LANG=en_GB.UTF-8
TZ=Europe/Madrid
MSYSTEM_CARCH=x86_64
DISPLAY=
HOSTNAME=DESKTOP-E1ER43H
PUBLIC=C:\Users\Public
OLDPWD=/home/eine
CONFIG_SITE=/mingw64/etc/config.site
WD=C:\msys64\usr\bin\
CONTITLE=MinGW x64
MSYSTEM_CHOST=x86_64-w64-mingw32
LOGINSHELL=bash
USERNAME=eine
MOZ_PLUGIN_PATH=C:\Program Files (x86)\Foxit Software\Foxit Reader\plugins\
LOGONSERVER=\\DESKTOP-E1ER43H
PROCESSOR_ARCHITECTURE=AMD64
tmp=C:\Users\eine\AppData\Local\Temp
Sharefolder=/c/Users/eine/x11docker/cache/ghdl-ext-6c6aaa/share
LOCALAPPDATA=C:\Users\eine\AppData\Local
COMPUTERNAME=DESKTOP-E1ER43H
FPS_BROWSER_APP_PROFILE_STRING=Internet Explorer
USER=eine
!::=::\
SYSTEMDRIVE=C:
USERPROFILE=C:\Users\eine
PATHEXT=.COM;.EXE;.BAT;.CMD;.VBS;.VBE;.JS;.JSE;.WSF;.WSH;.MSC
SYSTEMROOT=C:\WINDOWS
USERDOMAIN_ROAMINGPROFILE=DESKTOP-E1ER43H
PROCESSOR_IDENTIFIER=Intel64 Family 6 Model 94 Stepping 3, GenuineIntel
MINGW_PACKAGE_PREFIX=mingw-w64-x86_64
PWD=/d/data-dev/github/x11docker
HOME=/home/eine
GOROOT=C:\Go\
TMP=/tmp
MSYSTEM_PREFIX=/mingw64
OneDrive=C:\Users\eine\OneDrive
IFS=

!C:=C:\msys64
PROCESSOR_REVISION=5e03
KISYSMOD=C:\Program Files\KiCad\share\kicad\modules
FPS_BROWSER_USER_PROFILE_STRING=Default
KICAD_PTEMPLATES=C:\Program Files\KiCad\share\kicad\template
PROMPT=$P$G
NUMBER_OF_PROCESSORS=4
ProgramW6432=C:\Program Files
COMSPEC=C:\WINDOWS\system32\cmd.exe
APPDATA=C:\Users\eine\AppData\Roaming
SHELL=/bin/bash
TERM=xterm
WINDIR=C:\WINDOWS
MINGW_CHOST=x86_64-w64-mingw32
ProgramData=C:\ProgramData
SHLVL=7
PRINTER=Foxit Reader PDF Printer
ACLOCAL_PATH=/mingw64/share/aclocal:/usr/share/aclocal
PROGRAMFILES=C:\Program Files
MANPATH=/mingw64/local/man:/mingw64/share/man:/usr/local/man:/usr/share/man:/usr/man:/share/man
ORIGINAL_TEMP=/c/Users/eine/AppData/Local/Temp
ORIGINAL_TMP=/c/Users/eine/AppData/Local/Temp
ALLUSERSPROFILE=C:\ProgramData
TEMP=/tmp
temp=C:\Users\eine\AppData\Local\Temp
DriverData=C:\Windows\System32\Drivers\DriverData
MSYSTEM=MINGW64
Cachefolder=/c/Users/eine/x11docker/cache/ghdl-ext-6c6aaa
MINGW_PREFIX=/mingw64
XDG_RUNTIME_DIR=
SESSIONNAME=Console
ProgramFiles(x86)=C:\Program Files (x86)
PATH=/c/Program Files/docker:/c/Program Files/Docker/Docker/resources/bin:/mingw64/bin:/usr/local/bin:/usr/bin:/bin:/c/Windows/System32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0/:/usr/bin/site_perl:/usr/bin/vendor_perl:/usr/bin/core_perl:/c/Program Files/Docker/Docker/resources/bin:/usr/games:/usr/local/bin:/usr/sbin:/sbin:/c/Program Files/docker:/c/Program Files/Docker/Docker/resources/bin:/c/Program Files/VcXsrv
HOMEDRIVE=C:
PKG_CONFIG_PATH=/mingw64/lib/pkgconfig:/mingw64/share/pkgconfig
INFOPATH=/usr/local/info:/usr/share/info:/usr/info:/share/info
HOMEPATH=\Users\eine
ORIGINAL_PATH=/c/Windows/System32:/c/Windows:/c/Windows/System32/Wbem:/c/Windows/System32/WindowsPowerShell/v1.0/
BASH_FUNC_rmcr%%=() {  case "${1:-}" in
 "")
 sed "s/$(printf "\r")//g"
 ;;
 *)
 sed -i "s/$(printf "\r")//g" "${1:-}"
 ;;
 esac
}
x11docker WARNING: Failed to wait for file creation of
  /c/Users/eine/x11docker/cache/ghdl-ext-6c6aaa/pullterminalready

x11docker note: Interactive terminal to ask for and show "docker pull"
  has failed. Will run "docker pull ghdl/ext:vunit-gtkwave" nonetheless.

vunit-gtkwave: Pulling from ghdl/ext
5e6ec7f28fb7: Pulling fs layer
93bed770c432: Pulling fs layer
05cc6acbde1c: Pulling fs layer
2e7e7b22736c: Pulling fs layer
e13141912ca4: Pulling fs layer
28e44ac7f01f: Pulling fs layer
d5e180a1e841: Pulling fs layer
5ce338eecd41: Pulling fs layer
9f8fbcd646e2: Pulling fs layer
ca4888f9a26e: Pulling fs layer
e13141912ca4: Waiting
28e44ac7f01f: Waiting
d5e180a1e841: Waiting
ca4888f9a26e: Waiting
5ce338eecd41: Waiting
9f8fbcd646e2: Waiting
2e7e7b22736c: Waiting
93bed770c432: Verifying Checksum
93bed770c432: Download complete
5e6ec7f28fb7: Verifying Checksum
5e6ec7f28fb7: Download complete
05cc6acbde1c: Verifying Checksum
05cc6acbde1c: Download complete
2e7e7b22736c: Verifying Checksum
2e7e7b22736c: Download complete
5e6ec7f28fb7: Pull complete
93bed770c432: Pull complete
e13141912ca4: Verifying Checksum
e13141912ca4: Download complete
05cc6acbde1c: Pull complete
2e7e7b22736c: Pull complete
e13141912ca4: Pull complete
28e44ac7f01f: Verifying Checksum
28e44ac7f01f: Download complete
28e44ac7f01f: Pull complete
5ce338eecd41: Verifying Checksum
5ce338eecd41: Download complete
9f8fbcd646e2: Verifying Checksum
9f8fbcd646e2: Download complete
d5e180a1e841: Verifying Checksum
d5e180a1e841: Download complete
d5e180a1e841: Pull complete
5ce338eecd41: Pull complete
9f8fbcd646e2: Pull complete
ca4888f9a26e: Verifying Checksum
ca4888f9a26e: Download complete
ca4888f9a26e: Pull complete
Digest: sha256:2c7de9505c43b64a20b45b49c8a88d6a19c96a45a7628244a9ceb8266bc45409
Status: Downloaded newer image for ghdl/ext:vunit-gtkwave
eine@443035febc82:~$
mviereck commented 5 years ago

Thanks for reporting!

I've stumbled over this, too. It also happens on some other circumstances. Also, interactively asking sometimes failed with some terminals in different setups for different reasons. Unfortunately there is no universal way to prompt in a GUI as no GUI tool is available everywhere. This issue is in a terminal and could be fixed. I would always ask without a GUI, but I also cannot assume that x11docker is started in a terminal, but e.g. with x11docker-gui.

So I regularly have a hassle with this. Yesterday I decided to drop the question [Y/n] and just pull always without asking for it. See latest master version from yesterday.

I am not entirely sure if this is a good decision. I also think of dropping docker pull and asking the user to run it himself. What do you think?

Edit: I also consider to drop opening a terminal window to ask for sudo/root password if it is required to run docker. Instead x11docker would show an error and ask to run it as root. In that case a terminal emulator would only be used as a fallback GUI for error messages and would not have any workflow use anymore.

eine commented 5 years ago

Frankly, I did never like popups for this kind of interaction. If I run a script in a shell, I expect all interactions with it to be bound to the window. I did not bring this previously because it was a design decission of yours. However, since you brought it... I agree on failing if user interaction is required outside of the window where x11docker is executed.

Unfortunately there is no universal way to prompt in a GUI as no GUI tool is available everywhere. This issue is in a terminal and could be fixed. I would always ask without a GUI, but I also cannot assume that x11docker is started in a terminal, but e.g. with x11docker-gui.

Although you cannot assume it, can you tell when is x11docker started in a terminal? Is there any other context, apart from x11docker-gui and Applications Menu shortcuts, where x11docker is not started in a shell? I am thinking about adding some envvar or CLI option to explicitly tell when x11docker is started from a GUI.

I am not entirely sure if this is a good decision. I also think of dropping docker pull and asking the user to run it himself. What do you think?

I prefer not to execute actions that could potentially have a relevant impact in the host, without explicit permission from the user. Although pulling a docker image can seem harmless, there are some contexts where it should be avoided. E.g., if the image name was not spelled correctly but the wrongly written image exists, x11docker will pull an image that is not required. Time and bandwidth will be used, and it will fail later. Another example: an image which is larger than the available space on disk is pulled.

I would suggest adding option -y, which can force the response to be yes or true when interaction is required:

If you cannot tell when x11docker is started from a GUI, the timeout can be always applied. I.e., the user is given 30s, 1min, 5min to reply to the questions. Otherwise, it will fail.

I also consider to drop opening a terminal window to ask for sudo/root password if it is required to run docker. Instead x11docker would show an error and ask to run it as root.

I think it is important to be consistent. If docker pull is executed automatically when needed (without explicit user confirmation), then sudo/root should also be used automatically. Conversely, if sudo cannot be used without explicit user interaction, then none of them should be executed automatically.

BTW, what do you need sudo for? Is it just to execute docker in GNU/Linux if the user is not in group docker? If this is the only use-case, I would add CLI option --sudo instead. Then, the error message would explain three options to the user:

The difference between the two later options is that x11docker --sudo does allow x11docker to use sudo only when calling docker, while sudo x11docker would allow x11docker to execute arbitrary commands as admin. The difference is subtle, but I think that a well-written app should use sudo only when required.

mviereck commented 5 years ago

Thanks for your feedback!

Although you cannot assume it, can you tell when is x11docker started in a terminal?

It seems that a check with tty -s works well. It succeeds in a terminal and fails if started with launchers or x11docker-gui. I hope it works in mintty/MSYS2, too.

I prefer not to execute actions that could potentially have a relevant impact in the host, without explicit permission from the user.

I agree.

If x11docker is started from a terminal, the user can answer the questions interactively.

The only question is to pull or not. I've included an interactive question in terminal if x11docker runs in a terminal. It timeouts after 60 seconds and terminates with an error instead of pulling the image. This way a pull within x11docker is possible, but the question does not block scripts that might use x11docker without a user looking at it. Your timeout suggestion is useful. If x11docker does not run in a terminal, it just fails with an error message.

BTW, what do you need sudo for? Is it just to execute docker in GNU/Linux if the user is not in group docker?

Yes. Although it is widely used, membership in group docker is a bad idea from a security point of view. Unprivileged processes can abuse it to get root privileges on host. For that reason I want to allow password prompts. x11docker has an option --pw to choose between su, sudo, pkexec, gksu and others. x11docker checks if there is a possibility to run docker without a password prompt (with or without sudo). If not, it prompts for su or sudo password (or regards --pw). Now it asks in terminal if possible, otherwise it tries to run an X terminal. (Formerly it always tried to run an X terminal).

If x11docker is started from a terminal with option -y, all questions are accepted. This is the current behaviour for pull.

I consider to add an option --pull to allow a pull. --pull or --pull=yes allows to pull if the image is missing. --pull=always would always run docker pull. Docker only does a download if a newer image is available. This way an automatic update would be possible.

eine commented 5 years ago

Thanks for your feedback!

You are welcome!

It seems that a check with tty -s works well. It succeeds in a terminal and fails if started with launchers or x11docker-gui. I hope it works in mintty/MSYS2, too.

Great! Let me know when I can test it.

The only question is to pull or not. I've included an interactive question in terminal if x11docker runs in a terminal. It timeouts after 60 seconds and terminates with an error instead of pulling the image. This way a pull within x11docker is possible, but the question does not block scripts that might use x11docker without a user looking at it. Your timeout suggestion is useful. If x11docker does not run in a terminal, it just fails with an error message.

That's great.

Yes. Although it is widely used, membership in group docker is a bad idea from a security point of view. Unprivileged processes can abuse it to get root privileges on host.

Thanks for the remainder. Is there any note suggesting this when the user is a member of group docker?

For that reason I want to allow password prompts. x11docker has an option --pw to choose between su, sudo, pkexec, gksu and others. x11docker checks if there is a possibility to run docker without a password prompt (with or without sudo). If not, it prompts for su or sudo password (or regards --pw). Now it asks in terminal if possible, otherwise it tries to run an X terminal. (Formerly it always tried to run an X terminal).

Great too! Does it make sense to prompt for password only if --pw is set? So, if a user requires sudo, check if --pw is set. If true, proceed with the interaction. If false, show a note/error explaining that --pw is required.

I consider to add an option --pull to allow a pull. --pull or --pull=yes allows to pull if the image is missing. --pull=always would always run docker pull. Docker only does a download if a newer image is available. This way an automatic update would be possible.

Great idea!

mviereck commented 5 years ago

Great! Let me know when I can test it.

The update is already in master. Before updating you can just check tty -s && echo ok.

Is there any note suggesting this when the user is a member of group docker?

Currently not. But I might include it as x11docker targets to be a sandbox solution. That being said, it should also show a message on custom DOCKER_RUN_OPTIONS that they are not checked at all.

Does it make sense to prompt for password only if --pw is set?

I don't think so. Users not being member of group docker should not be forced to always type --pw=sudo or --pw=su.

eine commented 5 years ago

The update is already in master. Before updating you can just check tty -s && echo ok.

# tty -s && echo ok
ok

Currently not. But I might include it as x11docker targets to be a sandbox solution. That being said, it should also show a message on custom DOCKER_RUN_OPTIONS that they are not checked at all.

Agree.

I don't think so. Users not being member of group docker should not be forced to always type --pw=sudo or --pw=su.

I forgot that x11docker can start non-docker apps too.

eine commented 5 years ago

I pulled the master branch, I removed x11docker/xfce and I executed:

# ./x11docker -i -t --user=RETAIN --cap-default           -- -p=8080:8085 -e BROADWAY_DISPLAY=:5 -e GDK_BACKEND=broadway  --           x11docker/xfce "broadwayd :5 & sleep 2 && xfce4-terminal"
x11docker WARNING: Command 'xauth' not found.
  Please install 'xauth' to allow X cookie authentication.
  Fallback: Disabling X authentication protocol. (option --no-auth)

x11docker note: Per default x11docker stores its cache files on drive C:.
  docker setup may not allow to share files from drive C:.
  If startup fails with an 'access denied' error,
  please either allow access to drive C: or specify a custom folder for cache
  storage with option '--cachebasedir D:/some/cache/folder'.
  Same issue can occur with option '--home'.
  Use option '--homebasedir D:/some/home/folder' in that case.

x11docker note: Windows firewall settings can forbid application access
  to the X server. If no application window appears, but no obvious error
  is shown, please check your firewall settings. Compare issue #108 on github.

x11docker WARNING: Option --cap-default disables security hardening
  for containers. Granting docker's default capabilities is considered insecure.

x11docker WARNING: Option --no-auth: SECURITY RISK!
  Allowing access to X server for everyone.

x11docker note: Did not find container init system 'tini'.
  This is a bug in your distributions docker package.
  Normally, docker provides init system tini as '/usr/bin/docker-init'.

  x11docker uses tini for clean process handling and fast container shutdown.
  To provide tini yourself, please download tini-static:
    https://github.com/krallin/tini/releases/download/v0.18.0/tini-static
  Store it in one of:
    /home/eine/.local/share/x11docker/
    /usr/local/share/x11docker/

Image 'x11docker/xfce' not found locally.
Do you want to pull it from docker hub? [Y|n]
(Will wait up to 60s for a response, otherwise assuming no)y
Using default tag: latest
latest: Pulling from x11docker/xfce
Digest: sha256:a0e5226c367c2f92e76822bdf237adcc1c1ba8a30a89b67801e28f007b78be11
Status: Downloaded newer image for x11docker/xfce:latest
mkdir: missing operand
Try 'mkdir --help' for more information.
Unable to init server: Could not connect: Connection refused

(xfce4-terminal:83): Gtk-WARNING **: 03:43:51.372: cannot open display:
Listening on /tmp/XDG_RUNTIME_DIR/broadway6.socket
x11docker note: User in container: uid=0(root) gid=0(root) groups=0(root)
root:x:0:0:root:/root:/bin/bash
Failed to connect to session manager: Failed to connect to the session manager: SESSION_MANAGER environment variable not defined

It works as expected, but note mkdir: missing operand.

mviereck commented 5 years ago

It works as expected, but note mkdir: missing operand.

Thanks, is fixed now. The fix caused some changes in USER and HOME settings in container. I hope I did not break anything.


I have another design question: Currently containers are allowed to access the internet per default. It can be forbidden with --no-internet. I consider to change the default for next major release 6.0. Internet access would be forbidden as default and could be enabled with -I, --internet. I assume this will be an unpopular default. But it would fit the sandbox target. What do you think?

eine commented 5 years ago

Thanks, is fixed now. The fix caused some changes in USER and HOME settings in container. I hope I did not break anything.

I tested it. It works as expected.

Currently containers are allowed to access the internet per default. It can be forbidden with --no-internet. I consider to change the default for next major release 6.0. Internet access would be forbidden as default and could be enabled with -I, --internet. I assume this will be an unpopular default. But it would fit the sandbox target. What do you think?

I agree with both points: it will be an unpopular default and it fits the sandbox target. In order to work around the popularity issue, and related to this comment about --user=RETAIN, I think it would be sensible to add a -D, --DOCKER-DEFAULTS option. This option would be just an alias of --user=RETAIN --cap-default -I, plus any other option that fits the purpose. This would fit three targets:

Overall, I think that there is very interesting know-how in this project, and it might be more popular if the entry barrier could be slightly reduced for active docker users.

eine commented 5 years ago

I removed x11docker/xfce to try the pull feature again, and I got this output:

# ./x11docker -i -t --user=0 -- -p=8080:8085 -e BROADWAY_DISPLAY=:5 -e GDK_BACKEND=broadway  -- x11docker/xfce "broadwayd :5 & sleep 2 && xfce4-terminal"
...
Image 'x11docker/xfce' not found locally.
Do you want to pull it from docker hub? [Y|n]
(Will wait up to 60s for a response, otherwise assuming no)y
/c/Users/eine/x11docker/cache/x11docker-xfce-34ddc4/dockerrc: line 123: notify-send: command not found
Using default tag: latest
x11docker note: Pulling image x11docker/xfce from docker hub
...

Everything works ok. It's just that message about notify-send.

eine commented 5 years ago

The error in the comment above is fixed in https://github.com/mviereck/x11docker/commit/75e3c56405f587094d8cb732f5bdbf4864c8deef.

@mviereck, should we close this issue? The issues are fixed. The only remaining content is the discussion about -I and -D.

mviereck commented 5 years ago

Yes, let's close this ticket. We can open a new one called "Enhancement and design discussion". #113