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

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

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