From 041efc6d95609b998b47561dcfe66c926ea59096 Mon Sep 17 00:00:00 2001 From: Frando Date: Wed, 23 Apr 2025 22:53:44 +0200 Subject: [PATCH 1/2] feat: racenonce for deduplicating connection attempts --- quinn-proto/src/config/mod.rs | 12 +++++ quinn-proto/src/connection/mod.rs | 6 ++- quinn-proto/src/endpoint.rs | 31 +++++++++++- quinn-proto/src/tests/mod.rs | 49 +++++++++++++++++++ quinn-proto/src/transport_parameters.rs | 30 +++++++++++- quinn/src/tests.rs | 65 ++++++++++++++++++++++++- 6 files changed, 188 insertions(+), 5 deletions(-) diff --git a/quinn-proto/src/config/mod.rs b/quinn-proto/src/config/mod.rs index 8d9364a6c..152508d0f 100644 --- a/quinn-proto/src/config/mod.rs +++ b/quinn-proto/src/config/mod.rs @@ -525,6 +525,9 @@ impl fmt::Debug for ValidationTokenConfig { } } +/// Racenonce +pub(crate) type Racenonce = [u8; 32]; + /// Configuration for outgoing connections /// /// Default values should be suitable for most internet applications. @@ -545,6 +548,8 @@ pub struct ClientConfig { /// QUIC protocol version to use pub(crate) version: u32, + + pub(crate) racenonce: Option, } impl ClientConfig { @@ -558,6 +563,7 @@ impl ClientConfig { RandomConnectionIdGenerator::new(MAX_CID_SIZE).generate_cid() }), version: 1, + racenonce: None, } } @@ -597,6 +603,12 @@ impl ClientConfig { self.version = version; self } + + /// Set the racenonce. + pub fn racenonce(&mut self, racenonce: [u8; 32]) -> &mut Self { + self.racenonce = Some(racenonce); + self + } } #[cfg(any(feature = "rustls-aws-lc-rs", feature = "rustls-ring"))] diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 0ba712fdc..67eb78d0a 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -21,7 +21,7 @@ use crate::{ cid_generator::ConnectionIdGenerator, cid_queue::CidQueue, coding::BufMutExt, - config::{ServerConfig, TransportConfig}, + config::{Racenonce, ServerConfig, TransportConfig}, congestion::Controller, crypto::{self, KeyPair, Keys, PacketKey}, frame::{self, Close, Datagram, FrameStruct, NewToken, ObservedAddr}, @@ -4278,6 +4278,10 @@ impl Connection { new_tokens.push(remote_addr); } } + + pub(crate) fn racenonce(&self) -> Option { + self.peer_params.racenonce + } } impl fmt::Debug for Connection { diff --git a/quinn-proto/src/endpoint.rs b/quinn-proto/src/endpoint.rs index da6c7ee5a..6e701fa21 100644 --- a/quinn-proto/src/endpoint.rs +++ b/quinn-proto/src/endpoint.rs @@ -19,7 +19,7 @@ use crate::{ ResetToken, Side, Transmit, TransportConfig, TransportError, cid_generator::ConnectionIdGenerator, coding::BufMutExt, - config::{ClientConfig, EndpointConfig, ServerConfig}, + config::{ClientConfig, EndpointConfig, Racenonce, ServerConfig}, connection::{Connection, ConnectionError, SideArgs}, crypto::{self, Keys, UnsupportedVersion}, frame, @@ -356,6 +356,7 @@ impl Endpoint { &self.config, self.local_cid_generator.as_ref(), loc_cid, + config.racenonce, None, &mut self.rng, ); @@ -612,6 +613,7 @@ impl Endpoint { &self.config, self.local_cid_generator.as_ref(), loc_cid, + None, Some(&server_config), &mut self.rng, ); @@ -663,6 +665,31 @@ impl Endpoint { incoming.rest, ) { Ok(()) => { + if let Some(nonce) = conn.racenonce() { + if self + .connections + .iter() + .any(|(_idx, conn)| conn.racenonce == Some(nonce)) + { + debug!("handshake failed: duplicate racenonce"); + self.handle_event(ch, EndpointEvent(EndpointEventInner::Drained)); + let e = TransportError::CONNECTION_REFUSED("duplicate racenonce"); + let response = self.initial_close( + version, + incoming.addresses, + &incoming.crypto, + &src_cid, + e.clone(), + buf, + ); + return Err(AcceptError { + cause: e.into(), + response: Some(response), + }); + } else { + self.connections[ch].racenonce = Some(nonce); + } + } trace!(id = ch.0, icid = %dst_cid, "new connection"); for event in incoming_buffer.datagrams { @@ -853,6 +880,7 @@ impl Endpoint { addresses, side, reset_token: None, + racenonce: None, }); debug_assert_eq!(id, ch.0, "connection handle allocation out of sync"); @@ -1138,6 +1166,7 @@ pub(crate) struct ConnectionMeta { /// Reset token provided by the peer for the CID we're currently sending to, and the address /// being sent to reset_token: Option<(SocketAddr, ResetToken)>, + racenonce: Option, } /// Local connection IDs for a single path diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs index 05559b08c..6952a02eb 100644 --- a/quinn-proto/src/tests/mod.rs +++ b/quinn-proto/src/tests/mod.rs @@ -3569,3 +3569,52 @@ fn reject_short_idcid() { panic!("expected an initial close"); }; } + +#[test] +fn racenonce() { + let _guard = subscribe(); + let mut pair = Pair::default(); + + println!("connect1"); + // first connect with a racenonce: succeeds + { + let mut cfg = client_config(); + cfg.racenonce([1u8; 32]); + pair.connect_with(cfg); + } + + println!("connect2"); + // second connect with the same racenonce: fails + { + let mut cfg = client_config(); + cfg.racenonce([1u8; 32]); + + let client_ch = pair.begin_connect(cfg); + pair.drive(); + + assert_matches!( + pair.server.assert_accept_error(), + ConnectionError::TransportError(TransportError { + code, + reason, + .. + }) + if code == TransportErrorCode::CONNECTION_REFUSED && &reason == "duplicate racenonce" + ); + + let client_conn = pair.client_conn_mut(client_ch); + assert_matches!( + client_conn.poll(), + Some(Event::ConnectionLost { reason: ConnectionError::ConnectionClosed(err) }) if err.error_code == TransportErrorCode::CONNECTION_REFUSED && err.reason.as_ref() == b"duplicate racenonce" + ); + assert_matches!(client_conn.poll(), None); + } + + // third connect with a different racenonce: succeeds + println!("connect3"); + { + let mut cfg = client_config(); + cfg.racenonce([2u8; 32]); + pair.connect_with(cfg); + } +} diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index 9c1c5e2a9..3c98cfa43 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -21,7 +21,7 @@ use crate::{ cid_generator::ConnectionIdGenerator, cid_queue::CidQueue, coding::{BufExt, BufMutExt, UnexpectedEnd}, - config::{EndpointConfig, ServerConfig, TransportConfig}, + config::{EndpointConfig, Racenonce, ServerConfig, TransportConfig}, connection::PathId, shared::ConnectionId, }; @@ -118,6 +118,9 @@ macro_rules! make_struct { // Multipath extension pub(crate) initial_max_path_id: Option, + + // Racenonce + pub(crate) racenonce: Option, } // We deliberately don't implement the `Default` trait, since that would be public, and @@ -143,6 +146,7 @@ macro_rules! make_struct { write_order: None, address_discovery_role: address_discovery::Role::Disabled, initial_max_path_id: None, + racenonce: None } } } @@ -157,6 +161,7 @@ impl TransportParameters { endpoint_config: &EndpointConfig, cid_gen: &dyn ConnectionIdGenerator, initial_src_cid: ConnectionId, + racenonce: Option, server_config: Option<&ServerConfig>, rng: &mut impl RngCore, ) -> Self { @@ -193,6 +198,7 @@ impl TransportParameters { address_discovery_role: config.address_discovery_role, // TODO(@divma): TransportConfig or..? initial_max_path_id: config.initial_max_path_id.map(PathId::from), + racenonce, ..Self::default() } } @@ -409,6 +415,13 @@ impl TransportParameters { w.write(val); } } + TransportParameterId::Racenonce => { + if let Some(val) = self.racenonce { + w.write_var(id as u64); + w.write_var(val.len() as u64); + w.put_slice(&val); + } + } id => { macro_rules! write_params { {$($(#[$doc:meta])* $name:ident ($id:ident) = $default:expr,)*} => { @@ -535,6 +548,14 @@ impl TransportParameters { params.initial_max_path_id = Some(value); tracing::debug!(initial_max_path_id=%value, "multipath enabled"); } + TransportParameterId::Racenonce => { + if len != 32 || params.racenonce.is_some() { + return Err(Error::Malformed); + } + let mut val = [0; 32]; + r.copy_to_slice(&mut val); + params.racenonce = Some(val); + } _ => { macro_rules! parse { {$($(#[$doc:meta])* $name:ident ($id:ident) = $default:expr,)*} => { @@ -703,11 +724,13 @@ pub(crate) enum TransportParameterId { // https://datatracker.ietf.org/doc/html/draft-ietf-quic-multipath InitialMaxPathId = 0x0f739bbc1b666d0c, + + Racenonce = 0x0f138193fac, } impl TransportParameterId { /// Array with all supported transport parameter IDs - const SUPPORTED: [Self; 23] = [ + const SUPPORTED: [Self; 24] = [ Self::MaxIdleTimeout, Self::MaxUdpPayloadSize, Self::InitialMaxData, @@ -731,6 +754,7 @@ impl TransportParameterId { Self::MinAckDelayDraft07, Self::ObservedAddr, Self::InitialMaxPathId, + Self::Racenonce, ]; } @@ -772,6 +796,7 @@ impl TryFrom for TransportParameterId { id if Self::MinAckDelayDraft07 == id => Self::MinAckDelayDraft07, id if Self::ObservedAddr == id => Self::ObservedAddr, id if Self::InitialMaxPathId == id => Self::InitialMaxPathId, + id if Self::Racenonce == id => Self::Racenonce, _ => return Err(()), }; Ok(param) @@ -812,6 +837,7 @@ mod test { min_ack_delay: Some(2_000u32.into()), address_discovery_role: address_discovery::Role::SendOnly, initial_max_path_id: Some(PathId::MAX), + racenonce: Some([42u8; 32]), ..TransportParameters::default() }; params.write(&mut buf); diff --git a/quinn/src/tests.rs b/quinn/src/tests.rs index 834754f7a..e84ac045f 100755 --- a/quinn/src/tests.rs +++ b/quinn/src/tests.rs @@ -16,7 +16,10 @@ use std::{ use crate::runtime::TokioRuntime; use crate::{Duration, Instant}; use bytes::Bytes; -use proto::{RandomConnectionIdGenerator, crypto::rustls::QuicClientConfig}; +use proto::{ + ConnectionError, RandomConnectionIdGenerator, TransportErrorCode, + crypto::rustls::QuicClientConfig, +}; use rand::{RngCore, SeedableRng, rngs::StdRng}; use rustls::{ RootCertStore, @@ -310,6 +313,13 @@ impl EndpointFactory { endpoint } + + fn client_config(&self) -> ClientConfig { + let mut roots = rustls::RootCertStore::empty(); + roots.add(self.cert.cert.der().clone()).unwrap(); + + ClientConfig::with_root_certificates(Arc::new(roots)).unwrap() + } } #[tokio::test] @@ -892,3 +902,56 @@ async fn test_multipath_negotiated() { tokio::join!(server_task, client_task); } + +#[tokio::test] +async fn racenonce() { + let _guard = subscribe(); + let factory = EndpointFactory::new(); + let server = factory.endpoint("server"); + let server_addr = server.local_addr().unwrap(); + + let client = factory.endpoint("client1"); + + let client_task = async move { + let mut config = factory.client_config(); + config.racenonce([1u8; 32]); + let conn1 = client + .connect_with(config, server_addr, "localhost") + .unwrap() + .await + .unwrap(); + let mut config = factory.client_config(); + config.racenonce([1u8; 32]); + let conn2 = client + .connect_with(config, server_addr, "localhost") + .unwrap() + .await; + assert!(matches!(conn2, + Err(ConnectionError::ConnectionClosed(ref frame)) + if frame.error_code == TransportErrorCode::CONNECTION_REFUSED && frame.reason.as_ref() == b"duplicate racenonce" + )); + let mut config = factory.client_config(); + config.racenonce([2u8; 32]); + let conn3 = client + .connect_with(config, server_addr, "localhost") + .unwrap() + .await + .unwrap(); + drop(conn1); + drop(conn2); + drop(conn3); + }; + let server_task = async move { + let _client1 = server.accept().await.unwrap().await.unwrap(); + let incoming2 = server.accept().await.unwrap(); + let res = incoming2.accept(); + assert!(matches!( + res, + Err(ConnectionError::TransportError(err)) + if err.code == TransportErrorCode::CONNECTION_REFUSED && &err.reason == "duplicate racenonce" + )); + let _client1 = server.accept().await.unwrap().await.unwrap(); + } + .instrument(error_span!("server")); + tokio::join!(client_task, server_task); +} From 85e5daf650aa5598c9b6b43f89c32e7e0ee58cc8 Mon Sep 17 00:00:00 2001 From: Frando Date: Wed, 23 Apr 2025 23:02:50 +0200 Subject: [PATCH 2/2] chore: update sccache action --- .github/workflows/rust.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml index 8d3231847..d02f0574a 100644 --- a/.github/workflows/rust.yml +++ b/.github/workflows/rust.yml @@ -118,7 +118,7 @@ jobs: steps: - uses: actions/checkout@v4 - - uses: mozilla-actions/sccache-action@v0.0.4 + - uses: mozilla-actions/sccache-action@v0.0.9 - uses: dtolnay/rust-toolchain@master with: toolchain: ${{ matrix.rust }} @@ -181,7 +181,7 @@ jobs: SCCACHE_GHA_ENABLED: "on" steps: - uses: actions/checkout@v4 - - uses: mozilla-actions/sccache-action@v0.0.4 + - uses: mozilla-actions/sccache-action@v0.0.9 - uses: dtolnay/rust-toolchain@1.71.0 - uses: Swatinem/rust-cache@v2 - run: cargo check --locked --lib --all-features -p iroh-quinn-udp -p iroh-quinn-proto -p iroh-quinn @@ -193,7 +193,7 @@ jobs: SCCACHE_GHA_ENABLED: "on" steps: - uses: actions/checkout@v4 - - uses: mozilla-actions/sccache-action@v0.0.4 + - uses: mozilla-actions/sccache-action@v0.0.9 - uses: dtolnay/rust-toolchain@stable with: components: rustfmt, clippy