From cc884419de494ee873b2ed5a2efe6e2727c352a5 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, each before its parent, skipping any whose place a symlink has since taken. 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 | 102 ++++++- internal/vaultik/restore_metadata_test.go | 349 ++++++++++++++++++++++ 4 files changed, 447 insertions(+), 17 deletions(-) create mode 100644 internal/vaultik/restore_metadata_test.go diff --git a/TODO.md b/TODO.md index a0c35be..c129340 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, each before its parent. 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 `go.mod` what `go mod tidy` writes, so the pre-commit hook no longer stops every commit ([issue #246](https://git.eeqj.de/sneak/vaultik/issues/246)). A test diff --git a/go.mod b/go.mod index e8d6235..d00f7f3 100644 --- a/go.mod +++ b/go.mod @@ -24,6 +24,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..a1ec97d 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,90 @@ 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, each before its parent, 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 puts every + // directory before its parent. + 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 + } + + // A later entry can have put a symlink in the directory's place, + // for example one stored as "/d/" next to the directory "/d". The + // calls below follow symlinks, so they would change its target. + info, err := lstatIfPossible(s.v.Fs, targetPath) + if err != nil || !info.IsDir() { + log.Debug("Not setting directory metadata: no longer a directory", + "path", targetPath, "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 +1208,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..3264e52 --- /dev/null +++ b/internal/vaultik/restore_metadata_test.go @@ -0,0 +1,349 @@ +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. + +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" + memTargetDir = "/restore" + + // Owner bits a normal user needs on a directory to create an entry + // in it, and to change an entry in it. + ownerWriteAndSearch = os.FileMode(0o300) + ownerSearch = os.FileMode(0o100) +) + +// normalUserFs refuses what the kernel refuses a normal user. make test +// runs as root, which a read-only or unsearchable directory does not +// stop, so without it the tests below could not fail there. Creating an +// entry needs owner write and search on the directory holding it; +// changing an entry's mode or times needs owner search. Only that one +// directory is checked, not every ancestor. +type normalUserFs struct { + afero.Fs +} + +//nolint:ireturn // afero.Fs.OpenFile is defined to return the interface +func (fs normalUserFs) OpenFile( + name string, flag int, perm os.FileMode, +) (afero.File, error) { + if flag&os.O_CREATE != 0 { + err := fs.checkParent(name, ownerWriteAndSearch) + if err != nil { + return nil, err + } + } + + return fs.Fs.OpenFile(name, flag, perm) +} + +func (fs normalUserFs) MkdirAll(path string, perm os.FileMode) error { + _, err := fs.Stat(path) + if err != nil { + err = fs.checkParent(path, ownerWriteAndSearch) + if err != nil { + return err + } + } + + return fs.Fs.MkdirAll(path, perm) +} + +func (fs normalUserFs) Chmod(name string, mode os.FileMode) error { + err := fs.checkParent(name, ownerSearch) + if err != nil { + return err + } + + return fs.Fs.Chmod(name, mode) +} + +func (fs normalUserFs) Chtimes(name string, atime, mtime time.Time) error { + err := fs.checkParent(name, ownerSearch) + if err != nil { + return err + } + + return fs.Fs.Chtimes(name, atime, mtime) +} + +// checkParent returns a permission error when the directory holding name +// exists and its owner bits lack any of need. +func (fs normalUserFs) checkParent(name string, need os.FileMode) error { + info, err := fs.Stat(filepath.Dir(name)) + if err == nil && info.Mode().Perm()&need != need { + return &os.PathError{Op: "access", Path: name, Err: os.ErrPermission} + } + + return nil +} + +// TestRestoreFillsReadOnlyDirectory checks that a read-only directory +// still receives the entries inside it, and ends with its stored mode. +func TestRestoreFillsReadOnlyDirectory(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + + rows, repos := makeFiles(ctx, t, []*database.File{ + {Path: "/ro", Mode: readOnlyDirMode}, + {Path: "/ro/file", Mode: plainFileMode}, + {Path: "/ro/sub", Mode: readOnlyDirMode}, + {Path: "/ro/sub/file", Mode: plainFileMode}, + }) + + fs := afero.NewMemMapFs() + v := newContainmentVaultik(ctx, normalUserFs{Fs: fs}) + _, err := v.restoreAllFiles(rows, repos, + &RestoreOptions{TargetDir: memTargetDir}, nil, nil) + require.NoError(t, err) + + for _, path := range []string{"/ro/file", "/ro/sub/file"} { + _, err := fs.Stat(filepath.Join(memTargetDir, path)) + require.NoErrorf(t, err, "file inside a read-only directory: %s", path) + } + + for _, dir := range []string{"/ro", "/ro/sub"} { + info, err := fs.Stat(filepath.Join(memTargetDir, dir)) + require.NoError(t, err) + assert.Equalf(t, os.FileMode(0o555), info.Mode().Perm(), "mode of %s", dir) + } +} + +// TestRestoreKeepsNonEmptyDirectoryMTime checks that a directory keeps +// its stored mtime although entries were written into it. It runs on the +// real filesystem, where writing an entry changes its directory's mtime. +func TestRestoreKeepsNonEmptyDirectoryMTime(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: "/dir", Mode: plainDirMode, MTime: mtime}, + {Path: "/dir/file", Mode: plainFileMode, MTime: mtime}, + }) + + v := newContainmentVaultik(ctx, afero.NewOsFs()) + _, err := v.restoreAllFiles(rows, repos, + &RestoreOptions{TargetDir: targetDir}, nil, nil) + require.NoError(t, err) + + info, err := os.Stat(filepath.Join(targetDir, "dir")) + require.NoError(t, err) + assert.Truef(t, info.ModTime().Equal(mtime), + "directory mtime is %s, stored %s", 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() + 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}, + }) + + fs := afero.NewMemMapFs() + v := newContainmentVaultik(ctx, normalUserFs{Fs: fs}) + _, err := v.restoreAllFiles(rows, repos, + &RestoreOptions{TargetDir: memTargetDir}, nil, nil) + require.NoError(t, err) + + info, err := fs.Stat(filepath.Join(memTargetDir, "locked")) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + + info, err = fs.Stat(filepath.Join(memTargetDir, "locked", "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) +} + +// TestRestoreLeavesSymlinkedDirectoryTargetAlone checks that a directory +// whose place a later entry takes with a symlink does not hand its stored +// owner, mode and mtime to whatever the symlink points at. "/d" and "/d/" +// are different stored paths for the same place on disk. +func TestRestoreLeavesSymlinkedDirectoryTargetAlone(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + tempDir := t.TempDir() + targetDir := filepath.Join(tempDir, "target") + outsideDir := filepath.Join(tempDir, "outside") + require.NoError(t, os.Mkdir(outsideDir, writableTestMode)) + + before, err := os.Stat(outsideDir) + require.NoError(t, err) + + mtime := time.Date(2001, time.February, 3, 4, 5, 6, 0, time.UTC) + + rows, repos := makeFiles(ctx, t, []*database.File{ + { + Path: "/d", + Mode: readOnlyDirMode, + UID: otherOwnerID, + GID: otherOwnerID, + MTime: mtime, + }, + {Path: "/d/", LinkTarget: types.FilePath(outsideDir), MTime: mtime}, + }) + + v := newContainmentVaultik(ctx, afero.NewOsFs()) + _, err = v.restoreAllFiles(rows, repos, + &RestoreOptions{TargetDir: targetDir}, nil, nil) + require.NoError(t, err) + + after, err := os.Stat(outsideDir) + require.NoError(t, err) + assert.Equal(t, before.Mode(), after.Mode()) + assert.Truef(t, after.ModTime().Equal(before.ModTime()), + "mtime changed from %s to %s", before.ModTime(), after.ModTime()) + + beforeOwner, ok := before.Sys().(*syscall.Stat_t) + require.True(t, ok) + + afterOwner, ok := after.Sys().(*syscall.Stat_t) + require.True(t, ok) + assert.Equal(t, beforeOwner.Uid, afterOwner.Uid) + assert.Equal(t, beforeOwner.Gid, afterOwner.Gid) +} + +// 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) +} -- 2.54.0