Fix file capabilites droping in Dockerfile by abdulrahimiliasu · Pull Request #42934 · moby/moby

abdulrahimiliasu · 2021-10-13T15:41:55Z

- What I did
moved copyXattr function out of doCopyXattrs function, so that security capabilities are copied
- How I did it
doCopyXattrs() never reached due to copyXattrs boolean being false, as a result file capabilities not being copied.

- How to verify it

Test Case

FROM registry1-docker-io.repo.lab.pl.alcatel-lucent.com/library/alpine:latest
RUN apk --no-cache add libcap && setcap cap_net_admin=eip /sbin/apk
RUN setcap -v cap_net_admin=eip /sbin/apk

Test Result

Sending build context to Docker daemon  2.048kB
Step 1/3 : FROM registry1-docker-io.repo.lab.pl.alcatel-lucent.com/library/alpine:latest
latest: Pulling from library/alpine
a0d0a0d46f8b: Pull complete
Digest: sha256:e1c082e3d3c45cccac829840a25941e679c25d438cc8412c2fa221cf1a824e6a
Status: Downloaded newer image for registry1-docker-io.repo.lab.pl.alcatel-lucent.com/library/alpine:latest
 ---> 14119a10abf4
Step 2/3 : RUN apk --no-cache add libcap && setcap cap_net_admin=eip /sbin/apk
 ---> Running in 923b46395907
fetch https://dl-cdn.alpinelinux.org/alpine/v3.14/main/x86_64/APKINDEX.tar.gz
fetch https://dl-cdn.alpinelinux.org/alpine/v3.14/community/x86_64/APKINDEX.tar.gz
(1/1) Installing libcap (2.50-r0)
Executing busybox-1.33.1-r3.trigger
OK: 6 MiB in 15 packages
Removing intermediate container 923b46395907
 ---> 97d6ec51d4b3
Step 3/3 : RUN setcap -v cap_net_admin=eip /sbin/apk
 ---> Running in 33260d42c77d
/sbin/apk: OK
Removing intermediate container 33260d42c77d
 ---> 5fc3c8660150
Successfully built 5fc3c8660150
Successfully tagged test:cap

- Description for the changelog
Fixed issue of file capabilities dropping when moving to next command in Dockerfile during image building.

thaJeztah · 2021-10-13T16:02:18Z

doCopyXattrs() never reached due to copyXattrs boolean being false

A mentioned on #42655 (comment), the copyXattrs option was explicitly added at some point. This change looks to work around that by ignoring the option, which feels incorrect;

I see this line explicitly disables copyXattrs (last argument);

moby/daemon/graphdriver/vfs/copy_linux.go

Line 6 in 7f5e39b

return copy.DirCopy(srcDir, dstDir, copy.Content, false)

