Add staged ClientHello acceptor for ServerConfig selection - #2671
Add staged ClientHello acceptor for ServerConfig selection#2671iadev09 wants to merge 29 commits into
Conversation
190980e to
738f9f7
Compare
738f9f7 to
7dbd839
Compare
| # Enable rustls with the `aws-lc-rs` crypto provider | ||
| rustls-aws-lc-rs = ["dep:rustls", "aws-lc-rs", "proto/rustls-aws-lc-rs", "proto/aws-lc-rs"] | ||
| rustls-aws-lc-rs-fips = ["dep:rustls", "aws-lc-rs-fips", "proto/rustls-aws-lc-rs-fips", "proto/aws-lc-rs-fips"] | ||
| rustls-aws-lc-rs = ["__rustls", "dep:rustls-aws-lc-rs", "aws-lc-rs", "proto/rustls-aws-lc-rs", "proto/aws-lc-rs"] |
There was a problem hiding this comment.
I think this feature refactoring can be done separately, earlier (suggest in a separate PR). rustls 0.24 requires that callers provide a crypto provider explicitly. Since rustls is a public dependency for Quinn anyway, I suggest that we drop the provider-specific features in favor of requiring an explicit provider, too.
@Ralith do you agree?
If so, I'd suggest that the explicit provider passing can also become a separate commit that can be merged ahead of this one. (May obviate the need for a __rustls feature?)
There was a problem hiding this comment.
I suggest that we drop the provider-specific features in favor of requiring an explicit provider, too.
Yep, that should be much more composable. We may still want to spend some effort having a reasonable default, but we should try to avoid needing to know about other providers.
There was a problem hiding this comment.
I guess the challenge here is that we still have the raw primitives in use for packet protection. Maybe the first objective is finishing main...ctz:quinn:jbp-retry-calculation-uses-rustls...
7dbd839 to
86ccb0f
Compare
6f479c7 to
e42adb1
Compare
|
So I think if you want to make progress on getting this merged, this needs to be done as a separate PR:
I think the upgrade to 0.24 should also be a separate PR -- not sure which of these two makes sense to merge first. |
|
I split the rustls 0.24 upgrade out into a separate branch first. #2701 After that, I can look at the retry packet protection cleanup as another small PR before rebasing the staged ClientHello acceptor work on top. |
7167076 to
4aa70ce
Compare
Rebase the port on current Quinn main, pin the latest rustls main and verifier upgrade, and adapt Quinn to rustls' split providers and ordered QUIC input/event APIs.
Use the selected rustls QUIC suite for Retry integrity protection instead of directly invoking Ring or AWS-LC AEAD primitives.
No behavior change. Inline the helper at its only remaining call site. A follow-up generalizes CID registration. Keeping all the logic in register_connection makes that easier.
No behavior change. new_cid already registered each CID at mint time, so re-inserting is harmless. The upcoming split-accept change routes minted CIDs to the in-flight Incoming; this loop is then what redirects them to the finalized connection.
No behavior change. insert_initial's only caller was accept(), which passed the dst_cid stored as the connection's init_cid. register_connection is also used by the client path (connect), so gate on side.is_server().
Every action inside is a removal, and the upcoming helpers it pairs with (remove_incoming_buffer, remove_accept_reservation) are named remove_*. Rename it to match.
A dedicated helper to return the removed buffer. The split-accept path will use it to recover buffered datagrams.
accept() removed the Incoming's buffer up front and left each early error path to remove the initial-CID route by hand. Defer the buffer removal until the checks have passed, so the error paths can release everything through the existing ignore() helper. Same state changes, one exit path.
No behavior change: inline add_connection's body into accept() (connect() still uses add_connection). Connection::new does not need the Endpoint itself, only a few values read from it (rng seed, CID length/lifetime, configs). This prepares for the split, which reads those values while it still has &mut Endpoint and calls Connection::new later without it.
Name the endpoint's idle condition instead of repeating recv_state.connections.is_empty() at each site. Split accept will extend this predicate with in-flight accepts.
Register the connection only after first-packet handling succeeds. Previously accept() inserted into the slab and index up front and undid a failure with a synthetic Drained event; now the failure path only frees the initial-CID route and the newly minted CIDs. This is required by the split: the slab entry can't exist while the handshake runs outside the endpoint lock.
No observable change while accept() is still monolithic. accept()'s CIDs now route to the pending Incoming's buffer slot rather than a connection handle that doesn't exist yet; the slot stays alive until the outcome is known, and the handle is allocated only at registration. register_connection then re-points the initial and minted CIDs at the connection, and the failure path releases the buffer slot along with the routes. Once accept is split, datagrams arriving mid-handshake will land in the reserved slot and replay after registration.
accept previously held the endpoint lock across TLS session setup, Connection::new, and handle_first_packet. Under a high rate of new connections that stalls the endpoint driver and cuts throughput for established traffic. Split accept into three private phases, composed by accept(): - start_accept reserves routing/CID state under the endpoint lock (&mut Endpoint) and packs accept's remaining locals into an Accepting handle. - Accepting::finish_without_endpoint runs TLS session creation, Connection::new, and first-packet handling, with no endpoint access. - finish_accept / finish_accept_error finalize or clean up back under the lock. On success: buffer recovery, handle allocation, register_connection, and buffered-datagram replay. On failure: initial_close response and route/buffer release. Both go through remove_accept_reservation. Reservation state (routing, CIDs, buffer slot) lives in an AcceptReservation carried inside Accepting/Accepted/AcceptingError, not in the connection slab. Initials and 0-RTT packets arriving during the split window route as RouteDatagramTo::Incoming into the same buffer slot used before start_accept.
Make the split-accept phases callable from the quinn crate: the methods become pub but doc(hidden), and the private __internal_split_accept feature re-exports the otherwise-unnameable state types (Accepting, Accepted, AcceptingError) so quinn can store and pass them around. Not a supported public API; not covered by semver.
Count reservations taken by start_accept and released by remove_accept_reservation, and expose the count as Endpoint::pending_accepts. A connection being accepted is not in the connection slab yet, so code that checks whether the endpoint is busy by counting connections (e.g. quinn's wait_idle) would miss it.
A dropped Accepting is covered by the Incoming it holds (whose improper-drop warner stays armed), but once finish_without_endpoint consumes that Incoming, dropping the resulting Accepted or AcceptingError leaked the reservation silently. Give both an AcceptDropGuard that warns if dropped and is dismissed by finish_accept / finish_accept_error. Releasing the reservation requires the endpoint, which a Drop impl can't reach, so — as with Incoming — the guard only warns.
Drive proto::Endpoint::accept through its three-phase hooks so TLS session creation, Connection::new, and first-packet handling run without the endpoint mutex held. is_idle (used by wait_idle and EndpointDriver termination) now also waits for pending_accepts to drain. A failed accept wakes idle waiters explicitly: it never became a connection, so no Drained event will fire to do it.
Cover retransmitted Initials buffered during the `Accepting` window and `max_incoming` counting attempts in the `Accepting` phase.
Cover the `finish_without_endpoint` failure path: when the off-lock handshake fails (here via ALPN mismatch) after `start_accept` has reserved endpoint state, `finish_accept_error` must release the pending-accept slot, the reserved CIDs, and the buffered packets. Assert the endpoint is left with no leaked state.
Block inside `ServerConfig::crypto.start_session` to hold a connection in the `Accepting` state. Verify that `open_connections` stays 0 and `wait_idle` does not complete while a pending accept is in flight, and that `Endpoint::close` during a pending accept resolves both sides cleanly.
Capture each incoming attempt's buffer limits when its slot is created. Retransmissions during split accept no longer consult the endpoint's current server configuration. Replacing or disabling that configuration therefore only affects new attempts.
Describe incoming limits across the full attempt lifetime, mark the pending-accept accessor as doc-hidden, and distinguish the production inter-crate feature from test-only features.
4aa70ce to
d55fd0d
Compare
Closes #2024.
Adds a staged ClientHello accept path so a server can inspect the rustls
ClientHellobefore finalizing the QuinnServerConfig.Commit Structure
1. rustls 0.24 upgrade path
Kept as one compatibility commit. This is where the temporary rustls git dependencies live, including the rustls-platform-verifier 0.8 branch.
Related rustls 0.24 PR outside Quinn: rustls/rustls-platform-verifier#233.
2. staged ClientHello acceptor primitive
3. selector helper
Adds a
ServerConfigSelectorhelper trait on top of the staged acceptor API.Tests
Bench
https://github.com/iadev09/quinn-acceptor-bench
In local 1000-sample smoke runs, the selected path stayed in the same range as continuing with the default
ServerConfig.Comparison
Compared with the TCP/rustls shape, this exposes the staged primitive directly.
For TCP, config selection usually needs an intermediate acceptor/future shape:
For Quinn, the proposed primitive lets the caller inspect the
ClientHelloand choose thequinn::ServerConfigdirectly.Because the
Endpointalready owns the Initial/CRYPTO handling, it does not need aLazyConfigAcceptor-style stream wrapper.Same scenario on TCP/rustls
LazyConfigAcceptor:https://github.com/iadev09/lazy-config-acceptor-bench