crypto panic https web silently failing #803
Conversation
…rc/index.crates.io-1949cf8c6b5b557f/rustls-0.23.35/src/crypto/mod.rs:249:14: Could not automatically determine the process-level CryptoProvider from Rustls crate features. Call CryptoProvider::install_default() before this point to select a provider manually, or make sure exactly one of the 'aws-lc-rs' and 'ring' features is enabled.
WalkthroughThe pull request adds Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-native/src/crypto.rs (1)
26-48: Add inline tests forsha256()in crypto.rs.The
sha256()function lacks inline test coverage. Per the coding guidelines, Rust tests should be integrated within source files using inline test modules. Add a test module to verifysha256()works correctly with the configured crypto provider(s).
🧹 Nitpick comments (3)
rs/moq-native/src/crypto.rs (3)
10-24: Add a compile-time check when neither crypto feature is enabled.If both
aws-lc-rsandringfeatures are disabled, the function body after the early return on line 9 becomes empty, resulting in a compile error with an unclear message. Consider adding acompile_error!directive to surface a helpful message:🔎 Proposed fix
#[cfg(feature = "ring")] { let provider_inner = rustls::crypto::ring::default_provider(); let _ = rustls::crypto::CryptoProvider::install_default(provider_inner.clone()); return Arc::new(provider_inner); } + + #[cfg(not(any(feature = "aws-lc-rs", feature = "ring")))] + compile_error!("Either 'aws-lc-rs' or 'ring' feature must be enabled for crypto support"); }
14-14: Consider handling theinstall_defaultresult to avoid returning a stale provider.
install_defaultreturnsErrif another thread already installed a different provider between theget_default()check and this call. By discarding the result withlet _ =, you may return a provider that isn't actually the process-level default. Consider re-fetching the installed default on failure:🔎 Proposed fix (example for aws-lc-rs block)
#[cfg(feature = "aws-lc-rs")] { let provider_inner = rustls::crypto::aws_lc_rs::default_provider(); - let _ = rustls::crypto::CryptoProvider::install_default(provider_inner.clone()); - return Arc::new(provider_inner); + match rustls::crypto::CryptoProvider::install_default(provider_inner.clone()) { + Ok(()) => return Arc::new(provider_inner), + Err(_) => return rustls::crypto::CryptoProvider::get_default() + .cloned() + .expect("provider was just installed by another thread"), + } }Also applies to: 21-21
31-48: Consider returningResultinstead of panicking.Panicking on missing hash provider (line 47) is abrupt for library code. Callers cannot gracefully handle this failure. A
Result<hash::Output, CryptoError>return type would allow callers to decide how to handle the error.That said, if the provider is guaranteed to include SHA-256 suites when properly configured (which is typical for both
aws-lc-rsandringdefaults), the panic may be acceptable as an "unreachable" safeguard. If you keep the panic, consider usingunreachable!with a more descriptive message to clarify this is a configuration bug.🔎 Alternative: use unreachable! for clarity
- panic!("SHA-256 hash provider not found"); + unreachable!("SHA-256 hash provider not found; ensure the crypto provider includes TLS 1.3 cipher suites with SHA-256")
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rs/moq-native/src/crypto.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
In Rust crates, tests should be integrated within source files using inline test modules
Files:
rs/moq-native/src/crypto.rs
🧬 Code graph analysis (1)
rs/moq-native/src/crypto.rs (2)
rs/moq-native/src/server.rs (2)
new(89-142)new(324-332)rs/moq-native/src/client.rs (2)
new(73-139)new(254-256)
|
Fixed in #804 |
2 participants
FIX for:
thread 'tokio-runtime-worker' (8) panicked at /root/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/rustls-0.23.35/src/crypto/mod.rs:249:14:
Could not automatically determine the process-level CryptoProvider from Rustls crate features.
Call CryptoProvider::install_default() before this point to select a provider manually, or make sure exactly one of the 'aws-lc-rs' and 'ring' features is enabled.