benrubson commented on a change in pull request #469: GUACAMOLE-890: Security: Allow image to run as non-root user URL: https://github.com/apache/guacamole-client/pull/469#discussion_r392683568
########## File path: guacamole-docker/bin/start.sh ########## @@ -30,7 +30,7 @@ GUACAMOLE_HOME_TEMPLATE="$GUACAMOLE_HOME" -GUACAMOLE_HOME="$HOME/.guacamole" +GUACAMOLE_HOME="/tmp/guacamole" Review comment: > Guacamole can't be the only piece of software fighting this issue. Is what you've done here a commonly-implemented solution among other Tomcat + Docker web applications that run as non-root users? It is what Tomcat documentation recommends, it's the goal of `CATALINA_BASE` env var we then use in this PR. IMO, what is done in the Tomcat Docker image, 777-chmoding application / configuration / work / tmp... default directories is not the best way to go. Do you, on a standard installation, as non-root user, have write access to these directories ? No :-) However you're free to set your own `CATALINA_BASE` directory, in your home directory for example, and start a new Tomcat instance over it. As we do here in this PR. > Configuration directories should be in configuration directory locations - not in any place that happens to work because we have write access to it :-) We could `chmod 777 /home` at image build time (in `Dockerfile`), and then use `GUACAMOLE_HOME="/home/guacamole"` instead of `GUACAMOLE_HOME="/tmp/guacamole"`, but the whole idea would remain the same. Keep in mind that whatever is done when Docker container starts will be lost when it will stop, and will have to be done again (this is what `start.sh` does). So, configuration which is done when container starts, will be lost when it will stop, wherever it stands, in `/tmp`, `/etc`, `/home` etc... I've just verified, there's nothing in `/home` in the Tomcat Docker image. We could then safely `chmod 777` it and move `GUACAMOLE_HOME` into it, if you're more confortable with this. We could also come back to the original `GUACAMOLE_HOME="$HOME/.guacamole"`, then getting rid of hard-coded `/tmp`. Everything will run as today, from inside `/root` for those running the container as `root`. The others, switching to a different user, will have to set the `$HOME` environment variable to a writable directory. Could suit everyone's needs. I think this PR is the proper way to go to address this (non-)root security issue. Of course I run it since the beginning without any issue. Thank you again :+1: ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
