core: the store sink refuses custody-tombstone material on every write path

[iceteaSA]

You flagged this at the #28 merge: ck auth put --replace still accepted a tombstone sentinel unconditionally. It does, and the refusal was in the wrong layer.

The defect

CUSTODY_TOMBSTONE_PREFIX was checked at the OpenCode import entrances only — opencode_migration.rs, opencode_accounts.rs:87, and the OAuth source parsers at oauth.rs:173/:334. Reproduced on 90b69a3 before touching anything, against a scratch vault:

$ ck-auth put --id apikey:probe --payload 'sk-real-key-material'
created apikey:probe
$ ck-auth put --id apikey:probe --payload 'claustrum-tombstone:v1:anthropic' --replace
replaced apikey:probe (unconditional)

A tombstone is the marker the vault writes into a consumer's store to mean "the real material lives in the vault". Storing it in the vault inverts custody: the vault serves the marker that means the vault holds the secret, and the real family exists nowhere.

The fix

The refusal moves to seal_record — the chokepoint your empty-payload guard already uses, with the same reasoning you wrote there: a constructor is sidesteppable by a struct literal, a chokepoint is not. It covers a static payload prefix and both OAuth token fields, and is_custody_tombstone becomes pub so the parsers and the sink share one definition rather than two that drift.

The entrance checks stay. They give a better message at the CLI boundary, and they are cheap.

Why a sink and not another entrance

Entrance checks enumerate doors, and the door that matters is the one added next. The live caller that makes this concrete is the unattended re-sealer running on this box: it reads a consumer's credential store and re-seals whatever it finds, so the moment that store carries the tombstone — which is the whole point of the tombstone — the sealer is holding the sentinel as if it were a token. It is currently held off by a shape check in a shell script.

Evidence

every_write_path_refuses_custody_tombstone_material follows your every_write_path_refuses_an_empty_non_oauth_payload: it enumerates the writers rather than trusting the chokepoint. RED on 90b69a3 at create must refuse tombstone material before the guard existed.

Three mutations, each proving a different half:

mutation	expected red	observed
prefix → substring	ordinary key containing the sentinel	store.rs:4580 — sk-claustrum-tombstone:... refused
drop the OAuth arm	access-token arm	store.rs:4533
drop only the refresh half	refresh-token arm	store.rs:4549 — a refresh token carrying the sentinel must be refused

The third exists because the second could not reach it: the test aborts at the first failed assert, so deleting the whole OAuth arm proves only the access half. Each restored byte-identical and re-run green.

End-to-end on a scratch vault with the built binary:

ARM 1  replace with tombstone            exit 1, "custody-tombstone material is not a credential;
                                          storing it would invert custody"
ARM 2  replace with real material         replaced apikey:probe (unconditional)
ARM 3  key CONTAINING the sentinel        created apikey:contains        (prefix, not substring)
ARM 4  refusal echoes the payload?        0 occurrences  (arms 1-3 prove the vault opens,
                                          so the zero is not vacuous)
audit  3 rows for 3 successful writes; the 2 refused writes left none

The message is a fixed sentence and never interpolates the offending value — it is caller-supplied and may sit adjacent to real material.

Gate green: 600 tests measured. I have not touched the run_expect floor — #33 already moves that number, and a third branch editing it would make a conflict whose correct value can only be measured on the merged tree. Lockfile byte-identical to master's; branched from 90b69a3 with the siblings the daemon is built from.

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

---

Summary by cubic

Moves custody-tombstone refusal from the import entrances to the store sink so every write path rejects the sentinel, not just the parsers that happen to see it. Previously a generic put --replace could overwrite a working credential with the tombstone marker; now the sink refuses it on create, replace, and audited paths, and the OAuth access and refresh token fields are covered too.

• The entrance checks stay for better CLI errors; the sink covers the chokepoint all writers pass through.
• A new test enumerates every write path and confirms ordinary payloads and keys containing the sentinel as a substring still work.

^{Written for commit 3ed1d16. Summary will update on new commits.}

[cubic-dev-ai]

3 issues found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="crates/credentials-core/src/oauth.rs">

