fix(keys): typed keychain custody + never mint over a published identity (P6-XP-04)#62
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces a typed, recorded key-custody model (keychain vs. file) governing device and signing identity storage. It adds custody classification in the platform keyring error mapper, a ChangesKey custody model implementation
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Command
participant ResolveKeyStore as resolveKeyStore
participant Store as state.Store
participant HybridStore as devicekeys.HybridStore
participant Keychain as Platform Keychain
CLI->>ResolveKeyStore: resolve(ctx, paths, store)
ResolveKeyStore->>Store: KeyCustody(ctx)
Store-->>ResolveKeyStore: recorded custody
ResolveKeyStore->>HybridStore: WithCustody(EffectiveKeyCustody(recorded))
ResolveKeyStore-->>CLI: keyStore
CLI->>HybridStore: Ensure/Read(ctx, deviceID, published)
HybridStore->>HybridStore: mintGuard / resolveSecret
HybridStore->>Keychain: Load/Store (if custody=keychain)
Keychain-->>HybridStore: secret or ErrKeychainUnreachable
HybridStore-->>CLI: identity or refusal error
sequenceDiagram
participant Init as devstrap init
participant Record as recordKeyCustodyAtInit
participant Probe as HybridStore.Probe
participant Store as state.Store
Init->>Record: recordKeyCustodyAtInit(ctx, store)
Record->>Store: KeyCustody(ctx)
alt custody already recorded
Record-->>Init: no-op
else custody unset
Record->>Probe: check DEVSTRAP_NO_KEYCHAIN / reachability
Probe-->>Record: keychain reachable / unreachable
Record->>Store: RecordKeyCustody(ctx, decision)
Store-->>Record: persisted (write-once)
end
Init-->>Init: proceed with resolveKeyStore
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/state/store.go (2)
2491-2518: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
local_metaread logic betweentx.keyCustodyandStore.KeyCustody.Both methods run the identical query/scan/error-handling logic, differing only in the receiver (
tx.txvss.db). Since*sql.Txand*sql.DBshare the sameQueryRowContextsignature, this can be deduplicated behind a small interface.♻️ Proposed refactor to share the read logic
+type queryRower interface { + QueryRowContext(ctx context.Context, query string, args ...any) *sql.Row +} + +func readKeyCustody(ctx context.Context, q queryRower) (devicekeys.Custody, error) { + var v string + err := q.QueryRowContext(ctx, `SELECT value FROM local_meta WHERE key = ?;`, keyCustodyMetaKey).Scan(&v) + if errors.Is(err, sql.ErrNoRows) { + return devicekeys.CustodyUnset, nil + } + if err != nil { + return devicekeys.CustodyUnset, fmt.Errorf("read key custody: %w", err) + } + return devicekeys.Custody(v), nil +} + func (tx *Tx) keyCustody(ctx context.Context) (devicekeys.Custody, error) { - var v string - err := tx.tx.QueryRowContext(ctx, `SELECT value FROM local_meta WHERE key = ?;`, keyCustodyMetaKey).Scan(&v) - if errors.Is(err, sql.ErrNoRows) { - return devicekeys.CustodyUnset, nil - } - if err != nil { - return devicekeys.CustodyUnset, fmt.Errorf("read key custody: %w", err) - } - return devicekeys.Custody(v), nil + return readKeyCustody(ctx, tx.tx) } func (s *Store) KeyCustody(ctx context.Context) (devicekeys.Custody, error) { - var v string - err := s.db.QueryRowContext(ctx, `SELECT value FROM local_meta WHERE key = ?;`, keyCustodyMetaKey).Scan(&v) - if errors.Is(err, sql.ErrNoRows) { - return devicekeys.CustodyUnset, nil - } - if err != nil { - return devicekeys.CustodyUnset, fmt.Errorf("read key custody: %w", err) - } - return devicekeys.Custody(v), nil + return readKeyCustody(ctx, s.db) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/state/store.go` around lines 2491 - 2518, The key-custody read logic is duplicated between tx.keyCustody and Store.KeyCustody, so factor the shared local_meta query/scan/error handling into a small helper that accepts something with QueryRowContext and reuse it from both methods. Keep the existing behavior for sql.ErrNoRows and wrapped read errors, and leave the receiver-specific entry points (tx.tx and s.db) as thin wrappers around the shared helper.
2432-2434: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCustody-stamped store construction is duplicated with
resolveKeyStoreininternal/cli/init.go.
devicekeys.NewHybridStore(keyDir, platform.Detect().Keychain).WithCustody(EffectiveKeyCustody(custody))is the same composition asresolveKeyStore(internal/cli/init.go:375-382). Sinceinternal/clialready importsinternal/state, exposing an equivalent constructor here (e.g. astatepackage helper takingkeyDir+custody) would let both call sites share one implementation instead of evolving in parallel.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/state/store.go` around lines 2432 - 2434, The custody-stamped key store construction is duplicated between the current `devicekeys.NewHybridStore(...).WithCustody(EffectiveKeyCustody(custody))` path and `resolveKeyStore`, so introduce a shared helper in `state` that builds the hybrid store from `keyDir` and `custody`. Update `EnsureSigning`’s call site to use that helper, and have `resolveKeyStore` reuse the same shared construction so both paths stay in sync.internal/cli/root_test.go (1)
249-253: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the doctor custody assertion to reject unreachable keychain warnings.
This check still passes if doctor reports
keychain (currently unreachable), because it only searches forkeychainand later checks0 error(s), not warnings.Proposed test tightening
if !strings.Contains(stdout, "key custody") || !strings.Contains(stdout, "keychain") { t.Fatalf("doctor stdout = %q, want a key custody row reporting keychain", stdout) } + if strings.Contains(stdout, "keychain (currently unreachable)") { + t.Fatalf("doctor stdout = %q, want reachable keychain custody", stdout) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/root_test.go` around lines 249 - 253, The doctor custody test in root_test.go is too loose because it still passes when stdout contains a keychain warning like “currently unreachable.” Tighten the assertion around the doctor output check in the relevant test by matching the key-custody row more specifically and explicitly rejecting any unreachable keychain wording, using the existing stdout inspection and the doctor command expectations in the test helper.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/hub.go`:
- Line 414: The `hub login` help text is now misleading because `hubCredStore`
records credentials in the active custody backend, which can be the file store
even when a keychain is available. Update the help/usage wording associated with
`hub login` in `hub.go` so it describes custody-aware storage instead of
implying “OS keychain” is always used with file only as fallback. Make sure the
revised text matches the behavior of `hubCredStore` and the login flow around
`keys := hubCredStore(cmd.Context(), opts, store)`.
In `@internal/platform/platform.go`:
- Around line 238-247: Tighten the D-Bus error substring matching in the
classifier loop in platform.go so only missing-session-bus and missing-service
cases map to ErrUnsupported; remove or narrow the broad "dbus" and
"org.freedesktop.secrets" needles and keep the specific fallbacks like "session
bus", "was not provided", and "not provided by any". Update the logic around the
error classification function that uses this needle list, and add a negative
test covering a real/hard D-Bus failure to ensure it is not treated as
unsupported.
---
Nitpick comments:
In `@internal/cli/root_test.go`:
- Around line 249-253: The doctor custody test in root_test.go is too loose
because it still passes when stdout contains a keychain warning like “currently
unreachable.” Tighten the assertion around the doctor output check in the
relevant test by matching the key-custody row more specifically and explicitly
rejecting any unreachable keychain wording, using the existing stdout inspection
and the doctor command expectations in the test helper.
In `@internal/state/store.go`:
- Around line 2491-2518: The key-custody read logic is duplicated between
tx.keyCustody and Store.KeyCustody, so factor the shared local_meta
query/scan/error handling into a small helper that accepts something with
QueryRowContext and reuse it from both methods. Keep the existing behavior for
sql.ErrNoRows and wrapped read errors, and leave the receiver-specific entry
points (tx.tx and s.db) as thin wrappers around the shared helper.
- Around line 2432-2434: The custody-stamped key store construction is
duplicated between the current
`devicekeys.NewHybridStore(...).WithCustody(EffectiveKeyCustody(custody))` path
and `resolveKeyStore`, so introduce a shared helper in `state` that builds the
hybrid store from `keyDir` and `custody`. Update `EnsureSigning`’s call site to
use that helper, and have `resolveKeyStore` reuse the same shared construction
so both paths stay in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c395966-497b-4eda-ac76-acd981c4152a
📒 Files selected for processing (32)
internal/cli/blob_gc.gointernal/cli/blob_gc_test.gointernal/cli/custody_cli_test.gointernal/cli/devices.gointernal/cli/devices_grant_replay_test.gointernal/cli/doctor.gointernal/cli/env.gointernal/cli/hub.gointernal/cli/hub_test.gointernal/cli/init.gointernal/cli/keys.gointernal/cli/materialize.gointernal/cli/root_test.gointernal/cli/run.gointernal/cli/sync.gointernal/devicekeys/custody_test.gointernal/devicekeys/devicekeys.gointernal/devicekeys/devicekeys_test.gointernal/platform/platform.gointernal/platform/platform_test.gointernal/state/custody_wedge_test.gointernal/state/migrations/00016_local_meta.sqlinternal/state/store.gointernal/state/store_test.gointernal/workspacekeys/keyring_test.gospec/05_MAC_FIRST_IMPLEMENTATION.mdspec/06_LINUX_COMPATIBILITY.mdspec/09_SECRETS_AND_ENVIRONMENT.mdspec/12_DATA_MODEL_SQLITE.mdspec/13_CLI_DAEMON_API.mdspec/15_SECURITY_THREAT_MODEL.mdspec/18_WORK_LOG.md
…ity (P6-XP-04) keychainUnavailable classified keyring errors by substring-matching err.Error(); on headless Linux a dead D-Bus session read as 'not found', so EnsureSigning minted a divergent signing identity into the file store — a split-custody wedge the SQL mismatch guard caught only after the divergent key file was on disk. The same heuristic guarded StoreWCK/ LoadWCK, extending the blast radius to workspace-key custody. Secret-Service-unreachable recognition moves DOWN into the platform seam (mapKeyringError -> ErrUnsupported, where godbus's untyped errors are unavoidable); everything above uses errors.Is on typed sentinels. Mint paths take the device's published key and refuse to mint a divergent identity when custody is unreachable or a published key's private half is absent (ErrKeychainUnreachable). A per-store custody decision (migration: local_meta, local-only KV) is recorded once at init — only from safe evidence: explicit DEVSTRAP_NO_KEYCHAIN, a positive keychain probe, or a negative probe on a genuine first init with no published keys (never stranding an upgraded keychain-backed store) — and honored everywhere (signing, WCK, hub-credential slot, run path); keychain custody fails closed instead of silently downgrading to files. doctor reports the custody backend. Post-review (Codex P2 x2): custody resolution is side-effect-free and recording is init-only with the never-strand gating; the run path and hub-credential slot thread recorded custody. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reederey87
force-pushed
the
fix/p6-xp-04-keychain-custody
branch
from
fc82449 to
496bb98
Compare
…re hub login help (CodeRabbit) Bare 'dbus' / 'org.freedesktop.secrets' needles also match errors a LIVE Secret Service produces (timeouts, dismissed prompts); classifying those as backend-unavailable could record file custody from a transient failure at first init. Only missing-bus/missing-service signatures qualify now, with a negative test for live-service errors. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ion seam The custody tests inherited the ambient environment: CI sets DEVSTRAP_NO_KEYCHAIN=1 job-wide and runners have no usable keychain (dead session bus on Linux, interaction-not-allowed on macOS), so five tests validated locally against a live Mac keychain went red on CI. Every custody test now pins the env in both directions with t.Setenv and injects a fake backend through a new keychainBackend seam (cli + state) instead of touching the host keychain; the doctor custody-row assertion is pinned to file custody under an explicit DEVSTRAP_NO_KEYCHAIN=1. Verified green under both the CI job env and a cleared environment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/state/store_test.go (1)
209-227: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTable-list check doesn't cover the new
local_metatable.The
IN (...)list used fortableCountstill enumerates the pre-existing 19 tables and doesn't includelocal_meta, so this test doesn't assert the new migration's table actually exists (that coverage instead relies oncustody_wedge_test.goexercisingKeyCustody/RecordKeyCustody). Consider addinglocal_metato the list (and bumping the expected count) for direct migration-completeness coverage here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/state/store_test.go` around lines 209 - 227, The schema table-count assertion in the store migration test is still checking only the original 19 tables and misses the new local_meta table. Update the sqlite_master query in the test that verifies st.db so it includes local_meta in the IN list, and adjust the expected tableCount accordingly. Use the existing schema-version and table-count check in the test to keep direct migration coverage for the new table.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/09_SECRETS_AND_ENVIRONMENT.md`:
- Line 206: Update the `Recorded custody decision` paragraph in the
secrets/environment spec to use the correct `local_meta` migration identifier:
replace both references to migration `00016` with `00019`, matching the
`local_meta` table introduced by `00019_local_meta.sql` and the current schema
version. Keep the rest of the custody behavior description unchanged and verify
any nearby mentions of `local_meta`, `HybridStore`, or custody recording stay
consistent with the updated migration number.
---
Nitpick comments:
In `@internal/state/store_test.go`:
- Around line 209-227: The schema table-count assertion in the store migration
test is still checking only the original 19 tables and misses the new local_meta
table. Update the sqlite_master query in the test that verifies st.db so it
includes local_meta in the IN list, and adjust the expected tableCount
accordingly. Use the existing schema-version and table-count check in the test
to keep direct migration coverage for the new table.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 19827b0d-5134-4b0a-bd28-2922198b6ba9
📒 Files selected for processing (32)
internal/cli/blob_gc.gointernal/cli/blob_gc_test.gointernal/cli/custody_cli_test.gointernal/cli/devices.gointernal/cli/devices_grant_replay_test.gointernal/cli/doctor.gointernal/cli/env.gointernal/cli/hub.gointernal/cli/hub_test.gointernal/cli/init.gointernal/cli/keys.gointernal/cli/materialize.gointernal/cli/root_test.gointernal/cli/run.gointernal/cli/sync.gointernal/devicekeys/custody_test.gointernal/devicekeys/devicekeys.gointernal/devicekeys/devicekeys_test.gointernal/platform/platform.gointernal/platform/platform_test.gointernal/state/custody_wedge_test.gointernal/state/migrations/00019_local_meta.sqlinternal/state/store.gointernal/state/store_test.gointernal/workspacekeys/keyring_test.gospec/05_MAC_FIRST_IMPLEMENTATION.mdspec/06_LINUX_COMPATIBILITY.mdspec/09_SECRETS_AND_ENVIRONMENT.mdspec/12_DATA_MODEL_SQLITE.mdspec/13_CLI_DAEMON_API.mdspec/15_SECURITY_THREAT_MODEL.mdspec/18_WORK_LOG.md
✅ Files skipped from review due to trivial changes (5)
- internal/cli/devices_grant_replay_test.go
- internal/workspacekeys/keyring_test.go
- spec/05_MAC_FIRST_IMPLEMENTATION.md
- spec/13_CLI_DAEMON_API.md
- spec/18_WORK_LOG.md
🚧 Files skipped from review as they are similar to previous changes (18)
- internal/cli/keys.go
- internal/cli/blob_gc_test.go
- internal/cli/sync.go
- internal/cli/materialize.go
- internal/cli/init.go
- internal/cli/blob_gc.go
- internal/platform/platform.go
- internal/cli/run.go
- spec/15_SECURITY_THREAT_MODEL.md
- internal/cli/hub_test.go
- internal/cli/doctor.go
- internal/cli/custody_cli_test.go
- internal/devicekeys/devicekeys_test.go
- internal/cli/hub.go
- internal/cli/env.go
- spec/06_LINUX_COMPATIBILITY.md
- internal/cli/devices.go
- internal/devicekeys/custody_test.go
|
|
||
| - **Typed classification, not string matching.** The platform seam (`internal/platform`, `mapKeyringError`) is the single place that turns the keyring library's error vocabulary into typed sentinels: `ErrSecretNotFound` (the backend is reachable but holds no such secret — a mint may proceed) versus `ErrUnsupported` (the backend is unreachable/unsupported — including a missing Linux Secret Service / D-Bus session, which go-keyring surfaces as an untyped godbus error that the seam classifies here). `internal/devicekeys` consumes those sentinels with `errors.Is`; it never inspects error strings. A live-backend hard failure stays untyped and fails closed. | ||
| - **Never mint over a published identity.** `EnsureSigning`/`Ensure` receive the device's already-published public key and refuse to mint a replacement when the keychain is unreachable (the split-custody wedge) or when a key is published but its private half is absent from a reachable backend — either would diverge from `devices.signing_public_key`. The refusal carries the remedy (run from a desktop session, or set `DEVSTRAP_NO_KEYCHAIN=1` and migrate the key file). The same never-mint-over-held principle guards the WCK custody path. | ||
| - **Recorded custody decision.** At init the decision is recorded once in the local, never-synced `local_meta` table (migration `00016`) and never rewritten, but only from *safe evidence* so an existing store is never stranded: `file` when `DEVSTRAP_NO_KEYCHAIN=1` (explicit operator choice); `keychain` when the probe positively finds the keychain reachable (safe regardless of pre-existing secrets); and `file` from an unreachable probe *only* for a genuine first init (a brand-new device with no already-published keys). Crucially, an unreachable probe on an *already-initialized* store — e.g. a pre-`00016` store whose secrets live only in the keychain, first run headless after upgrade — records nothing and stays `CustodyUnset`, so a later desktop run still reads the keychain where the real secrets are. Later runs honor a recorded decision: a `file`-custody store keeps using files even if a keychain appears; a `keychain`-custody store refuses to silently degrade to file custody (it fails closed with a remedy) unless `DEVSTRAP_NO_KEYCHAIN=1` forces file custody. `doctor` reports the recorded backend and warns when it is currently unreachable, overridden, or unrecorded. Under legacy (unrecorded) custody the historical hybrid behavior is preserved — prefer the keychain, fall back to the file store when unavailable and nothing is published — so the mint guard above still prevents a divergent mint. All device/workspace/hub-credential key stores are stamped with the recorded custody, so a stale keychain entry can never shadow the authoritative file key on a file-custody machine. The same `HybridStore` also custodies Workspace Content Keys (WCK) for event-log envelope encryption (`P4-SEC-07`/`P6-SEC-02`, keyed `wck.<workspace_id>.<epoch>.<kid>` where `kid = hex(sha256(wck))`; the pre-kid `wck.<workspace_id>.<epoch>` form is the legacy slot, lazily upgraded by `Keyring.Prime`). The kid is validated (64 lowercase hex chars or empty) before it reaches any keychain account name or file path. Manual `devices enroll --approve` records an approved device age recipient so future captures include that recipient, and grants every held WCK epoch's fleet key to the newly-approved device. Production synced env blobs still require automatic remote enrollment; fingerprint confirmation is shipped (`P4-SEC-04`) and gates every approval that adds a recipient. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the migration number for local_meta (00016 → 00019).
The custody decision is stored in the local_meta table introduced by 00019_local_meta.sql, and the schema version this PR targets is 19 (see TestMigrateEnsureSummaryAndVersion / db status: schema version: 19). This paragraph references migration 00016 twice, which is inaccurate.
As per coding guidelines: "review every file in spec/ and update each file as needed so the specs remain accurate, complete, and current."
📝 Proposed doc fix
-- **Recorded custody decision.** At init the decision is recorded once in the local, never-synced `local_meta` table (migration `00016`) and never rewritten, but only from *safe evidence* so an existing store is never stranded: `file` when `DEVSTRAP_NO_KEYCHAIN=1` (explicit operator choice); `keychain` when the probe positively finds the keychain reachable (safe regardless of pre-existing secrets); and `file` from an unreachable probe *only* for a genuine first init (a brand-new device with no already-published keys). Crucially, an unreachable probe on an *already-initialized* store — e.g. a pre-`00016` store whose secrets live only in the keychain, first run headless after upgrade — records nothing and stays `CustodyUnset`, so a later desktop run still reads the keychain where the real secrets are.
+- **Recorded custody decision.** At init the decision is recorded once in the local, never-synced `local_meta` table (migration `00019`) and never rewritten, but only from *safe evidence* so an existing store is never stranded: `file` when `DEVSTRAP_NO_KEYCHAIN=1` (explicit operator choice); `keychain` when the probe positively finds the keychain reachable (safe regardless of pre-existing secrets); and `file` from an unreachable probe *only* for a genuine first init (a brand-new device with no already-published keys). Crucially, an unreachable probe on an *already-initialized* store — e.g. a pre-`00019` store whose secrets live only in the keychain, first run headless after upgrade — records nothing and stays `CustodyUnset`, so a later desktop run still reads the keychain where the real secrets are.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **Recorded custody decision.** At init the decision is recorded once in the local, never-synced `local_meta` table (migration `00016`) and never rewritten, but only from *safe evidence* so an existing store is never stranded: `file` when `DEVSTRAP_NO_KEYCHAIN=1` (explicit operator choice); `keychain` when the probe positively finds the keychain reachable (safe regardless of pre-existing secrets); and `file` from an unreachable probe *only* for a genuine first init (a brand-new device with no already-published keys). Crucially, an unreachable probe on an *already-initialized* store — e.g. a pre-`00016` store whose secrets live only in the keychain, first run headless after upgrade — records nothing and stays `CustodyUnset`, so a later desktop run still reads the keychain where the real secrets are. Later runs honor a recorded decision: a `file`-custody store keeps using files even if a keychain appears; a `keychain`-custody store refuses to silently degrade to file custody (it fails closed with a remedy) unless `DEVSTRAP_NO_KEYCHAIN=1` forces file custody. `doctor` reports the recorded backend and warns when it is currently unreachable, overridden, or unrecorded. Under legacy (unrecorded) custody the historical hybrid behavior is preserved — prefer the keychain, fall back to the file store when unavailable and nothing is published — so the mint guard above still prevents a divergent mint. All device/workspace/hub-credential key stores are stamped with the recorded custody, so a stale keychain entry can never shadow the authoritative file key on a file-custody machine. The same `HybridStore` also custodies Workspace Content Keys (WCK) for event-log envelope encryption (`P4-SEC-07`/`P6-SEC-02`, keyed `wck.<workspace_id>.<epoch>.<kid>` where `kid = hex(sha256(wck))`; the pre-kid `wck.<workspace_id>.<epoch>` form is the legacy slot, lazily upgraded by `Keyring.Prime`). The kid is validated (64 lowercase hex chars or empty) before it reaches any keychain account name or file path. Manual `devices enroll --approve` records an approved device age recipient so future captures include that recipient, and grants every held WCK epoch's fleet key to the newly-approved device. Production synced env blobs still require automatic remote enrollment; fingerprint confirmation is shipped (`P4-SEC-04`) and gates every approval that adds a recipient. | |
| - **Recorded custody decision.** At init the decision is recorded once in the local, never-synced `local_meta` table (migration `00019`) and never rewritten, but only from *safe evidence* so an existing store is never stranded: `file` when `DEVSTRAP_NO_KEYCHAIN=1` (explicit operator choice); `keychain` when the probe positively finds the keychain reachable (safe regardless of pre-existing secrets); and `file` from an unreachable probe *only* for a genuine first init (a brand-new device with no already-published keys). Crucially, an unreachable probe on an *already-initialized* store — e.g. a pre-`00019` store whose secrets live only in the keychain, first run headless after upgrade — records nothing and stays `CustodyUnset`, so a later desktop run still reads the keychain where the real secrets are. Later runs honor a recorded decision: a `file`-custody store keeps using files even if a keychain appears; a `keychain`-custody store refuses to silently degrade to file custody (it fails closed with a remedy) unless `DEVSTRAP_NO_KEYCHAIN=1` forces file custody. `doctor` reports the recorded backend and warns when it is currently unreachable, overridden, or unrecorded. Under legacy (unrecorded) custody the historical hybrid behavior is preserved — prefer the keychain, fall back to the file store when unavailable and nothing is published — so the mint guard above still prevents a divergent mint. All device/workspace/hub-credential key stores are stamped with the recorded custody, so a stale keychain entry can never shadow the authoritative file key on a file-custody machine. The same `HybridStore` also custodies Workspace Content Keys (WCK) for event-log envelope encryption (`P4-SEC-07`/`P6-SEC-02`, keyed `wck.<workspace_id>.<epoch>.<kid>` where `kid = hex(sha256(wck))`; the pre-kid `wck.<workspace_id>.<epoch>` form is the legacy slot, lazily upgraded by `Keyring.Prime`). The kid is validated (64 lowercase hex chars or empty) before it reaches any keychain account name or file path. Manual `devices enroll --approve` records an approved device age recipient so future captures include that recipient, and grants every held WCK epoch's fleet key to the newly-approved device. Production synced env blobs still require automatic remote enrollment; fingerprint confirmation is shipped (`P4-SEC-04`) and gates every approval that adds a recipient. |
🧰 Tools
🪛 LanguageTool
[grammar] ~206-~206: Use a hyphen to join words.
Context: ...to the newly-approved device. Production synced env blobs still require automatic...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/09_SECRETS_AND_ENVIRONMENT.md` at line 206, Update the `Recorded custody
decision` paragraph in the secrets/environment spec to use the correct
`local_meta` migration identifier: replace both references to migration `00016`
with `00019`, matching the `local_meta` table introduced by
`00019_local_meta.sql` and the current schema version. Keep the rest of the
custody behavior description unchanged and verify any nearby mentions of
`local_meta`, `HybridStore`, or custody recording stay consistent with the
updated migration number.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes P6-XP-04: the substring keychain-error heuristic minted divergent device identities on headless Linux (dead D-Bus session → "not found" → fresh key into the file store), wedging sync for exactly the future
service installtarget;StoreWCK/LoadWCKshared the heuristic.internal/platform.mapKeyringError(→ErrUnsupported; godbus errors are untyped, so the string inspection is unavoidable there and only there).devicekeysuseserrors.Ison typed sentinels exclusively.ErrKeychainUnreachable) instead of minting a divergent one — the audit's exact wedge, pinned by a regression test throughInsertLocalEvent.keychain|filedecision (newlocal_metatable, local-only) recorded once at init from safe evidence only — explicitDEVSTRAP_NO_KEYCHAIN, a positive probe, or a negative probe on a genuine first init with no published keys. An upgraded keychain-backed store first run headless records NOTHING (stays legacy-hybrid + mint guard) — never stranded. Keychain custody fails closed rather than silently downgrading. Honored at everyHybridStoresite (signing, WCK, run path, hub-credential slot);doctorreports the backend.Review
Opus-4.8 implementation; Codex review found 2 P2s (transient-probe persistence stranding upgraded stores; run path ignoring recorded custody) — both fixed, plus one more unstamped site (hub-credential slot) found in the sweep. Line-level main-session review of the classification core and the fix round.
Note for the merge train
This branch is based on pre-wave main; its
local_metamigration currently numbered 00016 collides with #61's 00016 by design of parallel development — it gets renumbered to the next free slot at its rebase (it merges last of the wave's code PRs). Do not merge before that rebase.Ledger rows move in the wave's docs close-out PR.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
doctornow reports key custody status alongside other device key checks.Bug Fixes