Skip to content

Fix folder access and mount problems with image volumes #951

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.

Sign up for GitHub

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

Closed
marcov wants to merge 1 commit into from
Closed

Fix folder access and mount problems with image volumes #951

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 41 additions & 3 deletions libpod/container_internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1361,19 +1361,57 @@ func (c *Container) addImageVolumes(ctx context.Context, g *generate.Generator)
continue
}
volumePath := filepath.Join(c.config.StaticDir, "volumes", k)
if _, err := os.Stat(volumePath); os.IsNotExist(err) {
srcPath := filepath.Join(mountPoint, k)

if _, err := os.Stat(srcPath); os.IsNotExist(err) {
logrus.Infof("Volume image mount point %s does not exist in root FS, need to create it", k)
if err = os.MkdirAll(volumePath, 0755); err != nil {
return errors.Wrapf(err, "error creating directory %q for volume %q in container %q", volumePath, k, c.ID)
}

if c.config.User != "" {
if !c.state.Mounted {
return errors.Wrapf(ErrCtrStateInvalid, "container %s must be mounted in order to translate User field", c.ID())
}
uid, gid, err := chrootuser.GetUser(c.state.Mountpoint, c.config.User)
if err != nil {
return err
}

if err = os.Chown(volumePath, int(uid), int(gid)); err != nil {
Copy link
Copy Markdown
Member

@mheon mheon Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))

Copy link
Copy Markdown
Collaborator Author

@marcov marcov Jun 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@rhatdan rhatdan Jun 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Collaborator Author

@marcov marcov Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Copy Markdown
Member

@rhatdan rhatdan Jun 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mheon Does that look good to you?

return errors.Wrapf(err, "error chowning directory %q for volume %q in container %q", volumePath, k, c.ID)
}
}
}

if _, err := os.Stat(volumePath); os.IsNotExist(err) {

if err = label.Relabel(volumePath, c.config.MountLabel, false); err != nil {
return errors.Wrapf(err, "error relabeling directory %q for volume %q in container %q", volumePath, k, c.ID)
}
srcPath := filepath.Join(mountPoint, k)
if err = chrootarchive.NewArchiver(nil).CopyWithTar(srcPath, volumePath); err != nil && !os.IsNotExist(err) {
return errors.Wrapf(err, "error populating directory %q for volume %q in container %q using contents of %q", volumePath, k, c.ID, srcPath)
}
mount.Source = volumePath

// Set the volume path with the same owner and permission of source path
sstat, _ := os.Stat(srcPath)
st, ok := sstat.Sys().(*syscall.Stat_t)
if !ok {
return fmt.Errorf("could not convert to syscall.Stat_t")
}
uid := int(st.Uid)
gid := int(st.Gid)

if err := os.Lchown(volumePath, uid, gid); err != nil {
return err
}
if os.Chmod(volumePath, sstat.Mode()); err != nil {
return err
}

}

mount.Source = volumePath
g.AddMount(mount)
}
return nil
Expand Down