Relabel BTRFS Content on container Creation #16452
Conversation
tiborvass
added
area/security/selinux
area/security
and removed
area/distribution
labels
|
makes sense for me |
|
Moved to code-review, unfortunately it needs a rebase. Change LGTM. |
rhatdan
force-pushed
the
btrfs-selinux
branch
from
80d8e4d to
23df289
Compare
|
@calavera Rebased. |
|
it doesn't look like this compiles any more. Check https://jenkins.dockerproject.org/job/Docker-PRs/16830/console |
rhatdan
force-pushed
the
btrfs-selinux
branch
6 times, most recently
from
b8660ea to
ad86722
Compare
|
How do I fix the golint problems with Windows? |
|
I've opened #16666 to fix those issues. |
rhatdan
force-pushed
the
btrfs-selinux
branch
from
ad86722 to
268ef44
Compare
|
Don't see how the Janky failures are related to my patch? |
|
@rhatdan janky runs go-lint in every changed file you commit. For some reason, it looks like |
rhatdan
force-pushed
the
btrfs-selinux
branch
2 times, most recently
from
b4ac89b to
90a0b76
Compare
rhatdan
force-pushed
the
btrfs-selinux
branch
from
90a0b76 to
f109b7c
Compare
|
rebased @calavera Any progress on this? |
rhatdan
force-pushed
the
btrfs-selinux
branch
2 times, most recently
from
92c7a0c to
9a44583
Compare
|
I moved the security check out of setHostConfig so I can call it before createRootFS. Hopefully this fixes the test issues. |
This change will allow us to run SELinux in a container with BTRFS back end. We continue to work on fixing the kernel/BTRFS but this change will allow SELinux Security separation on BTRFS. It basically relabels the content on container creation. Just relabling -init directory in BTRFS use case. Everything looks like it works. I don't believe tar/achive stores the SELinux labels, so we are good as far as docker commit. Tested Speed on startup with BTRFS on top of loopback directory. BTRFS not on loopback should get even better perfomance on startup time. The more inodes inside of the container image will increase the relabel time. This patch will give people who care more about security the option of runnin BTRFS with SELinux. Those who don't want to take the slow down can disable SELinux either in individual containers or for all containers by continuing to disable SELinux in the daemon. Without relabel: > time docker run --security-opt label:disable fedora echo test test real 0m0.918s user 0m0.009s sys 0m0.026s With Relabel test real 0m1.942s user 0m0.007s sys 0m0.030s Signed-off-by: Dan Walsh <dwalsh@redhat.com> Signed-off-by: Dan Walsh <dwalsh@redhat.com>
rhatdan
force-pushed
the
btrfs-selinux
branch
from
9a44583 to
1716d49
Compare
|
LGTM |
1 similar comment
|
LGTM |
Relabel BTRFS Content on container Creation
|
Why were the labels added to the |
|
You need the label at create time in order to recursively set the labels. Get is too late. We need the same thing to get labels for OverlayFS if the kernel ever gets fixed. |
|
The label is only added to one Considering this label is done at the very end of |
|
Then every get call would be relabeling, we only want this happening once. Think docker create versus docker start. |
|
I understand the difference, I am asking because this effects #17924 and we have to rebase and make interface updates as a result of this change. From my perspective, this is a btrfs specific change (possibly something similar for overlay in the future) which has now updated the interface to add mount labels on create, where previously create was not intended to mount. If relabeling must happen before first mount on btrfs then it is preferable to limit the change if that is possible. Except for on Somewhat separate question, why does this only apply to the init layer for the container and not any of the base layers or container writable layer? |
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
Signed-off-by: Derek McGowan <derek@mcgstyle.net> (github: dmcgowan)
|
Yes relabeling is going to be time consuming. We time it out at about 1-2 seconds on a base image, but it could grow depending on the number of inodes in the image. Since we only need to do this on create, no need to add this penalty to all Starts. By only applying it to the init layer. we end up being able to continue to share the image. IE Does not cause a COW change for every container. @rhvgoyal correct? |
|
@rhatdan right. If we apply label on container rootfs (and not init layer), then |
This script had two main functions: 1. Select the graphdriver This functionality is now handled in the docker daemon. It defaults to overlay2 on recent docker versions, and does its own fs detection for btrfs etc. We carry a patch for 1.12.6 now to prefer overlay to devicemapper 2. Avoid enabling selinux on btrfs This no longer matters since as of v1.10, selinux on btrfs is supported. See moby/moby#16452 It was replaced with a simpler systemd environment variable
This script had two main functions: 1. Select the graphdriver This functionality is now handled in the docker daemon. It defaults to overlay2 on recent docker versions, and does its own fs detection for btrfs etc. We carry a patch for 1.12.6 now to prefer overlay to devicemapper 2. Avoid enabling selinux on btrfs This no longer matters since as of v1.10, selinux on btrfs is supported. See moby/moby#16452 This PR replaces that original functionality with a simpler systemd environment variable, which is also more in-line with what we do for other similar choices. The environment variable is also more discoverable and easier for users to edit. Note: for backwards compatibility with DOCKER_OPTS=--selinux-enabled=false (to make that take precedent), we intentionally put the environment variable as the first option. However, for backwards compatibility with older units, we also retain the script. We are able to remove the graphdriver detection/selection since that behavior now happens appropriately in docker, but we need to keep the selinux defaulting so that people who are executing the script and expecting selinux to work (e.g. if they copied an old docker.service) will continue to get selinux as expected.
This script had two main functions: 1. Select the graphdriver This functionality is now handled in the docker daemon. It defaults to overlay2 on recent docker versions, and does its own fs detection for btrfs etc. We carry a patch for 1.12.6 now to prefer overlay to devicemapper 2. Avoid enabling selinux on btrfs This no longer matters since as of v1.10, selinux on btrfs is supported. See moby/moby#16452 This PR replaces that original functionality with a simpler systemd environment variable, which is also more in-line with what we do for other similar choices. The environment variable is also more discoverable and easier for users to edit. Note: for backwards compatibility with DOCKER_OPTS=--selinux-enabled=false (to make that take precedent), we intentionally put the environment variable as the first option. However, for backwards compatibility with older units, we also retain the script. We are able to remove the graphdriver detection/selection since that behavior now happens appropriately in docker, but we need to keep the selinux defaulting so that people who are executing the script and expecting selinux to work (e.g. if they copied an old docker.service) will continue to get selinux as expected.
This script had two main functions: 1. Select the graphdriver This functionality is now handled in the docker daemon. It defaults to overlay2 on recent docker versions, and does its own fs detection for btrfs etc. We carry a patch for 1.12.6 now to prefer overlay to devicemapper 2. Avoid enabling selinux on btrfs This no longer matters since as of v1.10, selinux on btrfs is supported. See moby/moby#16452 This PR replaces that original functionality with a simpler systemd environment variable, which is also more in-line with what we do for other similar choices. The environment variable is also more discoverable and easier for users to edit. Note: for backwards compatibility with DOCKER_OPTS=--selinux-enabled=false (to make that take precedent), we intentionally put the environment variable as the first option. However, for backwards compatibility with older units, we also retain the script. We are able to remove the graphdriver detection/selection since that behavior now happens appropriately in docker, but we need to keep the selinux defaulting so that people who are executing the script and expecting selinux to work (e.g. if they copied an old docker.service) will continue to get selinux as expected.
This change will allow us to run SELinux in a container with
BTRFS back end. We continue to work on fixing the kernel/BTRFS
but this change will allow SELinux Security separation on BTRFS.
It basically relabels the content on container creation.
Just relabling -init directory in BTRFS use case. Everything looks like it
works. I don't believe tar/achive stores the SELinux labels, so we are good
as far as docker commit.
Tested Speed on startup with BTRFS on top of loopback directory. BTRFS
not on loopback should get even better perfomance on startup time. The
more inodes inside of the container image will increase the relabel time.
This patch will give people who care more about security the option of
runnin BTRFS with SELinux. Those who don't want to take the slow down
can disable SELinux either in individual containers or for all containers
by continuing to disable SELinux in the daemon.
Without relabel:
real 0m0.918s
user 0m0.009s
sys 0m0.026s
With Relabel
test
real 0m1.942s
user 0m0.007s
sys 0m0.030s
Signed-off-by: Dan Walsh dwalsh@redhat.com