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

Enable userns by default #38795

Draft
wants to merge 8 commits into
base: master
from

Conversation

@cpuguy83
Copy link
Contributor

commented Feb 25, 2019

Posting this for discussion.
Note that LXC already defaults to an equivalent setting. It would be nice to push things along here.

This is a huge boost to default security.
Some things to work out:

  • Option to allow auto userns=host for privileged and net/pid/ipc=host
  • Support mapping uid/gids for container mounts to userns
  • Make mount id mapping optional (disabled by default?)
  • Make mount mapper pluggable (support external executable)
  • Support non-recursive bind mounts for idmapfs
  • Unmount mapped fs on container shutdown
  • Checkpoint/restore (currently broken)
  • Address with userns=host container fs has ownership set to the remapped root instead of real root.
  • Fix issue with docker-py tests relying on vieux/sshfs which is broken on userns
@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Also another benefit of this discussion, at the very least some of this work can help make running with remapped root enabled easier than it currently is regardless of the default.

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

One blocker may be CentOS / RHEL, where I think it's not supported with the default configuration; not sure if this complicates running k8s (I seem to recall it uses a lot of bind-mounts which may be problematic without shiftfs)

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

@thaJeztah

RHEL/CentOS: We can check the setting (it's in /proc/sys/kernel somewhere if I remember correctly) and handle/warn accordingly ([WARN] defaulting to insecure/no user namespaces due to lack of kernel support, enable support by setting /proc/sys/kernel/... and restart dockerd)

k8s: I think there is likely to be issues for many people (not just k8s), which we can try to mitigate through various things (fuse), but ultimately this is why the flag to disable is there. We just need to make more noise on whatever release this default changes so people can make the required configurations before deploying new systems with the new version.

@tonistiigi

This comment has been minimized.

Copy link
Member

commented Feb 25, 2019

@codecov

This comment has been minimized.

Copy link

commented Mar 9, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@33c3200). Click here to learn what that means.
The diff coverage is 13.67%.

@@            Coverage Diff            @@
##             master   #38795   +/-   ##
=========================================
  Coverage          ?   36.41%           
=========================================
  Files             ?      614           
  Lines             ?    45908           
  Branches          ?        0           
=========================================
  Hits              ?    16719           
  Misses            ?    26900           
  Partials          ?     2289

@cpuguy83 cpuguy83 force-pushed the cpuguy83:default_to_userns branch from 411a2fb to 26af875 Mar 9, 2019

@cpuguy83 cpuguy83 force-pushed the cpuguy83:default_to_userns branch from 26af875 to 00aeea9 Mar 9, 2019

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2019

Looks like there's some issues with the plugin tests with userns.
Seems the last time CI ran for userns was ahem ~1.5yr ago. /cc @andrewhsu


I've also added a 2nd commit for testing purposes... it does uid/gid mapping for bind mounts into the container's ID space using https://github.com/cpuguy83/idmapfs
Right now it does all user specified mounts, even if it may not be necessary to do so.

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

Seems breakage is likely caused by 28334c1

@cpuguy83 cpuguy83 force-pushed the cpuguy83:default_to_userns branch 5 times, most recently from 05ac442 to da03638 Mar 10, 2019

cpuguy83 added some commits Feb 25, 2019

Enable userns by default
This is a huge boost to default seucrity.
Some things to work out:

- [ ] How to not break users who already have unmapped images loaded
- [ ] How to not break users who mount stuff from the host
- [ ] Should `--privileged` mean `--userns=host` as well to prevent
breakage?

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Use idmapfs to map host bind mounts
idmapfs allows us to create a bind mount with the ownership of the
file/tree mapped to the container's user namespace mapping.

This change makes use of idmapfs to automatically map user requested
host bind mounts to the container's ID space.

For (a really bad) example, with this change with user namespces enabled
you can now do `docker run --rm --user=root -v /etc:/hostetc busybox cat
/hostetc/shadow`

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Fix test daemon config for userns path
When starting up the test daemon we set data root to some random dir,
and then update that dir after the daemon is started to whatever is
reported from calling the info API.
When the daemon is restarted we then use that updated path as the data
root for the new daemon process.

In the case of user namespaces, the daemon actually repoprts a new root
directory... e.g.

With this:

```
dockerd --data-root=/foo/bar --userns-remap=default
```

The daemon root gets reported as `/foo/bar/<mapped uid>.<mapped gid>`.
This means when we restart the test daemon we set a new data root of
`/foo/bar/<maped uid>.<mapped gid>` which then the daemon will update
again to `/foo/bar/<maped uid>.<mapped gid>/<maped uid>.<mapped gid>`
which causes tests to fail expecting state to stick around.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Fix plugin graphdriver tests for userns
Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 added some commits Mar 10, 2019

Add daemon opts to set auto userns=host certain conditions
These options allow an admin to preserve compatability with old behavior
when userns is enabled.
THey are disabled by default for security purposes.

Specifically this adds daemon options to automatically set userns=host
when:

1. --net=host
2. --pid=host
3. --privileged

The config change is not actually set on the container config so the
options can be turned off and have the policy change enforced without
recreating the container.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Fix domain and C/R tests to use userns=host
Signed-off-by: Brian Goff <cpuguy83@gmail.com>

@cpuguy83 cpuguy83 force-pushed the cpuguy83:default_to_userns branch from 4a87890 to 047ed26 Mar 10, 2019

Fix userns remapped file mounts
Signed-off-by: Brian Goff <cpuguy83@gmail.com>

@cpuguy83 cpuguy83 force-pushed the cpuguy83:default_to_userns branch from 047ed26 to 0d8d929 Mar 10, 2019

@AkihiroSuda

This comment has been minimized.

Copy link
Member

commented Mar 10, 2019

Option to allow auto userns=host for privileged and net/pid/ipc=host

and for containers with mounts, until we can get in-kernel idmapfs?

@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2019

It's on the todo checklist. 👼❤️

Add option to enable userns filesystem remapping
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

Looks like docker-py tests are using viex/sshfs which is mounting /var/lib/docker/plugins, which doesn't exist for userns (because it's under /var/lib/docker/<remappedroot>/plugins).
It's not clear to me why it needs it at all... but probably this could use a different plugin anyway.

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.