Skip to content

keep-web: fail closed instead of panicking when the RNG health check trips - #918

Merged
kwsantiago merged 2 commits into
mainfrom
keep-web-fallible-rng
Jul 29, 2026
Merged

keep-web: fail closed instead of panicking when the RNG health check trips#918
kwsantiago merged 2 commits into
mainfrom
keep-web-fallible-rng

Conversation

@kwsantiago

@kwsantiago kwsantiago commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Four paths in the web co-signer drew randomness through the panicking random_bytes() helper. Two are reachable while the service is running, so an entropy health-check failure would unwind inside a live signer rather than being reported; the other two abort startup with a raw panic message instead of the clean error their io::Result signature already supports. All four now use the fallible helper and fail closed.

Request paths

  • WebSocket ticket (ws_ticket): returns 503 with a retry hint instead of panicking. The ticket authorizes a WebSocket upgrade, so a value the health check has flagged must never be issued.
  • Approval id (random_approval_id): returns None when the check trips, and the caller refuses the approval and logs it, mirroring the existing refusal used when co-signing is disabled. Unguessability is the entire purpose of this id (it is defense-in-depth alongside the auth gate), so refusing beats falling back to anything predictable. The refusal happens before the approval slot is registered or broadcast, so no half-created approval is left behind.

Startup paths (both already returned io::Result, and the crate already defines the fallible wrapper used by its siblings)

  • The 32-byte admin bearer token.
  • The per-write temp-file suffix used to avoid clobbering an in-flight temp file.

After this, no caller of the panicking helper remains anywhere in the crate.

Scope note: this covers the entropy health-check verdict. A hard OS-entropy failure still unwinds inside the RNG itself before the check can report it; that is a pre-existing keep-core issue and is tracked separately rather than papered over here.

Part of #803.

Test plan

  • cargo test -p keep-web — 54 passed, 0 failed. The existing approval-id range test now asserts the healthy path yields an id.
  • cargo clippy -p keep-web --all-targets -- -D warnings and cargo fmt --check — clean.
  • Verified by grep that no occurrence of the panicking helper remains in the crate.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

Secure randomness failures are now handled explicitly for WebSocket tickets and NIP-46 approval IDs. Ticket requests return HTTP 503, while approval requests are denied when an approval ID cannot be generated. The related JavaScript-safety test now unwraps the optional ID.

Changes

Entropy failure handling

Layer / File(s) Summary
Fallible randomness across web flows
keep-web/src/api.rs, keep-web/src/bunker.rs
WebSocket ticket generation returns 503 SERVICE_UNAVAILABLE when entropy is unavailable. Approval ID generation returns an optional JavaScript-safe value, and approval requests are denied if generation fails; the corresponding test unwraps the healthy result before checking its range.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • privkeyio/keep#686: Both changes propagate secure-randomness failures instead of assuming RNG calls always succeed.

Poem

A rabbit found entropy shy,
So tickets wait and approvals deny.
Safe IDs still hop in range,
When random bytes are sound again.
\🐇 No bad luck shall pass nearby!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: keep-web now fails closed on RNG health-check failures instead of continuing with unsafe randomness.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch keep-web-fallible-rng

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kwsantiago
kwsantiago merged commit fd5e18c into main Jul 29, 2026
10 checks passed
@kwsantiago
kwsantiago deleted the keep-web-fallible-rng branch July 29, 2026 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant