Tighten BestEffortLchown to only swallow permission errors

Previously, any non-permission error from os.Lchown was logged
at warn level and then silently dropped — a failure mode where
the file's owner silently ended up wrong while the caller saw
no error. Tighten the contract so only permission-denied errors
(the expected non-root case) are absorbed; everything else
(ENOENT, EROFS, EIO, ...) propagates to the caller.

Tests cover both paths: a non-existent path returning ENOENT
surfaces an error; a non-root chown to a foreign UID returning
EPERM returns nil. The EPERM test is skipped when running as
root since root can chown to any UID.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: JAORMX

hooks/hooks.go

@@ -211,20 +211,21 @@ func shellEscape(s string) string {
 	return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
 }
 
-// BestEffortLchown attempts os.Lchown and silently ignores permission errors,
-// returning nil. On macOS non-root users cannot chown to a different UID;
-// the guest init will fix ownership at boot time. Non-permission errors are
-// logged at warn level and also swallowed. Callers that need strict chown
-// should call os.Lchown directly instead.
+// BestEffortLchown attempts os.Lchown and swallows permission errors (EPERM
+// and EACCES). On non-root Linux and on macOS the hook process cannot lchown
+// to a different UID; in those cases the override_stat xattr carries the
+// intended ownership to the guest, and the guest init fixes up ownership at
+// boot. Errors other than permission denied (e.g. ENOENT, EROFS, EIO) are
+// returned to the caller rather than silently dropped.
 // Lchown is used instead of Chown to avoid following symlinks in the rootfs.
 func BestEffortLchown(path string, uid, gid int) error {
 	if err := os.Lchown(path, uid, gid); err != nil {
 		if !os.IsPermission(err) {
-			slog.Warn("lchown failed", "path", path, "uid", uid, "gid", gid, "err", err)
+			return fmt.Errorf("lchown %s: %w", path, err)
 		}
+		slog.Debug("lchown permission denied; relying on xattr + guest fixup",
+			"path", path, "uid", uid, "gid", gid)
 	}
-	// On macOS, set the override_stat xattr so libkrun's virtiofs reports
-	// correct ownership to the guest (non-root cannot Lchown to a different UID).
 	xattr.SetOverrideStatFromPath(path, uid, gid)
 	return nil
 }

hooks/hooks_test.go

@@ -658,6 +658,34 @@ func TestStageSymlink(t *testing.T) {
 	assert.Equal(t, outside, dest)
 }
 
+func TestBestEffortLchown_PropagatesNonPermissionErrors(t *testing.T) {
+	t.Parallel()
+
+	// ENOENT from a non-existent path is not a permission error; the function
+	// must return it rather than silently swallowing.
+	missing := filepath.Join(t.TempDir(), "does-not-exist")
+	err := BestEffortLchown(missing, 1000, 1000)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "lchown")
+}
+
+func TestBestEffortLchown_SwallowsPermissionErrors(t *testing.T) {
+	t.Parallel()
+
+	if os.Geteuid() == 0 {
+		t.Skip("requires non-root: root can chown to any UID, so no EPERM")
+	}
+
+	// Create a file we own; chowning to a UID we don't own should fail with
+	// EPERM on Linux and macOS as a non-root user. BestEffortLchown must
+	// swallow that specific error.
+	target := filepath.Join(t.TempDir(), "target")
+	require.NoError(t, os.WriteFile(target, []byte("x"), 0o600))
+
+	err := BestEffortLchown(target, 1, 1)
+	require.NoError(t, err, "permission error must be swallowed, got: %v", err)
+}
+
 func TestShellEscape(t *testing.T) {
 	t.Parallel()