diff --git a/docs/REPOSTRUCTURE.md b/docs/REPOSTRUCTURE.md index a7c978c..e0e0300 100644 --- a/docs/REPOSTRUCTURE.md +++ b/docs/REPOSTRUCTURE.md @@ -145,10 +145,10 @@ An observer cannot determine: ## Pruning Safety The prune operation is safe because: -1. It only deletes blobs not referenced in any manifest +1. It keeps every blob listed in any snapshot's manifest and deletes only blobs that no manifest references 2. Manifests are unencrypted and can be read without keys -3. The operation compares the latest local DB snapshot with the latest S3 snapshot to ensure consistency -4. Pruning will fail if these don't match, preventing accidental deletion of needed blobs +3. If any manifest cannot be downloaded or decoded, prune deletes nothing and exits with an error, rather than treating that snapshot's blobs as unreferenced +4. Prune requires exclusive access to the destination: running it during a concurrent backup can race a snapshot whose manifest is not yet written, so do not prune while a backup is in progress ## Restoration Requirements diff --git a/internal/snapshot/manifest_gen_test.go b/internal/snapshot/manifest_gen_test.go new file mode 100644 index 0000000..9eae427 --- /dev/null +++ b/internal/snapshot/manifest_gen_test.go @@ -0,0 +1,64 @@ +//nolint:testpackage // exercises the unexported generateBlobManifest +package snapshot + +import ( + "context" + "path/filepath" + "testing" + "time" + + "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/types" +) + +// TestGenerateBlobManifest_MissingBlobFails is the regression guard for +// issue #157: a blob the snapshot references but that is absent from the +// blobs table used to be logged and skipped, yielding a manifest with +// fewer blobs than the snapshot needs. Since prune trusts the manifest +// alone, that omitted blob would be deleted at the next prune. Manifest +// generation must fail instead. +func TestGenerateBlobManifest_MissingBlobFails(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + dbPath := filepath.Join(t.TempDir(), "snapshot.db") + + db, err := database.New(ctx, dbPath) + require.NoError(t, err) + + repos := database.NewRepositories(db) + + // A real blob row satisfies the snapshot_blobs foreign key on + // blob_id; the snapshot then references a different, absent hash. + presentBlob := &database.Blob{ + ID: types.NewBlobID(), + Hash: types.BlobHash("present-blob-hash"), + CreatedTS: time.Now().Truncate(time.Second), + } + require.NoError(t, repos.Blobs.Create(ctx, nil, presentBlob)) + + snap := &database.Snapshot{ + ID: "testhost_home_2026-05-01T00:00:00Z", + Hostname: "testhost", + } + require.NoError(t, repos.Snapshots.Create(ctx, nil, snap)) + require.NoError(t, repos.Snapshots.AddBlob(ctx, nil, + snap.ID.String(), presentBlob.ID, types.BlobHash("absent-blob-hash"))) + + require.NoError(t, db.Close()) + + sm := &SnapshotManager{ + config: &config.Config{CompressionLevel: 3}, + fs: afero.NewOsFs(), + } + + _, err = sm.generateBlobManifest(ctx, dbPath, snap.ID.String()) + require.Error(t, err, "manifest generation must fail on a missing blob") + assert.Contains(t, err.Error(), "absent-blob-hash") +} diff --git a/internal/snapshot/snapshot.go b/internal/snapshot/snapshot.go index 0c010db..9e861ce 100644 --- a/internal/snapshot/snapshot.go +++ b/internal/snapshot/snapshot.go @@ -809,6 +809,11 @@ func (sm *SnapshotManager) copyFile(src, dst string) error { return nil } +// errBlobMissingFromDatabase means a snapshot references a blob that is +// absent from the blobs table, so a complete manifest cannot be built. +var errBlobMissingFromDatabase = errors.New( + "blob referenced by snapshot is not in the database") + // generateBlobManifest creates a compressed JSON list of all blobs in the snapshot func (sm *SnapshotManager) generateBlobManifest( ctx context.Context, dbPath string, snapshotID string, @@ -839,20 +844,26 @@ func (sm *SnapshotManager) generateBlobManifest( totalCompressedSize := int64(0) for _, hash := range blobHashes { + // Every blob the snapshot references must appear in the manifest. + // Prune consults only the manifest to decide what is still in use, + // so silently dropping a blob here would let a later prune delete + // it while this snapshot still needs it. A lookup failure or a + // missing blob row therefore fails manifest generation. blob, err := repos.Blobs.GetByHash(ctx, hash) if err != nil { - log.Warn("Failed to get blob details", "hash", hash, "error", err) - - continue + return nil, fmt.Errorf("getting blob details for %s: %w", hash, err) } - if blob != nil { - blobs = append(blobs, BlobInfo{ - Hash: hash, - CompressedSize: blob.CompressedSize, - }) - totalCompressedSize += blob.CompressedSize + if blob == nil { + return nil, fmt.Errorf("%w: blob %s, snapshot %s", + errBlobMissingFromDatabase, hash, snapshotID) } + + blobs = append(blobs, BlobInfo{ + Hash: hash, + CompressedSize: blob.CompressedSize, + }) + totalCompressedSize += blob.CompressedSize } // Create manifest. SnapshotID in the unencrypted manifest is the diff --git a/internal/vaultik/prune.go b/internal/vaultik/prune.go index 490f7dd..f197bc2 100644 --- a/internal/vaultik/prune.go +++ b/internal/vaultik/prune.go @@ -167,6 +167,12 @@ func (v *Vaultik) PruneBlobs(opts *PruneOptions) error { // collectReferencedBlobs downloads all manifests and returns the set of // referenced blob hashes. +// +// Every manifest must be read successfully. A manifest that cannot be +// downloaded or decoded means its snapshot's blobs are unknown, so +// treating them as unreferenced would let prune delete data a snapshot +// still needs. Rather than risk that silent loss, any failure returns an +// error naming the remote key and prune deletes nothing. func (v *Vaultik) collectReferencedBlobs() (map[string]bool, error) { log.Info("Listing remote snapshots") // IDs returned by listUniqueSnapshotIDs are remote keys (hashed @@ -179,27 +185,22 @@ func (v *Vaultik) collectReferencedBlobs() (map[string]bool, error) { log.Info("Found manifests in remote storage", "count", len(remoteKeys)) allBlobsReferenced := make(map[string]bool) - manifestCount := 0 for _, remoteKey := range remoteKeys { log.Debug("Processing manifest", "remote_key", remoteKey) manifest, err := v.downloadManifestByKey(remoteKey) if err != nil { - log.Error("Failed to download manifest", "remote_key", remoteKey, "error", err) - - continue + return nil, fmt.Errorf("reading manifest %s: %w", remoteKey, err) } for _, blob := range manifest.Blobs { allBlobsReferenced[blob.Hash] = true } - - manifestCount++ } log.Info("Processed manifests", - "count", manifestCount, "unique_blobs_referenced", len(allBlobsReferenced)) + "count", len(remoteKeys), "unique_blobs_referenced", len(allBlobsReferenced)) return allBlobsReferenced, nil } diff --git a/internal/vaultik/prune_manifest_safety_test.go b/internal/vaultik/prune_manifest_safety_test.go new file mode 100644 index 0000000..9ab56c1 --- /dev/null +++ b/internal/vaultik/prune_manifest_safety_test.go @@ -0,0 +1,47 @@ +package vaultik_test + +import ( + "bytes" + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/vaultik" +) + +// TestPruneBlobs_UnreadableManifestDeletesNothing is the regression guard +// for issue #157: prune identifies referenced blobs by reading every +// snapshot's manifest, and a manifest it cannot decode used to be logged +// and skipped. Blobs referenced only by that snapshot then looked +// unreferenced and were deleted, with a zero exit — silent backup loss, +// made worse by `snapshot create --prune` running unattended with force. +// +// The single blob here is referenced only by the snapshot whose manifest +// is corrupt, so the old behaviour would delete it and succeed. Prune +// must instead delete nothing and return an error. +func TestPruneBlobs_UnreadableManifestDeletesNothing(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + env := newListEnv(t) + ctx := context.Background() + + blobKey := "blobs/" + testBlobHashA[:2] + "/" + testBlobHashA[2:4] + + "/" + testBlobHashA + require.NoError(t, env.store.Put(ctx, blobKey, + bytes.NewReader([]byte("blob-bytes")))) + + // A manifest at the path prune reads, but with contents it cannot + // decode. + require.NoError(t, env.store.Put(ctx, + "metadata/corruptkey/manifest.json.zst", + bytes.NewReader([]byte("not a valid manifest")))) + + err := env.v.PruneBlobs(&vaultik.PruneOptions{Force: true}) + + require.Error(t, err, "prune must fail when a manifest cannot be read") + assert.True(t, env.store.hasKey(blobKey), + "no blob may be deleted when a manifest is unreadable") +} diff --git a/internal/vaultik/verify.go b/internal/vaultik/verify.go index 9a0d5bf..787f6d2 100644 --- a/internal/vaultik/verify.go +++ b/internal/vaultik/verify.go @@ -28,8 +28,10 @@ var ( errChunkHashMismatch = errors.New("chunk hash mismatch") errTrailingBlobData = errors.New( "blob has unexpected trailing bytes not covered by chunk list") - errManifestExtraBlob = errors.New("manifest contains blob not in database") - errBlobSizeMismatch = errors.New("blob size mismatch") + errManifestExtraBlob = errors.New("manifest contains blob not in database") + errManifestMissingBlob = errors.New( + "manifest omits blob present in database") + errBlobSizeMismatch = errors.New("blob size mismatch") ) // verifyStatusFailed is the JSON status value for a failed verification. @@ -575,16 +577,11 @@ func (v *Vaultik) verifyManifestAgainstDatabase( manifestBlobMap[blob.Hash] = blob.CompressedSize } - // Check counts match - if len(dbBlobMap) != len(manifestBlobMap) { - log.Warn("Manifest blob count mismatch", - "database_blobs", len(dbBlobMap), - "manifest_blobs", len(manifestBlobMap), - ) - // This is a warning, not an error - database is authoritative - } - - // Check each manifest blob exists in database with correct size + // The manifest is the only blob list prune consults, so it must match + // the database exactly. A blob in the manifest but not the database + // points at a corrupt manifest; a blob in the database but omitted + // from the manifest would be pruned away while this snapshot still + // needs it. Either divergence fails verification. for hash, manifestSize := range manifestBlobMap { dbSize, exists := dbBlobMap[hash] if !exists { @@ -598,6 +595,12 @@ func (v *Vaultik) verifyManifestAgainstDatabase( } } + for hash := range dbBlobMap { + if _, exists := manifestBlobMap[hash]; !exists { + return fmt.Errorf("%w: %s", errManifestMissingBlob, hash) + } + } + log.Info("✓ Manifest verified against database", "manifest_blobs", len(manifestBlobMap), "database_blobs", len(dbBlobMap), diff --git a/internal/vaultik/verify_manifest_test.go b/internal/vaultik/verify_manifest_test.go new file mode 100644 index 0000000..48342ab --- /dev/null +++ b/internal/vaultik/verify_manifest_test.go @@ -0,0 +1,61 @@ +package vaultik //nolint:testpackage // calls unexported verifyManifestAgainstDatabase + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/snapshot" +) + +// Blob hashes shared by the manifest-verification tests below. +const ( + manifestTestBlobA = "blob-a" + manifestTestBlobB = "blob-b" +) + +// TestVerifyManifestAgainstDatabase_MissingBlobFails is the regression +// guard for issue #157: deep verify must fail when the manifest omits a +// blob the database records. The divergence used to be logged as a +// warning while verification still returned ok, so an incomplete +// manifest — the exact defect that lets prune later delete a needed blob +// — passed unnoticed. +func TestVerifyManifestAgainstDatabase_MissingBlobFails(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + v := &Vaultik{} + + dbBlobs := []snapshot.BlobInfo{ + {Hash: manifestTestBlobA, CompressedSize: 10}, + {Hash: manifestTestBlobB, CompressedSize: 20}, + } + manifest := &snapshot.Manifest{ + Blobs: []snapshot.BlobInfo{ + {Hash: manifestTestBlobA, CompressedSize: 10}, + }, + } + + err := v.verifyManifestAgainstDatabase(manifest, dbBlobs) + require.Error(t, err, "verify must fail when the manifest omits a database blob") + assert.Contains(t, err.Error(), manifestTestBlobB) +} + +// TestVerifyManifestAgainstDatabase_MatchingSetsPass keeps the other half +// honest: identical blob sets still verify, so the check above cannot be +// satisfied by failing everything. +func TestVerifyManifestAgainstDatabase_MatchingSetsPass(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + v := &Vaultik{} + + blobs := []snapshot.BlobInfo{ + {Hash: manifestTestBlobA, CompressedSize: 10}, + {Hash: manifestTestBlobB, CompressedSize: 20}, + } + manifest := &snapshot.Manifest{Blobs: blobs} + + require.NoError(t, v.verifyManifestAgainstDatabase(manifest, blobs)) +}