merge #592 into opencontainers/umoci:main

Aleksa Sarai (1):
  oci: dir: ensure ownership of new files matches image dir ownership

LGTMs: tych0 cyphar

Author: cyphar

oci/cas/dir/dir.go

@@ -77,10 +77,15 @@ func blobPath(digest digest.Digest) (string, error) {
 	return filepath.Join(blobDirectory, algo.String(), hash), nil
 }
 
+type uidgid struct {
+	uid, gid int
+}
+
 type dirEngine struct {
 	path     string
 	temp     string
 	tempFile *os.File
+	owner    *uidgid
 }
 
 func (e *dirEngine) ensureTempDir() error {
@@ -107,6 +112,26 @@ func (e *dirEngine) ensureTempDir() error {
 	return nil
 }
 
+// chown changes the ownership of the provided file to match the owner of the
+// cas directory itself (in order to avoid a root "umoci repack" creating
+// inaccessible files for the original user).
+func (e *dirEngine) fchown(file *os.File) error {
+	if os.Geteuid() != 0 {
+		return nil // skip chown if not running as root
+	}
+	if e.owner == nil {
+		var stat unix.Stat_t
+		if err := unix.Stat(e.path, &stat); err != nil {
+			return fmt.Errorf("stat cas dir: %w", err)
+		}
+		e.owner = &uidgid{
+			uid: int(stat.Uid),
+			gid: int(stat.Gid),
+		}
+	}
+	return file.Chown(e.owner.uid, e.owner.gid)
+}
+
 // verify ensures that the image is valid.
 func (e *dirEngine) validate() error {
 	content, err := os.ReadFile(filepath.Join(e.path, layoutFile))
@@ -177,6 +202,9 @@ func (e *dirEngine) PutBlob(_ context.Context, reader io.Reader) (_ digest.Diges
 	if err != nil {
 		return "", -1, fmt.Errorf("copy to temporary blob: %w", err)
 	}
+	if err := e.fchown(fh); err != nil {
+		return "", -1, fmt.Errorf("fix ownership of new blob: %w", err)
+	}
 	if err := fh.Sync(); err != nil {
 		return "", -1, fmt.Errorf("sync temporary blob: %w", err)
 	}
@@ -268,6 +296,9 @@ func (e *dirEngine) PutIndex(_ context.Context, index ispec.Index) (Err error) {
 	if err := json.NewEncoder(fh).Encode(index); err != nil {
 		return fmt.Errorf("write temporary index: %w", err)
 	}
+	if err := e.fchown(fh); err != nil {
+		return fmt.Errorf("fix ownership of new index: %w", err)
+	}
 	if err := fh.Sync(); err != nil {
 		return fmt.Errorf("sync temporary index: %w", err)
 	}

test/helpers.bash

@@ -82,6 +82,11 @@ function teardown_tmpdirs() {
 IMAGE="$(setup_tmpdir)/image"
 TAG="${SOURCE_TAG}"
 
+function fail() {
+	echo "FAILURE:" "$@" >&2
+	false
+}
+
 # Allows a test to specify what things it requires. If the environment can't
 # support it, the test is skipped with a message.
 function requires() {
@@ -114,6 +119,17 @@ function image-verify() {
 		}
 		echo $f: valid tar archive
 	done
+
+	# Validate that all inodes are owned by the same uid:gid as the root
+	# directory, which is the correct behaviour for us.
+	owner="$(stat -c "%u:%g" "$ocidir")"
+	allowners="$(find "$ocidir" -print0 | xargs -0 stat -c "%u:%g %n")"
+	if grep -v "^$owner" <<<"$allowners" ; then
+		echo "$allowners"
+		fail "image $ocidir contains subpaths with incorrect owners"
+	fi
+
+	# oci-spec validation
 	oci-image-tool validate --type "imageLayout" "$ocidir"
 	return $?
 }

test/repack.bats

@@ -1055,3 +1055,34 @@ OCI_MEDIATYPE_LAYER="application/vnd.oci.image.layer.v1.tar"
 	[ "$status" -eq 0 ]
 	[[ "$output" == *"application/x-zstd"* ]]
 }
+
+@test "umoci repack [cas file ownership]" {
+	requires root
+
+	# Change the image ownership to a random uid:gid.
+	chown -R 1234:5678 "$IMAGE"
+	image-verify "$IMAGE"
+
+	# Unpack the image.
+	new_bundle_rootfs
+	umoci unpack --image "${IMAGE}:${TAG}" "$BUNDLE"
+	[ "$status" -eq 0 ]
+	bundle-verify "$BUNDLE"
+
+	# Make some changes.
+	touch "$ROOTFS/etc"
+	echo "first file" > "$ROOTFS/newfile"
+	mkdir "$ROOTFS/newdir"
+	echo "subfile" > "$ROOTFS/newdir/anotherfile"
+	ln -s "this is a dummy symlink" "$ROOTFS/newdir/link"
+
+	umoci repack --image "${IMAGE}:${TAG}" --refresh-bundle "$BUNDLE"
+	[ "$status" -eq 0 ]
+	image-verify "$IMAGE"
+
+	# image-verify checks that the ownership is correct, but double-check
+	# explicitly that all of the files are owned by the user we expected.
+	sane_run bats_pipe find "$IMAGE" -print0 \| xargs -0 stat -c "%u:%g %n" \| grep -v "^1234:5678 "
+	[ "$status" -ne 0 ]
+	[ -z "$output" ]
+}