Skip to content
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

sudo in Dockerfile? #65

Open
nhooyr opened this Issue Mar 7, 2019 · 21 comments

Comments

Projects
None yet
9 participants
@nhooyr
Copy link
Collaborator

nhooyr commented Mar 7, 2019

@incizzle

This comment has been minimized.

Copy link

incizzle commented Mar 7, 2019

I agree with @sr229 the container should not be launching commands as sudo all the time.

@nhooyr

This comment has been minimized.

Copy link
Collaborator Author

nhooyr commented Mar 7, 2019

@incizzle appreciate the input but could you elaborate on your reasoning why?

@incizzle

This comment has been minimized.

Copy link

incizzle commented Mar 7, 2019

I think @sr229 summed it up pretty well. I personally don't see running every command as sudo in this environment as a bad thing, but it doesn't instill good security practices & habits when this is transferred to other places.

@sr229

This comment has been minimized.

Copy link

sr229 commented Mar 7, 2019

This is how I see we're teaching people bad practices. They don't know linux, and they're learning via guides. If they decide to use Linux outside of Coder they won't know the concept behind sudo and permissions. We need to let them know "hey, you might need to run this specific command first before doing that", and this adds a certain familiarity of how the concept of permissions work.

We in turn, teach them how security usually works in Linux for beginners, and we give a sense of familiarity for other users. You don't see GCP giving you away a VM with a root account because you're expected to know about sudo and the concept of Linux security by permissions.

@frol

This comment has been minimized.

Copy link

frol commented Mar 7, 2019

Docker best-practices (“USER” section) describes how to do this.

As a side effect, running code-server as root will result in that the newly created files become owned by root, so soon enough user will end up with a project that cannot be modified from their regular user. To solve the permission issues, it is recommended to create a regular user and remap host UID & GID

@sr229

This comment has been minimized.

Copy link

sr229 commented Mar 7, 2019

I'm willing to reimplement the Dockerfile in the repo by basing off from my already existing image, should I get enough consensus that we should do usermode in this Dockerfile.

@NGTmeaty

This comment has been minimized.

Copy link

NGTmeaty commented Mar 7, 2019

I think this ^^ is the best idea at the moment. May want to wait for a code owner to give their opinions.

@PhilsLab

This comment has been minimized.

Copy link
Contributor

PhilsLab commented Mar 7, 2019

The design issue here is, that you should not make any modifications to the docker container after it started up.
Being able to install some dev tools etc. in the VS Code terminal while using it is very bad practice, and as mentioned in #86, there should be a seperate container for each terminal, which has the project directory mounted into it, and also the container image of that individually spawned container should have the required build/dev tools for each language or project type.

That said, adding a USER statement to the dockerfile would break the possibility to use the terminal for anything like package installation.

@sr229

This comment has been minimized.

Copy link

sr229 commented Mar 7, 2019

@PhilsLab I don't think it should be as locked down as you need to on #86. However, I did provide a variant that does exactly that which works for OpenShift Online, but its pain to compensate for common use cases.

@incizzle

This comment has been minimized.

Copy link

incizzle commented Mar 7, 2019

@PhilsLab, In order to have a separate container for each dev environment, it would require a lot of recoding of the actual VS code online program. It sounds likes its gonna be very hard to implement but it is a cool idea.

@sr229

This comment has been minimized.

Copy link

sr229 commented Mar 7, 2019

Theia did support this kind of work (Remember Eclipse Che 7?), however, this would require substantial backend re-engineering which would increase the amount of difficulty per new VSCode release since it might not be interopable with each other

@kylecarbs

This comment has been minimized.

Copy link
Member

kylecarbs commented Mar 7, 2019

You could just mount the projects and data-dir into your docker container on each start. Then you start where you were in a completely new environment.

@sr229

This comment has been minimized.

Copy link

sr229 commented Mar 11, 2019

Implementing it as per #183.

@nhooyr

This comment has been minimized.

Copy link
Collaborator Author

nhooyr commented Mar 11, 2019

https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#user

Avoid installing or using sudo as it has unpredictable TTY and signal-forwarding behavior that can cause problems. If you absolutely need functionality similar to sudo, such as initializing the daemon as root but running it as non-root), consider using “gosu”.

This is interesting, though I'm not sure if its applicable to us or whether the advice is about RUN commands or the entrypoint.

@frol

As a side effect, running code-server as root will result in that the newly created files become owned by root, so soon enough user will end up with a project that cannot be modified from their regular user. To solve the permission issues, it is recommended to create a regular user and remap host UID & GID

This is definitely a big issue. I think this alone is enough to justify the use of a user account. We definitely do not want all files written as root but instead whatever user the dev is using.

@nhooyr

This comment has been minimized.

Copy link
Collaborator Author

nhooyr commented Mar 11, 2019

Btw guys, container per terminal is being tracked at #190

@sr229

This comment has been minimized.

Copy link

sr229 commented Mar 11, 2019

@nhooyr From what I have done with user-mode containers, this might increase the image size, but I might be wrong.

@nhooyr

This comment has been minimized.

Copy link
Collaborator Author

nhooyr commented Mar 11, 2019

@nhooyr From what I have done with user-mode containers, this might increase the image size, but I might be wrong.

Thats fine. Though, as I mentioned earlier, this issue has not yet been discussed by the team and we're not sure of its direction. Please keep in mind your changes may not be accepted.

Also, since we're using ENTRYPOINT we can omit the CMD stage IMHO. We should be fixing this in the program-side.

Already fixed. See #188

@sr229 sr229 referenced a pull request that will close this issue Mar 11, 2019

Open

feat: user-mode docker #192

@avelino

This comment has been minimized.

Copy link
Contributor

avelino commented Mar 11, 2019

Do not like to run with root even inside the container, I think it important to have a user (ex: coder)
WIP in PR #192 by @sr229 (good job)

@krazyito65

This comment has been minimized.

Copy link

krazyito65 commented Mar 11, 2019

As an FYI: from what I've seen CentOS / Fedora based systems require Docker to be run as root.

https://www.projectatomic.io/blog/2015/08/why-we-dont-let-non-root-users-run-docker-in-centos-fedora-or-rhel/ -- is a good read

@nhooyr

This comment has been minimized.

Copy link
Collaborator Author

nhooyr commented Mar 11, 2019

That is interesting but I believe orthogonal to our discussion. We're discussing whether the processes inside the container should run as root.

@krazyito65

This comment has been minimized.

Copy link

krazyito65 commented Mar 11, 2019

My bad, misunderstood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.