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() +}