Merge pull request #3840 from giuseppe/create-workdir-with-user

buildah: create WORKDIR with USER permissions

add.go

@@ -655,3 +655,37 @@ func (b *Builder) userForCopy(mountPoint string, userspec string) (uint32, uint3
 	}
 	return owner.UID, owner.GID, nil
 }
+
+// EnsureContainerPathAs creates the specified directory owned by USER
+// with the file mode set to MODE.
+func (b *Builder) EnsureContainerPathAs(path, user string, mode *os.FileMode) error {
+	mountPoint, err := b.Mount(b.MountLabel)
+	if err != nil {
+		return err
+	}
+	defer func() {
+		if err2 := b.Unmount(); err2 != nil {
+			logrus.Errorf("error unmounting container: %v", err2)
+		}
+	}()
+
+	uid, gid := uint32(0), uint32(0)
+	if user != "" {
+		if uidForCopy, gidForCopy, err := b.userForCopy(mountPoint, user); err == nil {
+			uid = uidForCopy
+			gid = gidForCopy
+		}
+	}
+
+	destUIDMap, destGIDMap := convertRuntimeIDMaps(b.IDMappingOptions.UIDMap, b.IDMappingOptions.GIDMap)
+
+	idPair := &idtools.IDPair{UID: int(uid), GID: int(gid)}
+	opts := copier.MkdirOptions{
+		ChmodNew: mode,
+		ChownNew: idPair,
+		UIDMap:   destUIDMap,
+		GIDMap:   destGIDMap,
+	}
+	return copier.Mkdir(mountPoint, filepath.Join(mountPoint, path), opts)
+
+}

copier/copier.go

@@ -1466,6 +1466,13 @@ func copierHandlerGetOne(srcfi os.FileInfo, symlinkTarget, name, contentPath str
 			return errors.Wrapf(err, "error opening file for adding its contents to archive")
 		}
 		defer f.Close()
