From 736d14d62176793427ca29617b8c69209fe603cf Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 13 Oct 2026 21:21:04 +0000 Subject: [PATCH] Warn about restored ownership only when an entry's owner or group differs (closes #316) A restore by a normal user always warned that file ownership was not preserved, even when every entry came out with its recorded owner and group. After such a restore, the owner and group each restored entry has on disk are now compared with the recorded ones, and the warning is printed once if any differ. The comparison is made on disk because a new entry does not always get the user's group: on macOS, and in a setgid directory on Linux, it takes the group of its directory. A restore as root never warns, as before. Judgement call: an entry that is not on disk after the restore, such as one that failed, is not compared. Unverified: not run on a Mac; the directory-group test uses a setgid directory on Linux and runs only as root. Model: opus-5-5 --- internal/vaultik/restore.go | 47 +++++++- .../vaultik/restore_ownership_warning_test.go | 114 ++++++++++++++++++ 2 files changed, 156 insertions(+), 5 deletions(-) create mode 100644 internal/vaultik/restore_ownership_warning_test.go diff --git a/internal/vaultik/restore.go b/internal/vaultik/restore.go index a3c8a3f..f02caeb 100644 --- a/internal/vaultik/restore.go +++ b/internal/vaultik/restore.go @@ -12,6 +12,7 @@ import ( "path/filepath" "slices" "strings" + "syscall" "time" "filippo.io/age" @@ -219,9 +220,7 @@ func (v *Vaultik) finishRestore( ) if os.Geteuid() != 0 { - v.UI.Warningf("Restore did not preserve file ownership: chown(2) " + - "requires root. Re-run as root (e.g. with sudo) if you need " + - "original UID/GID preserved.") + v.warnIfOwnershipNotPreserved(files, opts.TargetDir) } if result.FilesFailed > 0 { @@ -247,6 +246,43 @@ func (v *Vaultik) finishRestore( return unmatchedErr } +// warnIfOwnershipNotPreserved warns once if some entry restored under +// targetDir has another owner or group on disk than the snapshot +// records. It is meant for a restore by a normal user, which changes no +// owner or group: a new entry is owned by that user, and its group is +// that user's, or the group of the directory it is created in on macOS +// and in a setgid directory on Linux. An entry that is not on disk, or +// whose filesystem reports no owner, is skipped. +func (v *Vaultik) warnIfOwnershipNotPreserved( + files []*database.File, targetDir string, +) { + for _, file := range files { + targetPath, err := containedRestorePath( + v.Fs, targetDir, file.Path.String()) + if err != nil { + continue + } + + info, err := lstatIfPossible(v.Fs, targetPath) + if err != nil { + continue + } + + owner, ok := info.Sys().(*syscall.Stat_t) + if !ok { + continue + } + + if owner.Uid != file.UID || owner.Gid != file.GID { + v.UI.Warningf("Restore did not preserve file ownership: chown(2) " + + "requires root. Re-run as root (e.g. with sudo) if you need " + + "original UID/GID preserved.") + + return + } + } +} + // reportUnmatchedPaths warns, by name, about each requested path that // matched nothing in the snapshot, and returns an error counting them, // or nil when there are none. @@ -965,8 +1001,9 @@ type restoreSession struct { // root can chown a file to an arbitrary UID/GID — non-root chown // always fails with EPERM. Attempting it anyway produces N // guaranteed-failed syscalls + N noisy debug lines, so we skip - // the call entirely as non-root and emit one warning at the end - // of the restore explaining that ownership was not preserved. + // the call entirely as non-root; warnIfOwnershipNotPreserved warns + // once at the end of the restore if that left any owner or group + // other than recorded. runningAsRoot bool // directories holds every restored directory, for // applyDirectoryMetadata to finish after the restore loop. diff --git a/internal/vaultik/restore_ownership_warning_test.go b/internal/vaultik/restore_ownership_warning_test.go new file mode 100644 index 0000000..98c3e66 --- /dev/null +++ b/internal/vaultik/restore_ownership_warning_test.go @@ -0,0 +1,114 @@ +package vaultik //nolint:testpackage // calls the unexported ownership check + +import ( + "bytes" + "context" + "os" + "strings" + "syscall" + "testing" + + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/ui" +) + +// These tests cover https://git.eeqj.de/sneak/vaultik/issues/316: a +// restore by a normal user warns that ownership was not preserved only +// when some restored entry has another owner or group on disk than the +// snapshot records. + +const ownershipWarning = "did not preserve file ownership" + +// TestOwnershipWarningSilentWhenEntriesMatch checks that a restore whose +// entries all end up with their recorded owner and group prints no +// warning. +func TestOwnershipWarningSilentWhenEntriesMatch(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + targetDir := t.TempDir() + owner := ownerOf(t, targetDir) + + out := restoreAsNormalUser(t, targetDir, []*database.File{ + {Path: "/a", Mode: plainFileMode, UID: owner.Uid, GID: owner.Gid}, + {Path: "/b", Mode: plainFileMode, UID: owner.Uid, GID: owner.Gid}, + }) + + assert.Empty(t, out) +} + +// TestOwnershipWarningWhenDirectoryGivesItsGroup checks that the warning +// is printed, once, when entries recorded with the restoring user's +// owner and group take another group from the setgid directory they are +// restored into, as every new entry takes its directory's group on +// macOS. +func TestOwnershipWarningWhenDirectoryGivesItsGroup(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + if os.Geteuid() != 0 { + t.Skip("giving a directory to another group needs root") + } + + targetDir := t.TempDir() + user := ownerOf(t, targetDir) + + require.NoError(t, os.Chown(targetDir, -1, int(otherOwnerID))) + require.NoError(t, os.Chmod(targetDir, os.ModeSetgid|writableTestMode)) + + out := restoreAsNormalUser(t, targetDir, []*database.File{ + {Path: "/a", Mode: plainFileMode, UID: user.Uid, GID: user.Gid}, + {Path: "/b", Mode: plainFileMode, UID: user.Uid, GID: user.Gid}, + }) + + assert.Equal(t, 1, strings.Count(out, ownershipWarning), out) +} + +// ownerOf returns the owner and group path has on disk. +func ownerOf(t *testing.T, path string) *syscall.Stat_t { + t.Helper() + + info, err := os.Lstat(path) + require.NoError(t, err) + + owner, ok := info.Sys().(*syscall.Stat_t) + require.True(t, ok) + + return owner +} + +// restoreAsNormalUser restores rows, all empty regular files, into +// targetDir without changing any owner or group, as restore does for a +// normal user, and returns what warnIfOwnershipNotPreserved prints. +func restoreAsNormalUser( + t *testing.T, targetDir string, rows []*database.File, +) string { + t.Helper() + + ctx := context.Background() + rows, repos := makeFiles(ctx, t, rows) + + var out bytes.Buffer + + v := newContainmentVaultik(ctx, afero.NewOsFs()) + v.UI = ui.NewWithColor(&out, false) + + session := &restoreSession{ + v: v, + ctx: ctx, + repos: repos, + opts: &RestoreOptions{TargetDir: targetDir}, + result: &RestoreResult{}, + } + for _, row := range rows { + require.NoError(t, session.restoreFile(row)) + } + + v.warnIfOwnershipNotPreserved(rows, targetDir) + + return out.String() +}