Fix folder access and mount problems with image volumes #951
Conversation
|
Can one of the admins verify this patch?
|
|
bot, add author to whitelist |
| return err | ||
| } | ||
|
|
||
| if err = os.Chown(volumePath, int(uid), int(gid)); err != nil { |
There was a problem hiding this comment.
I don't think this is the logic we want to use - we should use the owner and permissions of the parent folder in the image (parent path of filepath.Join(mountPoint, k))
There was a problem hiding this comment.
I agree that the way I did may not be optimal.
But I don't think that using the parent folder stats is correct, because it's likely that the parent folder has stricter permissions. E.g. when mounting a volume as sub folder of /mnt, if we use the same permissions of /mnt (root - 755), we end up again with a non-accessible folder for the container user.
There was a problem hiding this comment.
I think we should follow what Docker does in this situation. Create a container image with a volume owned by non root, and how does it
# cat Dockerfile
FROM alpine
RUN mkdir /data; chown 3267:3267 /data
VOLUME /data
# docker build -t volumetest .
# docker run --rm volumetest ls -lad /data
drwxr-xr-x 2 3267 3267 6 Jun 16 10:17 /data
# docker run --user 1234 --rm volumetest ls -lad /data
drwxr-xr-x 2 3267 3267 6 Jun 16 10:17 /data
# podman build -t volumetest .
# podman run --rm volumetest ls -lad /data
drwxr-xr-x 2 root root 6 Jun 16 10:22 /data
# podman run --user 1234 --rm volumetest ls -lad /data
drwxr-xr-x 2 root root 6 Jun 16 10:23 /data
Docker is definately getting the ownership/permissions from the image.
There was a problem hiding this comment.
@rhatdan, my implementation mimics Docker in this regard:
# ./bin/podman run --rm volumetest ls -lad /data
drwxr-xr-x 1 3267 3267 0 Jun 18 16:29 /data
# ./bin/podman run --user 1234 --rm volumetest ls -lad /data
drwxr-xr-x 1 3267 3267 0 Jun 18 16:29 /data
There was a problem hiding this comment.
@mheon Does that look good to you?
|
@mheon PTAL |
|
@rhatdan I'm still a little iffy on the logic for ownership of the directories, but it's definitely an improvement over what we have, and we can always adjust the algorithm for chown/chmod at a later date |
|
Let me just do a quick test with user namespaces first before we merge, want to make sure it won't mess up our chown |
|
Yes I guess the question would be is the chown happening within the User Namespace @giuseppe PTAL |
|
Just looking at the code, I'm strongly suspecting this won't handle user namespaces correctly. We'll need to translate the UID we get out of the |
|
Lets merge and we can deal with that separately. |
|
The chroot stuff will not fix this. We need the chmod to happen in the user namespace so when it sees the underlying UID, it will be mapped to what it is setting it too. No need to look into the chroot of the container image. |
|
@rhatdan I'm looking more into this, and I think we may not be handling the case where user namespaces are enabled and we're not running as UID 0 in the container at all. |
|
Still, this PR will break image volumes on user namespace enabled containers. |
|
Their broken now. This PR fixes the broken issue for the vast majority of use cases. |
|
Fair enough... |
|
Yup @giuseppe Could you check this with some of your work. |
|
📌 Commit 4a659f7 has been approved by |
|
yes, I am currently working (and struggling) on getting tests done well with userNS. For example: #936 is failing because it needs a newer runc (https://bodhi.fedoraproject.org/updates/FEDORA-2018-afddf09bfb). I'll rebase the other PR once this is merged and manually test it. |
|
🔒 Merge conflict |
|
Squashed commits into a single one and rebased. |
libpod/container_internal.go
Outdated
| srcPath := filepath.Join(mountPoint, k) | ||
|
67E6
|
||
| if _, err := os.Stat(srcPath); os.IsNotExist(err) { | ||
| logrus.Infof("Volume image mount point %s does not exits in root FS, need create it", k) |
There was a problem hiding this comment.
"Volume image mount point %s does not exist in root FS, need to create it"
Signed-off-by: Marco Vedovati <mvedovati@suse.com> - Set srcPath permissions so that the container user can R/W it. - Fix uninitialized spec.Mount when restarting a container. - Check for srcPath instead of volumePath existence when setting up a A3E2 volume mount point for a container. - Set the overlay volumePath with the same owner and permissions as srcPath to allow proper access by the container user. Closes containers#844
|
bot, retest this please |
|
📌 Commit 4830608 has been approved by |
|
☀️ Test successful - status-papr |
|
@mheon, I did some tests with user namespaces as I was curious of the results. In the test I did this change is not breaking support for user NS. This is for images with volume path already there in the root FS and for those without volume path already existing. You can see the tests and the results here: https://github.com/marcov/userns-test |
|
@marcov We have some tests that can be run outside of the test framework by humans to make sure things are good. I know we have these for buildah, but I am sure we could use stuff like this in podman as well. @TomSweeneyRedHat Could you take a look at what @marcov has done and see if this is something we should add to our "demos" |
|
We've a test file that I run on each proposed kit at tests/test_podman_baseline.sh. We've a similar one for Buildah. I'll see if I can massage some or all of these in there and also want to add some ONBUILD and possibly CNI tests to both. Thanks for the pointer! |
Fixes and improvements for container images with attached volumes:
spec.MountSourcefield (fixes Podman with image volume fails to create directory after restart #844).