Unify JWT crypto backend and improve validation panic handling#388
Merged
Conversation
- Replace the inline RS256 token and committed public key with a throwaway RSA keypair generated at runtime; the token is signed from readable claims so no key material or opaque blobs live in the repo - Add `rsa` as a dev-only dependency (getrandom feature) for in-test key generation; release binary is unaffected Addresses review feedback on #386.
Addresses review feedback on the validator panic-containment change: - Keep raw panic payloads out of the cached and user-visible `validation_response_body`, since a panic message can embed secret material (e.g. a token captured in a debug string). The visible body now reports only the stable rule id, and the detailed payload is emitted via truncated structured logging. - Replace the nested `Result<Result<(), String>, Elapsed>` with a self-describing `ValidationOutcome` enum (`Completed` / `Panicked` / `TimedOut`) so call sites and signatures read clearly. - Document why the `AssertUnwindSafe` panic boundary is sound: the recovery path deterministically resets the match's validation fields, and the shared counters/cache are only mutated after the boundary returns, so an unwind cannot leave them inconsistent. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…s during validation-heavy scans. Kingfisher built two Tokio runtimes (main + artifact-fetcher) that each defaulted to 512 blocking threads, which combined with Rayon pools and per-call spawns could exceed the OS per-user thread limit (RLIMIT_NPROC, default 8000 on macOS). Both runtimes now cap their blocking pools at max(num_jobs * 8, 32), and on Unix the soft RLIMIT_NPROC is raised to the hard limit at startup so users don't need to tune ulimit -u manually.
fix(jwt): unify jsonwebtoken crypto backend
fix(validation): contain validator panics
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves Kingfisher scan robustness and resource usage by (1) preventing validator panics from crashing the entire scan, (2) bounding Tokio blocking thread pools to reduce thread exhaustion, and (3) addressing asymmetric JWT (RS256) validation issues via a unified jsonwebtoken crypto backend and regression coverage.
Changes:
- Catch validator panics during per-match validation and convert them into deterministic validation failures (including caching behavior and regression tests).
- Cap Tokio blocking thread pools for both the main runtime and the artifact-fetcher runtime via a shared helper.
- On Unix, attempt to raise the soft
RLIMIT_NPROCto the hard limit at startup; add RS256 JWT regression test and update dependencies/changelog/version.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/int_jwt_provider.rs |
Adds an integration regression test covering RS256 validation through the fallback key path. |
src/util.rs |
Introduces tokio_blocking_threads_limit() and unit tests for scaling/capping behavior. |
src/scanner/validation.rs |
Wraps validation in panic-catching logic, adds outcome handling + tests, and adjusts caching behavior for panics/timeouts. |
src/scanner/runner.rs |
Applies the Tokio blocking thread cap to the artifact-fetcher runtime. |
src/main.rs |
Raises Unix RLIMIT_NPROC best-effort at startup and caps the main Tokio runtime blocking pool. |
CHANGELOG.md |
Documents the new release changes in v1.101.0. |
Cargo.toml |
Bumps version, switches gcloud-storage JWT backend feature, adds libc on Unix, and adds test-only deps for JWT/RSA regression coverage. |
Cargo.lock |
Updates resolved dependency graph for the version bump and crypto backend changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+992
to
+998
| // user-visible body. Emit the detail through structured logging | ||
| // (truncated), and keep the visible body to the stable rule id. | ||
| warn!( | ||
| rule_id = %om.rule.id(), | ||
| panic = %truncate_for_log(&panic_message), | ||
| "validator panicked; marking match as failed", | ||
| ); |
Comment on lines
+1001
to
+1005
| "Validation panicked for rule {}", | ||
| om.rule.id() | ||
| )); | ||
| om.validation_response_status = http::StatusCode::INTERNAL_SERVER_ERROR; | ||
| fail_count.fetch_add(1, Ordering::Relaxed); |
Comment on lines
+1040
to
+1056
| /// `AssertUnwindSafe` is required because the future borrows `&mut om`. It is | ||
| /// sound for this use because the unwind is never observed as a partial result: | ||
| /// on the panic path [`apply_validation_outcome`] unconditionally overwrites the | ||
| /// match's validation fields (`validation_success`, `validation_response_status`, | ||
| /// `validation_response_body`) with a deterministic failure state. The shared | ||
| /// counters and response cache are only mutated *after* this boundary returns, | ||
| /// so a panic cannot leave them inconsistent. | ||
| async fn catch_validation_panic<F>(future: F) -> std::result::Result<(), String> | ||
| where | ||
| F: Future<Output = ()>, | ||
| { | ||
| match AssertUnwindSafe(future).catch_unwind().await { | ||
| Ok(()) => Ok(()), | ||
| Err(payload) => Err(describe_panic_payload(payload)), | ||
| } | ||
| } | ||
|
|
Comment on lines
+134
to
+135
| // SAFETY: getrlimit/setrlimit are async-signal-safe and take a properly | ||
| // sized `rlimit` we own. |
Comment on lines
+194
to
+197
| let max_blocking = tokio_blocking_threads_limit(num_jobs); | ||
| let runtime = Builder::new_multi_thread() | ||
| .worker_threads(num_jobs) | ||
| .max_blocking_threads(max_blocking) |
Comment on lines
+506
to
+510
| Ok(Err(_panic_payload)) => { | ||
| warn!( | ||
| rule_id = %m.rule.syntax().id, | ||
| "validator panicked; marking match as failed", | ||
| ); |
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
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.
This pull request introduces several improvements to Kingfisher's validation reliability and resource management, particularly around error handling and thread usage. The most significant changes are improved handling of validator panics to prevent whole-scan crashes, a cap on Tokio blocking thread pools to reduce resource exhaustion, and a fix for asymmetric JWT validation issues. Additional regression tests and changelog updates are included.
Validation reliability and error handling:
jsonwebtoken,rsa) and regression coverage for RS256 JWT validation.Resource management improvements:
tokio_blocking_threads_limithelper to prevent excessive thread creation during validation-heavy scans. [1] [2] [3] [4] [5] [6]RLIMIT_NPROC(max processes/threads per user) is now raised to the hard limit at startup to reducefailed to spawn threaderrors. [1] [2] [3]Security and compatibility fixes:
jsonwebtokencrypto backend and updating thegcloud-storagedependency to usejwt-rust-crypto. [1] [2]Documentation:
CHANGELOG.mdwith details of these fixes and improvements.Versioning:
v1.101.0inCargo.toml.