Variable length connection IDs

fuzz/fuzz_targets/packet.rs

@@ -5,15 +5,23 @@ extern crate proto;
 use libfuzzer_sys::fuzz_target;
 use proto::{
     fuzzing::{PacketParams, PartialDecode},
-    FixedLengthConnectionIdParser, DEFAULT_SUPPORTED_VERSIONS,
+    ConnectionIdParser, RandomConnectionIdGenerator, ZeroLengthConnectionIdParser,
+    DEFAULT_SUPPORTED_VERSIONS,
 };
 
 fuzz_target!(|data: PacketParams| {
     let len = data.buf.len();
     let supported_versions = DEFAULT_SUPPORTED_VERSIONS.to_vec();
+    let cid_gen;
     if let Ok(decoded) = PartialDecode::new(
         data.buf,
-        &FixedLengthConnectionIdParser::new(data.local_cid_len),
+        match data.local_cid_len {
+            0 => &ZeroLengthConnectionIdParser as &dyn ConnectionIdParser,
+            _ => {
+                cid_gen = RandomConnectionIdGenerator::new(data.local_cid_len);
+                &cid_gen as &dyn ConnectionIdParser
+            }
+        },
         &supported_versions,
         data.grease_quic_bit,
     ) {

quinn-proto/src/cid_generator.rs

@@ -1,30 +1,31 @@
 use std::{hash::Hasher, time::Duration};
 
+use bytes::Buf;
 use rand::{Rng, RngCore};
 
 use crate::shared::ConnectionId;
-use crate::MAX_CID_SIZE;
+use crate::{ConnectionIdParser, PacketDecodeError, MAX_CID_SIZE};
 
 /// Generates connection IDs for incoming connections
-pub trait ConnectionIdGenerator: Send + Sync {
+pub trait ConnectionIdGenerator: Send + Sync + ConnectionIdParser {
     /// Generates a new CID
     ///
     /// Connection IDs MUST NOT contain any information that can be used by
     /// an external observer (that is, one that does not cooperate with the
     /// issuer) to correlate them with other connection IDs for the same
     /// connection. They MUST have high entropy, e.g. due to encrypted data
     /// or cryptographic-grade random data.
-    fn generate_cid(&mut self) -> ConnectionId;
+    fn generate_cid(&self) -> ConnectionId;
 
     /// Quickly determine whether `cid` could have been generated by this generator
     ///
-    /// False positives are permitted, but increase the cost of handling invalid packets.
+    /// False positives are permitted, but increase the cost of handling invalid packets. The input
+    /// CID is guaranteed to have been obtained from a successful call to the generator's
+    /// implementation of [`ConnectionIdParser::parse`].
     fn validate(&self, _cid: &ConnectionId) -> Result<(), InvalidCid> {
         Ok(())
     }
 
-    /// Returns the length of a CID for connections created by this generator
-    fn cid_len(&self) -> usize;
     /// Returns the lifetime of generated Connection IDs
     ///
     /// Connection IDs will be retired after the returned `Duration`, if any. Assumed to be constant.
@@ -60,6 +61,10 @@ impl RandomConnectionIdGenerator {
     /// The given length must be less than or equal to MAX_CID_SIZE.
     pub fn new(cid_len: usize) -> Self {
         debug_assert!(cid_len <= MAX_CID_SIZE);
+        assert!(
+            cid_len > 0,
+            "connection ID generators must produce non-empty IDs"
+        );
         Self {
             cid_len,
             ..Self::default()
@@ -73,19 +78,22 @@ impl RandomConnectionIdGenerator {
     }
 }
 
+impl ConnectionIdParser for RandomConnectionIdGenerator {
+    fn parse(&self, buffer: &mut dyn Buf) -> Result<ConnectionId, PacketDecodeError> {
+        (buffer.remaining() >= self.cid_len)
+            .then(|| ConnectionId::from_buf(buffer, self.cid_len))
+            .ok_or(PacketDecodeError::InvalidHeader("packet too small"))
+    }
+}
+
 impl ConnectionIdGenerator for RandomConnectionIdGenerator {
-    fn generate_cid(&mut self) -> ConnectionId {
+    fn generate_cid(&self) -> ConnectionId {
         let mut bytes_arr = [0; MAX_CID_SIZE];
         rand::thread_rng().fill_bytes(&mut bytes_arr[..self.cid_len]);
 
         ConnectionId::new(&bytes_arr[..self.cid_len])
     }
 
-    /// Provide the length of dst_cid in short header packet
-    fn cid_len(&self) -> usize {
-        self.cid_len
-    }
-
     fn cid_lifetime(&self) -> Option<Duration> {
         self.lifetime
     }
@@ -131,9 +139,17 @@ impl Default for HashedConnectionIdGenerator {
     }
 }
 
+impl ConnectionIdParser for HashedConnectionIdGenerator {
+    fn parse(&self, buffer: &mut dyn Buf) -> Result<ConnectionId, PacketDecodeError> {
+        (buffer.remaining() >= HASHED_CID_LEN)
+            .then(|| ConnectionId::from_buf(buffer, HASHED_CID_LEN))
+            .ok_or(PacketDecodeError::InvalidHeader("packet too small"))
+    }
+}
+
 impl ConnectionIdGenerator for HashedConnectionIdGenerator {
-    fn generate_cid(&mut self) -> ConnectionId {
-        let mut bytes_arr = [0; NONCE_LEN + SIGNATURE_LEN];
+    fn generate_cid(&self) -> ConnectionId {
+        let mut bytes_arr = [0; HASHED_CID_LEN];
         rand::thread_rng().fill_bytes(&mut bytes_arr[..NONCE_LEN]);
         let mut hasher = rustc_hash::FxHasher::default();
         hasher.write_u64(self.key);
@@ -154,17 +170,39 @@ impl ConnectionIdGenerator for HashedConnectionIdGenerator {
         }
     }
 
-    fn cid_len(&self) -> usize {
-        NONCE_LEN + SIGNATURE_LEN
-    }
-
     fn cid_lifetime(&self) -> Option<Duration> {
         self.lifetime
     }
 }
 
 const NONCE_LEN: usize = 3; // Good for more than 16 million connections
 const SIGNATURE_LEN: usize = 8 - NONCE_LEN; // 8-byte total CID length
+const HASHED_CID_LEN: usize = NONCE_LEN + SIGNATURE_LEN;
+
+/// HACK: Replace uses with `ZeroLengthConnectionIdParser` once [trait upcasting] is stable
+///
+/// CID generators should produce nonempty CIDs. We should be able to use
+/// `ZeroLengthConnectionIdParser` everywhere this would be needed, but that will require
+/// construction of `&dyn ConnectionIdParser` from `&dyn ConnectionIdGenerator`.
+///
+/// [trait upcasting]: https://github.com/rust-lang/rust/issues/65991
+pub(crate) struct ZeroLengthConnectionIdGenerator;
+
+impl ConnectionIdParser for ZeroLengthConnectionIdGenerator {
+    fn parse(&self, _: &mut dyn Buf) -> Result<ConnectionId, PacketDecodeError> {
+        Ok(ConnectionId::new(&[]))
+    }
+}
+
+impl ConnectionIdGenerator for ZeroLengthConnectionIdGenerator {
+    fn generate_cid(&self) -> ConnectionId {
+        unreachable!()
+    }
+
+    fn cid_lifetime(&self) -> Option<Duration> {
+        None
+    }
+}
 
 #[cfg(test)]
 mod tests {
@@ -173,7 +211,7 @@ mod tests {
     #[test]
     #[cfg(feature = "ring")]
     fn validate_keyed_cid() {
-        let mut generator = HashedConnectionIdGenerator::new();
+        let generator = HashedConnectionIdGenerator::new();
         let cid = generator.generate_cid();
         generator.validate(&cid).unwrap();
     }

quinn-proto/src/config.rs

@@ -616,11 +616,7 @@ impl Default for MtuDiscoveryConfig {
 pub struct EndpointConfig {
     pub(crate) reset_key: Arc<dyn HmacKey>,
     pub(crate) max_udp_payload_size: VarInt,
-    /// CID generator factory
-    ///
-    /// Create a cid generator for local cid in Endpoint struct
-    pub(crate) connection_id_generator_factory:
-        Arc<dyn Fn() -> Box<dyn ConnectionIdGenerator> + Send + Sync>,
+    pub(crate) connection_id_generator: Option<Arc<dyn ConnectionIdGenerator>>,
     pub(crate) supported_versions: Vec<u32>,
     pub(crate) grease_quic_bit: bool,
     /// Minimum interval between outgoing stateless reset packets
@@ -630,12 +626,10 @@ pub struct EndpointConfig {
 impl EndpointConfig {
     /// Create a default config with a particular `reset_key`
     pub fn new(reset_key: Arc<dyn HmacKey>) -> Self {
-        let cid_factory =
-            || -> Box<dyn ConnectionIdGenerator> { Box::<HashedConnectionIdGenerator>::default() };
         Self {
             reset_key,
             max_udp_payload_size: (1500u32 - 28).into(), // Ethernet MTU minus IP + UDP headers
-            connection_id_generator_factory: Arc::new(cid_factory),
+            connection_id_generator: Some(Arc::<HashedConnectionIdGenerator>::default()),
             supported_versions: DEFAULT_SUPPORTED_VERSIONS.to_vec(),
             grease_quic_bit: true,
             min_reset_interval: Duration::from_millis(20),
@@ -650,11 +644,11 @@ impl EndpointConfig {
     /// information in local connection IDs, e.g. to support stateless packet-level load balancers.
     ///
     /// Defaults to [`HashedConnectionIdGenerator`].
-    pub fn cid_generator<F: Fn() -> Box<dyn ConnectionIdGenerator> + Send + Sync + 'static>(
+    pub fn cid_generator(
         &mut self,
-        factory: F,
+        generator: Option<Arc<dyn ConnectionIdGenerator>>,
     ) -> &mut Self {
-        self.connection_id_generator_factory = Arc::new(factory);
+        self.connection_id_generator = generator;
         self
     }
 

quinn-proto/src/connection/cid_state.rs

@@ -21,19 +21,12 @@ pub(super) struct CidState {
     prev_retire_seq: u64,
     /// Sequence number to set in retire_prior_to field in NEW_CONNECTION_ID frame
     retire_seq: u64,
-    /// cid length used to decode short packet
-    cid_len: usize,
     //// cid lifetime
     cid_lifetime: Option<Duration>,
 }
 
 impl CidState {
-    pub(crate) fn new(
-        cid_len: usize,
-        cid_lifetime: Option<Duration>,
-        now: Instant,
-        issued: u64,
-    ) -> Self {
+    pub(crate) fn new(cid_lifetime: Option<Duration>, now: Instant, issued: u64) -> Self {
         let mut active_seq = FxHashSet::default();
         // Add sequence number of CIDs used in handshaking into tracking set
         for seq in 0..issued {
@@ -45,7 +38,6 @@ impl CidState {
             active_seq,
             prev_retire_seq: 0,
             retire_seq: 0,
-            cid_len,
             cid_lifetime,
         };
         // Track lifetime of CIDs used in handshaking
@@ -158,11 +150,6 @@ impl CidState {
         sequence: u64,
         limit: u64,
     ) -> Result<bool, TransportError> {
-        if self.cid_len == 0 {
-            return Err(TransportError::PROTOCOL_VIOLATION(
-                "RETIRE_CONNECTION_ID when CIDs aren't in use",
-            ));
-        }
         if sequence > self.issued {
             debug!(
                 sequence,
@@ -181,11 +168,6 @@ impl CidState {
         Ok(limit > self.active_seq.len() as u64)
     }
 
-    /// Length of local Connection IDs
-    pub(crate) fn cid_len(&self) -> usize {
-        self.cid_len
-    }
-
     /// The value for `retire_prior_to` field in `NEW_CONNECTION_ID` frame
     pub(crate) fn retire_prior_to(&self) -> u64 {
         self.retire_seq