<violation number="1" location="crates/credentials-core/src/oauth.rs:22">
P3: Both consumers of `is_custody_tombstone` live inside the `credentials-core` crate (oauth.rs and store.rs, the latter via `use crate::oauth::...`), so this helper only needs crate visibility. Making it fully `pub` unnecessarily expands this library crate's public API surface, permanently committing to the `fn(&str)->bool` signature. Use `pub(crate)` unless the function is genuinely intended for external callers.</violation>
</file>

<file name="crates/credentials-core/src/store.rs">

<violation number="1" location="crates/credentials-core/src/store.rs:2805">
P2: When a vault already contains a tombstone from the pre-fix path, `rotate_master_key` re-seals its plaintext directly and never invokes `seal_record`, so key rotation preserves the invalid credential. Validate decoded records before re-sealing or quarantine/refuse such rows.</violation>

<violation number="2" location="crates/credentials-core/src/store.rs:2805">
P2: When an OAuth record has a tombstone in `payload` but ordinary token fields, this `record.kind` guard skips the payload check and stores material that `get` serves. Check `payload` for the prefix regardless of kind; the existing OAuth-field checks still reject canonical tombstones.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: When a vault already contains a tombstone from the pre-fix path, rotate_master_key re-seals its plaintext directly and never invokes seal_record, so key rotation preserves the invalid credential. Validate decoded records before re-sealing or quarantine/refuse such rows.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-core/src/store.rs, line 2805:

<comment>When a vault already contains a tombstone from the pre-fix path, `rotate_master_key` re-seals its plaintext directly and never invokes `seal_record`, so key rotation preserves the invalid credential. Validate decoded records before re-sealing or quarantine/refuse such rows.</comment>

<file context>
@@ -2797,6 +2798,25 @@ impl EncryptedStore {
+        // that matters is the one added next. The unattended re-sealer is the live caller that
+        // makes this concrete: it re-seals whatever it finds in a consumer store, and that
+        // store is exactly where the tombstone gets written.
+        if (record.kind != CredentialKind::Oauth
+            && record
+                .payload
</file context>

[cubic-dev-ai]

P2: When an OAuth record has a tombstone in payload but ordinary token fields, this record.kind guard skips the payload check and stores material that get serves. Check payload for the prefix regardless of kind; the existing OAuth-field checks still reject canonical tombstones.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-core/src/store.rs, line 2805:

<comment>When an OAuth record has a tombstone in `payload` but ordinary token fields, this `record.kind` guard skips the payload check and stores material that `get` serves. Check `payload` for the prefix regardless of kind; the existing OAuth-field checks still reject canonical tombstones.</comment>

<file context>
@@ -2797,6 +2798,25 @@ impl EncryptedStore {
+        // that matters is the one added next. The unattended re-sealer is the live caller that
+        // makes this concrete: it re-seals whatever it finds in a consumer store, and that
+        // store is exactly where the tombstone gets written.
+        if (record.kind != CredentialKind::Oauth
+            && record
+                .payload
</file context>

Suggested change

	if (record.kind != CredentialKind::Oauth
	&& record
	.payload
	.expose()
	.starts_with(CUSTODY_TOMBSTONE_PREFIX.as_bytes()))
	if record.payload.expose().starts_with(CUSTODY_TOMBSTONE_PREFIX.as_bytes())

[cubic-dev-ai]

P3: Both consumers of is_custody_tombstone live inside the credentials-core crate (oauth.rs and store.rs, the latter via use crate::oauth::...), so this helper only needs crate visibility. Making it fully pub unnecessarily expands this library crate's public API surface, permanently committing to the fn(&str)->bool signature. Use pub(crate) unless the function is genuinely intended for external callers.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-core/src/oauth.rs, line 22:

<comment>Both consumers of `is_custody_tombstone` live inside the `credentials-core` crate (oauth.rs and store.rs, the latter via `use crate::oauth::...`), so this helper only needs crate visibility. Making it fully `pub` unnecessarily expands this library crate's public API surface, permanently committing to the `fn(&str)->bool` signature. Use `pub(crate)` unless the function is genuinely intended for external callers.</comment>

<file context>
@@ -18,7 +18,8 @@ use crate::secret::SecretString;
 
-fn is_custody_tombstone(value: &str) -> bool {
+/// Shared definition used by import parsers and the store sink so the two cannot drift.
+pub fn is_custody_tombstone(value: &str) -> bool {
     value.starts_with(CUSTODY_TOMBSTONE_PREFIX)
 }
</file context>

Suggested change

	pub fn is_custody_tombstone(value: &str) -> bool {
	pub(crate) fn is_custody_tombstone(value: &str) -> bool {

[ckcred-alfonso]

Gated at c9bb371 in a worktree beside the repo. The guard is in the right place and the test name is accurate — which is the first thing I check when a name claims more than one site can deliver.

seal_record is a genuine chokepoint, not one caller among several:

EncryptedStore::create_audited                              1046
EncryptedStore::overwrite_cas_audited                       1141
EncryptedStore::overwrite_unconditional_with_identity_policy 1301
EncryptedStore::set_identity_audited                        1404
EncryptedStore::commit_refresh                              2253

So a guard there covers every path that seals a record, and the CLI-level check it replaces could never have covered the route-plane admin path. That was the gap: I demonstrated it on my own live store when merging #28, with put --replace sealing a sentinel as active material.

Mutation-verified on my side rather than taken from your notes. Short-circuiting the guard condition to false reddens every_write_path_refuses_custody_tombstone_material by name on the full credentials-core target, and the tree diffs clean after restore.

Your red gate is not yours. Sixteenth sibling lock wave, subc-core 0.17.21, landed between your push and my review:

your branch, your lock       GATE FAILED: clippy — cannot update the lock file
your branch, master's lock   GATE PASSED

Master was refusing identically at the same moment, which is the control worth running before attributing a lockfile refusal to a branch. It is absorbed on master now, so a rebase clears it.

The arm I would like before merging

The test drives create_audited and overwrite_unconditional_audited. It does not drive commit_refresh, and that is the one caller whose input comes from a provider rather than an operator — every other path is fed by someone who typed the value.

Refusing there is almost certainly correct. What I cannot tell from the diff is what the refusal costs, because seal_record is called with ? before anything commits:

let blob = self.seal_record(credential_id, &new_record)?;

So a tombstone-shaped access token now aborts the commit — and the questions that decides are: does the durable refresh_intent get cleared, or does it dangle? Does the record stay serviceable at its previous version, or does the next boot reconciliation read the dangling intent and latch needs_reauth on a credential that was healthy?

There is precedent for the shape: an empty access token from a provider already clears the intent and fails closed with a decode error. If the tombstone case takes that same path then the answer is "nothing, it behaves like the empty-token case" and an arm asserting exactly that is cheap. If it takes a different path, that is worth knowing before this ships rather than after, because the failure would arrive on a healthy credential during an ordinary refresh.

Not a defect claim — I have not tested it, and I am not going to add the arm myself and take the finding off your branch. One assertion either way and this merges.

Smaller

The payload check excludes CredentialKind::Oauth and handles OAuth records through the token fields instead. That reads correct to me (for an OAuth record the payload duplicates the access token, so checking both would be redundant) — worth one line in the comment saying so, since the exclusion looks like a hole to a reader who does not know the payload/token relationship.

[iceteaSA]

Rebased past 3c509b3, head 3ed1d16. Same stale lockfile as #33 — subc-core 0.17.20 against your 0.17.21 — which per your note on that PR is invisible from my side, since Test is skipped on fork PRs and the fork-safe job reports SUCCESS for correctly declining to run. Re-gated rather than assuming a deps-only base move is inert:

workspace arm, its own summation   13 suites, 579 passed
real-daemon e2e                     9 passed
                                    GATE PASSED

I left the floor at 578 on purpose, and the number above is why you may want to know that. This branch adds one test, so the measured count is 579. #33 moves the same line to 590. A third branch editing it would create a conflict whose correct value cannot be derived from any of the three sides — the mistake I made on #33 this morning by reaching for arithmetic. Leaving it alone is safe in either merge order: #33 first gives floor 590 against a measured 591; this first leaves 578 against 579. Both pass, and neither hides a loss, because a floor is a minimum and you re-measure at merge anyway.

If you would rather the floor track exactly, the number to set after both land is 591 — measured, not summed.