Remove the unused second encryption path and unused secret types; write the blob-ID hash step once #151

New Issue

2026-09-22T00:54:59+02:00

clawbot commented

2026-09-22 00:54:59 +02:00

Found by the security review #73 (scope items 1 and 4). Severity: low (comprehension traps in security-relevant code; nothing leaks today).

What is wrong

There are two wrappers around age. All production encryption goes through blobgen.NewWriter (internal/blob/packer.go:342, internal/snapshot/snapshot.go:730). crypto.Encryptor, UpdateRecipients, the fx Module (internal/crypto/encryption.go:224) and Vaultik.GetEncryptor (internal/vaultik/vaultik.go:144-152) have no production caller. Decryption is split the same way: restore parses the key and reads through blobgen.NewReader; verify --deep uses crypto.NewDecryptor plus its own zstd reader and re-parses the key string for the database and once per blob (internal/vaultik/verify.go:276, :342). That duplication already drifted once (#131).
Writer.Sum256 returns the double hash and Reader.Sum256 returns the single hash under the same name (internal/blobgen/writer.go:126-137, reader.go:67-70). The second hash step is written out three times (writer.go:134, internal/vaultik/blob_fetch.go:47-51, verify.go:492-494). CompressData and CompressStream have no caller outside tests, and their result field is named SHA256 although it holds the double hash.
types.AgeSecretKey and types.AWSSecretAccessKey have a String() returning [REDACTED] and a comment "prevents accidental logging" (internal/types/types.go:164-167, :184-186, :220-229), but no code uses either type; Config holds both secrets as plain string (internal/config/config.go:132, :163). The advertised protection does not exist. Adopting the types as they are would not deliver it either: %#v ignores String(), and the JSON log handler prints a string-kinded value raw.
CleanupIncompleteSnapshots (internal/snapshot/snapshot.go:301-327) has no caller and stats db.zst where the object is db.zst.age. ARCHITECTURE.md:289 and :310 still describe it.

Acceptable

Delete internal/crypto, GetEncryptor and GetDecryptor. verify --deep parses the key once the way restore does and reads both the database and the blobs through blobgen.NewReader, still streaming the database to a temp file. Move TestEncryptorMultipleRecipients to blobgen first (it is the only multi-recipient test); remove TestTeeReaderWithDecryption (internal/vaultik/verify_test.go:20), which re-implements the pre-131 hash of encrypted bytes.
One exported function in internal/blobgen takes the plaintext SHA-256 digest and returns the double hash; the three sites call it. Writer and Reader stop sharing the name Sum256 for different quantities. The name must not say "blob name": Writer also serves the metadata database export. CompressData and CompressStream are deleted with their tests, or kept with the field renamed.
Delete the two unused secret types, their String() and Raw() methods and the comment; the other unused config newtypes in that file (AgeRecipient, S3Endpoint, BucketName, S3Prefix, AWSRegion, AWSAccessKeyID) can go in the same change.
Delete CleanupIncompleteSnapshots and correct the two ARCHITECTURE.md lines.

Definition of done

git grep finds no production or test reference to the removed identifiers.
Multi-recipient encryption is still covered by a test, now in blobgen.
make fmt over the changed markdown; no assertion weakened in any test that is kept; make check green.

If the owner prefers to keep and use the redacting types instead, they need GoString and MarshalText as well, Config must use them, and a test must cover %v, %+v, %#v and both log formats.

Line numbers are as of next at 6fcd8e1.

model: fable-5-1

Found by the security review https://git.eeqj.de/sneak/vaultik/issues/73 (scope items 1 and 4). Severity: **low** (comprehension traps in security-relevant code; nothing leaks today). ## What is wrong 1. There are two wrappers around age. All production encryption goes through `blobgen.NewWriter` (`internal/blob/packer.go:342`, `internal/snapshot/snapshot.go:730`). `crypto.Encryptor`, `UpdateRecipients`, the fx `Module` (`internal/crypto/encryption.go:224`) and `Vaultik.GetEncryptor` (`internal/vaultik/vaultik.go:144-152`) have no production caller. Decryption is split the same way: restore parses the key and reads through `blobgen.NewReader`; `verify --deep` uses `crypto.NewDecryptor` plus its own zstd reader and re-parses the key string for the database and once per blob (`internal/vaultik/verify.go:276`, `:342`). That duplication already drifted once (https://git.eeqj.de/sneak/vaultik/issues/131). 2. `Writer.Sum256` returns the double hash and `Reader.Sum256` returns the single hash under the same name (`internal/blobgen/writer.go:126-137`, `reader.go:67-70`). The second hash step is written out three times (`writer.go:134`, `internal/vaultik/blob_fetch.go:47-51`, `verify.go:492-494`). `CompressData` and `CompressStream` have no caller outside tests, and their result field is named `SHA256` although it holds the double hash. 3. `types.AgeSecretKey` and `types.AWSSecretAccessKey` have a `String()` returning `[REDACTED]` and a comment "prevents accidental logging" (`internal/types/types.go:164-167`, `:184-186`, `:220-229`), but no code uses either type; `Config` holds both secrets as plain `string` (`internal/config/config.go:132`, `:163`). The advertised protection does not exist. Adopting the types as they are would not deliver it either: `%#v` ignores `String()`, and the JSON log handler prints a string-kinded value raw. 4. `CleanupIncompleteSnapshots` (`internal/snapshot/snapshot.go:301-327`) has no caller and stats `db.zst` where the object is `db.zst.age`. `ARCHITECTURE.md:289` and `:310` still describe it. ## Acceptable - Delete `internal/crypto`, `GetEncryptor` and `GetDecryptor`. `verify --deep` parses the key once the way restore does and reads both the database and the blobs through `blobgen.NewReader`, still streaming the database to a temp file. Move `TestEncryptorMultipleRecipients` to `blobgen` first (it is the only multi-recipient test); remove `TestTeeReaderWithDecryption` (`internal/vaultik/verify_test.go:20`), which re-implements the pre-131 hash of encrypted bytes. - One exported function in `internal/blobgen` takes the plaintext SHA-256 digest and returns the double hash; the three sites call it. `Writer` and `Reader` stop sharing the name `Sum256` for different quantities. The name must not say "blob name": `Writer` also serves the metadata database export. `CompressData` and `CompressStream` are deleted with their tests, or kept with the field renamed. - Delete the two unused secret types, their `String()` and `Raw()` methods and the comment; the other unused config newtypes in that file (`AgeRecipient`, `S3Endpoint`, `BucketName`, `S3Prefix`, `AWSRegion`, `AWSAccessKeyID`) can go in the same change. - Delete `CleanupIncompleteSnapshots` and correct the two `ARCHITECTURE.md` lines. ## Definition of done 1. `git grep` finds no production or test reference to the removed identifiers. 2. Multi-recipient encryption is still covered by a test, now in `blobgen`. 3. `make fmt` over the changed markdown; no assertion weakened in any test that is kept; `make check` green. If the owner prefers to keep and use the redacting types instead, they need `GoString` and `MarshalText` as well, `Config` must use them, and a test must cover `%v`, `%+v`, `%#v` and both log formats. Line numbers are as of `next` at `6fcd8e1`. model: fable-5-1

clawbot referenced this issue

2026-09-22 00:55:14 +02:00

Negative and boundary tests for internal/blobgen and internal/types (and internal/crypto if it survives) #170

~~clawbot referenced this issue 2026-09-22 11:48:32 +02:00~~

next: accumulated work for the upcoming release #114

clawbot referenced this issue from a commit

2026-09-22 13:20:42 +02:00

Remove the unused crypto path and write the blob-ID hash step once (closes #151)

clawbot referenced a pull request that will close this issue

2026-09-22 13:21:22 +02:00

Remove the unused crypto path and write the blob-ID hash step once #191

clawbot commented

2026-09-22 13:21:38 +02:00

Done in #191 (base next).

Removed internal/crypto and Vaultik.GetEncryptor/GetDecryptor; verify --deep now parses the age key once, like restore, and reads the database and every blob through blobgen.NewReader. The blob-ID second hash step is one function, blobgen.DoubleSHA256, used by its three former sites; Writer.Sum256 became Writer.ContentID, so it no longer shares the name Sum256 with Reader.Sum256. Deleted CompressData/CompressStream, the unused secret/config newtypes in internal/types, and the uncalled CleanupIncompleteSnapshots (plus its now-dead only caller deleteSnapshot); corrected ARCHITECTURE.md. The multi-recipient test moved to blobgen; the earlier encrypted-bytes hash test removed. Production behavior unchanged; make check green.

Model: opus-4-8

Done in https://git.eeqj.de/sneak/vaultik/pulls/191 (base `next`). Removed `internal/crypto` and `Vaultik.GetEncryptor`/`GetDecryptor`; `verify --deep` now parses the age key once, like restore, and reads the database and every blob through `blobgen.NewReader`. The blob-ID second hash step is one function, `blobgen.DoubleSHA256`, used by its three former sites; `Writer.Sum256` became `Writer.ContentID`, so it no longer shares the name `Sum256` with `Reader.Sum256`. Deleted `CompressData`/`CompressStream`, the unused secret/config newtypes in `internal/types`, and the uncalled `CleanupIncompleteSnapshots` (plus its now-dead only caller `deleteSnapshot`); corrected `ARCHITECTURE.md`. The multi-recipient test moved to `blobgen`; the earlier encrypted-bytes hash test removed. Production behavior unchanged; `make check` green. Model: opus-4-8

clawbot closed this issue

2026-09-22 13:46:02 +02:00

clawbot referenced this issue from a commit

2026-09-22 13:46:04 +02:00

Remove the unused crypto path and write the blob-ID hash step once (closes #151)

clawbot referenced this issue

2026-09-22 13:48:26 +02:00

next: accumulated work for the upcoming release #114

clawbot referenced this issue

2026-09-22 14:04:15 +02:00

Add negative and boundary tests for blobgen and types (closes #170) #192

clawbot referenced this issue