Skip to content

fee: insurance-recovery integration test (FIL-474) - #13

Closed
Peeja wants to merge 1 commit into
claude/fil-569-nqlw8ifrom
claude/fil-474-7ni82g
Closed

fee: insurance-recovery integration test (FIL-474)#13
Peeja wants to merge 1 commit into
claude/fil-569-nqlw8ifrom
claude/fil-474-7ni82g

Conversation

@Peeja

@Peeja Peeja commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

tl:dr: Just an integration test to confirm that we can in fact recover data using the tenant key if needed. This may be overkill? Curious what people think.


What

Adds fee/integration_test.go (package fee_test) — the v1 confidence artifact for the Hilt recovery path (FIL-474). It proves the composed fee API (FIL-569) recovers plaintext from a FEE envelope using only an archived tenant X25519 private key — no region KEK, no Ingot DB.

The round trip, in one test run:

  1. fee.Encrypt seals a few-KB sample to a single ECDH-ES tenant recipient (WithChunkSize at the 4 KiB minimum, so the multi-chunk STREAM path runs).
  2. Decode the envelope header with fee/cose and assert the recipient kid names the tenant key — the explicit on-wire check the issue calls for before recovery.
  3. fee.Decrypt recovers with the archived tenant private key; assert the plaintext matches the original exactly.

Acceptance criteria

  • Round tripTestInsuranceRecoveryRoundTrip.
  • Wrong private keyTestInsuranceRecoveryWrongPrivateKeyFailsBeforeDecrypt: fee.Decrypt fails at the unwrap with aeskw.ErrIntegrity, returns no plaintext reader, and never reaches STREAM decryption (asserted via the absence of any aesstream error).
  • Corrupted protected headerTestInsuranceRecoveryCorruptedProtectedHeaderFailsDecode: flipping a byte in the protected-header bytes makes fee.Decrypt return a wrapped cose.ErrMalformed, rather than a reader over garbage.

Stacked on FIL-569 (PR #14)

This targets claude/fil-569-nqlw8i, not main. Merge #14 first, then this retargets to main automatically. The earlier version of this PR inlined the envelope-assembly glue because no composed API existed; now that FIL-569 provides fee.Encrypt/fee.Decrypt, the test drives that public API instead (the only sub-package call left is the explicit cose.Decode kid assertion).

Notes

  • Tenant keypair is a fixed, non-secret test fixture (the ASCII bytes of fil-474-fee-tenant-recovery-test), checked in for determinism as the issue requires; the public key is derived from the private scalar.
  • kid = the raw X25519 public-key bytes, matching how fee.NewECDHESRecipient treats the kid.
  • Chunk size is aesstream.MinChunkSize (4 KiB), not the 1 KiB the issue suggested — aesstream rejects anything below 4 KiB. Same intent: small chunks so the multi-chunk path runs without a large fixture. The 10 KB sample spans 3 chunks with a partial final chunk.

Testing

GOWORK=off go test -race ./fee/ -run TestInsuranceRecovery -count=1 — all three green under the race detector (fee.Encrypt is goroutine-backed). Full fee tree, go build ./..., go vet ./fee/..., and gofmt all clean.

Closes FIL-474.

🤖 Generated with Claude Code

@Peeja
Peeja force-pushed the claude/fil-474-7ni82g branch from 8fb6f6e to c28efa8 Compare July 3, 2026 20:53
@Peeja
Peeja changed the base branch from main to claude/fil-569-nqlw8i July 3, 2026 20:53
@Peeja
Peeja force-pushed the claude/fil-569-nqlw8i branch 2 times, most recently from fbb1ee8 to 6b0b9c8 Compare July 3, 2026 21:04
@Peeja
Peeja force-pushed the claude/fil-474-7ni82g branch from c28efa8 to 06155ac Compare July 3, 2026 21:38
@Peeja
Peeja force-pushed the claude/fil-569-nqlw8i branch from 19eb203 to 5435a62 Compare July 3, 2026 22:17
@Peeja
Peeja force-pushed the claude/fil-474-7ni82g branch from 06155ac to 3b7e86f Compare July 3, 2026 22:25
@Peeja
Peeja force-pushed the claude/fil-569-nqlw8i branch from 5435a62 to 1214a14 Compare July 3, 2026 22:36
@Peeja
Peeja force-pushed the claude/fil-474-7ni82g branch 2 times, most recently from f2aad36 to 23c529a Compare July 3, 2026 22:55
@Peeja
Peeja marked this pull request as ready for review July 3, 2026 22:56
@bajtos bajtos self-assigned this Jul 30, 2026
@alanshaw
alanshaw changed the base branch from claude/fil-569-nqlw8i to claude/fil-473-s7pjf9 July 31, 2026 13:43
@alanshaw
alanshaw changed the base branch from claude/fil-473-s7pjf9 to claude/fil-569-nqlw8i July 31, 2026 13:44

bajtos commented Jul 31, 2026

Copy link
Copy Markdown
Member

Review

Checked out the head and ran it: gofmt, go vet ./fee/..., the three new tests under -race, and the full fee tree are all green. Behavior is correct — the concern is redundancy.

Mostly duplicates fee_test.go from #14

To the "is this overkill?" question: largely yes. fee/fee_test.go on the base branch is the same package (fee_test) and already covers each acceptance criterion:

New Existing on base
TestInsuranceRecoveryRoundTrip TestRoundTripECDHES (same MinChunkSize, multi-chunk sizes) + TestEnvelopeWireConventions (already asserts the recipient kid)
…WrongPrivateKeyFailsBeforeDecrypt TestDecryptWrongECDHKey — identical ErrIntegrity / NotErrorIs(ErrCorrupted) / Nil(r) assertions
…CorruptedProtectedHeaderFailsDecode TestDecryptCorruptedProtectedHeader — same bytes.Index(blob, RawProtected) / ^= 0xFF technique

Helpers are duplicated in-package too: samplePlaintext is byte-for-byte patternBytes (including its doc comment), encryptToEnvelopeencrypt, streamChunkSize ≈ the local chunk consts. That's not inert — it's a second copy of the byte-flip mechanics to keep in lockstep if the envelope layout changes.

What is new and worth keeping: the fixed checked-in tenant keypair with the public key derived from the private scalar — the thing FIL-474 actually wants to show (recovery starts from the archived private key and nothing else).

Suggestion: keep the round trip as the recovery artifact, reusing patternBytes/encrypt/decryptAll rather than re-declaring near-clones; drop tests 2 and 3 (or reduce them to cases referencing the existing coverage). ~50 lines of signal instead of 186 that mostly restate #14.

One inaccurate comment

In TestInsuranceRecoveryRoundTrip:

With a single recipient the unwrap would still succeed if the kid were wrong or dropped…

Not true here: matchRecipient (fee/fee.go:508) requires an exact kid match with no single-recipient fallback and returns ErrNoMatchingRecipient on a wrong or absent kid — see TestDecryptKidMismatch. The next sentence contradicts it. The on-wire check is worth having as a direct wire-format assertion, not because Decrypt would let it slide.

Minor

  • bytes.Index(blob, RawProtected) could match a coincidental earlier occurrence; offsetting past the tag/array head would harden it (inherited from the existing test).
  • The file opens with a // Package fee_test's … doc comment. No conflict today, but a package doc on an external test file invites a future duplicate — a plain // This file … reads better.
  • It's a make test unit test, not an itest/ one — recovery_test.go would signal the tier better than integration_test.go.

Other notes

Not blocking on correctness; I'd trim before merge.


Generated by Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds an integration test in fee_test to validate the “insurance recovery” workflow: recovering plaintext from a FEE envelope using only an archived tenant X25519 private key (no region KEK, no DB), while asserting the on-wire kid and key failure modes.

Changes:

  • Add TestInsuranceRecoveryRoundTrip exercising fee.Encrypt → on-wire COSE kid assertion → fee.Decrypt.
  • Add negative coverage for wrong private key unwrap failure (must fail before STREAM decryption).
  • Add negative coverage for corrupted protected header decoding (must fail as malformed, no plaintext reader).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread fee/integration_test.go
Add fee/integration_test.go (package fee_test) proving the composed fee API
recovers plaintext from a FEE envelope using only an archived tenant X25519
private key: fee.Encrypt seals a few-KB sample to an ECDH-ES tenant recipient,
and fee.Decrypt recovers it with the tenant private key back to the exact
original.

Rides on the top-level fee package (FIL-569) rather than sequencing the
primitives by hand; the one drop to a sub-package is the explicit on-wire kid
assertion (cose.Decode), which the issue calls for before recovery.

Covers the three acceptance criteria: the round trip; a wrong private key
failing at unwrap (aeskw.ErrIntegrity) before any decryption is attempted, with
no plaintext reader produced; and a corrupted protected header making
fee.Decrypt return a wrapped cose.ErrMalformed rather than a reader over garbage.
The tenant keypair is a fixed, non-secret test fixture checked in for
determinism. Chunk size is aesstream.MinChunkSize (4 KiB), the smallest legal
value, so the multi-chunk path runs without a large fixture.

Stacked on FIL-569 (PR #14).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0199oDgYVzczErg9qbbqmL4B
@Peeja
Peeja force-pushed the claude/fil-474-7ni82g branch from 23c529a to 9ff30b0 Compare July 31, 2026 14:07
@alanshaw

Copy link
Copy Markdown
Member

Cherry picked into #32

@alanshaw alanshaw closed this Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants