overhangio / tutor

The Docker-based Open edX distribution designed for peace of mind
https://docs.tutor.overhang.io/
GNU Affero General Public License v3.0
916 stars 435 forks source link

Feature request: Don't suggest adding the user to docker group for security reasons in `tutor dev` #875

Closed OmarIthawi closed 9 months ago

OmarIthawi commented 1 year ago

Is your feature request related to a problem? Please describe. Adding the Linux user e.g. whoami > omar to the docker group is discouraged by docker these days -- for good reasons. It is the equivalent of providing passwordless sudo to all running applications and their plugins.

MacOS or even Linux Docker Desktop users don't have this issue because Docker runs in its own Virtual Machine, therefore there's no need to adding the user to the docker group.

The docker group grants root-level privileges to the user [and all running applications, which makes desktop users very vulnerable]. For details on how this impacts security in your system, see Docker Daemon Attack Surface. -- Docker docs in addition to my explanation between brackets.

Tutor gives this advice -- which I think it shouldn't:

https://github.com/overhangio/tutor/blob/faf43bd3b0adafc0f71b129fe71e82ab9c6a0e7d/tutor/commands/cli.py#L115-L119

Describe the solution you'd like

Use rootless docker. I recently started using rootless docker which is -- surprisingly -- compatible with the Open edX devstack. Last time I tested it with Tutor I had problems. I plan to test again in two weeks or so.

Describe alternatives you've considered Enforce sudo prefix to all docker calls.

I usually solve this by creating two scripts in the ~/bin directory, which is a non-standard solution that I came up with:

# ~/bin/docker
sudo /usr/bin/docker "$@"
# ~/bin/docker-compose
sudo /user/bin/docker-compose "$@"

Which works 99% of the cases, but sometimes fails if Docker-compose or devstack tries to do something "too smart"/complicated.

It's annoying since it requires entering a password for every new Terminal instance for even the simplest commands like docker ps.

Additional context

regisb commented 1 year ago

Adding the Linux user e.g. whoami > omar to the docker group is discouraged by docker these days

Is it? I found no clue in the Docker docs that this approach is now discouraged. AFAIK that link is still valid: https://docs.docker.com/engine/install/linux-postinstall/ And it is still present, for instance, at the bottom of the Ubuntu installation instructions: https://docs.docker.com/engine/install/ubuntu/#next-steps

Describe the solution you'd like

Use rootless docker.

Rootless Docker should be an option, but we cannot force it on anyone.

Tutor gives this advice -- which I think it shouldn't

I disagree. Running Tutor as root is most of the time a bad idea, and a very frequent source of error within the Open edX community. We could certainly disable that obnoxious warning when a TUTOR_RUN_AS_ROOT (or similarly named) environment variable is defined, such that users who know what they are doing can disable it.

But I assume you don't really want to get rid of that warning, right? You just want to phrase it differently? We could certainly add the following sentence (or something similar) to the warning:

Alternatively, you can run Docker in rootless mode: https://docs.docker.com/engine/security/rootless/

If we did that, we would first have to solve https://github.com/overhangio/tutor/issues/876. We would also have to check that ENABLE_WEB_PROXY is false and CADDY_HTTP_PORT >= 1024.

OmarIthawi commented 1 year ago

Is it?

Yes, for a start here's the warning:

image

It's really bad. It makes whoami > omar effectively a root, disabling sudo and making the system pretty much vulnerable because gaining docker access means that the user omar can mount anything and edit it e.g. /etc directory or anything else.

To simplify the situation, imagine you use VSCode and you install plugins. Those plugins cannot control privileged Docker, and would need to run through sudo. Those plugins cannot edit /etc/hosts either.

After adding omar to the docker group those plugins will gain control over privileged Docker and can run commands like the following:

$ docker run -v /my-host:/etc/hosts -i -t node:16 bash
docker: Error response from daemon: error while creating mount source path '/my-host': mkdir /my-host: permission denied.

With rootless Docker, it's not possible. With Privileged Docker, it needs sudo, which is acceptable. After adding the user to group it's dangerous and effectively sidesteps sudo.

