pkg/archive: adjust permissions for parent directories by vasiliy-ul · Pull Request #44140 · moby/moby

vasiliy-ul · 2022-09-13T13:21:11Z

Use 0755 by default if the tar archive does not explicitly provide the mode settings for parent directories.

This change makes the resulting directory permissions aligned with the implementation of the Upack function from pkg/archive/archive.go. Previously this was causing inconsistencies: depending on the configured graphdriver some directories were created with 0600 access bits.

Fixes #44106

- What I did

Changed the default permissions for parent directories in the UnpackLayer function: 0600 -> 0755.

For reference: archive.Unpack already sets the mode to 0755.

- How I did it

- How to verify it

docker run --rm -ti --entrypoint /bin/bash quay.io/kubevirt/virt-launcher:v0.55.0
bash-4.4# ls -la /usr/lib64/qemu-kvm/
total 300
drwxr-xr-x  2 root root  4096 Sep  5 06:12 .
...

- Description for the changelog

The default access mode for unpacked directories is now set to 0755

- A picture of a cute animal (not mandatory but encouraged)

vasiliy-ul · 2022-09-19T06:53:59Z

The failure does not seem related to the change in this PR:

=== CONT  TestClientGatewayIntegration/TestClientGatewayContainerSecurityMode/worker=dockerd/secmode=insecure
    build_test.go:1653: dockerd worker can not currently run this test due to missing features ()
    run.go:197: 
        	Error Trace:	run.go:197
        	            				panic.go:642
        	            				testing.go:858
        	            				testing.go:842
        	            				dockerd.go:208
        	            				build_test.go:1653
        	            				run.go:88
        	            				run.go:202
        	Error:      	Received unexpected error:
        	            	malformed MIME header: missing colon: "Upgrade"
        	            	dockerd grpc conn error
        	            	github.com/moby/buildkit/util/testutil/integration.dockerd.New.func3
        	            		/src/util/testutil/integration/dockerd.go:152
        	            	golang.org/x/sync/errgroup.(*Group).Go.func1
        	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:57
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1581
        	Test:       	TestClientGatewayIntegration/TestClientGatewayContainerSecurityMode/worker=dockerd/secmode=insecure

Flaky test?

thaJeztah · 2022-09-21T08:30:23Z

Looks to be one of the BuildKit integration tests; https://github.com/moby/buildkit/blob/8e2d9b9006caadb74c1745608889a37ba139acc1/client/build_test.go#L1624-L1626

/cc @crazy-max

FWIW; we're still pinning the tests to a specific commit for those integration tests, so it's possible fixes have been merged in BuildKit after that;

moby/.github/workflows/buildkit.yml

Line 80 in 3946173

echo "BUILDKIT_REF=8e2d9b9006caadb74c1745608889a37ba139acc1" >> $GITHUB_ENV

thaJeztah · 2022-09-21T08:33:51Z

@cpuguy83 @tonistiigi PTAL; perhaps one of you knows if there was a specific reason for picking these permissions; also see #44106 (comment), where I did some initial digging.

thaJeztah · 2022-09-22T19:25:15Z

For posterity; we discussed this in the maintainers meeting; so the reason these permissions are applies is that the image itself has no metadata for the parent directories. In that case, docker falls back to creating the parent paths with some default permissions.

For overlay, we hit a different code-path, with similar code, but different default permissions (which coincidentally were updated some time ago in #42016);

moby/pkg/archive/archive.go

Lines 1056 to 1069 in c5c3568

    
           // After calling filepath.Clean(hdr.Name) above, hdr.Name will now be in 
        
           // the filepath format for the OS on which the daemon is running. Hence 
        
           // the check for a slash-suffix MUST be done in an OS-agnostic way. 
        
           if !strings.HasSuffix(hdr.Name, string(os.PathSeparator)) { 
        
           	// Not the root directory, ensure that the parent directory exists 
        
           	parent := filepath.Dir(hdr.Name) 
        
           	parentPath := filepath.Join(dest, parent) 
        
           	if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) { 
        
           		err = idtools.MkdirAllAndChownNew(parentPath, 0755, rootIDs) 
        
           		if err != nil { 
        
           			return err 
        
           		} 
        
           	} 
        
           }

From a consistency perspective, it makes sense to align those permissions (which is what this PR does), but we should consider formalising the behaviour (and permissions) in the OCI specification.

