fix(sphragis): return typed entropy failures instead of panicking - #28
Conversation
31e8e57 to
284b679
Compare
|
Rebased onto current Two coherence decisions this rebase had to make, since #32 landed after this branch was written:
|
OsRng.fill_bytes panics on OS-RNG failure (rand_core 0.6 os.rs); this crate
called it directly in HybridKem::generate, EncapsulationKey::encapsulate,
and the AEAD nonce draw inside seal_for. A recoverable host entropy failure
crashed the process instead of returning a SealError, and the two already-
fallible operations had no error variant covering it.
Every entropy draw now goes through try_fill_bytes and propagates as a new
SealError::Entropy { source: rand_core::Error, location }. HybridKem::generate
becomes fallible (a breaking pre-1.0 change, taken now per the issue, before
the preview-pq API stabilizes). The RNG is caller-injectable at each site
(generate_with_rng, encapsulate_with_rng, seal_for_with_rng, each
&mut R: RngCore + CryptoRng) because the OS RNG cannot be made to fail on
demand and this is the only way to prove the typed-error path under test.
rand_core's std feature is enabled (alongside getrandom) so rand_core::Error
implements std::error::Error and chains behind SealError::Entropy's source
field.
tests/entropy_failure.rs injects a CountdownRng (succeeds N calls, then
fails every call after) through all three seams, including mid-batch across
two recipients inside seal_for_with_rng and specifically isolating the nonce
draw from the encapsulation draw, and asserts every case returns
Err(SealError::Entropy) with a call-site-accurate location, never panics.
Also: removed a stale CHANGELOG.md "Unreleased" heading that sat below the
already-released 0.1.2/0.1.1 sections describing commits shipped in 0.1.1.
Part of #16
clippy::similar_names (promoted to error by -D warnings) flagged the 64-byte randomness buffer 'rnd' against the new 'rng' CSPRNG parameter. Renamed to 'randomness', matching encapsulate_deterministic's own parameter name for the same buffer. Part of #16
…ilure.rs The crate's [lints.clippy] denies unwrap_used/expect_used/panic crate-wide, including tests/*.rs (matching every other integration test file's own #![expect(...)] overrides, not a per-target exemption). Missed two of the three needed here: CountdownRng::fill_bytes deliberately expect()-panics (mirroring OsRng's real behavior — the point of the mock), and several test bodies use panic!/an unmatched-arm panic! the same way assert! does internally. Also swapped a match-with-empty-arm for let-else in the last test, avoiding a possible clippy::single_match_else round-trip. Part of #16
…lic API Rebasing fix/16-entropy-error onto main (which now carries #23's hazmat narrowing) surfaced two defects the merge itself introduced, both caught by the preview-pq (no hazmat) CI job: - generate_recipient_keypair became fallible along with HybridKem::generate, but tests/profile_api.rs (added by #23, exercises the narrowed API without hazmat) still called it as if infallible. Added the missing .unwrap() at each of its three call sites; each test already carries a file-scoped #[expect(clippy::unwrap_used, ...)]. - EncapsulationKey::encapsulate's non-hazmat pub(crate) variant lost its only caller: seal_for now delegates to seal_for_with_rng, which is generic over the RNG and calls encapsulate_with_rng directly, never the fixed-OsRng wrapper. Without hazmat that left encapsulate() genuinely unused inside the crate -- dead_code, denied under -D warnings. Made it hazmat-only outright (no pub(crate) variant), matching its real reachability: its only consumer is tests/known_answer_vectors.rs, which already requires hazmat. DECISION.md corrected to match: it had described encapsulate_with_rng's hazmat split as mirroring encapsulate's, which stopped being true once encapsulate lost its non-hazmat variant. Part of #16
"matching HybridKem's gating above" was wrong: HybridKem's pub(crate)/pub split is declared after SharedSecret's in this file, not before it. Reworded to avoid a positional claim that silently goes stale if either declaration moves again. Part of #16
55213ae to
3bf69ad
Compare
|
Rebased onto current Checked the two specific things flagged for this rebase, by reading the merged file:
Also fixed a small positional error in the CI green on the current head. Not merging — leaving that to you. |
…ation (#33) ## Summary - `SharedSecret` was a bare `pub type SharedSecret = Zeroizing<[u8; 32]>` alias, so it inherited `[u8; 32]`'s derived `Debug` — `{:?}` printed the live X-Wing shared secret. `Zeroizing` protects memory on drop, not the value while it is alive. Replaced with a newtype carrying a manual, redacting `Debug` (`"SharedSecret([REDACTED])"`), matching `DecapsulationKey`'s existing pattern. Zeroize-on-drop is unchanged (the newtype still wraps `Zeroizing`). An `as_slice()` accessor preserves every existing call site (`derive_wrap_key`, the KAT assertions) — no behavior change beyond the `Debug` surface. - Finishes the `#[snafu(implicit)] location` retrofit onto `SealError` that #16/#28 started: every remaining variant (`WrongLength`, `InvalidMlKem`, `HkdfExpand`, `AeadSeal`, `AeadOpen`, `UnsupportedVersion`, `Serialization`, `EnvelopeTooLarge`, `TrailingData`) now carries a location, and every construction site that built the enum directly (`map_err` closures, a bare `return Err(..)`) now goes through its context selector (`.context()` / `.build()` / `.fail()`) so the field actually populates. ## Tests - New `shared_secret_debug_is_redacted` (`tests/known_answer_vectors.rs`): formats a real `SharedSecret` obtained from `encapsulate()`, asserts the output contains `"REDACTED"` and does **not** contain the hex-encoded secret bytes. This is the mandatory negative-case fixture — a type-compiles check would prove nothing. - Extended `wrong_length_ek_and_ct_rejected` to capture one `SealError::WrongLength` and assert `location.file.ends_with("hybrid.rs")`, mirroring the pattern `tests/entropy_failure.rs` already uses for `SealError::Entropy`. - All existing KAT / round-trip / negative tests are unchanged in behavior; `SealError` construction now goes through context selectors instead of struct literals, but every variant's fields (other than the new `location`) are identical. ## Notes - Adjudicated #26 vs #29 (duplicate SealError-location findings) separately; this PR closes out #26's scope. See the issue comments for the duplicate ruling. Closes #25 Closes #26 --------- Co-authored-by: forkwright <cody@forkwright.com>
🤖 I have created a release *beep* *boop* --- ## [0.2.0](v0.1.2...v0.2.0) (2026-08-16) ### ⚠ BREAKING CHANGES * **sphragis:** make deterministic encapsulation private ([#31](#31)) ### Features * **sphragis:** define revocation as key rotation, not re-wrapping ([#34](#34)) ([37ba8e3](37ba8e3)) * **sphragis:** narrow the public API to the envelope profile, define the adapter seam ([#32](#32)) ([d0a0bb8](d0a0bb8)) ### Bug Fixes * **sphragis:** bind the KAT gate to a machine-readable crypto provenance lock ([#27](#27)) ([65e3433](65e3433)) * **sphragis:** bound and fully consume untrusted CBOR before accepting a wrapped key ([#24](#24)) ([d165e5b](d165e5b)) * **sphragis:** make deterministic encapsulation private ([#31](#31)) ([cf48668](cf48668)) * **sphragis:** redact SharedSecret's Debug and retrofit SealError location ([#33](#33)) ([11216eb](11216eb)) * **sphragis:** return typed entropy failures instead of panicking ([#28](#28)) ([2ff3a08](2ff3a08)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
OsRng.fill_bytespanics on OS-RNG failure (rand_core0.6os.rs), and this crate called it directly at three sites:HybridKem::generate,EncapsulationKey::encapsulate, and the AEAD nonce draw insideseal_for. A recoverable host entropy failure (an early-boot RNG-not-seeded window, a sandboxed/misconfigured environmentgetrandomcannot service) crashed the process instead of returning aSealError.Every entropy draw now goes through
try_fill_bytesand propagates as a newSealError::Entropy { source: rand_core::Error, location }.HybridKem::generatebecomes fallible. The RNG is caller-injectable at each site (generate_with_rng,encapsulate_with_rng,seal_for_with_rng, each&mut R: RngCore + CryptoRng) — the trait boundx25519-dalek's ownrandom_from_rngrequires — because the OS RNG cannot be made to fail on demand and this is the only way to prove the typed-error path under test.rand_core'sstdfeature is now enabled (alongsidegetrandom) sorand_core::Errorimplementsstd::error::Errorand chains behindSealError::Entropy'ssourcefield rather than being flattened to a string.Closes #16
Breaking change
HybridKem::generate() -> (DecapsulationKey, EncapsulationKey)is nowHybridKem::generate() -> Result<(DecapsulationKey, EncapsulationKey), SealError>. Taken deliberately, per the issue, before thepreview-pqAPI stabilizes.akroasisconsumessphragisvia an unpinned baregit =reference and will take this break on its next build.EncapsulationKey::encapsulateandseal_forkeep their existingResult<_, SealError>signatures — addingSealError::Entropyto what they can return is non-breaking against#[non_exhaustive].What I verified, not just assumed
rand_core::Errordoes not implementstd::error::Errorunder this crate's prior feature set (getrandomonly, nostd) — confirmed by readingrand_core-0.6.4/src/error.rs, where theimpl std::error::Error for Errorblock is#[cfg(feature = "std")]-gated off. Enablingstd(additive, no new transitive dependencies —Cargo.lockis unchanged) was necessary beforeSealError::Entropy'ssourcefield could compile against.context().&mut Rrather thanx25519-dalek's by-valueT, deliberately:seal_for_with_rngdraws twice per recipient across N recipients from the same RNG, and a mid-batch injected-failure test needs that RNG's state to survive across draws, which a take-and-drop-per-call parameter would not allow.Test evidence (the failing-test-before)
The OS RNG cannot be made to fail on demand, so there is no way to git-bisect a literal "same test, red before / green after" against
main— the injectable seam this PR adds is what makes the defect observable at all.tests/entropy_failure.rs's injectedCountdownRng(succeeds N calls, then fails every call after) drivesgenerate_with_rng,encapsulate_with_rng, andseal_for_with_rng— including mid-batch across two recipients, and specifically isolating the nonce draw from the encapsulation draw insideseal_for_with_rng— asserting every case returnsErr(SealError::Entropy { .. })with a call-site-accuratelocation, never panics.The mock's own
fill_bytes(the trait's infallible method, distinct from thetry_fill_bytesproduction code calls) is written to.expect()-panic if anything ever calls it — mirroring exactly what realOsRng::fill_bytesdoes on failure — so it doubles as a live check that production code has not regressed onto the panicking path: if any_with_rngcall site were reverted to.fill_bytes(),generate_with_rng_returns_entropy_error_not_panic(which injects a zero-succeedsCountdownRng) would abort withCountdownRng: production code must use try_fill_bytes, not fill_bytesinstead of returningErr.cargo build --features preview-pqandcargo fmt --all -- --checkwere confirmed clean locally before the first push. metis was under sustained heavy load from a concurrent fleet-wide push at the time (uptimeload average in the 20s-30s against an 8-core box, vgate admits nothing above ~7.2), so the full localcargo clippy --all-targets --features preview-pq -- -D warnings/cargo test --features preview-pqrun never got admitted before I pushed — CI (ci.yml+ thegate/full-gate-buildreusable workflow, both GitHub-hostedubuntu-latest, uncontended) is what actually ran both, and caught three real findings the localcargo build-only check couldn't have:clippy::similar_names(the newrngparameter next to a pre-existing local namedrnd) and two moreclippy::expect_used/clippy::panicmisses in the new test file itself (this crate's[lints.clippy]denies those crate-wide, includingtests/*.rs, and I'd only carried overunwrap_used's file-level#![expect]from the existing test files' convention, not the other two). All fixed in follow-up commits on this branch; CI is green as of the current HEAD.Also fixed inline (adjacent, not scope creep)
CHANGELOG.mdhad a stale## [Unreleased]heading sitting below the already-released0.1.2/0.1.1sections — its rich prose documented commits already shipped in0.1.1(issues #1/#3/#4/#6), mislabeled as unreleased. Removed the stale heading (the prose now reads as0.1.1detail, where it belongs) and opened a fresh## [Unreleased]at the top for this change.Filed, not bundled
Found two adjacent issues while in this code and filed them separately rather than growing this diff:
SharedSecret's derivedDebugprints the raw shared secret (the same class of leakDecapsulationKeyalready got a manual redactingDebugfor).SealError's seven pre-existing variants lack#[snafu(implicit)] locationper the fleet Rust standard; retrofitting it touches every fallible construction site in three files and is a poor fit for this PR's diff.