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
2026-10-06 12:57:27 +00:00
parent 66c80a70e3
commit 3edc1889e3
4 changed files with 311 additions and 17 deletions
@@ -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
@@ -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
@@ -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
@@ -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)
+}