-
Notifications
You must be signed in to change notification settings - Fork 446
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature request: Don't suggest adding the user to docker group for security reasons in tutor dev
#875
Comments
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
Rootless Docker should be an option, but we cannot force it on anyone.
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 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:
If we did that, we would first have to solve #876. We would also have to check that |
Yes, for a start here's the warning: It's really bad. It makes To simplify the situation, imagine you use VSCode and you install plugins. Those plugins cannot control privileged Docker, and would need to run through After adding
With rootless Docker, it's not possible. With Privileged Docker, it needs sudo, which is acceptable. After adding the user to
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:
After putting ChatGPT and amending it again:
|
I agree with @OmarIthawi. Having your user in |
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:
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 |
You're right. This is still and issue in the upstream Tutor.
I'm not sure what
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 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 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 |
I must have had a brain freeze... Please see my edited comment. |
For dev installation Caddy isn't required. I think it might be productive to focus on Therefore, in the case of What do you think? |
We can't really do that. Consider the following scenario: someone runs 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:
I think that this proposition strikes the right balance. When a user runs Tutor with What do you think? |
Honestly, I wasn't aware of the consistency Tutor provides between different backends. This is good, and I support keeping it.
Great. Starting with documentation is a good step forward. It's better than forking Tutor to make for rootless tutor devdiff --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 The environment variable above, would make |
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. |
I've Googled this issue and I've found the following:
more verbose:
also
another way:
|
tutor dev
Just to catch up, is this waiting on something on Tutor's end @OmarIthawi ? |
Thanks for the reminder @DawoudSheraz, I think we can close this issue as "rejected" proposal. Docker rootless is already supported (#921) for those who want to avoid using sudo-less docker. |
Is your feature request related to a problem? Please describe.
Adding the Linux user e.g.
whoami > omar
to thedocker
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.Tutor gives this advice -- which I think it shouldn't:
tutor/tutor/commands/cli.py
Lines 115 to 119 in faf43bd
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: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
The text was updated successfully, but these errors were encountered: