FEE wrap material on blob_encryption_params (FIL-480) - #15
Conversation
There was a problem hiding this comment.
Pull request overview
Extends the local ingot.blob_locations record shape to include the per-blob FEE (FilOne encryption envelope) wrap material needed for range reads to decrypt encrypted blobs without an extra envelope-header fetch, along with SQL migration and store/test updates.
Changes:
- Adds a new SQL migration to extend
ingot.blob_locationswith six nullable FEE wrap-material columns (plus Down migration). - Extends
registry.BlobLocationand updates the Postgres/inmemLocationStoreimplementations to read/write these new fields (including deep-copying byte slices in-memory). - Adds/extends live-Postgres + migration + inmem tests to validate NULL/bytea round-trips, aliasing safety, and re-wrap updates.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| registry/stores.go | Expands BlobLocation to carry FEE wrap material and updates doc comments/contracts. |
| registry/stores_postgres.go | Updates PutLocation/GetLocation SQL to write/read new nullable columns and adds nullInt64. |
| registry/postgres_live_test.go | Adds live-Postgres tests for NULL semantics, bytea round-trips, and re-wrap-in-place behavior. |
| migrations/up_live_test.go | Verifies new columns exist and are nullable after migration. |
| migrations/sql/00004_blob_encryption.sql | Adds the actual schema migration for FEE wrap material columns on blob_locations. |
| inmem/stores.go | Ensures in-memory store deep-copies new byte-slice fields to avoid aliasing. |
| inmem/stores_test.go | Adds inmem tests for wrap-material round-trip, aliasing safety, and re-wrap-in-place behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
be552e3 to
a59b01d
Compare
Extend ingot.blob_locations with the FEE encryption columns a (range) GET needs to decrypt an encrypted blob without fetching the COSE envelope header: region_wrapped_cek, region_key_version, tenant_recipient_kid, base_nonce, chunk_size, and protected_header. All nullable — an unencrypted blob (the appliance topology today) leaves them NULL. Rather than a new object/segment table, these live on blob_locations: it is already keyed by (space, digest), and a fresh CEK per encryption event makes each ciphertext digest unique to one encryption, so the wrap material is a 1:1 fact about the row — like the existing provider/url/size. Object-level values (plaintext size/ETag/identity) stay in the content-addressed ObjectManifest; overwrite already swaps the MST leaf to a new manifest CID atomically, so no plaintext_* columns and no generation counter are needed. Raw CEK bytes are never stored — only the region-KEK-wrapped CEK plus opaque key-version/recipient identifiers, so no schema change is needed if the region-key or Hilt wrap-key cardinality decisions (FIL-572/FIL-574) later go multi-key. Per-blob crypto-shred is nulling these columns or deleting the row; a rotation re-wrap is a PutLocation with a new wrapped CEK + key version. - migration 00004: additive ALTER TABLE ADD COLUMN (+ Down) - registry.BlobLocation gains the six fields; Put/GetLocation carry them (nullString/nullInt64 for optional text/bigint, bytea via nil) - inmem MemStore deep-copies the new byte-slice fields - tests: inmem FEE round-trip + mutation-safety; live-PG bytea/NULL round-trip and re-wrap-in-place; migration column existence + nullability Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XSxwh7XfPtFQtyeoDR65hC
bc83565 to
e054d52
Compare
…IL-480) Addresses a PR review note: BlobLocation documents the FEE wrap fields as all-or-nothing, but PutLocation accepted a partial set (e.g. a wrapped CEK with no nonce, or an empty-but-non-nil byte slice stored as a non-NULL empty bytea), silently persisting a row the decrypt path cannot use. - BlobLocation.ValidateFEE enforces the invariant: either all six wrap fields are present (non-empty, chunk_size > 0) or none; a partial set returns ErrPartialFEE. Both PutLocation implementations (Postgres + inmem) call it. - nullBytes maps empty/nil byte slices to SQL NULL so an absent FEE column never lands as a non-NULL empty bytea. - tests: partial sets rejected (and leave no row) in the inmem and live-PG suites; the fully-absent (unencrypted) row is still accepted. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XSxwh7XfPtFQtyeoDR65hC
go-check flagged inmem/stores_test.go as not gofmt-ed: the TestLocations_ PartialFEE_Rejected map keys were aligned against a multi-line entry. Run gofmt -w. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XSxwh7XfPtFQtyeoDR65hC
bajtos
left a comment
bajtos
left a comment
There was a problem hiding this comment.
The code looks good to me 👍🏻
However, I strongly dislike that we are adding non-location fields to blob_locations table. Let's rename the table and interfaces (e.g. PutLocation) to use a more appropriate name, like BlobRecord or BlobMetadata.
Two textual conflicts (registry/stores.go imports, appended subtests in registry/postgres_live_test.go) plus two the merge resolved cleanly but would not have compiled: - main retyped the space column as did.DID (a struct); the FEE tests still passed string literals. Replaced with testutil.RandomDID. - main's migrations reached 00012, colliding with this branch's 00004 blob-encryption migration. Renumbered it to 00013; it only adds nullable columns, so running last is equivalent. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net> Assisted-by: Claude:claude-opus-5
Review of the FIL-480 draft rejected extending blob_locations with the per-blob FEE wrap material. blob_locations is a reconstructible cache of the indexing-service contract — every row can be re-derived from the indexer or the accept receipt, and the table disappears when the topology moves to a real indexer. A wrapped CEK is not reconstructible: lose the row and the ciphertext is unreadable forever. Key material therefore does not belong in the one table whose design allows it to be rebuilt, or truncated (the live test already does). blob_encryption_params holds it instead, with no foreign key to blob_locations: the two have independent lifecycles, and an FK would force location-before-params write ordering. Because nothing cascades, DeleteLocation no longer shreds — a caller removing a blob must delete from both tables. Two fixes to the material itself, both raised in review: - Add header_len. The stored blob is envelope||ciphertext and nothing recorded where the envelope ends, so a read could not locate byte 0 of the ciphertext without decoding the header, leaving the "no header round-trip" goal out of reach. - Store the whole COSE Enc_structure as aad instead of the protected header. The structure's context string differs between a COSE_Encrypt and a COSE_Encrypt0, which a bare row cannot record, so the protected header alone is not enough to rebuild the AAD. The header stays recoverable from the Enc_structure as element 1. Every column is NOT NULL, so the all-or-nothing invariant is structural rather than a nullable-column check: the existence of a row is what marks a blob as encrypted. BlobEncryptionParams.Validate rejects an incomplete set before SQL, replacing ValidateFEE/ErrPartialFEE. Migration 00013 is reshaped in place — it has not shipped anywhere. Assisted-by: Claude:claude-fable-5 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
|
I updated the pull request with te following changes:
A side benefit: the segment re-ship path at server.go:333 re-upserts a location with no FEE fields, which under the old single-table full-row upsert would have silently nulled a stored CEK. That can no longer happen. |
A region-key rotation replaces the wrapped CEK and its key version and nothing else, but the only way to write that was PutEncryptionParams, which rewrites every column. The ciphertext is unchanged across a rotation, so a caller that got the nonce, chunk size, AAD or header length wrong on the re-supplied set would corrupt a decryptable row. RewrapEncryptionParams takes only the two parameters that change: - Postgres issues a plain UPDATE of the two columns. Zero rows affected means the blob has no row — not encrypted, or already shredded — so it returns ErrNotFound instead of reporting success. - ValidateRewrap rejects an empty CEK or key version, so a rotation cannot blank out the material the decrypt path needs. PutEncryptionParams keeps its upsert semantics for re-encryption, which does replace the whole set. Assisted-by: Claude:claude-opus-5 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
bajtos
changed the title
We will store keys in OpenBao. Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
Both sides added a table to the live store test's TRUNCATE list; the resolution keeps both. Both sides also added a migration numbered 00013, which goose rejects as a duplicate version. Main's 00013_revocation_cursor is already published, so the branch's blob_encryption migration moves to 00014. Assisted-by: Claude:claude-opus-5 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
|
@codex review |
| // cloneLocation deep-copies a BlobLocation's digest so the stored copy and any | ||
| // returned copy never alias the caller's slice. | ||
| func cloneLocation(loc registry.BlobLocation) registry.BlobLocation { | ||
| loc.Digest = cloneBytes(loc.Digest) |
There was a problem hiding this comment.
Written by Claude.
Done, and removed the local cloneBytes helper in favour of bytes.Clone everywhere in the file.
| @@ -0,0 +1 @@ | |||
| /Users/bajtos/src/fil-forge/ingot/.claude No newline at end of file | |||
There was a problem hiding this comment.
Written by Claude.
Removed. It is a symlink my worktree setup creates, committed by accident. I left it out of .gitignore on purpose so the repo can still check in a shared .claude/ config later.
| @@ -0,0 +1,49 @@ | |||
| -- +goose Up | |||
| -- FEE (FilOne encryption envelope) per-blob encryption parameters. When Ingot | |||
There was a problem hiding this comment.
Technically Filecoin Encryption Envelope not FilOne.
| -- FEE (FilOne encryption envelope) per-blob encryption parameters. When Ingot | |
| -- FEE (Filecoin Encryption Envelope) per-blob encryption parameters. When Ingot |
There was a problem hiding this comment.
Written by Claude.
Fixed, here and in the Go doc comment.
| CREATE TABLE ingot.blob_encryption_params ( | ||
| space text NOT NULL, | ||
| digest bytea NOT NULL, -- ciphertext blob multihash, as in blob_locations | ||
| tenant_recipient_kid text NOT NULL -- opaque id of the Hilt wrap key (insurance-recovery unwrap) |
There was a problem hiding this comment.
Do we need to store this? It's not something we will ever use and is stored in the header anyway, right? Or am I misunderstanding?
There was a problem hiding this comment.
Is this bytes or a DID?
EDIT: seems to be a DID with a fragment.
There was a problem hiding this comment.
Written by Claude.
You are right, dropped. The table caches only what a range GET needs to skip the header fetch, and that read unwraps via the region KEK. The insurance-recovery unwrap is rare and out-of-band, and it reads the envelope header anyway, where the COSE recipients live. Column, struct field, and tests are gone.
There was a problem hiding this comment.
Written by Claude.
Moot now, the column is gone. It was a DID with a fragment.
| } | ||
|
|
||
| // BlobEncryptionParams is one row of ingot.blob_encryption_params: the FEE | ||
| // (FilOne encryption envelope) parameters a read needs to decrypt an encrypted |
There was a problem hiding this comment.
| // (FilOne encryption envelope) parameters a read needs to decrypt an encrypted | |
| // (Filecoin Encryption Envelope) parameters a read needs to decrypt an encrypted |
|
|
||
| // TenantRecipientKID identifies the Hilt wrap key used for insurance-recovery | ||
| // unwrap. Opaque, so it is agnostic to the tenant-vs-bucket granularity | ||
| // decision (FIL-574). |
There was a problem hiding this comment.
Lets not add references to private linear tickets in OSS code.
There was a problem hiding this comment.
Written by Claude.
Dropped both ticket ids from the doc comments. The explanation stands without them.
| // an AAD with no nonce, or an empty (non-nil) byte slice standing in for a real | ||
| // value — would persist a row a later GET cannot decrypt, so | ||
| // PutEncryptionParams rejects it before touching the store. | ||
| func (p BlobEncryptionParams) Validate() error { |
There was a problem hiding this comment.
Typically I'd follow the parse, don't validate approach. When the data has been parsed/decoded from untrusted source at an API boundary or is created by a constructor there's not a need to re-validate again. It's just duplication, and in this case duplication that is also applied by the database layer.
There was a problem hiding this comment.
Written by Claude.
Agreed. Validate and ErrInvalidEncryptionParams are gone; the NOT NULL/CHECK constraints are the single enforcement point. The live test now asserts the constraint rejects a partial set instead of asserting a Go error. The inmem fake will accept a row Postgres would refuse, which seems right for a fake.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf5de99614
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Address review on #15. Drop tenant_recipient_kid. The table caches only what a range GET needs to skip the header fetch, and that read unwraps via the region KEK. The insurance-recovery unwrap is rare, out-of-band, and reads the envelope header anyway, where the COSE recipients already live. Drop BlobEncryptionParams.Validate. Every column is NOT NULL with a CHECK, so a half-populated set was being rejected twice. Also: FEE expands to Filecoin Encryption Envelope, not FilOne; use bytes.Clone in place of the local cloneBytes helper; untrack the worktree's .claude symlink; and drop the Linear ticket ids from the doc comments. Assisted-by: Claude:claude-opus-5 Signed-off-by: Miroslav Bajtoš <oss@bajtos.net>
|
Written by Claude. Since the update comment above: The |
tl;dr: This adds everything Ingot needs to do the read-path decryption. It stores a wrapped CEK, as well as other info that's in the FEE header that's needed for decryption. That means that Ingot can perform a range read by grabbing just the bytes it needs to decrypt (rounded up to encryption chunk boundaries), and not also fetch the header, which is likely not contiguous.
It's all stored oningot.blob_locationsbecause that feels like the right entity shape-wise, but the name may no longer be appropriate. @frrist, should we broaden the name of this table?See #15 (comment) for what changed since the description below.
Summary
Implements FIL-480. Extends the existing
ingot.blob_locationstable with the per-blob FEE (FilOne encryption envelope) wrap material that the read path needs to decrypt an encrypted blob — so a (range) GET can unwrap the CEK and go straight to a body-range fetch, with no COSE envelope-header round-trip.Why extend
blob_locationsrather than add a tableAn earlier draft of the issue proposed a new
object_encryption/ per-segment table withplaintext_size/plaintext_etag/ agenerationcounter. On review those turned out redundant, and the issue was rewritten to an extension:ObjectManifestcarriesETagandBody.Size/MD5/SHA256— properties of the plaintext, computed pre-encryption in the PUT pipeline (FIL-481). No new columns.(space, digest)row. A fresh CEK per encryption event makes every ciphertext digest unique to one encryption (never shared across objects, even for identical plaintext) — exactly like the existingprovider/url/sizecolumns. No region column either: one Ingot instance is one region, soregion_key_versionalone identifies the KEK version.What changed
migrations/sql/00004_blob_encryption.sql— additiveALTER TABLE ingot.blob_locations ADD COLUMNfor six nullable columns (region_wrapped_cek,region_key_version,tenant_recipient_kid,base_nonce,chunk_size,protected_header), plus aDown. An unencrypted blob (the appliance topology today) leaves themNULL.registry.BlobLocationgains the six fields (documented);PutLocation/GetLocationcarry them (nullString/nullInt64for optional text/bigint,byteavia nil). The upsert overwrites full row state, so a rotation re-wrap is a read-modify-write.inmem.MemStoredeep-copies the new byte-slice fields so stored/returned copies don't alias.bytea/NULLround-trip, re-wrap, and the unencrypted (all-NULL) case; migration column existence + nullability.Security / acceptance
region_key_version/tenant_recipient_kidare opaque, so no schema change is needed if the region-key or Hilt wrap-key cardinality decisions (FIL-572 / FIL-574) later go multi-key.DeleteLocation); no separate mechanism. Worth confirming against FIL-489's reference-counted delete.Testing
GOWORK=off go build ./...✅ ·go vet ./...✅go test ./...passes for all packages except the pre-existing, environment-only failure in./testing/(unable to add custom RootCAs HTTPClient …— reproduced identically onmainwith none of these changes; it's the sandbox's HTTPS-proxy CA setup, not this diff).INGOT_TEST_DSNand were not run here (no DB in this sandbox). Please run them in CI/local:Open question for reviewers (non-blocking)
blob_locations's doc comment described it narrowly as the(space, digest) → provider/URLmapping. Adding encryption material broadens its role to "the full per-blob record." I've updated the doc comments to reflect that but left the table name as-is. Should it be renamed (e.g.blob_records)? Flagging per the issue — happy to do the rename in a follow-up if we want it.