fix(deps)!: link the TLS provider only with a crypto backend - #114
Conversation
rustls-rustcrypto was an unconditional dependency, so a `default-features = false` build (transcoding only, or an injected verifier) still linked it, and with it `rsa`, `paste` and `rustls-webpki` 0.102: a consumer's advisory scan failed on crates the build never needed. - The outbound rustls provider follows the crypto backend features: a provider the process installed wins, else aws-lc with `aws_lc_rs`, else RustCrypto with `rust_crypto`. With neither feature no provider is linked, and building a JWKS or rate-limit client without an installed one fails with an error naming both remedies (reqwest needs a provider even for http:// endpoints). - `JwksCache::new` returns a `Result` instead of falling back to a default client, which panics when no provider exists. - CI checks that the build without a crypto backend links none of the crates whose advisories deny.toml ignores. - The limit-service tests move out of resolve.rs into resolve/tests.rs. Tests cover the provider order, the error without one, a provider that cannot negotiate TLS 1.2 or 1.3, and which provider each backend brings. BREAKING CHANGE: a `default-features = false` build links no rustls crypto provider; install one before configuring a JWKS endpoint or the rate-limit service. `JwksCache::new` returns `Result<JwksCache, String>`. Closes #106
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (12)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughOutbound TLS now selects a process-installed or feature-provided rustls crypto provider. JWKS and rate-limit client construction returns provider errors instead of falling back to a default client. CI checks the injected-verifier dependency tree for specified crates. ChangesOutbound TLS configuration
Priority: ⬆️ High Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: High Merge Risk: ⚪ Minimal · up to The provider-free startup requirements and remedies are documented, and the dependency check targets the intended build configuration. No identified issue prevents merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Outbound clients now require an available TLS provider and fail during startup if one cannot be used. The examined paths retain certificate verification and do not silently start without configured authentication or rate limiting. Custom provider choices and deployment behavior remain to be validated by adopters. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The PR makes the TLS provider optional and disables it for Resolution Use a TLS provider with a clean advisory dependency tree, or patch ✨ Finishing Touches📝 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 |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
The handshake tests always handed the client the RustCrypto provider, so with `aws_lc_rs` the aws-lc provider production selects never completed a handshake in a test. The client now uses the provider the build brings, and RustCrypto only in a build without a backend. The `client_config` doc linked a function that no longer exists; it now links `select_provider`, so a private-item rustdoc build with warnings denied passes.
main moved the binary into this package behind the `cli` feature. The injected-verifier CI leg now builds with `--no-default-features --features cli`; the check that it links no crate with an ignored advisory runs on that build, next to main's check that the default build links no CLI dependency.
1 participant
Summary
A
default-features = falsebuild no longer links a TLS crypto provider, so a crate that only transcodes (or injects its own verifier) pulls in none ofrustls-rustcrypto,rsa,pasteorrustls-webpki0.102, and its advisory scan passes without ignores.The outbound rustls provider (JWKS fetches, the rate-limit service) now follows the crypto backend features, in this order:
rustls::crypto::CryptoProvider::install_default: an explicit choice wins, as in rustls itself;aws_lc_rs(it also wins the tie withrust_crypto, as for JWTs);rust_crypto(default).With neither feature and no installed provider, building a JWKS or rate-limit client fails at startup with an error naming both remedies. It applies to
http://endpoints too: reqwest builds its TLS connector with every client.The issue's other suggestion, making
ringor aws-lc the provider, is not taken: the default build stays free of C crypto, which CI checks. The default build keepsrsathrough therust_cryptoJWT backend, as it did before 4.3.0; that is covered by the existingdeny.tomlentry.Changes
Cargo.toml:rustls-rustcryptois optional, enabled byrust_crypto;aws_lc_rsenables rustls' aws-lc provider.src/tls.rs: provider selection (select_provider, pure, so its error path is tested without process state);client_configreturns aResult, including for an installed provider that cannot negotiate TLS 1.2 or 1.3.JwksCache::newreturns aResultinstead of falling back to a default reqwest client, which panics without a provider.deny.tomlignores.src/shield/resolve.rs: its tests move toresolve/tests.rs; in a build without a backend they install the RustCrypto provider, as an embedder of such a build would.deny.tomldescribe the provider order.Testing
fmt, clippy with
-D warningsand the test suite on all four backend legs (518 / 518 / 519, and 465 without a backend), doc tests with and without a backend, rustdoc with-D warnings,cargo deny check advisories. Acargo denyrun with no ignores and dev dependencies excluded passes on the build without a backend and fails on the default one.Closes #106
BREAKING CHANGE: a
default-features = falsebuild links no rustls crypto provider; install one before configuring a JWKS endpoint or the rate-limit service.JwksCache::newreturnsResult<JwksCache, String>.