I disagree. Running Tutor as root is most of the time a bad idea, and a very frequent source of error within the Open edX community. We could certainly disable that obnoxious warning when a TUTOR_RUN_AS_ROOT (or similarly named) environment variable is defined, such that users who know what they are doing can disable it.

Sure, I wasn't clear. I'm not recommending to run Tutor as root.

Let me provide the message I think it should be something like the following:

You are running Tutor as root. This is strongly not recommended.

Depending on your use case here's what we recommend:

 - If you're running Tutor in your computer for development in Linux, consider using Podman or Rootless Docker.
 - If you're running Tutor on Docker Desktop on MacOSX, Linux or Windows, run it as a non-root user. 
 - If you're running Tutor in a server, you can run it as root, consider adding your user to the 'docker' group. 
(see https://docs.docker.com
/install/linux/linux-postinstall/#manage-docker-as-a-non-root-user)

After putting ChatGPT and amending it again:

Warning: Running Tutor as root is highly discouraged. If you are doing this in order to access the Docker deamon there
are some alternative approaches based on your specific use case:

 1. For users running Tutor on Docker Desktop on MacOSX, Linux, or Windows the non-root user should be used.

 2. If you're using Linux for development on your local machine using Podman or Rootless Docker, which overcomes the need for root privileges or sudo. However, performance may vary.

 3. In Tutor is running on a server, add your user to the 'docker' group. See https://docs.docker.com/install/linux/linux-postinstall/#manage-docker-as-a-non-root-user for more information.
snglth commented 1 year ago

I agree with @OmarIthawi. Having your user in docker group is effectively the same as working from root. On desktop/development machine it's a major security concern, though it's convenient.

regisb commented 1 year ago

Sorry for the delay in answering.

I understand that running Docker raises security issues: this is a problem that has existed since Docker was created. I am all for providing alternate solutions to the user. The problem is that for the moment we cannot recommend rootless mode because of the following limitations:

  1. setting ulimits (#876)
  2. exposing Caddy ports 80/433

There might be other limitations in Tutor plugins that I'm not aware of. Codejail comes to mind but there might be others.

Currently, Tutor needs these sudo-like properties to work properly. I'm all for encouraging users to run tutor local in rootless mode, but to do that we must make sure that it does indeed work in rootless mode.

OmarIthawi commented 1 year ago

Sorry for the delay in answering.

I understand that running Docker raises security issues: this is a problem that has existed since Docker was created. I am all for providing alternate solutions to the user. The problem is that for the moment we cannot recommend rootless mode because of the following limitations:

  1. setting ulimits (Docker rootless: Error when setting ulimits on elasticsearch #876)

You're right. This is still and issue in the upstream Tutor.

  1. exposing

I'm not sure what exposing means.

There might be other limitations in Tutor plugins that I'm not aware of. Codejail comes to mind but there might be others.

Currently, Tutor needs these sudo-like properties to work properly. I'm all for encouraging users to run tutor local in rootless mode, but to do that we must make sure that it does indeed work in rootless mode.

So, I've been running Tutor in rootless mode for a month or two and it's been great. I've noticed that performance degrades a bit, which I've solved by getting faster CPU (a new laptop actually).

The ulimit for elasticsearch is commented out on my device and that's the only problem I've faced so far.

I'll assume that Codejail won't work securely in rootless mode, which seems like an acceptable trade-off to make -- but that's a topic for another repo.

I understand that running Docker raises security issues: this is a problem that has existed since Docker was created.

I understand that the topic is a bit niche, but Docker itself isn't the issue and Docker is still fairly a safe software to install. However, removing the need for sudo when controlling docker isn't secure. My understanding that there's no widely developed exploit so it shouldn't be a huge problem.

regisb commented 1 year ago

I'm not sure what exposing means.

I must have had a brain freeze... Please see my edited comment.

OmarIthawi commented 1 year ago

exposing Caddy ports 80/433

For dev installation Caddy isn't required. I think it might be productive to focus on tutor dev and exclude tutor local from this discussion. Even if we ended up supporting production tutor local in rootless mode, I think it's a very marginal use-case that may not practically matter.

Therefore, in the case of tutor dev Caddy isn't enabled so ports won't be an issue.

What do you think?

regisb commented 1 year ago

I think it might be productive to focus on tutor dev and exclude tutor local from this discussion.

We can't really do that. Consider the following scenario: someone runs tutor dev and we tell them to run rootless Docker. Then they decide to run tutor local to go to production (I do this all the time). We don't want to tell them at this point to change how they should run Docker.

People routinely run Tutor dev, local and k8s on the same machine. We need to give advice that is consistent for these 3 use cases. But at the same time we should try to keep the warning short and sweet.

With this in mind, here's what I propose:

  1. Add a "running with docker rootless" tutorial to the Tutor docs. This tutorial explains how to resolve the two issues we mentioned above. We can add as much information as we want there. This tutorial should probably replace the current podman tutorial.
  2. Change the wording of the warning: we still point to the Docker docs by default, but also encourage users to read the rootless mode tutorial.

I think that this proposition strikes the right balance. When a user runs Tutor with sudo it's sufficiently difficult to discourage them from running as root. It would be super difficult to tell them to change how they run Docker. And it would be awkward to tell them later (when they switch to tutor local) to do things differently.

What do you think?

OmarIthawi commented 1 year ago

Honestly, I wasn't aware of the consistency Tutor provides between different backends. This is good, and I support keeping it.

  1. Add a "running with docker rootless" tutorial to the Tutor docs. This tutorial explains how to resolve the two issues we mentioned above. We can add as much information as we want there. This tutorial should probably replace the current podman tutorial.
  2. Change the wording of the warning: we still point to the Docker docs by default, but also encourage users to read the rootless mode tutorial.

I think that this proposition strikes the right balance. When a user runs Tutor with sudo it's sufficiently difficult to discourage them from running as root. It would be super difficult to tell them to change how they run Docker. And it would be awkward to tell them later (when they switch to tutor local) to do things differently.

What do you think?

Great. Starting with documentation is a good step forward. It's better than forking Tutor to make tutor dev run in rootless Docker.

for rootless tutor dev ```diff diff --git a/tutor/templates/config/defaults.yml b/tutor/templates/config/defaults.yml index 29996e4..e2778bf 100644 --- a/tutor/templates/config/defaults.yml +++ b/tutor/templates/config/defaults.yml @@ -25,6 +25,7 @@ ELASTICSEARCH_HOST: "elasticsearch" ELASTICSEARCH_PORT: 9200 ELASTICSEARCH_SCHEME: "http" ELASTICSEARCH_HEAP_SIZE: 1g +ELASTICSEARCH_SET_ULIMITS: true ENABLE_HTTPS: false ENABLE_WEB_PROXY: true JWT_COMMON_AUDIENCE: "openedx" diff --git a/tutor/templates/local/docker-compose.yml b/tutor/templates/local/docker-compose.yml index 2dc6b72..0a017a7 100644 --- a/tutor/templates/local/docker-compose.yml +++ b/tutor/templates/local/docker-compose.yml @@ -57,10 +57,12 @@ services: - bootstrap.memory_lock=true - discovery.type=single-node - "ES_JAVA_OPTS=-Xms{{ ELASTICSEARCH_HEAP_SIZE }} -Xmx{{ ELASTICSEARCH_HEAP_SIZE }}" + {% if ELASTICSEARCH_SET_ULIMITS -%} ulimits: memlock: soft: -1 hard: -1 + {%- endif %} restart: unless-stopped user: "1000:1000" volumes: ```

There are many ways to fix the issue, but I haven't found one that doesn't impact tutor local config in which ulimits stays with the generated configurations but is disabled in rootless mode.

The environment variable above, would make ulimits removed regardless of the Tutor backend.

regisb commented 1 year ago

The environment variable above, would make ulimits removed regardless of the Tutor backend.

I'd rather avoid adding new configuration settings unless they are strictly necessary. Is there any way to automatically detect whether we are running rootless Docker? Checking the user groups might be one option, but I don't know if it's very consistent.

OmarIthawi commented 1 year ago

I've Googled this issue and I've found the following:

docker info 2>/dev/null | grep -C5 rootless 
 runc version: 
 init version: 
 Security Options:
  seccomp
   Profile: default
  rootless   # [Omar] This flag shows up on my machine
  cgroupns
 Kernel Version: 5.19.0-46-generic
 Operating System: Ubuntu 22.04.2 LTS
 OSType: linux
 Architecture: x86_64

more verbose:

$ docker info | grep -B3 -i rootless
WARNING: No cpu cfs quota support
WARNING: No cpu cfs period support
WARNING: No cpu shares support
WARNING: No cpuset support
WARNING: No io.weight support
WARNING: No io.weight (per device) support
WARNING: No io.max (rbps) support
WARNING: No io.max (wbps) support
WARNING: No io.max (riops) support
WARNING: No io.max (wiops) support
WARNING: bridge-nf-call-iptables is disabled
WARNING: bridge-nf-call-ip6tables is disabled
 Security Options:
  seccomp
   Profile: default
  rootless

also

 ps aux | grep docker | grep rootless
omar        3086  0.0  0.0 1977020 8408 ?        Ssl  Sep16   0:05 rootlesskit --net=slirp4netns --mtu=65520 --slirp4netns-sandbox=auto --slirp4netns-seccomp=auto --disable-host-loopback --port-driver=builtin --copy-up=/etc --copy-up=/run --propagation=rslave /usr/bin/dockerd-rootless.sh
omar        3145  0.0  0.0 1384476 5120 ?        Sl   Sep16   0:01 /proc/self/exe --net=slirp4netns --mtu=65520 --slirp4netns-sandbox=auto --slirp4netns-seccomp=auto --disable-host-loopback --port-driver=builtin --copy-up=/etc --copy-up=/run --propagation=rslave /usr/bin/dockerd-rootless.sh
omar     1009126  0.0  0.0 1159560 6560 ?        Sl   09:28   0:00 /usr/bin/rootlesskit-docker-proxy -proto tcp -host-ip 127.0.0.1 -host-port 1997 -container-ip 172.23.0.3 -container-port 1997
omar     1009284  0.0  0.0 1159560 6240 ?        Sl   09:28   0:00 /usr/bin/rootlesskit-docker-proxy -proto tcp -host-ip 127.0.0.1 -host-port 2000 -container-ip 172.23.0.5 -container-port 2000
omar     1010573  0.0  0.0 1233548 6720 ?        Sl   09:28   0:00 /usr/bin/rootlesskit-docker-proxy -proto tcp -host-ip 127.0.0.1 -host-port 1995 -container-ip 172.23.0.7 -container-port 1995
omar     1011224  0.0  0.0 1159816 6560 ?        Sl   09:28   0:00 /usr/bin/rootlesskit-docker-proxy -proto tcp -host-ip 0.0.0.0 -host-port 8000 -container-ip 172.23.0.11 -container-port 8000
omar     1011240  0.0  0.0 1233548 6880 ?        Sl   09:28   0:00 /usr/bin/rootlesskit-docker-proxy -proto tcp -host-ip :: -host-port 8000 -container-ip 172.23.0.11 -container-port 8000
omar     1011472  0.0  0.0 1159816 6400 ?        Sl   09:28   0:00 /usr/bin/rootlesskit-docker-proxy -proto tcp -host-ip 0.0.0.0 -host-port 8001 -container-ip 172.23.0.13 -container-port 8000
omar     1011485  0.0  0.0 1159816 6720 ?        Sl   09:28   0:00 /usr/bin/rootlesskit-docker-proxy -proto tcp -host-ip :: -host-port 8001 -container-ip 172.23.0.13 -container-port 8000

another way:

$ docker context ls
NAME            DESCRIPTION                               DOCKER ENDPOINT                                 ERROR
default *       Current DOCKER_HOST based configuration   unix:///run/user/1000/docker.sock               
desktop-linux                                             unix:///home/omar/.docker/desktop/docker.sock   
rootless        Rootless mode                             unix:///run/user/1000/docker.sock               
Warning: DOCKER_HOST environment variable overrides the active context. To use a context, either set the global --context flag, or unset DOCKER_HOST environment variable.
DawoudSheraz commented 9 months ago

Just to catch up, is this waiting on something on Tutor's end @OmarIthawi ?

OmarIthawi commented 9 months ago

Thanks for the reminder @DawoudSheraz, I think we can close this issue as "rejected" proposal.

Docker rootless is already supported (https://github.com/overhangio/tutor/pull/921) for those who want to avoid using sudo-less docker.