| Message ID | 20230726212009.221147-1-thomas.petazzoni@bootlin.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC,1/2] utils/docker-run: make it compatible with SELinux | expand |
Hi Thomas, Quoting Thomas Petazzoni via buildroot (2023-07-26 23:20:07) > > diff --git a/utils/docker-run b/utils/docker-run > index 17c587a484..eee1aad7a4 100755 > --- a/utils/docker-run > +++ b/utils/docker-run > @@ -12,8 +12,8 @@ declare -a docker_opts=( > -i > --rm > --user "$(id -u):$(id -g)" > - --mount "type=bind,src=${MAIN_DIR},dst=${MAIN_DIR}" > - --mount "type=bind,src=${GIT_DIR},dst=${GIT_DIR}" > + --volume "${MAIN_DIR}:${MAIN_DIR}:Z" > + --volume "${GIT_DIR}:${GIT_DIR}:Z" Using Z will label all the files in MAIN_DIR and GIT_DIR with a private label and IIRC that means relabeling all files each time a new container is started; which can take quite some time if there are lots if files in there. However z can be used to label files with a shared label and they won't be relabeled after the first run. Antoine
Hello Antoine, On Thu, 27 Jul 2023 10:13:53 +0200 Antoine Tenart <atenart@kernel.org> wrote: > > + --volume "${MAIN_DIR}:${MAIN_DIR}:Z" > > + --volume "${GIT_DIR}:${GIT_DIR}:Z" > > Using Z will label all the files in MAIN_DIR and GIT_DIR with a private > label and IIRC that means relabeling all files each time a new container > is started; which can take quite some time if there are lots if files in > there. However z can be used to label files with a shared label and they > won't be relabeled after the first run. Thanks for the hint, makes sense! Do you know why --mount, which is apparently "superior" and recommended over --volume, does not support this SELinux labeling mechanism? Thomas
Quoting Thomas Petazzoni via buildroot (2023-07-27 10:48:21) > > Do you know why --mount, which is apparently "superior" and recommended > over --volume, does not support this SELinux labeling mechanism? IIRC --mount was introduced for better consistency in the docker args and supporting names parameters (I don't know the full reasoning but it's probably linked to supporting APIs -like yaml definitions- in a better way?). While doing so they tried to fix issues in --volume and labeling files on mount was one of them. The issue is both logical and practical: files should be labeled when being created (not when being mounted) and relabeling files "magically" can cause issues (eg. don't `-v /home:/home:z` !). The reasoning with the new --mount option is a directory should be created and configured on the host and then only mounted by containers (hence the directory is not anymore created if not present when using --mount). So here, idk, not a docker expert :) I'd say using relabeling of a directory that is under Buildroot's control is probably OK. While replying, had a quick look at this and it seems the preferred solution would be instead to use the `--security-opt label=disable` option: labels would be kept in sync with the host and I guess the goal of using Docker here is not for isolating the build but to have a known environment. I never played with that option so please investigate before switching to it. Antoine
Hello, On Thu, 27 Jul 2023 12:19:48 +0200 Antoine Tenart <atenart@kernel.org> wrote: > IIRC --mount was introduced for better consistency in the docker args > and supporting names parameters (I don't know the full reasoning but > it's probably linked to supporting APIs -like yaml definitions- in a > better way?). While doing so they tried to fix issues in --volume and > labeling files on mount was one of them. > > The issue is both logical and practical: files should be labeled when > being created (not when being mounted) and relabeling files "magically" > can cause issues (eg. don't `-v /home:/home:z` !). The reasoning with > the new --mount option is a directory should be created and configured > on the host and then only mounted by containers (hence the directory is > not anymore created if not present when using --mount). > > So here, idk, not a docker expert :) I'd say using relabeling of a > directory that is under Buildroot's control is probably OK. While > replying, had a quick look at this and it seems the preferred solution > would be instead to use the `--security-opt label=disable` option: > labels would be kept in sync with the host and I guess the goal of using > Docker here is not for isolating the build but to have a known > environment. I never played with that option so please investigate > before switching to it. Thanks for the hint! I tried --security-opt label=disable, and it works, at least it fixes my permission issue, without having to do the z/Z thing. Probably it's a better option than using z/Z that has the side effect of adding SELinux labels on all files being mounted? To be honest, I'm not clear on the consequences of --security-opt label=disable. Thomas
Quoting Thomas Petazzoni (2023-07-27 12:24:49) > On Thu, 27 Jul 2023 12:19:48 +0200 > Antoine Tenart <atenart@kernel.org> wrote: > > > IIRC --mount was introduced for better consistency in the docker args > > and supporting names parameters (I don't know the full reasoning but > > it's probably linked to supporting APIs -like yaml definitions- in a > > better way?). While doing so they tried to fix issues in --volume and > > labeling files on mount was one of them. > > > > The issue is both logical and practical: files should be labeled when > > being created (not when being mounted) and relabeling files "magically" > > can cause issues (eg. don't `-v /home:/home:z` !). The reasoning with > > the new --mount option is a directory should be created and configured > > on the host and then only mounted by containers (hence the directory is > > not anymore created if not present when using --mount). > > > > So here, idk, not a docker expert :) I'd say using relabeling of a > > directory that is under Buildroot's control is probably OK. While > > replying, had a quick look at this and it seems the preferred solution > > would be instead to use the `--security-opt label=disable` option: > > labels would be kept in sync with the host and I guess the goal of using > > Docker here is not for isolating the build but to have a known > > environment. I never played with that option so please investigate > > before switching to it. > > I tried --security-opt label=disable, and it works, at least it fixes > my permission issue, without having to do the z/Z thing. Probably it's > a better option than using z/Z that has the side effect of adding > SELinux labels on all files being mounted? To be honest, I'm not clear > on the consequences of --security-opt label=disable. I agree on not relabeling files if that's an option. Not sure about the consequences of `--security-opt label=disable` too. My understanding is the containerized process won't be labeled with a container-specific label, but they still could be labeled with a permissive one not 100% matching the user label. Without having a full understanding of the implications we can still expect child processes not to have more rights than the parent and created file labels to match what would have happened on the host (could you check?). Given the goal is not to isolate the build, my guess is that is fine and the best option. Antoine
Quoting Antoine Tenart (2023-07-27 12:50:17) > > Without having a full understanding of the implications we can still > expect child processes not to have more rights than the parent Hmm, my point doesn't work for non-rootless containers, the Docker deamon runs as root. Not that this changes the other points.
Thomas, All, On 2023-07-26 23:20 +0200, Thomas Petazzoni spake thusly: > After switching to a fresh Fedora 38 installation with SELinux > disabled, we noticed that utils/docker-run doesn't work as the > applications running inside the container are not allowed to accept > the data mounted through the bind mount. > > Turns out that Docker has a "Z" option to do the appropriate magic for > SELinux. However, this "Z" option is only available for --volume, not > for --mount, as explained in > https://docs.docker.com/storage/bind-mounts/. hat about those systems that do not have SELinux: how is z/Z going to work for those? > So, this commit partially reverts 7f2020f9040f ("utils/docker-run: > improve user experience") that switched from --volume to > --mount. However, the justification in 7f2020f9040f to switch from > --volume to --mount was "Docker will create the destination if it does > not exist", but the current Docker documentation seems to say exactly > the opposite: > > If you use -v or --volume to bind-mount a file or directory that > does not yet exist on the Docker host, -v creates the endpoint for > you. It is always created as a directory. > > If you use --mount to bind-mount a file or directory that does not > yet exist on the Docker host, Docker does not automatically create > it for you, but generates an error. Note that this is about "on the docker host", so the source, while the commit mentions "the destination in the container". I can't where I initially read that, since the pointer in 7f2020f9040f is no longer valid (the anchor no longer exists). > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > --- > NOTE: I am not a Docker expert, and I certainly don't know if this is > the right solution, and I would appreciate feedback from folks with > more Docker experience. > --- > utils/docker-run | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/utils/docker-run b/utils/docker-run > index 17c587a484..eee1aad7a4 100755 > --- a/utils/docker-run > +++ b/utils/docker-run > @@ -12,8 +12,8 @@ declare -a docker_opts=( > -i > --rm > --user "$(id -u):$(id -g)" > - --mount "type=bind,src=${MAIN_DIR},dst=${MAIN_DIR}" > - --mount "type=bind,src=${GIT_DIR},dst=${GIT_DIR}" > + --volume "${MAIN_DIR}:${MAIN_DIR}:Z" > + --volume "${GIT_DIR}:${GIT_DIR}:Z" > --workdir "${MAIN_DIR}" > ) > if tty -s; then > -- > 2.41.0 >
Hello Christian, On Thu, 27 Jul 2023 20:13:08 -0700 Christian Stewart <christian@paral.in> wrote: > On Wed, Jul 26, 2023 at 2:20 PM Thomas Petazzoni > <thomas.petazzoni@bootlin.com> wrote: > > - --mount "type=bind,src=${MAIN_DIR},dst=${MAIN_DIR}" > > - --mount "type=bind,src=${GIT_DIR},dst=${GIT_DIR}" > > + --volume "${MAIN_DIR}:${MAIN_DIR}:Z" > > + --volume "${GIT_DIR}:${GIT_DIR}:Z" > > --workdir "${MAIN_DIR}" > > What is the purpose of the GIT_DIR mount here, doesn't MAIN_DIR contain .git? This is explained in 791c163b2f9f07d4c02b18eabd9b195918e1c603: commit 791c163b2f9f07d4c02b18eabd9b195918e1c603 Author: Yann E. MORIN <yann.morin.1998@free.fr> Date: Sat May 6 23:46:18 2023 +0200 utils/docker-run: make it work in workdirs/woktrees It is quite customary to use a single repository with multiple workdirs, one for each active branch, with either the aging 'git new-workdir' or the more recent 'git worktree'. However, in a workdir/worktree, most entries in .git/ are only symlinks to the actual files in the main repository. Currently, utils/docker-run only bind-mounts the current working copy. If that is a workdir/worktree, then it is going to be missing the actual git data, resulting in errors like: $ ./utils/docker-run make check-package fatal: not a git repository (or any parent up to mount point [....]/buildroot) Stopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set). No files to check style make: *** [Makefile:1257: check-package] Error 1 So, we also bind-mount the actual git directory. If that is a subdir of the current working copy, then it is already mounted and thus the bind-mount is superfluous but harmless; for simplicity, we mount it unconditionally. Signed-off-by: Yann E. MORIN <yann.morin.1998@free.fr> Cc: Ricardo Martincoski <ricardo.martincoski@datacom.com.br> > I think this mount might be causing some unpredictable behavior in the > host-go package, but it's just a hunch at the moment. Did you confirm this? Thomas
Thomas, All, On 2023-07-26 23:20 +0200, Thomas Petazzoni via buildroot spake thusly: > After switching to a fresh Fedora 38 installation with SELinux > disabled, we noticed that utils/docker-run doesn't work as the > applications running inside the container are not allowed to accept > the data mounted through the bind mount. > > Turns out that Docker has a "Z" option to do the appropriate magic for > SELinux. However, this "Z" option is only available for --volume, not > for --mount, as explained in > https://docs.docker.com/storage/bind-mounts/. > > So, this commit partially reverts 7f2020f9040f ("utils/docker-run: > improve user experience") that switched from --volume to > --mount. However, the justification in 7f2020f9040f to switch from > --volume to --mount was "Docker will create the destination if it does > not exist", but the current Docker documentation seems to say exactly > the opposite: > > If you use -v or --volume to bind-mount a file or directory that > does not yet exist on the Docker host, -v creates the endpoint for > you. It is always created as a directory. > > If you use --mount to bind-mount a file or directory that does not > yet exist on the Docker host, Docker does not automatically create > it for you, but generates an error. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> I've included this change in my respin: https://patchwork.ozlabs.org/project/buildroot/list/?series=368134 In doing so, I switched to the proposal by Antoine, to use --security-opt label=disable I did not see an adverse effect on a non-SELinux host, and you reported that it also worked for you, so let's ship it. I've also included patch 2 in the respin. Regards, Yann E. MORIN. > --- > NOTE: I am not a Docker expert, and I certainly don't know if this is > the right solution, and I would appreciate feedback from folks with > more Docker experience. > --- > utils/docker-run | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/utils/docker-run b/utils/docker-run > index 17c587a484..eee1aad7a4 100755 > --- a/utils/docker-run > +++ b/utils/docker-run > @@ -12,8 +12,8 @@ declare -a docker_opts=( > -i > --rm > --user "$(id -u):$(id -g)" > - --mount "type=bind,src=${MAIN_DIR},dst=${MAIN_DIR}" > - --mount "type=bind,src=${GIT_DIR},dst=${GIT_DIR}" > + --volume "${MAIN_DIR}:${MAIN_DIR}:Z" > + --volume "${GIT_DIR}:${GIT_DIR}:Z" > --workdir "${MAIN_DIR}" > ) > if tty -s; then > -- > 2.41.0 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
diff --git a/utils/docker-run b/utils/docker-run index 17c587a484..eee1aad7a4 100755 --- a/utils/docker-run +++ b/utils/docker-run @@ -12,8 +12,8 @@ declare -a docker_opts=( -i --rm --user "$(id -u):$(id -g)" - --mount "type=bind,src=${MAIN_DIR},dst=${MAIN_DIR}" - --mount "type=bind,src=${GIT_DIR},dst=${GIT_DIR}" + --volume "${MAIN_DIR}:${MAIN_DIR}:Z" + --volume "${GIT_DIR}:${GIT_DIR}:Z" --workdir "${MAIN_DIR}" ) if tty -s; then
After switching to a fresh Fedora 38 installation with SELinux disabled, we noticed that utils/docker-run doesn't work as the applications running inside the container are not allowed to accept the data mounted through the bind mount. Turns out that Docker has a "Z" option to do the appropriate magic for SELinux. However, this "Z" option is only available for --volume, not for --mount, as explained in https://docs.docker.com/storage/bind-mounts/. So, this commit partially reverts 7f2020f9040f ("utils/docker-run: improve user experience") that switched from --volume to --mount. However, the justification in 7f2020f9040f to switch from --volume to --mount was "Docker will create the destination if it does not exist", but the current Docker documentation seems to say exactly the opposite: If you use -v or --volume to bind-mount a file or directory that does not yet exist on the Docker host, -v creates the endpoint for you. It is always created as a directory. If you use --mount to bind-mount a file or directory that does not yet exist on the Docker host, Docker does not automatically create it for you, but generates an error. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> --- NOTE: I am not a Docker expert, and I certainly don't know if this is the right solution, and I would appreciate feedback from folks with more Docker experience. --- utils/docker-run | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)