That was added in d2b71b2 (part of #35537). Before that it seems like it was enabled by default.

Perhaps @sargun or @cpuguy83 recalls why it's disabled for vfs; perhaps the expectation was that the copy_file_range would/should take care of this?

If this code must be called for VFS, I guess the boolean should be set to true, but we should have an understanding on why it was disabled instead of ignoring

abdulrahimiliasu · 2021-10-14T13:08:43Z

That's correct the Boolean should be set to true for those capabilities to be copied, but this change only makes sure that the
"security-capability" attribute are copied. Everything else stay the same. But I do not know the specific reason for why copyXattrs default to false.

szubersk · 2021-11-06T09:34:02Z

@thaJeztah I can't speak for @sargun but the feeling I get is that copying xattrs has been removed to speed copying up. d2b71b2 Have VFS graphdriver use accelerated in-kernel copy introduces two major optimizations: copy_file_range usage to transfer file content without switching from kernel- to userspace and back and xattrs ignoring.

copy_file_range can't take care of file attributes (or any other metadata) as it operates only on file content.

ssize_t copy_file_range(int fd_in, off64_t *off_in,
                        int fd_out, off64_t *off_out,
                        size_t len, unsigned int flags);

copyXattr requires one getxattr(2) call per file/directory + memory allocation (sometimes two) to store the result. No wonder @sargun was looking for an optimization there.

moby/daemon/graphdriver/copy/copy.go

Lines 90 to 91 in c09789c

    
           func copyXattr(srcPath, dstPath, attr string) error { 
        
           	data, err := system.Lgetxattr(srcPath, attr)

moby/pkg/system/xattrs_linux.go

Lines 8 to 11 in c09789c

    
           func Lgetxattr(path string, attr string) ([]byte, error) { 
        
           	// Start with a 128 length byte array 
        
           	dest := make([]byte, 128) 
        
           	sz, errno := unix.Lgetxattr(path, attr, dest)

I'd lean towards suggestion of unconditionally enabling xattrs copying as current implementation is simply incorrect.

abdulrahimiliasu · 2021-11-10T13:25:58Z

I've updated the commit by changing copyXattrs to true instead of moving the block

moby/daemon/graphdriver/copy/copy.go

Lines 259 to 261 in 33a3680

    
           if err := copyXattr(srcPath, dstPath, "security.capability"); err != nil { 
        
           	return err 
        
           }

out of

moby/daemon/graphdriver/copy/copy.go

Line 258 in 33a3680

func doCopyXattrs(srcPath, dstPath string) error {

daemon/graphdriver/copy/copy.go

sargun · 2021-11-10T20:41:50Z

You should only copy the security.capability xattr in the VFS graphdriver, and not the trusted.overlay.opaque one.

szubersk · 2021-11-16T08:08:46Z

@sargun @thaJeztah Is the PR in current form ok with you?

szubersk · 2022-01-04T13:03:09Z

@sargun @thaJeztah
Is there an issue that prevent merging this PR?

mikebrow

nit.. comments for the modified DirCopy() func and field name copyXattrs should be modified to reflect the boolean is just for switching off copying xattr trusted.overlay.opaque

mikebrow

LGTM functionally.. just the nit comment regarding variable naming and existing api // comments

doCopyXattrs() never reached due to copyXattrs boolean being false, as a result file capabilities not being copied. moved copyXattr() out of doCopyXattrs() Signed-off-by: Illo Abdulrahim <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2022-07-27T15:22:50Z

I rebased the PR, renamed the copyXattrs argument to copyOpaqueXattrs, and updated the GoDoc for CopyDir()

thaJeztah

LGTM, thanks! And sorry for the long delay; this one dropped of my radar, and I saw the PR was still marked with "missing DCO", but that looks to have been resolved since.

mikebrow

LGTM

thaJeztah added area/storage Image Storage dco/no Automatically set by a bot when one of the commits lacks proper signature labels Oct 13, 2021

abdulrahimiliasu force-pushed the 42655-vfs-storage-driver branch from ae672cd to 97ec8da Compare November 10, 2021 12:54

szubersk suggested changes Nov 10, 2021

View reviewed changes

daemon/graphdriver/copy/copy.go Show resolved Hide resolved

abdulrahimiliasu force-pushed the 42655-vfs-storage-driver branch 2 times, most recently from 674ddc7 to 2509ff0 Compare November 11, 2021 17:14

szubersk approved these changes Nov 16, 2021

View reviewed changes

abdulrahimiliasu force-pushed the 42655-vfs-storage-driver branch from 2e2c06e to c5a883b Compare November 16, 2021 10:01

abdulrahimiliasu requested review from tianon and tonistiigi as code owners November 16, 2021 10:01

abdulrahimiliasu force-pushed the 42655-vfs-storage-driver branch from c5a883b to b99128e Compare November 16, 2021 15:53

mikebrow reviewed Jul 20, 2022

View reviewed changes

mikebrow approved these changes Jul 20, 2022

View reviewed changes

thaJeztah force-pushed the 42655-vfs-storage-driver branch from b99128e to a73e49d Compare July 27, 2022 15:18

thaJeztah added status/2-code-review process/cherry-pick kind/bugfix PR's that fix bugs and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Jul 27, 2022

thaJeztah added this to the v-next milestone Jul 27, 2022

thaJeztah force-pushed the 42655-vfs-storage-driver branch from a73e49d to 31f654a Compare July 27, 2022 15:21

thaJeztah approved these changes Jul 27, 2022

View reviewed changes

mikebrow approved these changes Jul 27, 2022

View reviewed changes

cpuguy83 approved these changes Jul 27, 2022

View reviewed changes

cpuguy83 merged commit 9772c68 into moby:master Jul 27, 2022

This was referenced Jul 28, 2022

[22.06 backport] Fix file capabilities dropping in Dockerfile #43875

Merged

[20.10 backport] Fix file capabilities dropping in Dockerfile #43876

Merged

Improve documentation, and verify consistency for xattr (extended attributes) support #43882

Open

thaJeztah added process/cherry-picked and removed process/cherry-pick labels Aug 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix file capabilites droping in Dockerfile#42934

Fix file capabilites droping in Dockerfile#42934
cpuguy83 merged 1 commit intomoby:masterfrom
abdulrahimiliasu:42655-vfs-storage-driver

abdulrahimiliasu commented Oct 13, 2021 •

edited

Loading

Uh oh!

thaJeztah commented Oct 13, 2021

Uh oh!

abdulrahimiliasu commented Oct 14, 2021 •

edited

Loading

Uh oh!

szubersk commented Nov 6, 2021

Uh oh!

abdulrahimiliasu commented Nov 10, 2021 •

edited

Loading

Uh oh!

Uh oh!

sargun commented Nov 10, 2021

Uh oh!

szubersk commented Nov 16, 2021

Uh oh!

szubersk commented Jan 4, 2022

Uh oh!

mikebrow left a comment

Uh oh!

mikebrow left a comment •

edited

Loading

Uh oh!

thaJeztah commented Jul 27, 2022

Uh oh!

thaJeztah left a comment

Uh oh!

mikebrow left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

abdulrahimiliasu commented Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Oct 13, 2021

Uh oh!

abdulrahimiliasu commented Oct 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szubersk commented Nov 6, 2021

Uh oh!

abdulrahimiliasu commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sargun commented Nov 10, 2021

Uh oh!

szubersk commented Nov 16, 2021

Uh oh!

szubersk commented Jan 4, 2022

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 27, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

mikebrow left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

abdulrahimiliasu commented Oct 13, 2021 •

edited

Loading

abdulrahimiliasu commented Oct 14, 2021 •

edited

Loading

abdulrahimiliasu commented Nov 10, 2021 •

edited

Loading

mikebrow left a comment •

edited

Loading