Never let a degraded RNG path succeed quietly - #932
Conversation
|
Warning Review limit reached
Next review available in: 20 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughThis PR reports NSM entropy fallbacks, propagates RNG failures in mock attestation generation, and adds a tracked-source scanner with GitHub Actions enforcement and security documentation. ChangesRNG hygiene and failure handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 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 @.github/workflows/rng-hygiene.yml:
- Around line 22-29: Harden the check job around the actions/checkout step and
execution of check-rng-hygiene.sh by disabling persisted checkout credentials
and declaring an explicit least-privilege permissions block for the job, with
all token permissions denied. Keep the existing checkout and script execution
behavior unchanged.
In `@scripts/check-rng-hygiene.sh`:
- Around line 172-186: Update the rule 2 `swallow_bad` scan to reuse or extract
the statement-joining logic from `scan_rust`, ensuring `fill_bytes` and
`random_bytes` calls followed by `unwrap_or`, `unwrap_or_default`,
`unwrap_or_else`, or `ok` on later lines are detected. Preserve the existing
comment filtering, opt-out filtering, and failure reporting.
- Around line 46-47: Guard the directory change immediately after the shell
options so a failed cd "$(dirname "$0")/.." terminates the script with a nonzero
status. Keep the existing source-listing and rule-check logic unchanged,
ensuring the script cannot report success from the wrong working directory.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d317635-16d1-40b0-aa1f-0c6d612bec68
📒 Files selected for processing (6)
.github/workflows/rng-hygiene.ymldocs/SECURITY.mdkeep-agent/src/entropy.rskeep-enclave/enclave/src/signer.rskeep-enclave/host/src/mock.rsscripts/check-rng-hygiene.sh
| check: | ||
| name: No predictable-RNG fallbacks | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Check for silent RNG fallbacks | ||
| run: ./scripts/check-rng-hygiene.sh |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict the token available to a job that runs PR-branch-controlled code.
This job checks out and executes scripts/check-rng-hygiene.sh from the PR's own branch. A PR author can modify that script. actions/checkout@v4 defaults to persist-credentials: true, leaving the ambient GITHUB_TOKEN in .git/config for that PR-controlled script to read. No permissions: block is set either, so the default token scope applies to the whole job.
🔒️ Proposed fix
jobs:
check:
name: No predictable-RNG fallbacks
runs-on: ubuntu-latest
+ permissions:
+ contents: read
steps:
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v4
+ with:
+ persist-credentials: false
- name: Check for silent RNG fallbacks
run: ./scripts/check-rng-hygiene.sh📝 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.
| check: | |
| name: No predictable-RNG fallbacks | |
| runs-on: ubuntu-latest | |
| steps: | |
| - uses: actions/checkout@v4 | |
| - name: Check for silent RNG fallbacks | |
| run: ./scripts/check-rng-hygiene.sh | |
| check: | |
| name: No predictable-RNG fallbacks | |
| runs-on: ubuntu-latest | |
| permissions: | |
| contents: read | |
| steps: | |
| - uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false | |
| - name: Check for silent RNG fallbacks | |
| run: ./scripts/check-rng-hygiene.sh |
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 26-26: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 22-30: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
🤖 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 @.github/workflows/rng-hygiene.yml around lines 22 - 29, Harden the check job
around the actions/checkout step and execution of check-rng-hygiene.sh by
disabling persisted checkout credentials and declaring an explicit
least-privilege permissions block for the job, with all token permissions
denied. Keep the existing checkout and script execution behavior unchanged.
Source: Linters/SAST tools
1 participant
Summary
rand::rng()panics on init failure and (since rand 0.9.3) on reseed failure,rand_core::OsRng::fill_bytespanics,getrandom::fillresults are propagated with?at every one of the sixteen production call sites, andkeep-core::entropygates every public entry onensure_entropy_health(), re-validated on pid change and every 4096 generations. No#ifndef-on-a-value equivalent, no seeded PRNG standing in for the OS RNG, no zeroed-buffer fallback, and nothing security-relevant inkeep-agent-py,keep-agent-ts,keep-web/ui, ordeploy/.What changed
The NSM fallbacks are no longer silent.
keep-agent::get_entropyreturns OS randomness when a Nitro Secure Module request fails, and the enclave binary'sgetrandomdoes the same when the NSM device is not there at all. Neither is cryptographically weak (an enclave's OS RNG is itself NSM-seeded) and neither should become a hard error, because outside an enclave the fallback is the only path that works and dev boxes aretarget_os = "linux"too. But losing the attested source while still handing back bytes is exactly the shape this audit is about, so both now say so:tracing::error!on the per-request failure, and aOnce-guarded warning for the missing device, since key generation calls it in a loop.keep-enclave-host's mock backend no longer hands back constants.create_mock_attestation_document,create_mock_certificate, andrand_u64each warned and continued on an RNG failure, leaving their[0u8; N]buffers untouched: a well-formed attestation document whose signature, certificate, and module id are all fixed.MockEnclaveClientis exported from the crate's public API, and its caller already had an error branch, so all three now return aResultinto it.The CI guard.
scripts/check-rng-hygiene.shplus anrng-hygieneworkflow pin four shapes: an unhandledgetrandom/try_fill_byteserror, an RNG failure collapsed into a value (.ok(),unwrap_or,unwrap_or_default), a seeded PRNG in production code, and any public entry point inkeep-core/src/entropy.rsthat reaches the unchecked mixer withoutensure_entropy_health()?. That last rule is the load-bearing one: the mixer folds timing jitter and process context in with the OS bytes, so its output looks random even when a source has degraded. (The review round below found the gate itself had that same blind spot, and fixes it.) Adocs/SECURITY.mdRandomness section states the rule and the guard's blind spots.Review notes
The guard is deliberately more forgiving than a
?-only rule.if let Err(e) = getrandom::fill(..) { return Err(..) }andmatch rng() { .., Err(e) => return .. }handle a failure just as well, and flagging them would push people to rewrite correct code to satisfy a grep. Deciding that needs the block body, so the script slurps each file and scans forward for an exit before judging.#[cfg(test)]bodies are skipped by brace depth, not by filename, because unit tests live inline insrc/throughout this workspace and a filename filter would either miss them or exclude the production code sitting beside them.Decision log
Hardest decision: whether to make the NSM fallbacks fail closed. They should not. The enclave binary compiles and runs on ordinary Linux where
nsm_init()legitimately fails, so an error there breaks every local test and dev run to defend against a case the type system cannot distinguish. Logging is the honest fix: the fallback is sound, the silence was not.Alternatives rejected:
keep_core::crypto::random_bytes()in production. There are nine production call sites, and the migration totry_random_bytesis separately tracked (keep-core: RNG health-check failure panics in key/secret generation instead of returning Result #685). A panic is fail-loud, so those sites are the wrong bug class for this PR; folding a nine-site migration in would bury the actual findings.?/expect/unwrapas a defect, which flags three correct sites in this workspace, and a guard that cries wolf on correct code is a guard people learn to skip.Least confident about: rule 4 is structural, not semantic. It checks that
ensure_entropy_health()?is called before the mixer is reached, not that the health check itself is sound. That gap was not hypothetical -- see the review round -- and the answer was a test rather than a smarter grep. Stated in the script header and indocs/SECURITY.mdrather than left implied.What is not verified
The three fixed mock sites have no test that pins them:
getrandom::fillcannot be made to fail without injecting a seam, so the old code passes the suite too. That is the defect being fixed, not an excuse. The#[cfg(feature = "tracing")]arm of the enclave warning was compile-checked, not run, and no test observes either log line.Test plan
cargo check/clippy -p keep-enclave-host -p keep-agent --all-targets: clean, no new warningscargo check -p keep-agent --features enclave: clean (needs a newer toolchain than the 1.89 MSRV pin, because a transitive nsm-api dependency requires it; verified on 1.96)keep-enclave/enclave(workspace-excluded) checks and clippies clean with and without thetracingfeaturecargo test -p keep-enclave-host -p keep-agent: 61 passed, 0 failedcargo fmt --checkcleanscripts/check-rng-hygiene.shexits 0 on the treeReview round
A security review and a production review ran over the branch. The security review's job was to falsify the "everything else is already clean" claim above, and it half succeeded: the narrow claim held (no zeroed-buffer fallback, no seeded PRNG, no time-derived key material anywhere in production Rust), but it found the claim's justification was wrong, and a separate signing-path defect.
The entropy gate could not see a degraded OS source.
check_entropy_health_internalsampledrandom_bytes_mixed_internal(), the post-mix output. Every mix folds in a monotonically incrementing counter, 64 timing deltas, and live heap and stack addresses, so three post-mix samples come out non-zero, distinct, and roughly 128 bits apart no matter what the OS pool contained. Substituting a constant for the OS entropy left every criterion satisfied. The gate was checking the combiner, not the source, which is the same shape as the bug the check exists to catch, and thedocs/SECURITY.mdparagraph I wrote asserted the opposite.check_entropy_health_internalnow samplesgather_os_entropydirectly, before anything is mixed in, and checks the mixed output as well (that catches a broken mixer rather than a broken source).mixing_hides_a_dead_os_sourcepins the masking: it mixes a deliberately all-zero OS pool with the real jitter and context and asserts the result still passes every criterion, so if anyone reverts the pre-mix check the reason it exists is written down in an executing test.The NSM-unavailable case this PR set out to report was still silent.
is_nitro_enclave()is defined as "nsm_init()succeeded", cached for the process lifetime, so the likelier fault -- device node missing, or the open failing under fd pressure -- made it returnfalse, skipped the enclave branch entirely, and used the OS RNG with nothing logged. Detecting enclave-ness by probing the very device whose loss you want to hear about cannot distinguish the two; building with theenclavefeature is the statement that an NSM is expected, so a failed probe now warns once from there. The per-drawtracing::error!stays per-draw and says why in a comment: unlike a missing device, a failing request is transient and its rate is the signal.A partial NSM fill was topping up from the OS RNG in silence.
getrandomin the enclave binary copied a shortGetRandomresponse and filled the remainder from::getrandom, with no report -- a partial downgrade of the attested entropy on every draw larger than one response, which is every 32-byte key. It now loops the NSM for the remainder, matching whatkeep-agent'sfill_from_nsmalready did, with an empty response treated as a failure rather than an infinite loop.Five working bypasses in the guard, plus two false positives, each now covered and verified. These mattered more than they look, because a guard nobody can trust is the thing this PR is arguing against:
match rng() { Ok(()) => return k, Err(_) => {} }read as handled, because the block scan looked at the whole block including the Ok arm. It now narrows to the error arm, splitting single-line match arms at the=>.let _ = rng(&mut k).map_err(|e| f(n)?)passed on the nested?. Propagation now has to survive stripping balanced paren groups, so it must belong to the RNG call.try_random_bytes()\n .unwrap_or_default()escaped rule 2 because it was a single-line grep. Rule 2 now detects on the call name and judges the assembled statement.fn getrandom(buf) { let _ = ::getrandom::fill(buf); }passed, because the fn-declaration filter dropped the whole line. It now skips past the declaration and keeps searching, which is what its comment always claimed.#[cfg(test)]on a brace-less item latched the skip onto the next brace block. There is a live instance of that shape atkeep-enclave/host/src/mock.rs:16; today it only swallows a struct, but it would have exempted animplblock wholesale./dev/nullbehind|| true, so a broken awk reported a clean tree, andxargs -ris GNU-only so rule 2 was a silent no-op on macOS. Verified by running with a stubawkthat exits 2. It now fails closed on a scanner error, a non-git tree, or an empty file list, and rule 4 additionally fails if it finds no gated entry point at all, which is what happens when the module is reshaped and the rule quietly stops meaning anything.keep-web/src/bunker.rs:56is correct fail-closed code (.ok()into anOptionthe caller refuses on) that only escaped the old rule because rustfmt split the line. It now carries an explicit// rng-hygiene: okmarker naming the reason, rather than passing by accident.Also from the reviews: the workflow now pins
actions/checkoutby commit SHA and setspermissions: contents: read, matchingci.ymlandrelease.yml; twociborium::into_writer(..).unwrap_or_default()calls in the same mock function -- the identical error-into-default shape this PR removes from the RNG calls beside them -- now propagate; anddocs/SECURITY.mdnames thekeep_core::crypto::random_bytesexception (it panics rather than returning aResult, nine production call sites, migration tracked in #685) instead of asserting a convention the code does not fully follow.Filed separately, not in this PR: the security review found that
NonceStore::check_and_add_noncecould never reject a reused FROST nonce commitment, because its reject branch tested a flag nothing ever set. That is a different bug class in a different subsystem and it is #933.Test plan (updated)
cargo test -p keep-core --lib entropy: 12 passed, including the two new onescargo test -p keep-enclave-host -p keep-agent -p keep-web: 115 passed, 0 failedcargo clippyclean on the changed crates,cargo fmt --checkclean,keep-enclave/enclave(workspace-excluded) checks clean with and without thetracingfeaturecargo check -p keep-agent --features enclaveclean (needs a newer toolchain than the 1.89 MSRV pin because of a transitive nsm-api dependency; verified on 1.96)matchwhose Err arm only logs, the original warn-and-continue defect,let _ =,.ok(), andStdRng::seed_from_u64. Stays clean onif let Err(e) = .. { return Err(..) }, both one-line and multi-linematchwith a returning Err arm,.map_err(..)?, an opt-out marker, and code inside#[cfg(test)]awkexiting 2, and running outside a git work tree, both report failure instead of OK