Security review of the crypto and blob-generation seams #73

New Issue

clawbot · 2026-08-09T03:43:47+02:00

clawbot commented

2026-08-09 03:43:47 +02:00

README:409-414 already names a security audit as the number one pre-1.0
blocker. This issue tracks it.

The crypto path is the least-tested part of the codebase relative to its
risk: internal/crypto has 178 test LOC and internal/blobgen 190,
between them covering the entire compress → encrypt → hash writer and the
matching reader. For comparison, internal/database has 4,203.

Scope

Review, with findings written up on this issue:

Key handling. age recipient/identity plumbing end to end. Confirm
the private key is never required, read, or logged on the backup path.
config.AgeSecretKey (internal/config/config.go:132) puts a private
key on the backed-up host when set — audit every use, and confirm it is
optional in fact and not just in intent. Note that #64 concerns a code
path currently gated on this field.
Nonce/IV handling and reuse. Any construction where a nonce could
repeat across blobs or across runs.
Integrity before use. Is authentication verified before decrypted
plaintext is acted on, or is there anywhere plaintext is used ahead of
its tag being checked?
Hash usage. internal/types (229 lines of ID/hash newtypes with
driver.Valuer/sql.Scanner) sits on the DB boundary with zero
tests — a faulty Scan silently corrupts restores. Audit and test.
Compression-then-encryption. Confirm the ordering, and document
what it leaks (compressed-size side channel) rather than leaving it
implicit.
Error paths. Confirm no key material, plaintext, or partial
plaintext reaches logs or error strings, including at --debug.
Chunk boundaries as a side channel. FastCDC boundaries are
content-derived; document what that reveals to an observer of blob
sizes.

Definition of done

Every scope item above is explicitly addressed with a written finding
on this issue — including the ones that come back clean. "No issue
found in X" is a required output, not an omission.
internal/crypto, internal/blobgen, and internal/types reach
coverage proportionate to their risk, including negative tests: wrong
key, truncated ciphertext, corrupted tag, wrong recipient, empty
input, and boundary-size inputs.
Any vulnerability found is filed as its own issue with a severity
assessment and linked here. Do not fix silently inside this issue.
Findings that are accepted risks rather than bugs are documented in
docs/ so users can evaluate them, and cross-referenced from the
threat-model section (see #67, which corrects the privacy docs).
No weakening of any existing assertion or test to make something pass.
make check green.

Note

This is a review issue, so its output is findings plus tests, not a
refactor. If the review concludes the design is sound, that conclusion —
written down with reasoning — is a complete and successful outcome.

README:409-414 already names a security audit as the number one pre-1.0 blocker. This issue tracks it. The crypto path is the least-tested part of the codebase relative to its risk: `internal/crypto` has 178 test LOC and `internal/blobgen` 190, between them covering the entire compress → encrypt → hash writer and the matching reader. For comparison, `internal/database` has 4,203. ## Scope Review, with findings written up on this issue: 1. **Key handling.** `age` recipient/identity plumbing end to end. Confirm the private key is never required, read, or logged on the backup path. `config.AgeSecretKey` (`internal/config/config.go:132`) puts a private key on the backed-up host when set — audit every use, and confirm it is optional in fact and not just in intent. Note that #64 concerns a code path currently gated on this field. 2. **Nonce/IV handling and reuse.** Any construction where a nonce could repeat across blobs or across runs. 3. **Integrity before use.** Is authentication verified before decrypted plaintext is acted on, or is there anywhere plaintext is used ahead of its tag being checked? 4. **Hash usage.** `internal/types` (229 lines of ID/hash newtypes with `driver.Valuer`/`sql.Scanner`) sits on the DB boundary with **zero tests** — a faulty `Scan` silently corrupts restores. Audit and test. 5. **Compression-then-encryption.** Confirm the ordering, and document what it leaks (compressed-size side channel) rather than leaving it implicit. 6. **Error paths.** Confirm no key material, plaintext, or partial plaintext reaches logs or error strings, including at `--debug`. 7. **Chunk boundaries as a side channel.** FastCDC boundaries are content-derived; document what that reveals to an observer of blob sizes. ## Definition of done 1. Every scope item above is explicitly addressed with a written finding on this issue — including the ones that come back clean. "No issue found in X" is a required output, not an omission. 2. `internal/crypto`, `internal/blobgen`, and `internal/types` reach coverage proportionate to their risk, including negative tests: wrong key, truncated ciphertext, corrupted tag, wrong recipient, empty input, and boundary-size inputs. 3. Any vulnerability found is filed as its own issue with a severity assessment and linked here. Do not fix silently inside this issue. 4. Findings that are accepted risks rather than bugs are documented in `docs/` so users can evaluate them, and cross-referenced from the threat-model section (see #67, which corrects the privacy docs). 5. No weakening of any existing assertion or test to make something pass. 6. `make check` green. ## Note This is a review issue, so its output is findings plus tests, not a refactor. If the review concludes the design is sound, that conclusion — written down with reasoning — is a complete and successful outcome.

clawbot added this to the 1.0.0 milestone 2026-08-09 03:43:47 +02:00

clawbot referenced this issue

2026-08-09 03:44:09 +02:00

Documentation accuracy sweep: undocumented behavior and stale claims #74

clawbot referenced this issue

2026-08-09 03:44:52 +02:00

Triage the 15 stale remote branches; two may contain unlanded fixes #71

clawbot referenced this issue

2026-08-09 03:52:01 +02:00

Decide on renaming internal/log, internal/crypto and internal/types (revive var-naming) #76

clawbot referenced this issue

2026-08-09 10:13:00 +02:00