internal/vaultik/blob_fetch.go

@@ -18,6 +18,13 @@ import (
 // not match the expected double-SHA-256 hash.
 var errBlobHashMismatch = errors.New("blob hash mismatch")
 
+// errBlobNotFullyRead is returned when the verifying reader is closed
+// before its plaintext reached EOF. The hash can only be checked once
+// the whole stream has been read, so an early or short-read close must
+// fail rather than silently skip verification.
+var errBlobNotFullyRead = errors.New(
+	"blob closed before fully read; hash not verified")
+
 // hashVerifyReader wraps a blobgen.Reader and verifies the double-SHA-256 hash
 // of decrypted plaintext when Close is called. It reuses the hash that
 // blobgen.Reader already computes internally via its TeeReader, avoiding
@@ -38,21 +45,26 @@ func (h *hashVerifyReader) Read(p []byte) (int, error) {
 	return n, err
 }
 
-// Close verifies the hash (if the stream was fully read) and closes underlying readers.
+// Close closes the underlying readers and verifies the blob hash. The
+// hash check cannot be skipped: closing before the plaintext reached
+// EOF (a short read or an early close) is an error, so a caller can
+// never obtain unverified blob bytes.
 func (h *hashVerifyReader) Close() error {
 	readerErr := h.reader.Close()
 	fetcherErr := h.fetcher.Close()
 
-	if h.done {
+	if !h.done {
-		firstHash := h.reader.Sum256()
+		return errBlobNotFullyRead
-		secondHasher := sha256.New()
+	}
-		secondHasher.Write(firstHash)
 
-		actualHashHex := hex.EncodeToString(secondHasher.Sum(nil))
+	firstHash := h.reader.Sum256()
-		if actualHashHex != h.blobHash {
+	secondHasher := sha256.New()
-			return fmt.Errorf("%w: expected %s, got %s",
+	secondHasher.Write(firstHash)
-				errBlobHashMismatch, h.blobHash[:16], actualHashHex[:16])
+
-		}
+	actualHashHex := hex.EncodeToString(secondHasher.Sum(nil))
+	if actualHashHex != h.blobHash {
+		return fmt.Errorf("%w: expected %s, got %s",
+			errBlobHashMismatch, h.blobHash[:16], actualHashHex[:16])
 	}
 
 	if readerErr != nil {

internal/vaultik/blob_fetch_hash_test.go

@@ -133,3 +133,51 @@ func TestFetchAndDecryptBlobVerifiesHash(t *testing.T) {
 		}
 	})
 }
+
+// TestFetchAndDecryptBlobCloseBeforeEOFFails verifies the hash check
+// cannot be skipped: a caller that reads only part of the blob and then
+// closes gets an error rather than silently unverified bytes.
+func TestFetchAndDecryptBlobCloseBeforeEOFFails(t *testing.T) {
+	t.Parallel()
+
+	identity, err := age.GenerateX25519Identity()
+	if err != nil {
+		t.Fatalf("generating identity: %v", err)
+	}
+
+	plaintext := []byte("hello world test data for blob hash verification")
+	encryptedData, correctHash := buildHashTestBlob(t, identity, plaintext)
+
+	mockStorage := NewMockStorer()
+	blobPath := "blobs/" + correctHash[:2] + "/" +
+		correctHash[2:4] + "/" + correctHash
+
+	mockStorage.mu.Lock()
+	mockStorage.data[blobPath] = encryptedData
+	mockStorage.mu.Unlock()
+
+	tv := vaultik.NewForTesting(mockStorage)
+
+	rc, err := tv.FetchAndDecryptBlob(
+		context.Background(), correctHash, int64(len(encryptedData)), identity)
+	if err != nil {
+		t.Fatalf("unexpected error opening stream: %v", err)
+	}
+
+	// Read one byte, far short of the plaintext length, then close.
+	buf := make([]byte, 1)
+
+	_, err = rc.Read(buf)
+	if err != nil {
+		t.Fatalf("reading first byte: %v", err)
+	}
+
+	err = rc.Close()
+	if err == nil {
+		t.Fatal("expected error closing before EOF, got nil")
+	}
+
+	if !strings.Contains(err.Error(), "hash not verified") {
+		t.Fatalf("expected not-verified error, got: %v", err)
+	}
+}

internal/vaultik/restore.go

@@ -37,6 +37,8 @@ var (
 	errShortChunkRead           = errors.New("short read")
 	errRestorePathEscapesTarget = errors.New(
 		"refusing to restore path outside the target directory")
+	errTrailingRestoreData = errors.New(
+		"restored file has trailing data after its last chunk")
 )
 
 // restoreDirMode is the permission mode for directories created while
@@ -44,6 +46,13 @@ var (
 // directories themselves get their stored mode).
 const restoreDirMode = 0o755
 
+// 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,
+// so a file whose stored mode is restrictive is never briefly readable by
+// other local users while its content is being written.
+const restoreFileMode = 0o600
+
 // sweepIntervalDivisor sets the sweeper threshold to one N-th of the
 // configured blob size limit.
 const sweepIntervalDivisor = 100
@@ -889,6 +898,13 @@ func (s *restoreSession) restoreDirectory(
 		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.result.FilesRestored++
@@ -896,25 +912,22 @@ func (s *restoreSession) restoreDirectory(
 	return nil
 }
 
-// applyFileMetadata applies stored permissions, ownership (when running
+// applyFileMetadata applies ownership (when running as root on a real
-// as root on a real filesystem), and mtime to a restored path. Failures
+// filesystem) and mtime to a restored path. Permission mode is applied
-// are logged at debug level and do not abort the restore.
+// 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.
 func (s *restoreSession) applyFileMetadata(file *database.File, targetPath string) {
-	err := s.v.Fs.Chmod(targetPath, os.FileMode(file.Mode))
-	if err != nil {
-		log.Debug("Failed to set permissions", "path", targetPath, "error", err)
-	}
-
 	if s.runningAsRoot {
 		if _, ok := s.v.Fs.(*afero.OsFs); ok {
-			err = os.Chown(targetPath, int(file.UID), int(file.GID))
+			err := os.Chown(targetPath, int(file.UID), int(file.GID))
 			if err != nil {
 				log.Debug("Failed to set ownership", "path", targetPath, "error", err)
 			}
 		}
 	}
 
-	err = s.v.Fs.Chtimes(targetPath, file.MTime, file.MTime)
+	err := s.v.Fs.Chtimes(targetPath, file.MTime, file.MTime)
 	if err != nil {
 		log.Debug("Failed to set mtime", "path", targetPath, "error", err)
 	}
@@ -948,17 +961,30 @@ func (s *restoreSession) restoreRegularFile(
 
 	t0 = time.Now()
 
-	outFile, err := s.v.Fs.Create(targetPath)
+	// Remove any existing entry, then create the file with a restrictive
+	// mode via O_EXCL. The stored mode is applied only after the content
+	// is written and the file closed, so a file whose stored mode is
+	// restrictive is never briefly readable by other local users while
+	// its content is written. Removing first (rather than failing on a
+	// leftover file) matches the documented behaviour that re-running
+	// restore overwrites partial output.
+	_ = s.v.Fs.Remove(targetPath)
+
+	outFile, err := s.v.Fs.OpenFile(
+		targetPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, restoreFileMode)
 	createDur := time.Since(t0)
 
 	if err != nil {
 		return fmt.Errorf("creating output file: %w", err)
 	}
 
-	defer func() { _ = outFile.Close() }()
-
 	bytesWritten, timings, err := s.writeFileChunks(outFile, fileChunks)
 	if err != nil {
+		// Do not leave a partial file behind.
+		_ = outFile.Close()
+
+		s.removePartialRestore(targetPath)
+
 		return err
 	}
 
@@ -976,9 +1002,12 @@ func (s *restoreSession) restoreRegularFile(
 
 	err = outFile.Close()
 	if err != nil {
+		s.removePartialRestore(targetPath)
+
 		return fmt.Errorf("closing output file: %w", err)
 	}
 
+	s.applyRestoredFileMode(file, targetPath)
 	s.applyFileMetadata(file, targetPath)
 
 	s.result.FilesRestored++
@@ -989,6 +1018,31 @@ func (s *restoreSession) restoreRegularFile(
 	return nil
 }
 
+// applyRestoredFileMode applies the stored permission bits to a
+// just-written regular file (created with restoreFileMode). A failure is
+// a user-visible warning, not a fatal error: the file's content is
+// intact and it remains at the restrictive create-time mode, so the
+// restore is not aborted or discarded over it.
+func (s *restoreSession) applyRestoredFileMode(
+	file *database.File, targetPath string,
+) {
+	err := s.v.Fs.Chmod(targetPath, os.FileMode(file.Mode))
+	if err != nil {
+		s.v.UI.Warningf("Failed to set mode %s on %s: %v",
+			os.FileMode(file.Mode).Perm(), s.v.UI.Path(targetPath), err)
+	}
+}
+
+// removePartialRestore deletes a restore output file whose write did not
+// complete, so a failed restore never leaves a partial file behind.
+func (s *restoreSession) removePartialRestore(targetPath string) {
+	err := s.v.Fs.Remove(targetPath)
+	if err != nil {
+		log.Debug("Failed to remove partial restore file",
+			"path", targetPath, "error", err)
+	}
+}
+
 // writeFileChunks streams each of the file's chunks from the blob disk
 // cache into outFile, crediting restored bytes to the sweeper as it
 // goes. Returns the bytes written plus per-phase timing accumulators.
@@ -1070,11 +1124,19 @@ func (s *restoreSession) downloadBlobToCache(
 	streamDur := time.Since(t0)
 	closeErr := rc.Close()
 
+	// closeErr carries the blob's hash-verification result (a mismatch,
+	// or the stream not being fully read). On any failure, drop the
+	// cache entry so a blob that failed verification is never read back
+	// as if it were valid.
 	if copyErr != nil {
+		s.blobCache.Delete(blobHash)
+
 		return copyErr
 	}
 
 	if closeErr != nil {
+		s.blobCache.Delete(blobHash)
+
 		return closeErr
 	}
 
@@ -1242,6 +1304,17 @@ func (v *Vaultik) verifyFile(
 		bytesVerified += int64(n)
 	}
 
+	// The stored chunks account for the whole file, so the reader must
+	// be at EOF now. Trailing bytes past the last chunk are corruption
+	// the per-chunk loop cannot see.
+	extra := make([]byte, 1)
+
+	n, err := f.Read(extra)
+	if n != 0 || !errors.Is(err, io.EOF) {
+		return bytesVerified, fmt.Errorf("%w: file longer than its %d chunk(s)",
+			errTrailingRestoreData, len(fileChunks))
+	}
+
 	log.Debug("File verified",
 		"path", file.Path, "bytes", bytesVerified, "chunks", len(fileChunks))
 

internal/vaultik/restore_perms_test.go

@@ -0,0 +1,306 @@
+package vaultik //nolint:testpackage // drives restore through unexported session
+
+import (
+	"bytes"
+	"context"
+	"errors"
+	"io"
+	"os"
+	"path/filepath"
+	"strings"
+	"sync"
+	"syscall"
+	"testing"
+
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/vaultik/internal/config"
+	"sneak.berlin/go/vaultik/internal/database"
+	"sneak.berlin/go/vaultik/internal/log"
+	"sneak.berlin/go/vaultik/internal/snapshot"
+	"sneak.berlin/go/vaultik/internal/storage"
+	"sneak.berlin/go/vaultik/internal/ui"
+)
+
+// errSpyWrite is the injected write failure used to exercise the
+// partial-file cleanup path.
+var errSpyWrite = errors.New("injected write failure")
+
+// modeSpyFs wraps a real filesystem so restore tests can observe and
+// perturb the single output file whose path contains watch. It records
+// the on-disk permission bits seen at the moment content is first
+// written (the window during which another local user could read it),
+// and can inject a write failure or append trailing bytes on close.
+type modeSpyFs struct {
+	afero.Fs
+
+	watch string
+
+	mu         sync.Mutex
+	writeModes []os.FileMode
+	failWrite  bool
+	trailing   int
+}
+
+//nolint:ireturn // afero.Fs.OpenFile is defined to return the interface
+func (m *modeSpyFs) OpenFile(
+	name string, flag int, perm os.FileMode,
+) (afero.File, error) {
+	f, err := m.Fs.OpenFile(name, flag, perm)
+	if err != nil || !strings.Contains(name, m.watch) {
+		return f, err
+	}
+
+	return &modeSpyFile{File: f, fs: m, path: name}, nil
+}
+
+type modeSpyFile struct {
+	afero.File
+
+	fs      *modeSpyFs
+	path    string
+	written bool
+}
+
+func (f *modeSpyFile) Write(p []byte) (int, error) {
+	if !f.written {
+		f.written = true
+
+		info, err := f.fs.Stat(f.path)
+		if err == nil {
+			f.fs.mu.Lock()
+			f.fs.writeModes = append(f.fs.writeModes, info.Mode().Perm())
+			f.fs.mu.Unlock()
+		}
+	}
+
+	if f.fs.failWrite {
+		return 0, errSpyWrite
+	}
+
+	return f.File.Write(p)
+}
+
+func (f *modeSpyFile) Close() error {
+	if f.fs.trailing > 0 {
+		_, _ = f.File.Write(bytes.Repeat([]byte{'x'}, f.fs.trailing))
+	}
+
+	return f.File.Close()
+}
+
+// backupOneFile writes a single source file with the given mode and
+// backs it up into a fresh file storer, returning everything a restore
+// needs. The index database is closed before returning so the restore
+// half runs from the exported metadata and remote bytes only.
+func backupOneFile(
+	ctx context.Context, t *testing.T, fs afero.Fs, tempDir, name string,
+	content []byte, mode os.FileMode,
+) (*config.Config, *storage.FileStorer, string, string) {
+	t.Helper()
+
+	dataDir := filepath.Join(tempDir, "src")
+	require.NoError(t, fs.MkdirAll(dataDir, 0o755))
+
+	srcPath := filepath.Join(dataDir, name)
+	require.NoError(t, afero.WriteFile(fs, srcPath, content, mode))
+	require.NoError(t, fs.Chmod(srcPath, mode))
+
+	storeDir := filepath.Join(tempDir, "remote")
+	dbPath := filepath.Join(tempDir, "index.sqlite")
+
+	storer, err := storage.NewFileStorer(storeDir)
+	require.NoError(t, err)
+
+	cfg := &config.Config{
+		AgeRecipients: []string{
+			"age1ezrjmfpwsc95svdg0y54mums3zevgzu0x0ecq2f7tp8a05gl0sjq9q9wjg",
+		},
+		AgeSecretKey: "AGE-SECRET-KEY-19CR5YSFW59HM4TLD6GXVEDMZFTVVF7PPHKU" +
+			"T68TXSFPK7APHXA2QS2NJA5",
+		CompressionLevel: 3,
+		Hostname:         "test-host",
+		BlobSizeLimit:    config.Size(5 * 1024 * 1024),
+	}
+
+	db, err := database.New(ctx, dbPath)
+	require.NoError(t, err)
+
+	repos := database.NewRepositories(db)
+
+	sm := snapshot.NewSnapshotManager(snapshot.SnapshotManagerParams{
+		Repos:   repos,
+		Storage: storer,
+		Config:  cfg,
+	})
+	sm.SetFilesystem(fs)
+
+	scanner := snapshot.NewScanner(snapshot.ScannerConfig{
+		FS:               fs,
+		Storage:          storer,
+		ChunkSize:        4 * 1024 * 1024,
+		MaxBlobSize:      5 * 1024 * 1024,
+		CompressionLevel: cfg.CompressionLevel,
+		AgeRecipients:    cfg.AgeRecipients,
+		Repositories:     repos,
+	})
+
+	snapshotID, err := sm.CreateSnapshotWithName(
+		ctx, cfg.Hostname, "perms", "test-version", "test-git")
+	require.NoError(t, err)
+
+	_, err = scanner.Scan(ctx, dataDir, snapshotID)
+	require.NoError(t, err)
+
+	require.NoError(t, sm.CompleteSnapshot(ctx, snapshotID))
+	require.NoError(t, sm.ExportSnapshotMetadata(ctx, dbPath, snapshotID))
+	require.NoError(t, db.Close())
+
+	return cfg, storer, snapshotID, srcPath
+}
+
+// restoredPathFor returns where backupOneFile's source lands under a
+// restore target: restore recreates each file at its original absolute
+// path beneath TargetDir.
+func restoredPathFor(restoreDir, srcPath string) string {
+	return filepath.Join(restoreDir, srcPath)
+}
+
+// withUmask022 forces the process umask to 022 for the duration of a
+// test, so the difference between a 0600 create and a default create is
+// observable. Restored serially (no t.Parallel) so it does not race
+// other tests.
+func withUmask022(t *testing.T) {
+	t.Helper()
+
+	old := syscall.Umask(0o022)
+
+	t.Cleanup(func() { syscall.Umask(old) })
+}
+
+// TestRestoreCreatesFileNeverWiderThanStoredMode checks that a file with
+// a restrictive stored mode (0600) is never observable with a wider mode
+// while its content is being written, and ends at its stored mode.
+//
+//nolint:paralleltest // sets the process umask; must run serially
+func TestRestoreCreatesFileNeverWiderThanStoredMode(t *testing.T) {
+	log.Initialize(log.Config{})
+	withUmask022(t)
+
+	fs := afero.NewOsFs()
+	tempDir := t.TempDir()
+	ctx := context.Background()
+
+	content := randomBytes(t, 4096)
+
+	cfg, storer, snapshotID, srcPath := backupOneFile(
+		ctx, t, fs, tempDir, "secret.bin", content, 0o600)
+
+	restoreDir := filepath.Join(tempDir, "restored")
+	spy := &modeSpyFs{Fs: fs, watch: "secret.bin"}
+
+	v := newRestoreVaultik(ctx, cfg, storer, spy)
+	require.NoError(t, v.Restore(&RestoreOptions{
+		SnapshotID: snapshotID,
+		TargetDir:  restoreDir,
+	}))
+
+	spy.mu.Lock()
+	observed := append([]os.FileMode(nil), spy.writeModes...)
+	spy.mu.Unlock()
+
+	require.NotEmpty(t, observed,
+		"spy never saw the output file being written")
+
+	for _, m := range observed {
+		assert.Equalf(t, os.FileMode(0o600), m,
+			"file was observable at mode %o during write; must be 0600", m)
+	}
+
+	// The stored mode is applied after the content is written.
+	info, err := fs.Stat(restoredPathFor(restoreDir, srcPath))
+	require.NoError(t, err)
+	assert.Equal(t, os.FileMode(0o600), info.Mode().Perm())
+
+	got, err := afero.ReadFile(fs, restoredPathFor(restoreDir, srcPath))
+	require.NoError(t, err)
+	require.True(t, bytes.Equal(got, content))
+}
+
+// TestRestoreRemovesPartialFileOnWriteFailure checks that a file whose
+// content write fails is not left behind.
+//
+//nolint:paralleltest // sets the process umask; must run serially
+func TestRestoreRemovesPartialFileOnWriteFailure(t *testing.T) {
+	log.Initialize(log.Config{})
+	withUmask022(t)
+
+	fs := afero.NewOsFs()
+	tempDir := t.TempDir()
+	ctx := context.Background()
+
+	cfg, storer, snapshotID, srcPath := backupOneFile(
+		ctx, t, fs, tempDir, "doomed.bin", randomBytes(t, 4096), 0o600)
+
+	restoreDir := filepath.Join(tempDir, "restored")
+	spy := &modeSpyFs{Fs: fs, watch: "doomed.bin", failWrite: true}
+
+	v := newRestoreVaultik(ctx, cfg, storer, spy)
+	err := v.Restore(&RestoreOptions{
+		SnapshotID: snapshotID,
+		TargetDir:  restoreDir,
+	})
+	require.Error(t, err, "restore should fail when the write fails")
+
+	exists, err := afero.Exists(fs, restoredPathFor(restoreDir, srcPath))
+	require.NoError(t, err)
+	assert.False(t, exists, "partial file must be removed after a failed write")
+}
+
+// TestVerifyRejectsTrailingBytes checks that --verify fails a restored
+// file that has bytes past its last chunk.
+//
+//nolint:paralleltest // sets the process umask; must run serially
+func TestVerifyRejectsTrailingBytes(t *testing.T) {
+	log.Initialize(log.Config{})
+	withUmask022(t)
+
+	fs := afero.NewOsFs()
+	tempDir := t.TempDir()
+	ctx := context.Background()
+
+	cfg, storer, snapshotID, _ := backupOneFile(
+		ctx, t, fs, tempDir, "padded.bin", randomBytes(t, 4096), 0o600)
+
+	restoreDir := filepath.Join(tempDir, "restored")
+	// Append one byte to the file as it is written, so its content still
+	// matches the stored chunks but it is one byte too long.
+	spy := &modeSpyFs{Fs: fs, watch: "padded.bin", trailing: 1}
+
+	v := newRestoreVaultik(ctx, cfg, storer, spy)
+	err := v.Restore(&RestoreOptions{
+		SnapshotID: snapshotID,
+		TargetDir:  restoreDir,
+		Verify:     true,
+	})
+	require.Error(t, err, "verify should fail on a file with trailing bytes")
+	assert.ErrorIs(t, err, errFilesFailedVerify)
+}
+
+// newRestoreVaultik builds a Vaultik wired for a restore-only test.
+func newRestoreVaultik(
+	ctx context.Context, cfg *config.Config, storer storage.Storer, fs afero.Fs,
+) *Vaultik {
+	v := &Vaultik{
+		Config:  cfg,
+		Storage: storer,
+		Fs:      fs,
+		Stdout:  io.Discard,
+		Stderr:  io.Discard,
+		UI:      ui.NewWithColor(io.Discard, false),
+	}
+	v.SetContext(ctx)
+
+	return v
+}