fix: keyring key mints go through the SP 800-90B latch (#207 item 6); name the steward-key drift (#176) - #271
Conversation
… name the steward-key drift (#176) #207 item 6 -- the #74 invariant did not hold where it matters most. v5.6.0 made keygen fail-secure so "no weak key is ever produced". But ciris-keyring's OWN mints drew raw OsRng, bypassing the SP 800-90B health latch. The invariant therefore held for ciris-crypto-constructed keys and NOT for keyring-sealed ones -- which are the federation identity keys. Every key-material draw now routes through ciris_crypto::random::fill: the sealed Ed25519 seed, the sealed ML-DSA-65 seed, the USB-wrapped PQC seed, the transport identity, the software wrapper key, and three P-256 mints via a new mint_p256_signing_key. That helper puts the BYTES through the latch rather than probing it and then drawing unchecked, so the key material is literally what the checked path produced. This required making ciris-crypto a NON-optional keyring dependency, at the `random` feature only. Gating the latch behind pqc-ml-dsa meant the DEFAULT keyring build had no latch at all -- the gap was structural, not incidental. Incidental draws are deliberately untouched: a temp-dir suffix and a TPM attestation nonce are not key material, and routing them would blur what the rule is for. Proven by a fail-secure test -- the one #74 gave every ciris-crypto primitive and the keyring never had. It uses the already-public __force_health_for_test hatch; exposing test_support across the crate boundary was tried first and reverted, because dev-dependency features do not propagate that way. #176 -- the title says build record; the failing decode is the steward key. validation::query_https_source -> get_steward_key -> StewardKeyResponse, which requires `classical`. Two months of that message pointing people at build records. The parse itself cannot be fixed here. The registry sends the multi-steward shape; verify's modern parser for it REQUIRES a response_signature the registry does not send; and relaxing that would mean silently accepting unsigned trust-root material. That is a cross-repo contract decision (CIRISRegistry#133), so this does not paper over it. What IS fixable is the misdirection: the error now names the endpoint, the drift, the tracking issue, and "NOT a build-record problem and NOT a network problem". An unrelated parse error passes through untouched, asserted by test. Note v13.3.0 already fixed the "HTTPS unreachable" mislabel on this path; I reported this half as unfixed during the backlog pass because I checked the build-record path rather than the one that actually fails. 1399 workspace green, clippy clean, doc clean, guards pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZPhbXWTzykcbotEX9BN2p
![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: 3ec0aa49a5
ℹ️ 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".
| let mut bytes = [0u8; 32]; | ||
| ciris_crypto::random::fill(&mut bytes).map_err(|e| KeyringError::KeyGenerationFailed { | ||
| reason: format!("RNG health check failed; refusing to mint a P-256 key: {e}"), | ||
| })?; |
There was a problem hiding this comment.
ciris_crypto::random::fill only reads the existing health verdict and explicitly treats the uninitialized state as healthy; the only production call to run_startup_health_check() found repo-wide is in ciris-verify-ffi. Consequently, direct ciris-keyring consumers such as the documented standalone API and downstream services can reach this mint without ever running the startup test, so these changed key-generation paths still draw from OsRng without the promised SP 800-90B check. Ensure the latch is initialized by the keyring before any mint, or make initialization an enforced constructor/API requirement.
Useful? React with 👍 / 👎.
| __force_health_for_test(RngHealth::Failed { | ||
| test: ciris_crypto::rng_health::TEST_REPETITION_COUNT, | ||
| detail: "forced for the keyring fail-secure test".to_string(), | ||
| }); |
There was a problem hiding this comment.
When ciris-crypto is compiled as a dependency of the keyring unit-test target, its cfg(test) thread-local override is disabled, so this call to __force_health_for_test writes the process-global latch. While this test runs, any parallel keyring test that generates a key can therefore fail spuriously with KeyGenerationFailed; the local Restore only limits the duration and does not prevent the race. Use a dependency API that provides a thread-local test override, or serialize every keyring test that can consult the latch.
Useful? React with 👍 / 👎.
| fn diagnose_steward_key_drift(detail: &str) -> String { | ||
| if detail.contains("missing field `classical`") || detail.contains("missing field `pqc`") { | ||
| format!( |
There was a problem hiding this comment.
Any response missing either root field is labeled as the specific multi-steward contract drift even though the serde error contains no evidence that stewards or verification_policy was present. For example, a malformed single-steward response that still has classical but accidentally omits pqc will now be reported as CIRISRegistry#133 and claim a DNS-only degradation, sending operators toward the wrong incident. Inspect the response shape before attaching this diagnosis, or limit the helper to evidence that uniquely identifies the known payload.
Useful? React with 👍 / 👎.
| VerifyError::ResponseSchemaMismatch { | ||
| url: url.clone(), | ||
| status: status.as_u16(), | ||
| detail: e.to_string(), | ||
| detail: diagnose_steward_key_drift(&e.to_string()), |
There was a problem hiding this comment.
On Android and iOS, get_steward_key still delegates directly to mobile_http::get_json, which converts this same serde failure into the generic HttpsError("JSON parse error: ..."); only the desktop reqwest branch calls the new diagnostic and returns ResponseSchemaMismatch. Thus mobile operators encountering the known registry payload continue to receive neither the contract-drift explanation nor the correct structured error, despite using the same /v1/steward-key API. Route the mobile parse failure through the same diagnosis and error variant.
Useful? React with 👍 / 👎.
Review found 11 across both PRs. One of them meant #224's fix did not work. DEAD CODE -- VerifiedJson could never fire on a real manifest. ciris-build-tool hashes the WRAPPER `{"files": {...}}` (register.rs:601); my branch hashed the bare map. So the "try the other construction" fix -- the entire point of #224 -- would have classified every real manifest as Unrecognized while appearing to work. My test passed because it hashed the same wrong shape: a fixture agreeing with the code instead of the producer, which is the exact failure mode that let the v10.4.0 preimage bug ship. Also fixed: - malformed digests are a hard failure again ("sha256:" alone passed the emptiness check and fell through to Unrecognized). - manifest_hash_check propagates to FileCheckSummary and the CLI JSON; without it an Unrecognized hash reads as an ordinary pass. - the v3 decoder enforces kind == User -- a scanned code is untrusted input and a foreign impl can mint a CRC-valid non-user one. - the fedcode payload is bounded by SIZE (1024 bytes), not only node count; 16 x 255-byte ids is >7KB encoded, which QrCode::new refuses. - owned_nodes appear in `fedcode show` JSON and human output. - FSD-003 documents the v3 wire format normatively. - the keyring RNG latch is INITIALIZED, not just read. Only the FFI called run_startup_health_check, and an uninitialized latch reads healthy -- so a direct keyring consumer could mint having never run it. - the latch test serializes; ciris-crypto's thread-local override is cfg(test) and inactive across the crate boundary. - the steward-key diagnosis requires EVIDENCE of the multi-steward shape. - mobile gets the same diagnosis instead of a generic HttpsError. 1400 workspace green, clippy clean, doc clean, guards + self-test pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZPhbXWTzykcbotEX9BN2p
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d97de99f02
ℹ️ 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".
| crate::ensure_rng_health_checked()?; | ||
| ciris_crypto::random::fill(&mut s).map_err(|e| { |
There was a problem hiding this comment.
With the default tpm-plugin feature, a first-time open_or_create calls open_existing and create_platform_storage before reaching this check; PluginTpmSecureBlobStorage::new_with_plugin then mints and persists its master key using raw OsRng at storage/tpm_plugin_storage.rs:123-127. If the RNG is stuck, the subsequent startup sample rejects the identity seed but leaves the predictable TPM master on disk, and a later healthy process reuses it to protect the eventual identity. Run the health check before any storage constructor capable of minting key material, including the adopted-seed path.
Useful? React with 👍 / 👎.
| #[serde(default = "crate::security::file_integrity::manifest_hash_check_default")] | ||
| pub manifest_hash_check: crate::security::file_integrity::ManifestHashCheck, |
There was a problem hiding this comment.
This 14.1 release adds a field to the exhaustive public FileCheckSummary struct, so downstream Rust code constructing it with a struct literal now fails with a missing-field error. #[serde(default)] only preserves deserialization compatibility and does not help source compatibility. Defer this shape change to a major release or expose the measurement without adding a required field to this public struct.
Useful? React with 👍 / 👎.
| let looks_multi_steward = | ||
| body.contains("\"stewards\"") || body.contains("\"verification_policy\""); | ||
| if looks_multi_steward | ||
| && (detail.contains("missing field `classical`") || detail.contains("missing field `pqc`")) |
There was a problem hiding this comment.
The fresh evidence in this revision is that the new guard uses raw substring searches rather than inspecting the JSON root: a malformed single-steward response can contain a nested stewards property or merely mention "stewards" in a string and, when pqc is missing, still be labeled as CIRISRegistry#133. Because mobile now applies this helper to every GET endpoint, an unrelated response is also eligible for the steward-key diagnosis. Parse the body as a JSON object and require the expected root-level multi-steward fields before attaching this incident-specific message.
Useful? React with 👍 / 👎.
| if out.len() > MAX_PAYLOAD_BYTES { | ||
| return Err(FedCodeError::Malformed(format!( | ||
| "encoded payload is {} bytes, over the {MAX_PAYLOAD_BYTES}-byte budget — \ | ||
| the code would not render as a scannable QR", | ||
| out.len() | ||
| ))); |
There was a problem hiding this comment.
This unconditional cap also applies when owned_nodes is empty and the payload remains v2. A previously valid v2 value with a 255-byte key_id and three 255-byte hints is about 1,090 payload bytes, so encode now returns Malformed even though each field satisfies the existing v2 limit and the change promises byte-identical v2 behavior. Apply the new aggregate budget only to the node-carrying v3 format, or explicitly version the breaking v2 restriction.
Useful? React with 👍 / 👎.
| let count = usize::from(*payload.get(offset).ok_or_else(trunc)?); | ||
| offset += 1; | ||
| if count > MAX_OWNED_NODES { |
There was a problem hiding this comment.
A foreign implementation can produce a CRC-valid v3 code whose count is within 16 but whose node IDs push the payload over 1,024 bytes, and this decoder accepts it because the new size budget is checked only by the encoder. It also accepts count == 0, despite the v3 format specifying 1..=16 and reserving empty lists for byte-identical v2 encoding. Validate both the payload length and the lower count bound here so nonconforming v3 inputs are rejected consistently.
Useful? React with 👍 / 👎.
| serde_json::from_str::<T>(&text).map_err(|e| VerifyError::ResponseSchemaMismatch { | ||
| url: url.to_string(), | ||
| status: 200, | ||
| detail: crate::https::diagnose_steward_key_drift(&e.to_string(), &text), |
There was a problem hiding this comment.
A successful ureq call can return any 2xx status, but this path discards the response before recording its status and reports every decode failure as HTTP 200. For example, an empty 204 response or a malformed 206 response is surfaced as a 200 contract drift, giving operators and callers incorrect evidence about what the endpoint returned. Capture response.status() before consuming the body and store that value in ResponseSchemaMismatch.
Useful? React with 👍 / 👎.
…cted nothing CI caught a real tension between two of my own review fixes, via the test I added for one of them. `ensure_rng_health_checked` called `run_startup_health_check`, which is `get_or_init` + `store_state` -- so on an already-`Failed` latch it RE-RAN the test and OVERWROTE the verdict. A mint could then proceed off a fresh pass after the process had already latched a failure, which defeats the latch's whole reason for existing: the verdict is sticky by design. Now an already-failed latch short-circuits before any re-run. My local `cargo test --lib` was green because this only fails under nextest, which CI uses and which gives every test its own PROCESS. That also makes the module-local mutex I added last commit useless -- there is no shared global to serialize -- so it is removed rather than left implying a protection it does not provide. The Restore guard stays, for plain `cargo test`. Verified with `cargo nextest run --workspace` this time, not `--lib`: 1478/1478 passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FZPhbXWTzykcbotEX9BN2p
1 participant
The second pair for the 14.1.0 cut (#224 and #269 are already on
main, untagged).#207 item 6 — the #74 invariant did not hold where it matters most
v5.6.0 made keygen fail-secure so "no weak key is ever produced". But
ciris-keyring's own mints drew rawOsRng, bypassing the SP 800-90B health latch.So the invariant held for
ciris-crypto-constructed keys and not for keyring-sealed ones — which are the federation identity keys. The gap was exactly inverted from where the guarantee was needed.Every key-material draw now routes through
ciris_crypto::random::fill:sealed_ed25519sealed_mldsa65usb_wrapped_mldsa65transport_identitysoftwarekeyring_storagemint_p256_signing_keyputs the bytes through the latch rather than probing it and then drawing unchecked, so the key material is literally what the checked path produced.The structural part
This required making
ciris-cryptoa non-optional keyring dependency, at therandomfeature only. It was optional behindpqc-ml-dsa— so the default keyring build had no latch at all. The gap wasn't an oversight at one call site; it was unreachable by construction.Incidental draws are deliberately untouched — a temp-dir suffix and a TPM attestation nonce are not key material, and routing them would blur what the rule is for.
Proven by a fail-secure test: the one #74 gave every
ciris-cryptoprimitive and the keyring never had. It uses the already-public__force_health_for_testhatch — I tried exposingtest_supportacross the crate boundary first and reverted it, because dev-dependency features don't propagate that way.#176 — the title says build record; the failing decode is the steward key
validation::query_https_source→get_steward_key→StewardKeyResponse, which requiresclassical. Two months of that message pointing people at build records.The parse itself cannot be fixed here. The registry sends the multi-steward shape; verify's modern parser for it requires a
response_signaturethe registry does not send; and relaxing that would mean silently accepting unsigned trust-root material. That's a cross-repo contract decision (CIRISRegistry#133), so this doesn't paper over it.What is fixable is the misdirection. The error now names the endpoint, the drift, the tracking issue, and "NOT a build-record problem and NOT a network problem". An unrelated parse error passes through untouched, asserted by test.
A correction to my own backlog report: I said this half was unfixed. v13.3.0 had already fixed the "HTTPS unreachable" mislabel on this path — I checked the build-record path rather than the one that actually fails.
1399 workspace green, clippy clean, doc clean, all guards + self-test pass.
Merging this completes the four-fix 14.1.0 (#224, #269, #207-6, #176).
🤖 Generated with Claude Code
https://claude.ai/code/session_01FZPhbXWTzykcbotEX9BN2p