use groups for managing write-access to files #435

minrk · 2017-08-11T11:46:46Z

any files the user should be able to write should have group user-permissions with g+rwX
remove chown from start.sh because it is no longer needed
add fix-permissions script for setting the user-writable permissions on a path
users group is GID 100
containers can set group with --group-add users if they want to run with a different uid/gid
(without -u root -e NB_UID -e NB_GID, which make this unnecessary), for instance:
```
 docker run -u 501:501 --group-add users -it jupyter/base-notebook
```

Questions:

is there a reason we need a dedicated high-gid group for this? Or can we leave it as the default users group 100?

closes #434
closes #395

I believe this resolves issues brought up in #188 without needing to consider the possible implications of running with gid=0

wakonp · 2017-08-11T12:10:36Z

Exactly want I wanted 👍

akhmerov · 2017-08-15T14:13:28Z

This won't fix permissions on mounted containers, right?

Continuation: Since home is often mounted, shouldn't fix_permissions still be run on startup on home?

minrk · 2017-08-16T13:21:00Z

This won't fix permissions on mounted containers, right?

Correct, and rightly so - mounted volumes shouldn't have their permissions touched, right?

Since home is often mounted, shouldn't fix_permissions still be run on startup on home?

I don't think so. Since home is often mounted, it should already be mounted with the right permissions. It shouldn't be modified after mounting.

parente · 2017-08-16T13:23:46Z

Correct, and rightly so - mounted volumes shouldn't have their permissions touched, right?

They should not. Whenever we've accidentally done that, we've had bugs opened here with users saying their host permissions were changed. I think it's up to the user to match the host permission to whatever UID / GID they decide to use in the container.

akhmerov · 2017-08-16T13:27:18Z

Thanks for the clarifications.

parente · 2017-08-16T14:38:54Z

base-notebook/README.md

+* `--group-add user-writable` - use this argument if you are also specifying
+  a specific user id to launch the container (`-u 5000`), rather than launching the container as root and relying on NB_UID and NB_GID to set the user and group.
+
+  It does not 


Leftover? Same with line below?

parente · 2017-08-16T14:41:30Z

all-spark-notebook/Dockerfile

-    'r-rcurl=1.95*' && conda clean -tipsy
+    'r-rcurl=1.95*' && \
+    conda clean -tipsy && \
+    fix-permissions $CONDA_DIR


Is it possible to set the group sticky bit (+s) on the root conda directory so that group permissions are preserved across all installs without running this script each time?

Setting the sticky bit gets us almost there, and I did set it. Good call. New files have the right owners, but not the right permissions. For that, I think we need acls or the ability to set umask for subsequent RUN commands, but AUFS doesn't support ACLs and I don't see a way to set umask, so I think we're stuck. I'd love to be wrong, though.

parente · 2017-08-22T15:28:18Z

Giving this a shot locally, mostly to better understand the changes and to see if we really need the NB_USER and such overrides going forward. I did immediately see that docker run on v 17.xCE does not recognize -g. Is that flag a typo in the description of the PR or am I missing something?

parente · 2017-08-22T18:13:43Z

base-notebook/README.md

-# Have the Spawner override the Docker run command
-c.DockerSpawner.extra_create_kwargs.update({
-	'command': '/usr/local/bin/start-singleuser.sh'
-})


Is start-singleuser no longer needed? If so, we should pull it from the other READMEs as well.

Yes. start.sh already dispatches to start-singleuser.sh based on environment variables. Users don't need to know the difference.

- any files the user should be able to write should have group `user-permissions` with `g+rwX` - remove `chown` from start.sh because it is no longer needed - add `fix-permissions` script for setting the user-writable permissions on a path - user-permissions group as GID 10000 (is there a reason for it to have a different value?) - containers can set group with `--group-add user-writable` if they want to run with a different uid/gid (without -u root -e NB_UID -e NB_GID, which make this unnecessary)

ensures files have the right owner:group unfortunately, not enough to get group-writable permissions (need acl or umask for that), so we still need to run it after each install

use `users` instead

it’s handled internally

parente · 2017-08-26T14:02:23Z

I've tested this locally and it works great. We should consider adding more doc about when / how it's useful vs the NB_USER, NB_UID, etc. env vars.

Will merge this after the current round of images finishes building. (There's no way to communicate from one build to the next on docker cloud which git SHA to pull and build, so merging again in the meantime means we'll get a mix of code changes across the images for the same tag).

minrk · 2017-08-26T14:27:25Z

The -g was just a typo. It should have been -u uid[:gid].

minrk force-pushed the permissions branch from ea7a09f to 9c2dfcb Compare August 11, 2017 11:47

wakonp mentioned this pull request Aug 11, 2017

Added driver_config support cassinyio/SwarmSpawner#15

Merged

minrk mentioned this pull request Aug 11, 2017

Force ownership change on /home/jovyan/work #395

Closed

minrk force-pushed the permissions branch from 9c2dfcb to 201e274 Compare August 11, 2017 14:10

parente reviewed Aug 16, 2017

View reviewed changes

parente reviewed Aug 22, 2017

View reviewed changes

minrk added 4 commits August 26, 2017 09:50

set setuid/setgid bits in fix-permissions

2df9c49

ensures files have the right owner:group unfortunately, not enough to get group-writable permissions (need acl or umask for that), so we still need to run it after each install

leftover mentions of user-writable group

80359f3

use `users` instead

remove user-facing start-singleuser.sh docs

b69f43e

it’s handled internally

minrk force-pushed the permissions branch from af37915 to b69f43e Compare August 26, 2017 13:55

parente merged commit c879782 into jupyter:master Aug 26, 2017

parente mentioned this pull request Nov 3, 2017

Fix wrong permissions when changing NB_UID or NB_GID. #491

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use groups for managing write-access to files #435

use groups for managing write-access to files #435

minrk commented Aug 11, 2017 •

edited

Loading

wakonp commented Aug 11, 2017

akhmerov commented Aug 15, 2017 •

edited

Loading

minrk commented Aug 16, 2017

parente commented Aug 16, 2017

akhmerov commented Aug 16, 2017

parente Aug 16, 2017

parente Aug 16, 2017

minrk Aug 19, 2017

parente commented Aug 22, 2017

parente Aug 22, 2017

minrk Aug 26, 2017

parente commented Aug 26, 2017

minrk commented Aug 26, 2017

use groups for managing write-access to files #435

use groups for managing write-access to files #435

Conversation

minrk commented Aug 11, 2017 • edited Loading

wakonp commented Aug 11, 2017

akhmerov commented Aug 15, 2017 • edited Loading

minrk commented Aug 16, 2017

parente commented Aug 16, 2017

akhmerov commented Aug 16, 2017

parente Aug 16, 2017

Choose a reason for hiding this comment

parente Aug 16, 2017

Choose a reason for hiding this comment

minrk Aug 19, 2017

Choose a reason for hiding this comment

parente commented Aug 22, 2017

parente Aug 22, 2017

Choose a reason for hiding this comment

minrk Aug 26, 2017

Choose a reason for hiding this comment

parente commented Aug 26, 2017

minrk commented Aug 26, 2017

minrk commented Aug 11, 2017 •

edited

Loading

akhmerov commented Aug 15, 2017 •

edited

Loading