From 3edc1889e3fbecc55494db2a791df5d3d5b26e90 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 6 Oct 2026 12:57:27 +0000 Subject: [PATCH] Apply restored owners, modes and times in an order that keeps them (closes #219) A directory got its stored mode and mtime before its contents were written, so a read-only directory came back without its files and a non-empty one carried the time of the restore. Directories are now created owner-only (0700) and get their stored owner, mode and mtime after the restore loop, deepest first, so a parent that denies search does not block its children. A file's mode is now applied after its chown, which on Linux clears setuid and setgid. A symlink gets its stored owner (as root) and mtime on the link itself, through golang.org/x/sys/unix, now a direct dependency. An interrupted restore leaves its directories at 0700. Model: opus-5-5 --- TODO.md | 11 ++ go.mod | 2 +- internal/vaultik/restore.go | 90 +++++++-- internal/vaultik/restore_metadata_test.go | 225 ++++++++++++++++++++++ 4 files changed, 311 insertions(+), 17 deletions(-) create mode 100644 internal/vaultik/restore_metadata_test.go diff --git a/TODO.md b/TODO.md index a59deca..78a5e58 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,17 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-06: Made restore apply owners, modes and times in an order + that keeps them + ([issue #219](https://git.eeqj.de/sneak/vaultik/issues/219)). A + directory got its stored mode and mtime before its contents were + written, so a read-only directory came back without its files and a + non-empty one carried the time of the restore. Directories are now + created owner-only and get their stored owner, mode and mtime once the + restore loop is done, deepest first. A file's mode is applied after + its chown, which on Linux clears setuid and setgid, and a symlink gets + its stored owner (as root) and mtime on the link itself. + - 2026-10-06: Made `snapshot restore --skip-errors` skip the files that need a blob it cannot download ([issue #218](https://git.eeqj.de/sneak/vaultik/issues/218)). A missing diff --git a/go.mod b/go.mod index 23e63f2..06e5ff1 100644 --- a/go.mod +++ b/go.mod @@ -23,6 +23,7 @@ require ( github.com/stretchr/testify v1.11.1 go.uber.org/fx v1.24.0 golang.org/x/sync v0.18.0 + golang.org/x/sys v0.38.0 golang.org/x/term v0.37.0 gopkg.in/yaml.v3 v3.0.1 modernc.org/sqlite v1.38.0 @@ -263,7 +264,6 @@ require ( golang.org/x/exp v0.0.0-20251023183803-a4bb9ffd2546 // indirect golang.org/x/net v0.47.0 // indirect golang.org/x/oauth2 v0.33.0 // indirect - golang.org/x/sys v0.38.0 // indirect golang.org/x/text v0.31.0 // indirect golang.org/x/time v0.14.0 // indirect golang.org/x/tools v0.38.0 // indirect diff --git a/internal/vaultik/restore.go b/internal/vaultik/restore.go index f8372d6..7b4ec50 100644 --- a/internal/vaultik/restore.go +++ b/internal/vaultik/restore.go @@ -10,11 +10,13 @@ import ( "math" "os" "path/filepath" + "slices" "strings" "time" "filippo.io/age" "github.com/spf13/afero" + "golang.org/x/sys/unix" "sneak.berlin/go/vaultik/internal/blobgen" "sneak.berlin/go/vaultik/internal/database" "sneak.berlin/go/vaultik/internal/log" @@ -63,6 +65,12 @@ const snapshotDBFilename = "snapshot.db" // directories themselves get their stored mode). const restoreDirMode = 0o755 +// restoreDirCreateMode is the owner-only mode a directory from the +// snapshot is created with during restore, so its contents can be written +// whatever its stored mode. The stored mode is applied after the restore +// loop, by applyDirectoryMetadata. +const restoreDirCreateMode = 0o700 + // restoreFileMode is the restrictive mode a regular file is created with // during restore. Content is written while the file holds this mode; the // stored mode is applied only after the file is fully written and closed, @@ -332,6 +340,8 @@ func (v *Vaultik) restoreAllFiles( return nil, err } + session.applyDirectoryMetadata() + return result, nil } @@ -901,6 +911,9 @@ type restoreSession struct { // the call entirely as non-root and emit one warning at the end // of the restore explaining that ownership was not preserved. runningAsRoot bool + // directories holds every restored directory, for + // applyDirectoryMetadata to finish after the restore loop. + directories []*database.File } // containedRestorePath resolves rel — a path read from the snapshot @@ -1007,6 +1020,8 @@ func (s *restoreSession) restoreSymlink(file *database.File, targetPath string) if err != nil { return fmt.Errorf("creating symlink: %w", err) } + + s.applySymlinkMetadata(file, targetPath) } else { log.Debug("Symlink creation not supported on this filesystem", "path", file.Path, "target", file.LinkTarget) @@ -1019,35 +1034,78 @@ func (s *restoreSession) restoreSymlink(file *database.File, targetPath string) return nil } -// restoreDirectory restores a directory with its permissions, mtime, -// and (on real filesystems, with sufficient privileges) ownership. +// restoreDirectory creates a directory with restoreDirCreateMode. Its +// stored mode, owner and mtime are applied after the restore loop by +// applyDirectoryMetadata: a read-only stored mode would block writing +// its contents, and writing them changes its mtime. func (s *restoreSession) restoreDirectory( file *database.File, targetPath string, ) error { - err := s.v.Fs.MkdirAll(targetPath, os.FileMode(file.Mode)) + err := s.v.Fs.MkdirAll(targetPath, restoreDirCreateMode) if err != nil { return fmt.Errorf("creating directory: %w", err) } - // MkdirAll applies the process umask, so chmod to the exact stored - // mode. A failure here is non-fatal. - err = s.v.Fs.Chmod(targetPath, os.FileMode(file.Mode)) - if err != nil { - log.Debug("Failed to set permissions", "path", targetPath, "error", err) - } - - s.applyFileMetadata(file, targetPath) + s.directories = append(s.directories, file) s.result.FilesRestored++ return nil } +// applyDirectoryMetadata applies the stored owner, mtime and mode to every +// restored directory, deepest first, so a parent whose stored mode denies +// search does not block its children. Failures are logged at debug level +// and do not abort the restore. +func (s *restoreSession) applyDirectoryMetadata() { + // A path sorts after its parent's, so reverse order is deepest first. + slices.SortFunc(s.directories, func(a, b *database.File) int { + return strings.Compare(b.Path.String(), a.Path.String()) + }) + + for _, dir := range s.directories { + targetPath, err := containedRestorePath( + s.v.Fs, s.opts.TargetDir, dir.Path.String()) + if err != nil { + log.Debug("Failed to set directory metadata", + "path", dir.Path, "error", err) + + continue + } + + s.applyFileMetadata(dir, targetPath) + + err = s.v.Fs.Chmod(targetPath, os.FileMode(dir.Mode)) + if err != nil { + log.Debug("Failed to set permissions", "path", targetPath, "error", err) + } + } +} + +// applySymlinkMetadata applies ownership (when running as root) and mtime +// to a restored symlink itself; os.Chown and Chtimes would follow it. +// Failures are logged at debug level and do not abort the restore. +func (s *restoreSession) applySymlinkMetadata(file *database.File, targetPath string) { + if s.runningAsRoot { + err := os.Lchown(targetPath, int(file.UID), int(file.GID)) + if err != nil { + log.Debug("Failed to set ownership", "path", targetPath, "error", err) + } + } + + mtime := unix.NsecToTimeval(file.MTime.UnixNano()) + + err := unix.Lutimes(targetPath, []unix.Timeval{mtime, mtime}) + if err != nil { + log.Debug("Failed to set mtime", "path", targetPath, "error", err) + } +} + // applyFileMetadata applies ownership (when running as root on a real -// filesystem) and mtime to a restored path. Permission mode is applied -// separately by each caller, with different failure handling, so it is -// not touched here. Failures are logged at debug level and do not abort -// the restore. +// filesystem) and mtime to a restored path. The caller applies the mode +// afterwards: on Linux a chown clears the setuid and setgid bits of a +// regular file. Failures are logged at debug level and do not abort the +// restore. func (s *restoreSession) applyFileMetadata(file *database.File, targetPath string) { if s.runningAsRoot { if _, ok := s.v.Fs.(*afero.OsFs); ok { @@ -1138,8 +1196,8 @@ func (s *restoreSession) restoreRegularFile( return fmt.Errorf("closing output file: %w", err) } - s.applyRestoredFileMode(file, targetPath) s.applyFileMetadata(file, targetPath) + s.applyRestoredFileMode(file, targetPath) s.result.FilesRestored++ s.result.BytesRestored += bytesWritten diff --git a/internal/vaultik/restore_metadata_test.go b/internal/vaultik/restore_metadata_test.go new file mode 100644 index 0000000..2293bcb --- /dev/null +++ b/internal/vaultik/restore_metadata_test.go @@ -0,0 +1,225 @@ +package vaultik //nolint:testpackage // drives unexported restore internals + +import ( + "context" + "os" + "path/filepath" + "syscall" + "testing" + "time" + + "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/types" +) + +// These tests check that restore applies each entry's owner, mode and +// mtime in an order that keeps them. They are written to fail as a normal +// user; as root a read-only directory does not stop a write. + +const ( + readOnlyDirMode = uint32(os.ModeDir | 0o555) + unsearchableMode = uint32(os.ModeDir | 0o600) + plainDirMode = uint32(os.ModeDir | 0o755) + plainFileMode = uint32(0o644) + setuidFileMode = uint32(os.ModeSetuid | 0o755) + otherOwnerID = uint32(4321) + writableTestMode = 0o755 + symlinkTargetPath = "/nonexistent/target" +) + +// TestRestoreFillsReadOnlyDirectory checks that a read-only directory +// still receives the entries inside it, and that it ends with its stored +// mode and mtime even though entries were written into it. +func TestRestoreFillsReadOnlyDirectory(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + targetDir := t.TempDir() + mtime := time.Date(2001, time.February, 3, 4, 5, 6, 0, time.UTC) + + rows, repos := makeFiles(ctx, t, []*database.File{ + {Path: "/ro", Mode: readOnlyDirMode, MTime: mtime}, + {Path: "/ro/file", Mode: plainFileMode, MTime: mtime}, + {Path: "/ro/sub", Mode: readOnlyDirMode, MTime: mtime}, + {Path: "/ro/sub/file", Mode: plainFileMode, MTime: mtime}, + }) + + roDir := filepath.Join(targetDir, "ro") + subDir := filepath.Join(roDir, "sub") + + // Let t.TempDir remove the tree afterwards. + t.Cleanup(func() { + _ = os.Chmod(roDir, writableTestMode) + _ = os.Chmod(subDir, writableTestMode) + }) + + v := newContainmentVaultik(ctx, afero.NewOsFs()) + _, err := v.restoreAllFiles(rows, repos, + &RestoreOptions{TargetDir: targetDir}, nil, nil) + require.NoError(t, err) + + for _, path := range []string{ + filepath.Join(roDir, "file"), filepath.Join(subDir, "file"), + } { + _, err := os.Stat(path) + require.NoErrorf(t, err, "file inside a read-only directory: %s", path) + } + + for _, dir := range []string{roDir, subDir} { + info, err := os.Stat(dir) + require.NoError(t, err) + assert.Equalf(t, os.FileMode(0o555), info.Mode().Perm(), "mode of %s", dir) + assert.Truef(t, info.ModTime().Equal(mtime), + "mtime of %s is %s, stored %s", dir, info.ModTime(), mtime) + } +} + +// TestRestoreFinishesChildBeforeUnsearchableParent checks that a +// directory inside one whose stored mode denies search still gets its +// own stored mode and mtime. +func TestRestoreFinishesChildBeforeUnsearchableParent(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + targetDir := t.TempDir() + mtime := time.Date(2001, time.February, 3, 4, 5, 6, 0, time.UTC) + + rows, repos := makeFiles(ctx, t, []*database.File{ + {Path: "/locked", Mode: unsearchableMode, MTime: mtime}, + {Path: "/locked/sub", Mode: plainDirMode, MTime: mtime}, + }) + + lockedDir := filepath.Join(targetDir, "locked") + + // Let t.TempDir remove the tree afterwards. + t.Cleanup(func() { _ = os.Chmod(lockedDir, writableTestMode) }) + + v := newContainmentVaultik(ctx, afero.NewOsFs()) + _, err := v.restoreAllFiles(rows, repos, + &RestoreOptions{TargetDir: targetDir}, nil, nil) + require.NoError(t, err) + + info, err := os.Stat(lockedDir) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + + // Open the parent so a normal user can look inside it. + require.NoError(t, os.Chmod(lockedDir, writableTestMode)) + + info, err = os.Stat(filepath.Join(lockedDir, "sub")) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o755), info.Mode().Perm()) + assert.Truef(t, info.ModTime().Equal(mtime), + "mtime of sub is %s, stored %s", info.ModTime(), mtime) +} + +// TestRestoreKeepsSetuidThroughChown checks that a setuid file keeps the +// bit when restore changes its owner. Linux clears setuid on any chown of +// a regular file, even one to its current owner, so the file is recorded +// with the current user as owner and the session is told it runs as +// root: the chown then needs no privilege. +func TestRestoreKeepsSetuidThroughChown(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + targetDir := t.TempDir() + + info, err := os.Stat(targetDir) + require.NoError(t, err) + + owner, ok := info.Sys().(*syscall.Stat_t) + require.True(t, ok) + + rows, repos := makeFiles(ctx, t, []*database.File{{ + Path: "/suid", + Mode: setuidFileMode, + UID: owner.Uid, + GID: owner.Gid, + MTime: time.Date(2001, time.February, 3, 4, 5, 6, 0, time.UTC), + }}) + + session := &restoreSession{ + v: newContainmentVaultik(ctx, afero.NewOsFs()), + ctx: ctx, + repos: repos, + opts: &RestoreOptions{TargetDir: targetDir}, + result: &RestoreResult{}, + runningAsRoot: true, + } + require.NoError(t, session.restoreFile(rows[0])) + + info, err = os.Stat(filepath.Join(targetDir, "suid")) + require.NoError(t, err) + assert.Equal(t, os.FileMode(setuidFileMode), + info.Mode()&(os.ModeSetuid|os.ModePerm)) +} + +// TestRestoreSetsSymlinkMTime checks that a restored symlink gets its +// stored mtime on the link itself. The link dangles, so a call that +// follows it would fail. +func TestRestoreSetsSymlinkMTime(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + targetDir := t.TempDir() + mtime := time.Date(2001, time.February, 3, 4, 5, 6, 0, time.UTC) + + rows, repos := makeFiles(ctx, t, []*database.File{{ + Path: "/link", + LinkTarget: types.FilePath(symlinkTargetPath), + MTime: mtime, + }}) + + v := newContainmentVaultik(ctx, afero.NewOsFs()) + _, err := v.restoreAllFiles(rows, repos, + &RestoreOptions{TargetDir: targetDir}, nil, nil) + require.NoError(t, err) + + info, err := os.Lstat(filepath.Join(targetDir, "link")) + require.NoError(t, err) + assert.Truef(t, info.ModTime().Equal(mtime), + "symlink mtime is %s, stored %s", info.ModTime(), mtime) +} + +// TestRestoreSetsSymlinkOwnerAsRoot checks that a symlink restored as +// root gets its stored owner on the link itself. +func TestRestoreSetsSymlinkOwnerAsRoot(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + if os.Geteuid() != 0 { + t.Skip("giving a file to another user needs root") + } + + ctx := context.Background() + targetDir := t.TempDir() + + rows, repos := makeFiles(ctx, t, []*database.File{{ + Path: "/link", + LinkTarget: types.FilePath(symlinkTargetPath), + UID: otherOwnerID, + GID: otherOwnerID, + MTime: time.Date(2001, time.February, 3, 4, 5, 6, 0, time.UTC), + }}) + + v := newContainmentVaultik(ctx, afero.NewOsFs()) + _, err := v.restoreAllFiles(rows, repos, + &RestoreOptions{TargetDir: targetDir}, nil, nil) + require.NoError(t, err) + + info, err := os.Lstat(filepath.Join(targetDir, "link")) + require.NoError(t, err) + + owner, ok := info.Sys().(*syscall.Stat_t) + require.True(t, ok) + assert.Equal(t, otherOwnerID, owner.Uid) + assert.Equal(t, otherOwnerID, owner.Gid) +}