diff --git a/internal/vaultik/blob_fetch.go b/internal/vaultik/blob_fetch.go index fde9eb2..045335a 100644 --- a/internal/vaultik/blob_fetch.go +++ b/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 { - firstHash := h.reader.Sum256() - secondHasher := sha256.New() - secondHasher.Write(firstHash) + if !h.done { + return errBlobNotFullyRead + } - actualHashHex := hex.EncodeToString(secondHasher.Sum(nil)) - if actualHashHex != h.blobHash { - return fmt.Errorf("%w: expected %s, got %s", - errBlobHashMismatch, h.blobHash[:16], actualHashHex[:16]) - } + firstHash := h.reader.Sum256() + secondHasher := sha256.New() + secondHasher.Write(firstHash) + + 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 { diff --git a/internal/vaultik/blob_fetch_hash_test.go b/internal/vaultik/blob_fetch_hash_test.go index 8334eea..a9db0a3 100644 --- a/internal/vaultik/blob_fetch_hash_test.go +++ b/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) + } +} diff --git a/internal/vaultik/restore.go b/internal/vaultik/restore.go index e847939..1cb6fb1 100644 --- a/internal/vaultik/restore.go +++ b/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 -// as root on a real filesystem), and mtime to a restored path. Failures -// are logged at debug level and do not abort the restore. +// 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. 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)) diff --git a/internal/vaultik/restore_perms_test.go b/internal/vaultik/restore_perms_test.go new file mode 100644 index 0000000..9dbcfc5 --- /dev/null +++ b/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 +}