From dd34d57a0a184ae23707d5ae045ec62d8f0b6ecb Mon Sep 17 00:00:00 2001 From: Michael Chen Date: Mon, 23 Oct 2023 05:46:15 -0400 Subject: [PATCH] Make `quinn-proto::{Connection, Endpoint}` deterministic (#1691) `quinn-proto::{Connection, Endpoint}` structs call `StdRng::from_entropy()` in their implementation. It makes it hard to reproduce errors in an otherwise deterministic test case. This PR addresses the issue by adding a `StdRng` argument to the respective `new` functions. For other functions that may create an endpoint/connection, an argument of type `impl FnOnce() -> StdRng` is added. This avoids eager calls to generate fresh entropy. e.g. the `Endpoint::handle` function is frequently called but we'd only need the rng when handling a new incoming connection. --- quinn-proto/src/connection/mod.rs | 3 ++- quinn-proto/src/endpoint.rs | 6 +++++- quinn-proto/src/tests/mod.rs | 27 +++++++++++++++++++++------ quinn-proto/src/tests/util.rs | 9 +++++++-- quinn/src/endpoint.rs | 7 ++++++- 5 files changed, 41 insertions(+), 11 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index e0c0c37da..0dd56f1c5 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -252,6 +252,7 @@ impl Connection { now: Instant, version: u32, allow_mtud: bool, + rng_seed: [u8; 32], ) -> Self { let side = if server_config.is_some() { Side::Server @@ -267,7 +268,7 @@ impl Connection { expected_token: Bytes::new(), client_hello: None, }); - let mut rng = StdRng::from_entropy(); + let mut rng = StdRng::from_seed(rng_seed); let path_validated = server_config.as_ref().map_or(true, |c| c.use_retry); let mut this = Self { endpoint_config, diff --git a/quinn-proto/src/endpoint.rs b/quinn-proto/src/endpoint.rs index 3e0719369..2cc0677b3 100644 --- a/quinn-proto/src/endpoint.rs +++ b/quinn-proto/src/endpoint.rs @@ -57,9 +57,10 @@ impl Endpoint { config: Arc, server_config: Option>, allow_mtud: bool, + rng_seed: Option<[u8; 32]>, ) -> Self { Self { - rng: StdRng::from_entropy(), + rng: rng_seed.map_or(StdRng::from_entropy(), StdRng::from_seed), index: ConnectionIndex::default(), connections: Slab::new(), local_cid_generator: (config.connection_id_generator_factory.as_ref())(), @@ -589,6 +590,8 @@ impl Endpoint { server_config: Option>, transport_config: Arc, ) -> Connection { + let mut rng_seed = [0; 32]; + self.rng.fill_bytes(&mut rng_seed); let conn = Connection::new( self.config.clone(), server_config, @@ -603,6 +606,7 @@ impl Endpoint { now, version, self.allow_mtud, + rng_seed, ); let id = self.connections.insert(ConnectionMeta { diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs index f1bfda432..583466331 100644 --- a/quinn-proto/src/tests/mod.rs +++ b/quinn-proto/src/tests/mod.rs @@ -27,7 +27,12 @@ use util::*; fn version_negotiate_server() { let _guard = subscribe(); let client_addr = "[::2]:7890".parse().unwrap(); - let mut server = Endpoint::new(Default::default(), Some(Arc::new(server_config())), true); + let mut server = Endpoint::new( + Default::default(), + Some(Arc::new(server_config())), + true, + None, + ); let now = Instant::now(); let event = server.handle( now, @@ -63,6 +68,7 @@ fn version_negotiate_client() { }), None, true, + None, ); let (_, mut client_ch) = client .connect(Instant::now(), client_config(), server_addr, "localhost") @@ -178,7 +184,8 @@ fn server_stateless_reset() { let mut pair = Pair::new(endpoint_config.clone(), server_config()); let (client_ch, _) = pair.connect(); pair.drive(); // Flush any post-handshake frames - pair.server.endpoint = Endpoint::new(endpoint_config, Some(Arc::new(server_config())), true); + pair.server.endpoint = + Endpoint::new(endpoint_config, Some(Arc::new(server_config())), true, None); // Force the server to generate the smallest possible stateless reset pair.client.connections.get_mut(&client_ch).unwrap().ping(); info!("resetting"); @@ -203,7 +210,8 @@ fn client_stateless_reset() { let mut pair = Pair::new(endpoint_config.clone(), server_config()); let (_, server_ch) = pair.connect(); - pair.client.endpoint = Endpoint::new(endpoint_config, Some(Arc::new(server_config())), true); + pair.client.endpoint = + Endpoint::new(endpoint_config, Some(Arc::new(server_config())), true, None); // Send something big enough to allow room for a smaller stateless reset. pair.server.connections.get_mut(&server_ch).unwrap().close( pair.time, @@ -1343,8 +1351,9 @@ fn cid_rotation() { }), Some(Arc::new(server_config())), true, + None, ); - let client = Endpoint::new(Arc::new(EndpointConfig::default()), None, true); + let client = Endpoint::new(Arc::new(EndpointConfig::default()), None, true, None); let mut pair = Pair::new_from_endpoint(client, server); let (_, server_ch) = pair.connect(); @@ -1922,7 +1931,12 @@ fn big_cert_and_key() -> (rustls::Certificate, rustls::PrivateKey) { fn malformed_token_len() { let _guard = subscribe(); let client_addr = "[::2]:7890".parse().unwrap(); - let mut server = Endpoint::new(Default::default(), Some(Arc::new(server_config())), true); + let mut server = Endpoint::new( + Default::default(), + Some(Arc::new(server_config())), + true, + None, + ); server.handle( Instant::now(), client_addr, @@ -2024,12 +2038,13 @@ fn migrate_detects_new_mtu_and_respects_original_peer_max_udp_payload_size() { Arc::new(server_endpoint_config), Some(Arc::new(server_config())), true, + None, ); let client_endpoint_config = EndpointConfig { max_udp_payload_size: VarInt::from(client_max_udp_payload_size), ..EndpointConfig::default() }; - let client = Endpoint::new(Arc::new(client_endpoint_config), None, true); + let client = Endpoint::new(Arc::new(client_endpoint_config), None, true, None); let mut pair = Pair::new_from_endpoint(client, server); pair.mtu = 1300; diff --git a/quinn-proto/src/tests/util.rs b/quinn-proto/src/tests/util.rs index 7141d133c..688b2794a 100644 --- a/quinn-proto/src/tests/util.rs +++ b/quinn-proto/src/tests/util.rs @@ -49,8 +49,13 @@ impl Pair { } pub(super) fn new(endpoint_config: Arc, server_config: ServerConfig) -> Self { - let server = Endpoint::new(endpoint_config.clone(), Some(Arc::new(server_config)), true); - let client = Endpoint::new(endpoint_config, None, true); + let server = Endpoint::new( + endpoint_config.clone(), + Some(Arc::new(server_config)), + true, + None, + ); + let client = Endpoint::new(endpoint_config, None, true, None); Self::new_from_endpoint(client, server) } diff --git a/quinn/src/endpoint.rs b/quinn/src/endpoint.rs index 429afbb9a..38dee2a9c 100644 --- a/quinn/src/endpoint.rs +++ b/quinn/src/endpoint.rs @@ -110,7 +110,12 @@ impl Endpoint { let allow_mtud = !socket.may_fragment(); let rc = EndpointRef::new( socket, - proto::Endpoint::new(Arc::new(config), server_config.map(Arc::new), allow_mtud), + proto::Endpoint::new( + Arc::new(config), + server_config.map(Arc::new), + allow_mtud, + None, + ), addr.is_ipv6(), runtime.clone(), );