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

Volume mounting changes owner of existing directory #10776

Closed
matejvasek opened this issue Jun 24, 2021 · 14 comments · Fixed by #10905
Closed

Volume mounting changes owner of existing directory #10776

matejvasek opened this issue Jun 24, 2021 · 14 comments · Fixed by #10905
Labels
Good First Issue This issue would be a good issue for a first time contributor to undertake. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Comments

@matejvasek
Copy link
Contributor

podman volume mounting doesn't preserve original directory owner. it changes owner to user that is currently running the container. docker preserves the original owner.

# quay.io/boson/faas-go-builder:v0.8.3 contains /workspace directory ownd by `cnb` user.

~> docker run -it --rm -v someVol:/workspace quay.io/boson/faas-go-builder:v0.8.3 ls -la /workspace
total 0
drwxr-xr-x. 1 cnb  cnb    0 Jan  1  1980 .
dr-xr-xr-x. 1 root root 242 Jun 24 15:29 ..
~> podman run -it --rm -v someVol:/workspace quay.io/boson/faas-go-builder:v0.8.3 ls -la /workspace
total 0
drwxr-xr-x.  1 root root 0 Jun 24 15:29 .
dr-xr-xr-x. 22 root root 0 Jun 24 15:29 ..
@matejvasek
Copy link
Contributor Author

it changes owner to user that is currently running the container

I think that's all right only if the directory is not yet existing in the image.

@rhatdan
Copy link
Member

rhatdan commented Jun 24, 2021

Care to open a PR for this. Basically you believe that the destination directory in an image should match the existing directory UID/GID/Permissions if a named volume is created and mounted on the container.

I thought I had a similar bug like this and fixed it. but maybe not.

@rhatdan rhatdan added the Good First Issue This issue would be a good issue for a first time contributor to undertake. label Jun 24, 2021
@matejvasek
Copy link
Contributor Author

For more context we need this to use pack for building buildpacks.

The tool spawns container that is running as a root and it is mounting new volume (it is created together with the container) to /workspace, the /workspace already exits in the base image (owned by cnb user). Latter the container switches from root to the cnb user and tries to do stuff with /workspace. With docker it works since the mounted volume is owned by cnb. With podman the /workspace is owned by root as it it was initial user for the container. This leads to access denied when it tries to write stuff to /workspace.

@rhatdan
Copy link
Member

rhatdan commented Jun 24, 2021

Yes this makes total sense that it should work this way.

@cardil
Copy link

cardil commented Jun 24, 2021

To play a devil advocate here, the manpage of mount(8) says:

The previous contents (if any) and owner and mode of dir become invisible, and as long as this filesystem remains mounted, the pathname dir refers to the root of the filesystem on device.

That's why if the first command is run as user root the volume is created, and mounted with that user UID. Subsequent calls with different user, are trying to reuse that volume, but they see it is owned by root.

All this can be avoided by calling run with proper user for each time:

$ podman volume rm someVol || true
$ podman run --rm \
  --user cnb \
  -v someVol:/workspace \
  quay.io/boson/faas-go-builder:v0.8.3 \
  ls -ld /workspace
drwxr-xr-x. 2 cnb cnb 6 Jun 24 19:05 /workspace

It seems that, Podman closer mimics the standard mount behavior. Docker is doing the opposite, by preserving owner for some strange reason.

Despite this, I think Podman should follow Docker in his behavior, as it's marketed as a Docker drop-in replacement.

@mheon
Copy link
Member

mheon commented Jun 24, 2021

I think the patches from @rhatdan may have changed the behavior on main - @matejvasek are you testing with the latest code?

@cardil Unfortunately, while I tend to agree with you, our compatibility promise with Docker means we are largely tied to their decisions, including bad ones. As @rhatdan says - "Bug for bug compatible"

@matejvasek
Copy link
Contributor Author

@mheon What patches? I run code from master the issue is still present.

@mheon
Copy link
Member

mheon commented Jun 24, 2021

They're already merged (I just backported them for 3.2.2), so if master is broken, the issue isn't fixed.

@rhatdan
Copy link
Member

rhatdan commented Jun 24, 2021

I fixed the permissions, but not the ownership, I guess.

@rhatdan
Copy link
Member

rhatdan commented Jun 24, 2021

I had thought that it already worked.

@matejvasek
Copy link
Contributor Author

matejvasek commented Jun 28, 2021

@rhatdan @mheon I tried to hackaround this in our software by creating the volume in advance.

I am doing something like docker volume create --opt device=tmpfs --opt type=tmpfs --opt o=uid=1000,gid=1001 someVol but not via CLI but via Go client library.

There is an issues with this approach:

The thing is that for docker daemon the device and type options are mandatory, however if I set these to tmpfs then podman daemon fails to mount it: Error: error mounting volume someVol for container 6e4eda11ca302227085c46295d3b2296766ca93756d9698d59ecd175463e1788: cannot mount volumes without root privileges: operation requires root privileges, meaning with podman I have to leave there options empty. What are valid values for the two options?

Even if I fix the problem above there is another problem:
podman cp seems to be broken, it doesn't preserve uid/gid from tar, it chowns everything to root probably because it's present user of the container.

@matejvasek
Copy link
Contributor Author

That chown is probably deliberate but that's not what docker does.

@matejvasek
Copy link
Contributor Author

@rhatdan can you give me some pointer where to fix this?

@rhatdan
Copy link
Member

rhatdan commented Jul 9, 2021

This should fix the problem, you can grab it and add tests, if you want.

diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go
index 850af235f..b69ad4105 100644
--- a/libpod/container_internal_linux.go
+++ b/libpod/container_internal_linux.go
@@ -2490,6 +2490,11 @@ func (c *Container) fixVolumePermissions(v *ContainerNamedVolume) error {
                // https://github.com/containers/podman/issues/10188
                st, err := os.Lstat(filepath.Join(c.state.Mountpoint, v.Dest))
                if err == nil {
+                       if stat, ok := st.Sys().(*syscall.Stat_t); ok {
+                               if err := os.Lchown(mountPoint, int(stat.Uid), int(stat.Gid)); err != nil {
+                                       return err
+                               }
+                       }
                        if err := os.Chmod(mountPoint, st.Mode()|0111); err != nil {
                                return err
                        }

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Good First Issue This issue would be a good issue for a first time contributor to undertake. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants