auth: records carry an account identity; set-identity and import set it, replace keeps it - #30
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 12 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
0754ebc to
31b00c6
Compare
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
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/admin_ops.rs">
<violation number="1" location="crates/credentials-core/src/admin_ops.rs:420">
P2: When `clear_identity` is true but the wire record contains identity fields, this call only disables preservation of the old identity; it does not clear the incoming identity. Clear or reject `record.identity` before calling the store so the operation guarantees that `--clear-identity` drops identity for every authenticated caller.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| store.overwrite_unconditional_with_identity_policy_audited( | ||
| &id, | ||
| &record, | ||
| !clear_identity, |
There was a problem hiding this comment.
P2: When clear_identity is true but the wire record contains identity fields, this call only disables preservation of the old identity; it does not clear the incoming identity. Clear or reject record.identity before calling the store so the operation guarantees that --clear-identity drops identity for every authenticated caller.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/credentials-core/src/admin_ops.rs, line 420:
<comment>When `clear_identity` is true but the wire record contains identity fields, this call only disables preservation of the old identity; it does not clear the incoming identity. Clear or reject `record.identity` before calling the store so the operation guarantees that `--clear-identity` drops identity for every authenticated caller.</comment>
<file context>
@@ -365,6 +407,29 @@ pub fn apply(
+ store.overwrite_unconditional_with_identity_policy_audited(
+ &id,
+ &record,
+ !clear_identity,
+ AuditCtx::route_admin(audit_op.to_audit_op(), actor),
+ )?;
</file context>
|
Verified the parts I would have got wrong myself, at The re-seal pin is non-vacuous, which is the thing I most wanted to check. It also pins the four things a re-seal could quietly move: payload bytes, lifecycle state,
One precondition worth naming at the siteThe doc comment on
True, and the premise is doing more work than it says. It holds when the replacement token belongs to the same account. When it does not, the retained label attaches to a different account's material, and the vault serves a confident wrong answer where it previously served none. That asymmetry is the whole reason I am not asking you to solve the general case, because it is not solvable — for most adapters nothing in the incoming material names its account. But What I would take as the minimum is the precondition stated at the doc comment, so the next reader sees what the preservation assumes rather than only why it is good. The mismatch check for the detectable adapter is a follow-up I would review, not a condition on this. Smaller
Floor at 491 on the current head (484 in the description, so it moved with the branch): check it against the measured total rather than leaving slack. A floor under its population stops being a floor for the difference, which is a lesson this gate learned the hard way. Written against |
Live evidence — identity survives every write path (2026-09-02)The parent of this PR's head (
Hazard found by the same exercise, not a defect in the PR: because the label is sticky and Commit |
31b00c6 to
44d4446
Compare
|
Both asks and the follow-up, at Precondition named at the site. The doc comment on Floor re-measured in the debug profile with the gate's own expression: 495 on this head (it moved with the new tests), The follow-up, done — at the store sink, not the CLI: on a replace without identity flags, when the adapter can derive the incoming token's account ( Live note since the first comment: the sticky rule was exercised on the running daemon across |
|
Verified the follow-up at source at The sink placement is the part that matters: the offline lease path and the route path reach the same refusal because it lives below both, rather than each rendering its own version of the rule. And the arm is genuinely its own — Derivation failure preserving the label is the right call and the reasoning is stated at the site. An undecodable token cannot contradict the label, and refusing there would block every non-JWT material for a JWT-deriving adapter. That is the fail-open direction I would have argued for, on the narrower ground that a preserved label is recoverable ( One thing to fix before this merges, and it is not your codeThe gate fails at HEAD, and the cause is the lockfile: Your branch sits on top of current master, but the commit moves the lock backwards — master absorbed those sibling bumps at The mechanism is worth naming because it is silent and I hit the mirror of it this morning: a local
Two smaller notesFloor at 495, and I measured 495 on your head with master's lock — agrees, so it is the real count rather than slack. The runbook line for a different-account re-login ( |
44d4446 to
9a5cc5f
Compare
|
Rebased onto The rebase surfaced a worse defect of mine, and you verified the head that carried it. The refusal now names the after-the-fact remedy with real values — |
A contributor found a defect in a head I had reviewed and reported clean, and the defect was invisible to the way I reviewed it. A squashed commit whose tree was built on an older base silently reverts everything that landed in between. No conflict, no warning, and the gate stays green because the removed code's tests leave with it. PR #30's head 44d4446 was squashed with parent a31a34b while its tree came from 0679dea, reverting four upstream commits -- including 3568159, the create-time reachability advisory I staged and deployed hours later. Reading the head "at source" examines what is PRESENT; this defect is entirely in what is absent. Same class as auditing a payload store by iterating the files you hold. RECORDED AS A JUDGEMENT WITH A CHEAP TELL, NOT AS A GATE, and the failed attempt is the part worth keeping. I built the obvious sweep -- lines the base's recent history added, that the head removes -- and it fires correctly on the poisoned head. It also reported ~60 findings on two CORRECTLY REBASED heads. That is not tuning: after a rebase, a PR that legitimately EDITS recently-landed upstream code is indistinguishable by line-set arithmetic from one that reverts it. A distinctiveness filter (>= 30 chars, no pure punctuation) halved the noise and no more; the residue is real edits. Sixty findings on clean input is the alarm-that-always-fires failure -- the same one that killed the unreachable-credential gauge. So the script is deleted and the tell is written down instead: `git diff --stat <base>..<head>` listing files the PR has no business touching. On 44d4446 that was store.rs at -21 lines in a comment region, which a PR about account identity has no reason to produce. Read the file list before the diff. Two instrument notes from building it. The first version used merge-base arithmetic and could NOT fire for this defect at all -- when the head's parent IS the base, merge-base equals base and the "any upstream commits to revert" early exit reports clean, which is exactly the poisoned case. The control caught that. And the lock moved twice in this sitting (subc-core 0.16.0 gaining ed25519-dalek, then 0.16.2); this commit carries the second, version-only with no new crates, checked before taking it.
…it, replace keeps it Imported identity remains inside the encrypted record body. set-identity re-seals unchanged credential material, keeps lifecycle state, bumps record_version, and audits the metadata-only mutation so vault-custodied tokens need not be replaced from a source file.\n\nToken-only replace preserves an existing identity; explicit import flags override it and clear is deliberate. The gate floor rises from 470 to the measured 481 tests. Identity is validated where it is stored: RecordIdentity::validate runs at every record write sink, so the connected admin route and the offline CLI share one rule. set-identity re-seals the same OAuthCredential (pinned field-for-field with non-empty scopes), keeps handles, state and stale_pending, and bumps record_version exactly once. A new admin op, admin.store_with_identity_policy, carries the replace-with-preservation semantics so an older daemon refuses it instead of silently dropping identity; the legacy admin.store wire shape is byte-frozen. Labels are validated at every write sink, legacy email-only identities are normalised on preserve, control characters render as <invalid> in `ck auth usable`, and the sticky-replace tests pin the rotated material as well as the label.
…m is refused on create too The overwrite path already compared the retained identity against the incoming material's derived claim. The create path trusted a supplied --account-id over the claim the adapter can derive from the incoming token, so a record could be sealed with an identity that contradicts its own material. Both paths now share one derivation helper; on create a supplied id that differs from the derived claim is refused as SuppliedIdentityContradictsClaim (the token's own claim is authoritative), non-derivable adapters accept the supplied label as before, and the identity that is actually sealed is validated once, after the derived claim is attached.
9a5cc5f to
20c7920
Compare
Master went red on clippy immediately after merging #30, and the merge was not the cause: the merged commit does not touch Cargo.lock. A sibling moved for the third time tonight (0.16.0 gaining ed25519-dalek, then 0.16.2, now 0.16.3). Version-only, no new crates, checked before taking it. Worth recording as a rate rather than as three incidents: with path dependencies on an actively developed sibling, `--locked` on master is a claim that expires on someone else's schedule. The gate cannot catch it -- a green gate is a statement about the sibling checkout at that instant -- so the only reliable detector is CI on master after every push, which is why that check is a rule here rather than a habit.
Two oauth records imported from a consumer's auth store serve
account_id: null, and a quota consumer that labels accounts per provider collapses identity-less accounts of one provider into a single unlabelled row. The read surface already serves a storedRecordIdentity(the login path captures it); this adds the write side for records that arrive by import, without touching secret material.ck auth import --account-id <id> [--email] [--org-name] | --clear-identityon every import source.ck auth set-identity <credential-id> --account-id … | --clear— identity-only mutation: decrypts, attaches the label, re-seals the SAMEOAuthCredential(pinned field-for-field with non-empty scopes and client_id), keeps handles, state andstale_pending, bumpsrecord_versiononce, audited. Exists becauseimport --replacereads a source file and would overwrite a vault-custodied token family with stale material.import --replacewithout identity flags PRESERVES the existing record's identity at the store layer (both the offline path and the connected admin op), so an unattended re-import from a rotating source keeps the label;--clear-identitydrops it; a corrupt existing record is still repaired, with the incoming identity only.RecordIdentity::validateruns at every record write sink (create, CAS overwrite, identity-policy overwrite, set-identity): non-empty, no control chars, ≤256 bytes; email-without-account_id is normalised away per the existingwith_identityrule.ck auth usableshowsaccount=<id|none>per oauth row; runbook updated.Gate:
bash scripts/gate.shpasses (workspace floor 484, debug profile); core 316, cli_admin 31+1 ignored, clippy, fmt. Mutations red-checked: preservation removed → sticky test red; store-level validation removed → connected-route test red; scopes cleared / client_id dropped in set-identity → OAuth equality red; stale_pending cleared → invariant red.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Adds account identity metadata to imported OAuth records and a
set-identitycommand. Token-onlyimport --replacenow preserves existing identity instead of dropping it, while explicit identity flags override or clear it;set-identitychanges metadata without replacing token material.ck auth importaccepts--account-id,--email,--org-name, and--clear-identity; email and organization name require an account ID.--account-idthat contradicts the incoming token's own claim is refused on create.set-identityre-seals the same OAuth credential, keeps handles, lifecycle state, andstale_pending, incrementsrecord_version, and writes an audit event.admin.storewire shape stays byte-frozen.ck auth usabledisplaysaccount=<id|none>and renders invalid stored identity metadata as<invalid>.Written for commit 20c7920. Summary will update on new commits.