fix(store): stop retaining a verifier for redacted secrets - #133
Conversation
The staging replay digest hashed the request before redaction and stored it beside every other component in plaintext, which turns it into an offline verifier: a low-entropy payload can be recovered by enumerating candidates and comparing hashes. R10 also restricts detection metadata to detector ID, secret type, offset, and length, so retaining the digest violated it outright. The digest is gone. Replay now compares only what R10 permits, and a detection on either side makes the comparison unprovable, so the retry returns Conflict rather than assuming the request is unchanged. A detection-free candidate stays replayable because its stored payload is exact.
|
Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. Your 107 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
You have reached your Codex usage limits for security reviews. Please try again later. |
![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: e4409a6738
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "SELECT extraction_run_id,sensitivity_class,payload,redaction_metadata, | ||
| terminal_state | ||
| FROM candidates WHERE candidate_id=?1", |
There was a problem hiding this comment.
When a detection-free request reuses a candidate ID and payload but changes candidate_kind, this query does not load the stored kind, and every subsequent replay check still passes because the sensitivity and payload are unchanged. The method then reports success and renews the lease while leaving the original kind in candidates, so the caller believes the differently typed candidate was accepted; include candidate_kind in the stored-value comparison.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1afd107. Kilo found the same gap independently, so it is clearly the obvious consequence of the change and I missed it.
Removing the digest meant the comparison had to be rebuilt from real columns, and I left candidate_kind out. It now takes part, so a restage that changes only the kind returns Conflict instead of reporting success and renewing the lease over a stored row of the original kind.
Pinned by a_changed_candidate_kind_is_not_an_idempotent_replay, which also asserts the stored kind is unchanged. Verified it fails when the comparison is removed.
| fn stored_had_detections(metadata: &[u8]) -> bool { | ||
| serde_json::from_slice::<Vec<serde_json::Value>>(metadata) | ||
| .map(|entries| !entries.is_empty()) | ||
| .unwrap_or(true) |
There was a problem hiding this comment.
On upgrade from the parent build, existing core.sqlite files are still classified as the same exact format because neither the schema identity nor format epoch changes, so candidates retain the old {request_digest, detections} blobs—including the offline secret verifier this commit is intended to eliminate—until retention eventually deletes them. This new array-only parser also treats every such blob as having detections, making detection-free active candidates unexpectedly lose idempotent replay after an upgrade; trigger the repository's discard-and-rebuild path or otherwise rewrite/remove the old metadata before accepting the store.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 1afd107. Both halves were right, and this was the more serious of the two findings — the commit removed the verifier from new writes but left it on disk for anyone upgrading, which defeats its own purpose.
I did not take the discard-and-rebuild route. KERNEL_FORMAT_EPOCH aliases DIRECT_FORMAT_EPOCH, which is shared with every direct-format store and its own marker table, so bumping it would invalidate databases this change has no claim over.
Instead, opening a store rewrites any candidates.redaction_metadata still in the {request_digest, detections} shape down to the detection array alone. That removes the verifier from existing rows without discarding the store, and it keeps R10's required metadata intact rather than blanking the column.
Your second observation is fixed too, and it would have been a silent regression: the array-only parser returned unwrap_or(true) for a legacy object blob, so every pre-upgrade candidate was treated as detection-bearing and lost idempotent replay. The reader now accepts both shapes, which also matters because a mid-sweep failure or a concurrent read-only opener can still encounter the old form.
Pinned by opening_a_store_strips_a_legacy_pre_redaction_digest, which plants the parent build's blob, reopens, asserts the digest is gone and the detection array survives, and asserts a detection-free candidate is still replayable afterwards.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| || candidate_terminal.is_some() | ||
| || incoming_redacted | ||
| || stored_had_detections(&stored_metadata) | ||
| || stored_payload != spec.payload.text.as_bytes() |
There was a problem hiding this comment.
WARNING: candidate_kind is not validated during candidate replay
When request_digest was removed in favor of direct column checks, candidate_kind was omitted from the candidate query and replay validation. If a candidate is restaged with the same candidate_id and extraction_run_id but a modified candidate_kind, the check will evaluate the request as an idempotent retry and succeed without returning KernelError::Conflict, leaving the original candidate_kind in the database.
candidate_kind should be selected from candidates and compared against spec.candidate_kind.text.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed and fixed in 1afd107. Codex reported the same thing on the same commit, which is a good sign the finding is real rather than stylistic.
candidate_kind is now selected and compared against spec.candidate_kind.text, exactly as you suggest. Comparing the redacted text is sound here because replay is only offered when neither side carries a detection, so the stored text is byte-exact.
Pinned by a_changed_candidate_kind_is_not_an_idempotent_replay.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous Review Summaries (2 snapshots, latest commit 1afd107)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 1afd107)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit e4409a6)Status: 1 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Reviewed by gemini-3.7-flash · Input: 61.9K · Output: 5.1K · Cached: 151K |
Removing the digest also removed candidate_kind from the replay comparison, so a restage that changed only the kind reported success and renewed the lease while leaving the original kind stored. The kind now takes part in the comparison. A store written by the parent build still holds the digest inside candidates.redaction_metadata, and neither the schema digest nor the shared format epoch changes here, so those rows would have survived until retention removed them. Opening a store now rewrites that metadata to the detection array alone. The reader also accepts both shapes, so a detection-free candidate does not lose idempotent replay on upgrade. DIRECT_FORMAT_EPOCH is shared with every direct-format store, so bumping it to force a rebuild would invalidate databases this change has no claim over.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1afd107141
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| stamp_writer_fence(&mut writer, lease_epoch)?; | ||
| // A store written by the parent build retains a digest of pre-redaction | ||
| // candidate input, so it is rewritten before the store is handed out. | ||
| super::envelope::strip_legacy_candidate_verifiers(&mut writer)?; |
There was a problem hiding this comment.
When upgrading a parent-build database, this migration only performs a logical UPDATE while the connection is in WAL mode. The prior database page—and potentially an older WAL frame—can therefore still contain the complete request_digest blob after KernelStore::open returns, leaving the offline verifier recoverable by anyone reading the SQLite family during the process lifetime. Fresh evidence in this revision is the new rewrite at this line; it needs a purging checkpoint or the discard-and-rebuild path before the store is exposed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Partly mitigated in 08bf437, and I am not claiming it is closed.
Mitigation: secure_delete is enabled for the rewrite so freed pages are zeroed rather than left intact, and the WAL is truncated once the rewrite completes. That removes the straightforward residue.
What I am not asserting: that this proves the old digest bytes are unrecoverable. A logical UPDATE plus zeroing plus truncation is defence in depth, not a guarantee — page reuse, prior WAL frames already checkpointed into the main file, and filesystem-level copies are all outside what this code can promise.
Your alternative is the discard-and-rebuild path, and I agree that is the only construction that actually proves the property. I have not taken it unilaterally because it discards the operator's kernel store, and the fence it needs is entangled with the point you raise in the encoding thread. I have put the choice to the repository owner rather than deciding it inside a follow-up fix. Flagging your comment as the reason.
| detections: metadata, | ||
| }) | ||
| .map_err(|_| KernelError::InvalidInput) | ||
| serde_json::to_vec(&metadata).map_err(|_| KernelError::InvalidInput) |
There was a problem hiding this comment.
When an operator rolls back to the parent binary, the database still has the same schema digest and format epoch, so that binary accepts rows written with this new bare-array encoding and can subsequently stage candidates using its old {request_digest, detections} encoding, reintroducing the offline verifier this change removes. Fresh evidence is that this revision changes the durable serialization here without changing any format identity; the new format must prevent digest-writing releases from reopening it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed, not fixed, and I think this is the decisive finding on the PR.
You are right: the encoding changed without any change to format identity, so a rollback to the parent binary reopens the store happily and stages new candidates with the old {request_digest, detections} encoding, reintroducing exactly what this PR removes.
I want to be explicit that I cannot fix this with a data migration, and that the two P1s here are the same problem. An older binary checks only application_id, the format epoch, and the schema digest. It cannot be taught to reject anything new, so the only possible fence is a change to one of those three. A schema-digest change means a DDL change, which routes through OpenIdentity::Mismatch to quarantine-and-rebuild — the operator's kernel store is set aside and replaced by an empty one. That also resolves the byte-residue question, because no old page survives into the new database.
So there is no fence that preserves the existing store. The choice is between keeping operator data with a mitigation that does not fence rollback, or fencing properly and discarding the kernel store into quarantine.
That is a data-loss decision and not mine to make quietly in a follow-up, so I have raised it with the repository owner rather than picking. DIRECT_FORMAT_EPOCH is also shared with every direct-format store and its own marker table, so bumping the epoch specifically is off the table; the kernel-local lever is the schema digest.
| .map_err(map_sqlite)? | ||
| .collect::<rusqlite::Result<Vec<_>>>() |
There was a problem hiding this comment.
On the first open of a large parent-build store, this collects every matching candidate ID and metadata blob into one in-memory vector and then rewrites all of them in one transaction. Candidate retention is cleaned only by explicit batched maintenance, so the table is unbounded; a long-lived store can therefore exhaust memory or create an oversized WAL transaction during startup before KernelStore::open returns. Iterate and commit the migration in bounded batches instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 08bf437.
The rewrite now runs in committed batches of 256 instead of collecting the whole table. You are right that candidates is bounded only by retention, so a long-lived store could have made both the vector and the WAL transaction arbitrarily large during startup.
Pinned by the_legacy_rewrite_commits_in_batches_past_one_batch_size, which stages 300 candidates, plants the legacy blob on all of them, reopens, and asserts none remain — so it fails if only the first batch is processed.
candidates is bounded only by retention, so loading every legacy row into one transaction at open could exhaust memory or build an oversized WAL transaction. The rewrite now runs in committed batches of 256. secure_delete is enabled for the rewrite so freed pages are zeroed, and the WAL is truncated afterwards. That shrinks the residue but does not prove the old digest bytes are unrecoverable, and it does not stop an older binary from reopening the store and writing the old encoding again. Both require a format identity change, which is left for a decision because the epoch is shared with every direct-format store.
1 participant
Follow-up to #115, which reached
mainwithout this commit.Why this is separate from #115
#115 was never merged deliberately. #116 merged, and because its base was
stack/kernel-05-envelope, the merge carried #115's commits intomainand GitHub marked both PRs merged against the same commit,a64db77c. The sweep capturedb98f2895, one commit before this fix, so the defect below is live onmaintoday. #115 cannot be reopened — GitHub refuses to reopen a merged PR — hence a new PR.The defect
The staging replay path stored
request_digest, a SHA-256 over the candidate request before redaction, insidecandidates.redaction_metadata.Every other component of that digest —
extraction_run_id,candidate_id,extractor,source_kind,source_id,candidate_kind, provenance,source_revision— is stored in plaintext in the same database. The digest therefore reduces to an unsalted hash of the payload with every other input known, which is an offline verifier: anyone with read access tocore.sqlitecan enumerate candidate secret values and compare hashes. For a low-entropy secret that is practical recovery of the value redaction was supposed to destroy.It also violates
magic-context-kh8.1R10 outright: "Detection metadata stores only detector ID, secret type, offset, and length."The digest was added to solve a real problem — two staging requests whose payloads differ only inside a secret redact to identical text, so comparing stored payloads would treat them as the same request and reply with a stale success. The fix for that aliasing reintroduced the exposure it was meant to protect.
What changed
The digest is gone. Replay now compares only R10-permitted metadata plus the stored payload, and a detection on either side makes an unchanged retry unprovable, so it returns
Conflict.The honest consequence, stated plainly: a producer staging a secret-bearing candidate can no longer retry idempotently after a lost response. That is not a gap left by accident. Proving the retry is identical requires exactly the information redaction deliberately destroys. Detection-free candidates remain replayable, because their stored payload is byte-exact.
A keyed verifier (HMAC with a key held outside
core.sqlite) would preserve replay for the secret case. I did not do that here: key placement, rotation, and R12a's "a backup contains a consistentcore.sqlitesnapshot plus every referenced artifact" all need answers, and that belongs in its own change rather than riding along on a security fix.Tests
staging_metadata_retains_no_verifier_for_a_redacted_secret— asserts the stored metadata keys are exactly the R10 set and that nodigest,hash, orfingerprintfield survives. Verified it fails when a digest field is reintroduced.a_secret_bearing_candidate_is_not_replayed_from_a_lossy_payload— asserts the secret-bearing restage conflicts while a detection-free restage still replays.248
mc-storetests pass on this base, including #116's retention and outbox suites.cargo fmt --checkandcargo clippy --all-features --all-targetsclean.Process note
This will recur. Merging a stacked child carries its parent in and marks the parent merged, with whatever commits the parent had at that moment. Worth either merging parents before children or flattening the stack before it lands.