Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughPath-based TLS was replaced with direct certificate-chain and private-key handling using rustls_pki_types. Builder APIs for client and server replaced 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 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
🤖 Fix all issues with AI agents
In `@web-transport-quiche/src/ez/client.rs`:
- Around line 138-156: The code creates a dummy TlsCertificatePaths and
constructs a StaticCertHook with alpn: Vec::new() inside the match on self.tls,
but this behavior is undocumented and means the client advertises no ALPN during
TLS; add a concise comment next to the TlsCertificatePaths/StaticCertHook
construction explaining that the empty paths are intentional because
StaticCertHook intercepts loading (mirroring server.rs comment) and why ALPN is
empty by design, and then either (1) add a with_alpn(...) builder method on the
client builder to accept ALPN values and pass them into StaticCertHook::alpn
when setting self.tls, or (2) document in the same comment that the client
intentionally does not advertise ALPN (e.g., "h3 is implicit") if you choose not
to add configuration; update references to self.tls, StaticCertHook.alpn,
TlsCertificatePaths, and the client builder to implement the chosen option.
In `@web-transport-quiche/src/ez/driver.rs`:
- Around line 180-182: The code currently sets self.state.lock().alpn =
Some(qconn.application_proto().to_vec()) even when qconn.application_proto() is
an empty slice; change this to detect an empty ALPN and leave alpn as None in
that case: call qconn.application_proto(), if its length is zero set
self.state.lock().alpn = None, otherwise set it to Some(alpn_vec) (using the vec
from application_proto()) so callers using alpn().is_some() correctly reflect
whether ALPN was negotiated.
In `@web-transport-quiche/src/ez/tls.rs`:
- Around line 127-132: Replace the client-side call to ssl.set_alpn_protos(...)
with a server-side ALPN selection callback via ssl.set_alpn_select_cb(...):
implement a callback that receives the client's offered protocols (wire format
produced by alpn_wire_format/parsed input) and searches for the first matching
protocol in the server's configured list (self.alpn / configured protocols),
returning that selected protocol to the TLS stack or mapping failure to
SelectCertError::ERROR (or appropriate ALPN failure) when there is no overlap;
ensure the callback uses the same wire-format parsing logic as alpn_wire_format
and returns errors through the set_alpn_select_cb API instead of calling
set_alpn_protos.
🧹 Nitpick comments (5)
web-transport-quiche/src/ez/tls.rs (3)
51-80: Silent error swallowing makes TLS failures hard to diagnose.Every fallible step uses
.ok()?, so if the cert chain is malformed, the key doesn't match, or any BoringSSL call fails, the method returnsNonewith no logging or error context. The caller inbuild_with_hookwon't know why TLS setup failed.Consider logging or converting to a concrete error. At a minimum, a
tracing::warn!before returningNonewould help operators debug misconfigured certificates.♻️ Example: add minimal diagnostics
- let leaf = X509::from_der(self.chain.first()?.as_ref()).ok()?; - builder.set_certificate(&leaf).ok()?; + let leaf_der = self.chain.first()?; + let leaf = X509::from_der(leaf_der.as_ref()).inspect_err(|e| tracing::warn!("invalid leaf cert: {e}")).ok()?; + builder.set_certificate(&leaf).inspect_err(|e| tracing::warn!("set_certificate failed: {e}")).ok()?;(Apply similar treatment to the other
.ok()?calls.)
36-43:proto.len() as u8silently truncates protocols longer than 255 bytes.Per RFC 7301, ALPN protocol identifiers are 1–255 bytes, so this is fine in practice. But a debug assertion or a length check returning an error would guard against misuse.
23-34: Catch-all arm manufactures an error by callingprivate_key_from_der(&[]).This works but is fragile — the error message will be about a DER parsing failure, not about an unsupported key type. A brief comment explaining the intent ("
PrivateKeyDeris#[non_exhaustive]") would help future readers.♻️ Suggested comment
- _ => Err(PKey::<Private>::private_key_from_der(&[]).unwrap_err()), + // PrivateKeyDer is #[non_exhaustive]; produce an opaque error for unknown variants. + _ => Err(PKey::<Private>::private_key_from_der(&[]).unwrap_err()),web-transport-quiche/src/ez/server.rs (2)
180-186:unwrap()onlocal_addr()can be replaced with?since this function returnsio::Result.While a bound UDP socket should always have a local address, using
?is trivially safer and consistent with the error-returning signature ofbuild_with_hook.♻️ Proposed fix
let local_addrs: Vec<SocketAddr> = self .state .listeners .iter() - .map(|l| l.socket.local_addr().unwrap()) - .collect(); + .map(|l| l.socket.local_addr()) + .collect::<io::Result<Vec<_>>>()?;
165-196: The dummyTlsCertificatePathsworkaround is pragmatic but fragile.If
tokio-quicheever validates these paths (e.g., checks file existence at construction time), this will break. The comment at line 169 is helpful — consider also adding a note about why the hook API requirestls_certto be set (i.e., it's gated on that field being present).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web-transport-quiche/src/ez/tls.rs`:
- Around line 40-53: alpn_select can panic when parsing malformed ALPN wire data
because it slices without validating the length prefix; update the function
(alpn_select) to defensively validate that rest has at least one byte for the
length and that 1 + len does not exceed rest.len() before taking
&rest[1..1+len]; on invalid/truncated data, handle gracefully (e.g. break the
inner loop or return None) rather than slicing, so a malformed client ALPN blob
cannot trigger an out-of-bounds panic.
🧹 Nitpick comments (2)
web-transport-quiche/src/ez/tls.rs (2)
25-36: Wildcard arm synthesises an error by forcing a deliberate parse failure.Line 34 manufactures an error by calling
private_key_from_der(&[]).unwrap_err(). This works but produces a misleading error message ("bad DER" rather than "unsupported key type"). Consider returning a custom or more descriptive error, or usingErrorStack::get()/ a logged message so callers can diagnose unexpected key formats.
61-92:StaticCertHooksilently swallows every error viaok()?.Every fallible operation maps its error to
None, which means the caller (tokio-quiche) gets no indication of why context creation failed — bad DER, missing leaf, wrong key type, etc. For a "happy-path helper" this may be acceptable, but at minimum atracing::warn!before returningNonewould save debugging time.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@web-transport-quiche/src/ez/tls.rs`:
- Around line 106-111: Replace the incorrect AlpnError::NOACK return in the ALPN
selection callbacks with AlpnError::ALERT_FATAL: when self.alpn is non-empty and
you set the builder.set_alpn_select_callback (the closure that calls
alpn_select(alpn.as_slice(), client)), return Err(AlpnError::ALERT_FATAL)
instead of Err(AlpnError::NOACK) so a failed match aborts the handshake with a
no_application_protocol alert; make the same change for the second identical
ALPN callback occurrence.
🧹 Nitpick comments (2)
web-transport-quiche/src/ez/tls.rs (2)
76-83: Empty certificate chain silently returnsNonewithout a warning.If
self.chainis empty,self.chain.first()?returnsNoneand the function exits silently—unlike every other failure path which logs atracing::warn. A missing chain is a configuration error worth surfacing.♻️ Suggested fix
- let leaf = X509::from_der(self.chain.first()?.as_ref()) + let leaf_der = self.chain.first().or_else(|| { + tracing::warn!("certificate chain is empty"); + None + })?; + let leaf = X509::from_der(leaf_der.as_ref())
34-37: Replace fragile error synthesis withErrorStack::get().The current approach of calling
PKey::<Private>::private_key_from_der(&[]).unwrap_err()to synthesize an error is a code smell. Since no BoringSSL error is actually set in this code path, use the proper boring API instead:Suggested fix
_ => { tracing::warn!("unsupported private key format"); - Err(PKey::<Private>::private_key_from_der(&[]).unwrap_err()) + Err(boring::error::ErrorStack::get()) }
1 participant
Was the missing piece before I felt comfortable enough using the backend.