+	} else if hdr.Typeflag == tar.TypeDir {
+		// open the directory file first to make sure we can access it.
+		f, err = os.Open(contentPath)
+		if err != nil {
+			return errors.Wrapf(err, "error opening directory for adding its contents to archive")
+		}
+		defer f.Close()
 	}
 	// output the header
 	if err = tw.WriteHeader(hdr); err != nil {

go.mod

@@ -27,7 +27,7 @@ require (
 	github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417
 	github.com/opencontainers/runtime-tools v0.9.0
 	github.com/opencontainers/selinux v1.10.0
-	github.com/openshift/imagebuilder v1.2.2
+	github.com/openshift/imagebuilder v1.2.3
 	github.com/pkg/errors v0.9.1
 	github.com/prometheus/client_golang v1.11.1 // indirect
 	github.com/seccomp/libseccomp-golang v0.9.2-0.20210429002308-3879420cc921

go.sum

@@ -878,8 +878,8 @@ github.com/opencontainers/selinux v1.8.2/go.mod h1:MUIHuUEvKB1wtJjQdOyYRgOnLD2xA
 github.com/opencontainers/selinux v1.8.5/go.mod h1:HTvjPFoGMbpQsG886e3lQwnsRWtE4TC1OF3OUvG9FAo=
 github.com/opencontainers/selinux v1.10.0 h1:rAiKF8hTcgLI3w0DHm6i0ylVVcOrlgR1kK99DRLDhyU=
 github.com/opencontainers/selinux v1.10.0/go.mod h1:2i0OySw99QjzBBQByd1Gr9gSjvuho1lHsJxIJ3gGbJI=
-github.com/openshift/imagebuilder v1.2.2 h1:++jWWMkTVJKP2MIjTPaTk2MqwWIOYYlDaQbZyLlLBh0=
-github.com/openshift/imagebuilder v1.2.2/go.mod h1:TRYHe4CH9U6nkDjxjBNM5klrLbJBrRbpJE5SaRwUBsQ=
+github.com/openshift/imagebuilder v1.2.3 h1:jvA7mESJdclRKkTe3Yl6UWlliFNVW6mLY8RI+Rrfhfo=
+github.com/openshift/imagebuilder v1.2.3/go.mod h1:TRYHe4CH9U6nkDjxjBNM5klrLbJBrRbpJE5SaRwUBsQ=
 github.com/opentracing/opentracing-go v1.1.0/go.mod h1:UkNAQd3GIcIGf0SeVgPpRdFStlNbqXla1AfSYxPUl2o=
 github.com/ostreedev/ostree-go v0.0.0-20190702140239-759a8c1ac913/go.mod h1:J6OG6YJVEWopen4avK3VNQSnALmmjvniMmni/YFYAwc=
 github.com/ostreedev/ostree-go v0.0.0-20210805093236-719684c64e4f h1:/UDgs8FGMqwnHagNDPGOlts35QkhAZ8by3DR7nMih7M=

imagebuildah/stage_executor.go

@@ -1570,5 +1570,9 @@ func (s *StageExecutor) commit(ctx context.Context, createdBy string, emptyLayer
 }
 
 func (s *StageExecutor) EnsureContainerPath(path string) error {
-	return copier.Mkdir(s.mountPoint, filepath.Join(s.mountPoint, path), copier.MkdirOptions{})
+	return s.builder.EnsureContainerPathAs(path, "", nil)
+}
+
+func (s *StageExecutor) EnsureContainerPathAs(path, user string, mode *os.FileMode) error {
+	return s.builder.EnsureContainerPathAs(path, user, mode)
 }

run_linux.go

@@ -191,16 +191,19 @@ func (b *Builder) Run(command []string, options RunOptions) error {
 		return err
 	}
 
-	// Figure out who owns files that will appear to be owned by UID/GID 0 in the container.
-	rootUID, rootGID, err := util.GetHostRootIDs(spec)
-	if err != nil {
-		return err
+	uid, gid := spec.Process.User.UID, spec.Process.User.GID
+	if spec.Linux != nil {
+		uid, gid, err = util.GetHostIDs(spec.Linux.UIDMappings, spec.Linux.GIDMappings, uid, gid)
+		if err != nil {
+			return err
+		}
 	}
-	rootIDPair := &idtools.IDPair{UID: int(rootUID), GID: int(rootGID)}
+
+	idPair := &idtools.IDPair{UID: int(uid), GID: int(gid)}
 
 	mode := os.FileMode(0755)
 	coptions := copier.MkdirOptions{
-		ChownNew: rootIDPair,
+		ChownNew: idPair,
 		ChmodNew: &mode,
 	}
 	if err := copier.Mkdir(mountPoint, filepath.Join(mountPoint, spec.Process.Cwd), coptions); err != nil {
@@ -211,6 +214,13 @@ func (b *Builder) Run(command []string, options RunOptions) error {
 	namespaceOptions := append(b.NamespaceOptions, options.NamespaceOptions...)
 	volumes := b.Volumes()
 
+	// Figure out who owns files that will appear to be owned by UID/GID 0 in the container.
+	rootUID, rootGID, err := util.GetHostRootIDs(spec)
+	if err != nil {
+		return err
+	}
+	rootIDPair := &idtools.IDPair{UID: int(rootUID), GID: int(rootGID)}
+
 	if !options.NoHosts && !contains(volumes, "/etc/hosts") {
 		hostFile, err := b.generateHosts(path, spec.Hostname, b.CommonBuildOpts.AddHost, rootIDPair)
 		if err != nil {

tests/bud.bats

@@ -4003,3 +4003,11 @@ _EOF
     expect_output --substring "10.88."
   fi
 }
+
+@test "bud WORKDIR owned by USER" {
+  _prefetch alpine
+  target=alpine-image
+  ctr=alpine-ctr
+  run_buildah build --signature-policy ${TESTSDIR}/policy.json -t ${target} ${TESTSDIR}/bud/workdir-user
+  expect_output --substring "1000:1000 /home/http/public"
+}

tests/bud/namespaces/Containerfile

@@ -7,3 +7,6 @@ RUN echo "StatSomefileResult" && stat -c '%u:%g' /somefile
 COPY somedir /somedir
 RUN echo "StatSomedirResult" && stat -c '%u:%g' /somedir
 RUN echo "StatSomeotherfileResult" && stat -c '%u:%g %a' /somedir/someotherfile
+USER guest
+WORKDIR /new-workdir
+RUN echo "StatNewWorkdir" && stat -c '%U:%G' $PWD

tests/bud/workdir-user/Dockerfile

@@ -0,0 +1,6 @@
+FROM alpine
+RUN adduser -D http -h /home/http
+USER http
+WORKDIR /home/http/public
+RUN stat -c '%u:%g %n' $PWD
+RUN touch foobar

tests/conformance/conformance_test.go

@@ -2959,4 +2959,16 @@ var internalTestCases = []testCase{
 		dockerfile: "Dockerfile.4",
 		fsSkip:     []string{"(dir):tree:mtime"},
 	},
+
+	{
+		name: "workdir-owner", // from issue #3620
+		dockerfileContents: strings.Join([]string{
+			`FROM alpine`,
+			`USER daemon`,
+			`WORKDIR /created/directory`,
+			`RUN ls /created`,
+		}, "\n"),
+		fsSkip:            []string{"(dir):created:mtime", "(dir):created:(dir):directory:mtime"},
+		dockerUseBuildKit: true,
+	},
 }

tests/namespaces.bats

@@ -262,9 +262,11 @@ idmapping_check_permission() {
     # Check that if we copy a directory into the container, its contents get the right permissions.
     output_dir_stat="$(grep -A1 'StatSomedirResult' <<< "$output" | tail -n1)"
     output_otherfile_stat="$(grep -A1 'StatSomeotherfileResult' <<< "$output" | tail -n1)"
+    output_workdir_stat="$(grep -A1 'StatNewWorkdir' <<< "$output" | tail -n1)"
     # bud strips suid.
     idmapping_check_permission "$output_file_stat" "$output_dir_stat"
     expect_output --from="${output_otherfile_stat}" "0:0 700" "stat(someotherfile), in bud test"
+    expect_output --from="${output_workdir_stat}" "guest:users" "stat(new-workdir), in bud test"
   done
 }