thaJeztah · 2022-09-22T19:32:16Z


 			if _, err := os.Lstat(parentPath); err != nil && os.IsNotExist(err) {
-				err = system.MkdirAll(parentPath, 0600)
+				err = system.MkdirAll(parentPath, 0755)


Discussing further with @neersighted @corhere ; we may want to factor out this block, and also apply ownership (same as we do for overlay)), so that ownership is also consistent (may be important for rootless).

Updated the ownership. Should be the same as for overlay now.

Use 0755 by default if the tar archive does not explicitly provide the mode settings for parent directories. This change makes the permissions and ownership of the resulting directory aligned with the implementation of the Upack function from pkg/archive/archive.go. Previously this was causing inconsistencies: depending on the configured graphdriver some directories were created with 0600 access bits. Signed-off-by: Vasiliy Ulyanov <[email protected]>

vasiliy-ul · 2022-09-23T08:16:48Z

but we should consider formalising the behaviour (and permissions) in the OCI specification

And meanwhile, what about this fix? Does it make sense to make the permissions consistent across various drivers for now?

Also, is it smth I can help with? Just not familiar with the process...

neersighted · 2022-09-23T08:52:59Z

Hi @vasiliy-ul -- we discussed the correct behavior here extensively and came to the conclusion that we knew what we wanted the code to look like, and that it would be easier to just have someone present in the discussion write it than to try and communicate that through prose through multiple cycles.

I'll try to have a successor PR up tomorrow, including your changes, but adding a regression test and some other refinements (e.g. hoisting out this common code shared with archive.go) -- feel free to keep this one open as a comparison until we pick one to merge.

Regarding the OCI spec, I'll summarize the discussion on the linked issue. It's not going to stop us from changing things in Moby to be more consistent, but it's also an important for anyone coming across this work later to understand that this is currently implementation defined, and thus by definition whatever Moby does is correct. While 0600 was surprising, Moby currently could change that value to anything, and builders of images will have to deal with that (by being explicit if they care about the permissions of a directory).

thaJeztah · 2022-09-27T17:02:21Z

Alternative implementation (with the code extracted to a function) is up in #44196

I see @neersighted added @vasiliy-ul as Co-Author (thanks!), so I marked that one as "supersedes" and "closes" this one.

vasiliy-ul mentioned this pull request Sep 13, 2022

Wrong directory permissions with btrfs or devicemapper storage drivers #44106

Closed

thaJeztah added area/storage Image Storage status/2-code-review kind/bugfix PR's that fix bugs labels Sep 21, 2022

thaJeztah added this to the v-next milestone Sep 21, 2022

thaJeztah reviewed Sep 22, 2022

View reviewed changes

vasiliy-ul force-pushed the fix-untar-dir-perms branch from e961014 to dbc0756 Compare September 23, 2022 08:05

neersighted mentioned this pull request Sep 26, 2022

refactor(pkg/archive): factor out createImpliedDirectories helper #44196

Merged

thaJeztah closed this in #44196 Sep 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/archive: adjust permissions for parent directories#44140

pkg/archive: adjust permissions for parent directories#44140
vasiliy-ul wants to merge 1 commit intomoby:masterfrom
vasiliy-ul:fix-untar-dir-perms

vasiliy-ul commented Sep 13, 2022

Uh oh!

vasiliy-ul commented Sep 19, 2022

Uh oh!

thaJeztah commented Sep 21, 2022

Uh oh!

thaJeztah commented Sep 21, 2022

Uh oh!

thaJeztah commented Sep 22, 2022

Uh oh!

thaJeztah Sep 22, 2022

Uh oh!

vasiliy-ul Sep 23, 2022

Uh oh!

vasiliy-ul commented Sep 23, 2022

Uh oh!

neersighted commented Sep 23, 2022 •

edited

Loading

Uh oh!

thaJeztah commented Sep 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

vasiliy-ul commented Sep 13, 2022

Uh oh!

vasiliy-ul commented Sep 19, 2022

Uh oh!

thaJeztah commented Sep 21, 2022

Uh oh!

thaJeztah commented Sep 21, 2022

Uh oh!

thaJeztah commented Sep 22, 2022

Uh oh!

thaJeztah Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

vasiliy-ul Sep 23, 2022

Choose a reason for hiding this comment

Uh oh!

vasiliy-ul commented Sep 23, 2022

Uh oh!

neersighted commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Sep 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

neersighted commented Sep 23, 2022 •

edited

Loading