From ef6e6f41d7489bc2f12133548a005d1ed4a86148 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Fri, 14 Feb 2025 12:34:34 +0200 Subject: [PATCH 1/4] chore: Use `strum` macros (#2440) * chore: Use `strum` macros And remove some boilerplate code. Fixes #2439 * Fix * More * More * Suggestion from @mxinden * Fix --- Cargo.lock | 31 ++++++++ Cargo.toml | 1 + neqo-common/Cargo.toml | 1 + neqo-common/src/tos.rs | 65 ++-------------- neqo-crypto/Cargo.toml | 1 + neqo-crypto/src/auth.rs | 109 +++++++-------------------- neqo-crypto/src/constants.rs | 19 ++--- neqo-crypto/src/secrets.rs | 15 ++-- neqo-crypto/src/ssl.rs | 46 ++++------- neqo-transport/Cargo.toml | 1 + neqo-transport/src/connection/mod.rs | 15 ++-- neqo-transport/src/frame.rs | 41 +--------- neqo-transport/src/recovery/mod.rs | 11 +-- neqo-transport/src/tparams.rs | 30 +------- neqo-transport/src/tracking.rs | 18 +---- 15 files changed, 118 insertions(+), 286 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1e887d9b3b..ba9dfb9d8f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -844,6 +844,7 @@ dependencies = [ "neqo-crypto", "qlog", "regex", + "strum", "test-fixture", "windows", ] @@ -860,6 +861,7 @@ dependencies = [ "semver", "serde", "serde_derive", + "strum", "test-fixture", "toml", ] @@ -908,6 +910,7 @@ dependencies = [ "qlog", "smallvec", "static_assertions", + "strum", "test-fixture", ] @@ -1075,6 +1078,12 @@ version = "1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "08d43f7aa6b08d49f382cde6a7982047c3426db949b1424bc4b7ec9ae12c6ce2" +[[package]] +name = "rustversion" +version = "1.0.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f7c45b9784283f1b2e7fb61b42047c2fd678ef0960d4f6f1eba131594cc369d4" + [[package]] name = "ryu" version = "1.0.12" @@ -1204,6 +1213,28 @@ version = "0.11.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7da8b5736845d9f2fcb837ea5d9e2628564b3b043a70948a3f0b778838c5fb4f" +[[package]] +name = "strum" +version = "0.26.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8fec0f0aef304996cf250b31b5a10dee7980c85da9d759361292b8bca5a18f06" +dependencies = [ + "strum_macros", +] + +[[package]] +name = "strum_macros" +version = "0.26.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4c6bee85a5a24955dc440386795aa378cd9cf82acd5f764469152d2270e581be" +dependencies = [ + "heck", + "proc-macro2", + "quote", + "rustversion", + "syn", +] + [[package]] name = "syn" version = "2.0.87" diff --git a/Cargo.toml b/Cargo.toml index bb46298543..fece3f774c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -36,6 +36,7 @@ qlog = { version = "0.13", default-features = false } quinn-udp = { version = "0.5.6", default-features = false, features = ["direct-log", "fast-apple-datapath"] } regex = { version = "1.9", default-features = false, features = ["unicode-perl"] } static_assertions = { version = "1.1", default-features = false } +strum = { version = "0.26", default-features = false, features = ["derive"] } url = { version = "2.5.3", default-features = false, features = ["std"] } [workspace.lints.rust] diff --git a/neqo-common/Cargo.toml b/neqo-common/Cargo.toml index ee11980862..cb8cf72c31 100644 --- a/neqo-common/Cargo.toml +++ b/neqo-common/Cargo.toml @@ -22,6 +22,7 @@ env_logger = { version = "0.10", default-features = false } hex = { version = "0.4", default-features = false, features = ["alloc"], optional = true } log = { workspace = true } qlog = { workspace = true } +strum = { workspace = true } [target."cfg(windows)".dependencies] # Checked against https://searchfox.org/mozilla-central/source/Cargo.lock 2024-11-11 diff --git a/neqo-common/src/tos.rs b/neqo-common/src/tos.rs index ed6a5d2724..198193d11e 100644 --- a/neqo-common/src/tos.rs +++ b/neqo-common/src/tos.rs @@ -7,23 +7,21 @@ use std::fmt::Debug; use enum_map::Enum; +use strum::FromRepr; /// ECN (Explicit Congestion Notification) codepoints mapped to the /// lower 2 bits of the TOS field. /// -#[derive(Copy, Clone, PartialEq, Eq, Enum, Default, Debug)] +#[derive(Copy, Clone, PartialEq, Eq, Enum, Default, Debug, FromRepr)] #[repr(u8)] pub enum IpTosEcn { #[default] /// Not-ECT, Not ECN-Capable Transport, RFC3168 NotEct = 0b00, - /// ECT(1), ECN-Capable Transport(1), RFC8311 and RFC9331 Ect1 = 0b01, - /// ECT(0), ECN-Capable Transport(0), RFC3168 Ect0 = 0b10, - /// CE, Congestion Experienced, RFC3168 Ce = 0b11, } @@ -36,13 +34,7 @@ impl From for u8 { impl From for IpTosEcn { fn from(v: u8) -> Self { - match v & 0b0000_0011 { - 0b00 => Self::NotEct, - 0b01 => Self::Ect1, - 0b10 => Self::Ect0, - 0b11 => Self::Ce, - _ => unreachable!(), - } + Self::from_repr(v & 0b0000_0011).expect("all ECN values are covered") } } @@ -64,76 +56,54 @@ impl IpTosEcn { /// Diffserv codepoints, mapped to the upper six bits of the TOS field. /// -#[derive(Copy, Clone, PartialEq, Eq, Enum, Default, Debug)] +#[derive(Copy, Clone, PartialEq, Eq, Enum, Default, Debug, FromRepr)] #[repr(u8)] pub enum IpTosDscp { #[default] /// Class Selector 0, RFC2474 Cs0 = 0b0000_0000, - /// Class Selector 1, RFC2474 Cs1 = 0b0010_0000, - /// Class Selector 2, RFC2474 Cs2 = 0b0100_0000, - /// Class Selector 3, RFC2474 Cs3 = 0b0110_0000, - /// Class Selector 4, RFC2474 Cs4 = 0b1000_0000, - /// Class Selector 5, RFC2474 Cs5 = 0b1010_0000, - /// Class Selector 6, RFC2474 Cs6 = 0b1100_0000, - /// Class Selector 7, RFC2474 Cs7 = 0b1110_0000, - /// Assured Forwarding 11, RFC2597 Af11 = 0b0010_1000, - /// Assured Forwarding 12, RFC2597 Af12 = 0b0011_0000, - /// Assured Forwarding 13, RFC2597 Af13 = 0b0011_1000, - /// Assured Forwarding 21, RFC2597 Af21 = 0b0100_1000, - /// Assured Forwarding 22, RFC2597 Af22 = 0b0101_0000, - /// Assured Forwarding 23, RFC2597 Af23 = 0b0101_1000, - /// Assured Forwarding 31, RFC2597 Af31 = 0b0110_1000, - /// Assured Forwarding 32, RFC2597 Af32 = 0b0111_0000, - /// Assured Forwarding 33, RFC2597 Af33 = 0b0111_1000, - /// Assured Forwarding 41, RFC2597 Af41 = 0b1000_1000, - /// Assured Forwarding 42, RFC2597 Af42 = 0b1001_0000, - /// Assured Forwarding 43, RFC2597 Af43 = 0b1001_1000, - /// Expedited Forwarding, RFC3246 Ef = 0b1011_1000, - /// Capacity-Admitted Traffic, RFC5865 VoiceAdmit = 0b1011_0000, - /// Lower-Effort, RFC8622 Le = 0b0000_0100, } @@ -146,32 +116,7 @@ impl From for u8 { impl From for IpTosDscp { fn from(v: u8) -> Self { - match v & 0b1111_1100 { - 0b0000_0000 => Self::Cs0, - 0b0010_0000 => Self::Cs1, - 0b0100_0000 => Self::Cs2, - 0b0110_0000 => Self::Cs3, - 0b1000_0000 => Self::Cs4, - 0b1010_0000 => Self::Cs5, - 0b1100_0000 => Self::Cs6, - 0b1110_0000 => Self::Cs7, - 0b0010_1000 => Self::Af11, - 0b0011_0000 => Self::Af12, - 0b0011_1000 => Self::Af13, - 0b0100_1000 => Self::Af21, - 0b0101_0000 => Self::Af22, - 0b0101_1000 => Self::Af23, - 0b0110_1000 => Self::Af31, - 0b0111_0000 => Self::Af32, - 0b0111_1000 => Self::Af33, - 0b1000_1000 => Self::Af41, - 0b1001_0000 => Self::Af42, - 0b1001_1000 => Self::Af43, - 0b1011_1000 => Self::Ef, - 0b1011_0000 => Self::VoiceAdmit, - 0b0000_0100 => Self::Le, - _ => unreachable!(), - } + Self::from_repr(v & 0b1111_1100).expect("all DCSP values are covered") } } diff --git a/neqo-crypto/Cargo.toml b/neqo-crypto/Cargo.toml index 86f6ee6e87..3ceab32c20 100644 --- a/neqo-crypto/Cargo.toml +++ b/neqo-crypto/Cargo.toml @@ -20,6 +20,7 @@ workspace = true enum-map = { workspace = true } log = { workspace = true } neqo-common = { path = "../neqo-common" } +strum = { workspace = true} [build-dependencies] # Checked against https://searchfox.org/mozilla-central/source/Cargo.lock 2024-11-11 diff --git a/neqo-crypto/src/auth.rs b/neqo-crypto/src/auth.rs index 2932cdf2eb..a697959897 100644 --- a/neqo-crypto/src/auth.rs +++ b/neqo-crypto/src/auth.rs @@ -4,72 +4,42 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. +use strum::FromRepr; + use crate::err::{mozpkix, sec, ssl, PRErrorCode}; /// The outcome of authentication. -#[derive(Clone, Copy, Debug, PartialEq, Eq)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, FromRepr)] +#[repr(i32)] pub enum AuthenticationStatus { Ok, - CaInvalid, - CaNotV3, - CertAlgorithmDisabled, - CertExpired, - CertInvalidTime, - CertIsCa, - CertKeyUsage, - CertMitm, - CertNotYetValid, - CertRevoked, - CertSelfSigned, - CertSubjectInvalid, - CertUntrusted, - CertWeakKey, - IssuerEmptyName, - IssuerExpired, - IssuerNotYetValid, - IssuerUnknown, - IssuerUntrusted, - PolicyRejection, - Unknown, + CaInvalid = sec::SEC_ERROR_CA_CERT_INVALID, + CaNotV3 = mozpkix::MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA, + CertAlgorithmDisabled = sec::SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED, + CertExpired = sec::SEC_ERROR_EXPIRED_CERTIFICATE, + CertInvalidTime = sec::SEC_ERROR_INVALID_TIME, + CertIsCa = mozpkix::MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY, + CertKeyUsage = sec::SEC_ERROR_INADEQUATE_KEY_USAGE, + CertMitm = mozpkix::MOZILLA_PKIX_ERROR_MITM_DETECTED, + CertNotYetValid = mozpkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE, + CertRevoked = sec::SEC_ERROR_REVOKED_CERTIFICATE, + CertSelfSigned = mozpkix::MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT, + CertSubjectInvalid = ssl::SSL_ERROR_BAD_CERT_DOMAIN, + CertUntrusted = sec::SEC_ERROR_UNTRUSTED_CERT, + CertWeakKey = mozpkix::MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE, + IssuerEmptyName = mozpkix::MOZILLA_PKIX_ERROR_EMPTY_ISSUER_NAME, + IssuerExpired = sec::SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE, + IssuerNotYetValid = mozpkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE, + IssuerUnknown = sec::SEC_ERROR_UNKNOWN_ISSUER, + IssuerUntrusted = sec::SEC_ERROR_UNTRUSTED_ISSUER, + PolicyRejection = mozpkix::MOZILLA_PKIX_ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED, + Unknown = sec::SEC_ERROR_LIBRARY_FAILURE, } impl From for PRErrorCode { #[must_use] fn from(v: AuthenticationStatus) -> Self { - match v { - AuthenticationStatus::Ok => 0, - AuthenticationStatus::CaInvalid => sec::SEC_ERROR_CA_CERT_INVALID, - AuthenticationStatus::CaNotV3 => mozpkix::MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA, - AuthenticationStatus::CertAlgorithmDisabled => { - sec::SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED - } - AuthenticationStatus::CertExpired => sec::SEC_ERROR_EXPIRED_CERTIFICATE, - AuthenticationStatus::CertInvalidTime => sec::SEC_ERROR_INVALID_TIME, - AuthenticationStatus::CertIsCa => { - mozpkix::MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY - } - AuthenticationStatus::CertKeyUsage => sec::SEC_ERROR_INADEQUATE_KEY_USAGE, - AuthenticationStatus::CertMitm => mozpkix::MOZILLA_PKIX_ERROR_MITM_DETECTED, - AuthenticationStatus::CertNotYetValid => { - mozpkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE - } - AuthenticationStatus::CertRevoked => sec::SEC_ERROR_REVOKED_CERTIFICATE, - AuthenticationStatus::CertSelfSigned => mozpkix::MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT, - AuthenticationStatus::CertSubjectInvalid => ssl::SSL_ERROR_BAD_CERT_DOMAIN, - AuthenticationStatus::CertUntrusted => sec::SEC_ERROR_UNTRUSTED_CERT, - AuthenticationStatus::CertWeakKey => mozpkix::MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE, - AuthenticationStatus::IssuerEmptyName => mozpkix::MOZILLA_PKIX_ERROR_EMPTY_ISSUER_NAME, - AuthenticationStatus::IssuerExpired => sec::SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE, - AuthenticationStatus::IssuerNotYetValid => { - mozpkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE - } - AuthenticationStatus::IssuerUnknown => sec::SEC_ERROR_UNKNOWN_ISSUER, - AuthenticationStatus::IssuerUntrusted => sec::SEC_ERROR_UNTRUSTED_ISSUER, - AuthenticationStatus::PolicyRejection => { - mozpkix::MOZILLA_PKIX_ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED - } - AuthenticationStatus::Unknown => sec::SEC_ERROR_LIBRARY_FAILURE, - } + v as Self } } @@ -78,31 +48,6 @@ impl From for PRErrorCode { impl From for AuthenticationStatus { #[must_use] fn from(v: PRErrorCode) -> Self { - match v { - 0 => Self::Ok, - sec::SEC_ERROR_CA_CERT_INVALID => Self::CaInvalid, - mozpkix::MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA => Self::CaNotV3, - sec::SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED => Self::CertAlgorithmDisabled, - sec::SEC_ERROR_EXPIRED_CERTIFICATE => Self::CertExpired, - sec::SEC_ERROR_INVALID_TIME => Self::CertInvalidTime, - mozpkix::MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY => Self::CertIsCa, - sec::SEC_ERROR_INADEQUATE_KEY_USAGE => Self::CertKeyUsage, - mozpkix::MOZILLA_PKIX_ERROR_MITM_DETECTED => Self::CertMitm, - mozpkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE => Self::CertNotYetValid, - sec::SEC_ERROR_REVOKED_CERTIFICATE => Self::CertRevoked, - mozpkix::MOZILLA_PKIX_ERROR_SELF_SIGNED_CERT => Self::CertSelfSigned, - ssl::SSL_ERROR_BAD_CERT_DOMAIN => Self::CertSubjectInvalid, - sec::SEC_ERROR_UNTRUSTED_CERT => Self::CertUntrusted, - mozpkix::MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE => Self::CertWeakKey, - mozpkix::MOZILLA_PKIX_ERROR_EMPTY_ISSUER_NAME => Self::IssuerEmptyName, - sec::SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE => Self::IssuerExpired, - mozpkix::MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE => Self::IssuerNotYetValid, - sec::SEC_ERROR_UNKNOWN_ISSUER => Self::IssuerUnknown, - sec::SEC_ERROR_UNTRUSTED_ISSUER => Self::IssuerUntrusted, - mozpkix::MOZILLA_PKIX_ERROR_ADDITIONAL_POLICY_CONSTRAINT_FAILED => { - Self::PolicyRejection - } - _ => Self::Unknown, - } + Self::from_repr(v).unwrap_or(Self::Unknown) } } diff --git a/neqo-crypto/src/constants.rs b/neqo-crypto/src/constants.rs index 5ce615faee..8c96fa861b 100644 --- a/neqo-crypto/src/constants.rs +++ b/neqo-crypto/src/constants.rs @@ -5,6 +5,7 @@ // except according to those terms. use enum_map::Enum; +use strum::FromRepr; use crate::{ssl, Error}; @@ -13,7 +14,8 @@ use crate::{ssl, Error}; pub type Alert = u8; -#[derive(Default, Debug, Enum, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Default, Debug, Enum, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, FromRepr)] +#[repr(u16)] pub enum Epoch { // TLS doesn't really have an "initial" concept that maps to QUIC so directly, // but this should be clear enough. @@ -29,24 +31,13 @@ impl TryFrom for Epoch { type Error = Error; fn try_from(value: u16) -> Result { - match value { - 0 => Ok(Self::Initial), - 1 => Ok(Self::ZeroRtt), - 2 => Ok(Self::Handshake), - 3 => Ok(Self::ApplicationData), - _ => Err(Error::InvalidEpoch), - } + Self::from_repr(value).ok_or(Error::InvalidEpoch) } } impl From for usize { fn from(e: Epoch) -> Self { - match e { - Epoch::Initial => 0, - Epoch::ZeroRtt => 1, - Epoch::Handshake => 2, - Epoch::ApplicationData => 3, - } + e as Self } } diff --git a/neqo-crypto/src/secrets.rs b/neqo-crypto/src/secrets.rs index a3c11dceb0..bbc1fd70b5 100644 --- a/neqo-crypto/src/secrets.rs +++ b/neqo-crypto/src/secrets.rs @@ -10,6 +10,7 @@ use std::{mem, os::raw::c_void, pin::Pin}; use enum_map::EnumMap; use neqo_common::qdebug; +use strum::FromRepr; use crate::{ agentio::as_c_void, @@ -25,20 +26,18 @@ experimental_api!(SSL_SecretCallback( arg: *mut c_void, )); -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, FromRepr)] +#[cfg_attr(windows, repr(i32))] // Windows has to be different, of coourse. +#[cfg_attr(not(windows), repr(u32))] pub enum SecretDirection { - Read, - Write, + Read = SSLSecretDirection::ssl_secret_read, + Write = SSLSecretDirection::ssl_secret_write, } impl From for SecretDirection { #[must_use] fn from(dir: SSLSecretDirection::Type) -> Self { - match dir { - SSLSecretDirection::ssl_secret_read => Self::Read, - SSLSecretDirection::ssl_secret_write => Self::Write, - _ => unreachable!(), - } + Self::from_repr(dir).expect("Invalid secret direction") } } diff --git a/neqo-crypto/src/ssl.rs b/neqo-crypto/src/ssl.rs index e654c991e4..8e855c6036 100644 --- a/neqo-crypto/src/ssl.rs +++ b/neqo-crypto/src/ssl.rs @@ -28,43 +28,27 @@ pub const SECSuccess: SECStatus = _SECStatus_SECSuccess; pub const SECFailure: SECStatus = _SECStatus_SECFailure; #[derive(Debug, Copy, Clone)] +#[repr(u32)] pub enum Opt { - Locking, - Tickets, - OcspStapling, - Alpn, - ExtendedMasterSecret, - SignedCertificateTimestamps, - EarlyData, - RecordSizeLimit, - Tls13CompatMode, - HelloDowngradeCheck, - SuppressEndOfEarlyData, - Grease, - EnableChExtensionPermutation, + Locking = SSLOption::SSL_NO_LOCKS, + Tickets = SSLOption::SSL_ENABLE_SESSION_TICKETS, + OcspStapling = SSLOption::SSL_ENABLE_OCSP_STAPLING, + Alpn = SSLOption::SSL_ENABLE_ALPN, + ExtendedMasterSecret = SSLOption::SSL_ENABLE_EXTENDED_MASTER_SECRET, + SignedCertificateTimestamps = SSLOption::SSL_ENABLE_SIGNED_CERT_TIMESTAMPS, + EarlyData = SSLOption::SSL_ENABLE_0RTT_DATA, + RecordSizeLimit = SSLOption::SSL_RECORD_SIZE_LIMIT, + Tls13CompatMode = SSLOption::SSL_ENABLE_TLS13_COMPAT_MODE, + HelloDowngradeCheck = SSLOption::SSL_ENABLE_HELLO_DOWNGRADE_CHECK, + SuppressEndOfEarlyData = SSLOption::SSL_SUPPRESS_END_OF_EARLY_DATA, + Grease = SSLOption::SSL_ENABLE_GREASE, + EnableChExtensionPermutation = SSLOption::SSL_ENABLE_CH_EXTENSION_PERMUTATION, } impl Opt { - // Cast is safe here because SSLOptions are within the i32 range - #[allow(clippy::cast_possible_wrap)] #[must_use] pub const fn as_int(self) -> PRInt32 { - let i = match self { - Self::Locking => SSLOption::SSL_NO_LOCKS, - Self::Tickets => SSLOption::SSL_ENABLE_SESSION_TICKETS, - Self::OcspStapling => SSLOption::SSL_ENABLE_OCSP_STAPLING, - Self::Alpn => SSLOption::SSL_ENABLE_ALPN, - Self::ExtendedMasterSecret => SSLOption::SSL_ENABLE_EXTENDED_MASTER_SECRET, - Self::SignedCertificateTimestamps => SSLOption::SSL_ENABLE_SIGNED_CERT_TIMESTAMPS, - Self::EarlyData => SSLOption::SSL_ENABLE_0RTT_DATA, - Self::RecordSizeLimit => SSLOption::SSL_RECORD_SIZE_LIMIT, - Self::Tls13CompatMode => SSLOption::SSL_ENABLE_TLS13_COMPAT_MODE, - Self::HelloDowngradeCheck => SSLOption::SSL_ENABLE_HELLO_DOWNGRADE_CHECK, - Self::SuppressEndOfEarlyData => SSLOption::SSL_SUPPRESS_END_OF_EARLY_DATA, - Self::Grease => SSLOption::SSL_ENABLE_GREASE, - Self::EnableChExtensionPermutation => SSLOption::SSL_ENABLE_CH_EXTENSION_PERMUTATION, - }; - i as PRInt32 + self as PRInt32 } // Some options are backwards, like SSL_NO_LOCKS, so use this to manage that. diff --git a/neqo-transport/Cargo.toml b/neqo-transport/Cargo.toml index a3013d6024..14496caa77 100644 --- a/neqo-transport/Cargo.toml +++ b/neqo-transport/Cargo.toml @@ -26,6 +26,7 @@ mtu = { version = "0.2.3", default-features = false } # neqo is only user curren qlog = { workspace = true } smallvec = { version = "1.13", default-features = false } static_assertions = { workspace = true } +strum = { workspace = true } [dev-dependencies] criterion = { version = "0.5", default-features = false } diff --git a/neqo-transport/src/connection/mod.rs b/neqo-transport/src/connection/mod.rs index 294ed07622..74d06312a2 100644 --- a/neqo-transport/src/connection/mod.rs +++ b/neqo-transport/src/connection/mod.rs @@ -30,6 +30,7 @@ use neqo_crypto::{ Server, ZeroRttChecker, }; use smallvec::SmallVec; +use strum::IntoEnumIterator as _; use crate::{ addr_valid::{AddressValidation, NewTokenState}, @@ -2399,7 +2400,7 @@ impl Connection { let mut encoder = Encoder::with_capacity(profile.limit()); for space in PacketNumberSpace::iter() { // Ensure we have tx crypto state for this epoch, or skip it. - let Some((epoch, tx)) = self.crypto.states.select_tx_mut(self.version, *space) else { + let Some((epoch, tx)) = self.crypto.states.select_tx_mut(self.version, space) else { continue; }; @@ -2416,7 +2417,7 @@ impl Connection { let pn = Self::add_packet_number( &mut builder, tx, - self.loss_recovery.largest_acknowledged_pn(*space), + self.loss_recovery.largest_acknowledged_pn(space), ); // The builder will set the limit to 0 if there isn't enough space for the header. if builder.is_full() { @@ -2445,10 +2446,10 @@ impl Connection { let payload_start = builder.len(); let (mut tokens, mut ack_eliciting, mut padded) = (Vec::new(), false, false); if let Some(close) = closing_frame { - self.write_closing_frames(close, &mut builder, *space, now, path, &mut tokens); + self.write_closing_frames(close, &mut builder, space, now, path, &mut tokens); } else { (tokens, ack_eliciting, padded) = - self.write_frames(path, *space, &profile, &mut builder, header_start != 0, now); + self.write_frames(path, space, &profile, &mut builder, header_start != 0, now); } if builder.packet_empty() { // Nothing to include in this packet. @@ -2503,7 +2504,7 @@ impl Connection { self.loss_recovery.on_packet_sent(path, sent, now); } - if *space == PacketNumberSpace::Handshake + if space == PacketNumberSpace::Handshake && self.role == Role::Server && self.state == State::Confirmed { @@ -2518,8 +2519,8 @@ impl Connection { // do not support, because they may not save packets they can't // decrypt yet. if self.role == Role::Client - && *space == PacketNumberSpace::Initial - && !self.crypto.streams.is_empty(*space) + && space == PacketNumberSpace::Initial + && !self.crypto.streams.is_empty(space) { break; } diff --git a/neqo-transport/src/frame.rs b/neqo-transport/src/frame.rs index 9bcdf0d091..7c113e5e45 100644 --- a/neqo-transport/src/frame.rs +++ b/neqo-transport/src/frame.rs @@ -9,6 +9,7 @@ use std::ops::RangeInclusive; use neqo_common::{qtrace, Decoder, Encoder}; +use strum::FromRepr; use crate::{ cid::MAX_CONNECTION_ID_LEN, @@ -19,7 +20,7 @@ use crate::{ }; #[repr(u64)] -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, Copy, PartialEq, Eq, FromRepr)] pub enum FrameType { Padding = 0x0, Ping = 0x1, @@ -75,43 +76,7 @@ impl TryFrom for FrameType { type Error = Error; fn try_from(value: u64) -> Result { - match value { - 0x0 => Ok(Self::Padding), - 0x1 => Ok(Self::Ping), - 0x2 => Ok(Self::Ack), - 0x3 => Ok(Self::AckEcn), - 0x4 => Ok(Self::ResetStream), - 0x5 => Ok(Self::StopSending), - 0x6 => Ok(Self::Crypto), - 0x7 => Ok(Self::NewToken), - 0x8 => Ok(Self::Stream), - 0x9 => Ok(Self::StreamWithFin), - 0xa => Ok(Self::StreamWithLen), - 0xb => Ok(Self::StreamWithLenFin), - 0xc => Ok(Self::StreamWithOff), - 0xd => Ok(Self::StreamWithOffFin), - 0xe => Ok(Self::StreamWithOffLen), - 0xf => Ok(Self::StreamWithOffLenFin), - 0x10 => Ok(Self::MaxData), - 0x11 => Ok(Self::MaxStreamData), - 0x12 => Ok(Self::MaxStreamsBiDi), - 0x13 => Ok(Self::MaxStreamsUniDi), - 0x14 => Ok(Self::DataBlocked), - 0x15 => Ok(Self::StreamDataBlocked), - 0x16 => Ok(Self::StreamsBlockedBiDi), - 0x17 => Ok(Self::StreamsBlockedUniDi), - 0x18 => Ok(Self::NewConnectionId), - 0x19 => Ok(Self::RetireConnectionId), - 0x1a => Ok(Self::PathChallenge), - 0x1b => Ok(Self::PathResponse), - 0x1c => Ok(Self::ConnectionCloseTransport), - 0x1d => Ok(Self::ConnectionCloseApplication), - 0x1e => Ok(Self::HandshakeDone), - 0xaf => Ok(Self::AckFrequency), - 0x30 => Ok(Self::Datagram), - 0x31 => Ok(Self::DatagramWithLen), - _ => Err(Error::UnknownFrameType), - } + Self::from_repr(value).ok_or(Error::UnknownFrameType) } } diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs index e7af86a7b3..df21010574 100644 --- a/neqo-transport/src/recovery/mod.rs +++ b/neqo-transport/src/recovery/mod.rs @@ -24,6 +24,7 @@ use enum_map::{enum_map, EnumMap}; use neqo_common::{qdebug, qinfo, qlog::NeqoQlog, qtrace, qwarn}; pub use sent::SentPacket; use sent::SentPackets; +use strum::IntoEnumIterator as _; pub use token::{RecoveryToken, StreamRecoveryToken}; use crate::{ @@ -825,17 +826,17 @@ impl LossRecovery { // The spaces in which we will allow probing. let mut allow_probes = PacketNumberSpaceSet::default(); for pn_space in PacketNumberSpace::iter() { - if let Some(t) = self.pto_time(rtt, *pn_space) { - allow_probes[*pn_space] = true; + if let Some(t) = self.pto_time(rtt, pn_space) { + allow_probes[pn_space] = true; if t <= now { qdebug!("[{self}] PTO timer fired for {pn_space}"); - if let Some(space) = self.spaces.get_mut(*pn_space) { + if let Some(space) = self.spaces.get_mut(pn_space) { lost.extend( space - .pto_packets(PtoState::pto_packet_count(*pn_space)) + .pto_packets(PtoState::pto_packet_count(pn_space)) .cloned(), ); - pto_space = pto_space.or(Some(*pn_space)); + pto_space = pto_space.or(Some(pn_space)); } } } diff --git a/neqo-transport/src/tparams.rs b/neqo-transport/src/tparams.rs index 956efe7d2d..1bf9103366 100644 --- a/neqo-transport/src/tparams.rs +++ b/neqo-transport/src/tparams.rs @@ -20,6 +20,7 @@ use neqo_crypto::{ ext::{ExtensionHandler, ExtensionHandlerResult, ExtensionWriterResult}, random, HandshakeMessage, ZeroRttCheckResult, ZeroRttChecker, }; +use strum::FromRepr; use TransportParameterId::{ AckDelayExponent, ActiveConnectionIdLimit, DisableMigration, GreaseQuicBit, IdleTimeout, InitialMaxData, InitialMaxStreamDataBidiLocal, InitialMaxStreamDataBidiRemote, @@ -36,7 +37,7 @@ use crate::{ Error, Res, }; -#[derive(Debug, Clone, Enum, PartialEq, Eq, Copy)] +#[derive(Debug, Clone, Enum, PartialEq, Eq, Copy, FromRepr)] #[repr(u64)] pub enum TransportParameterId { OriginalDestinationConnectionId = 0x00, @@ -80,32 +81,7 @@ impl TryFrom for TransportParameterId { type Error = Error; fn try_from(value: u64) -> Result { - match value { - 0x00 => Ok(Self::OriginalDestinationConnectionId), - 0x01 => Ok(Self::IdleTimeout), - 0x02 => Ok(Self::StatelessResetToken), - 0x03 => Ok(Self::MaxUdpPayloadSize), - 0x04 => Ok(Self::InitialMaxData), - 0x05 => Ok(Self::InitialMaxStreamDataBidiLocal), - 0x06 => Ok(Self::InitialMaxStreamDataBidiRemote), - 0x07 => Ok(Self::InitialMaxStreamDataUni), - 0x08 => Ok(Self::InitialMaxStreamsBidi), - 0x09 => Ok(Self::InitialMaxStreamsUni), - 0x0a => Ok(Self::AckDelayExponent), - 0x0b => Ok(Self::MaxAckDelay), - 0x0c => Ok(Self::DisableMigration), - 0x0d => Ok(Self::PreferredAddress), - 0x0e => Ok(Self::ActiveConnectionIdLimit), - 0x0f => Ok(Self::InitialSourceConnectionId), - 0x10 => Ok(Self::RetrySourceConnectionId), - 0x11 => Ok(Self::VersionInformation), - 0x2ab2 => Ok(Self::GreaseQuicBit), - 0xff02_de1a => Ok(Self::MinAckDelay), - 0x0020 => Ok(Self::MaxDatagramFrameSize), - #[cfg(test)] - 0xce16 => Ok(Self::TestTransportParameter), - _ => Err(Error::UnknownTransportParameter), - } + Self::from_repr(value).ok_or(Error::UnknownTransportParameter) } } diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs index 6a36d8d192..fa51e3462e 100644 --- a/neqo-transport/src/tracking.rs +++ b/neqo-transport/src/tracking.rs @@ -16,6 +16,7 @@ use std::{ use enum_map::{enum_map, Enum, EnumMap}; use neqo_common::{qdebug, qinfo, qtrace, qwarn, IpTosEcn}; use neqo_crypto::Epoch; +use strum::{EnumIter, IntoEnumIterator as _}; use crate::{ ecn, @@ -26,24 +27,13 @@ use crate::{ Error, Res, }; -#[derive(Clone, Copy, Debug, PartialEq, PartialOrd, Ord, Eq, Enum)] +#[derive(Clone, Copy, Debug, PartialEq, PartialOrd, Ord, Eq, Enum, EnumIter)] pub enum PacketNumberSpace { Initial, Handshake, ApplicationData, } -impl PacketNumberSpace { - pub fn iter() -> impl Iterator { - const SPACES: &[PacketNumberSpace] = &[ - PacketNumberSpace::Initial, - PacketNumberSpace::Handshake, - PacketNumberSpace::ApplicationData, - ]; - SPACES.iter() - } -} - impl From for PacketNumberSpace { fn from(epoch: Epoch) -> Self { match epoch { @@ -122,12 +112,12 @@ impl std::fmt::Debug for PacketNumberSpaceSet { let mut first = true; f.write_str("(")?; for sp in PacketNumberSpace::iter() { - if self[*sp] { + if self[sp] { if !first { f.write_str("+")?; first = false; } - std::fmt::Display::fmt(sp, f)?; + std::fmt::Display::fmt(&sp, f)?; } } f.write_str(")") From 98fbc0a3f4bacbff5bbbc407f7110e9a1b08b776 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Sat, 15 Feb 2025 18:29:58 +0200 Subject: [PATCH 2/4] chore: Use `EnumSet` instead of a Boolean `EnumMap` (#2443) * chore: Use `EnumSet` instead of a Boolean `EnumMap` For `PacketNumberSpaceSet`. May be faster? * Remove tests as suggested by @martinthomson * Fix * clippy --------- Signed-off-by: Lars Eggert --- Cargo.lock | 1 + Cargo.toml | 1 + neqo-http3/Cargo.toml | 2 +- neqo-transport/Cargo.toml | 1 + neqo-transport/src/recovery/mod.rs | 13 ++-- neqo-transport/src/tracking.rs | 110 ++--------------------------- 6 files changed, 17 insertions(+), 111 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ba9dfb9d8f..72dc7aaac0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -901,6 +901,7 @@ version = "0.12.2" dependencies = [ "criterion", "enum-map", + "enumset", "indexmap", "log", "mtu", diff --git a/Cargo.toml b/Cargo.toml index fece3f774c..16505d8f5f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -31,6 +31,7 @@ rust-version = "1.76.0" [workspace.dependencies] # Checked against https://searchfox.org/mozilla-central/source/Cargo.lock 2024-11-11 enum-map = { version = "2.7", default-features = false } +enumset = { version = "1.1", default-features = false } log = { version = "0.4", default-features = false } qlog = { version = "0.13", default-features = false } quinn-udp = { version = "0.5.6", default-features = false, features = ["direct-log", "fast-apple-datapath"] } diff --git a/neqo-http3/Cargo.toml b/neqo-http3/Cargo.toml index ee09eb7729..f640ee0fa6 100644 --- a/neqo-http3/Cargo.toml +++ b/neqo-http3/Cargo.toml @@ -17,7 +17,7 @@ workspace = true [dependencies] # Checked against https://searchfox.org/mozilla-central/source/Cargo.lock 2024-11-11 -enumset = { version = "1.1", default-features = false } +enumset = { workspace = true } log = { workspace = true } neqo-common = { path = "./../neqo-common" } neqo-crypto = { path = "./../neqo-crypto" } diff --git a/neqo-transport/Cargo.toml b/neqo-transport/Cargo.toml index 14496caa77..cb6dafcb09 100644 --- a/neqo-transport/Cargo.toml +++ b/neqo-transport/Cargo.toml @@ -18,6 +18,7 @@ workspace = true [dependencies] # Checked against https://searchfox.org/mozilla-central/source/Cargo.lock 2024-11-11 enum-map = { workspace = true } +enumset = { workspace = true } indexmap = { version = "2.2", default-features = false } # See https://github.com/mozilla/neqo/issues/1858 log = { workspace = true } neqo-common = { path = "../neqo-common" } diff --git a/neqo-transport/src/recovery/mod.rs b/neqo-transport/src/recovery/mod.rs index df21010574..14d99a4e92 100644 --- a/neqo-transport/src/recovery/mod.rs +++ b/neqo-transport/src/recovery/mod.rs @@ -21,6 +21,7 @@ use std::{ }; use enum_map::{enum_map, EnumMap}; +use enumset::enum_set; use neqo_common::{qdebug, qinfo, qlog::NeqoQlog, qtrace, qwarn}; pub use sent::SentPacket; use sent::SentPackets; @@ -94,7 +95,7 @@ impl SendProfile { #[must_use] pub fn new_pto(pn_space: PacketNumberSpace, mtu: usize, probe: PacketNumberSpaceSet) -> Self { debug_assert!(mtu > ACK_ONLY_SIZE_LIMIT); - debug_assert!(probe[pn_space]); + debug_assert!(probe.contains(pn_space)); Self { limit: mtu, pto: Some(pn_space), @@ -108,7 +109,7 @@ impl SendProfile { /// that has the PTO timer armed. #[must_use] pub fn should_probe(&self, space: PacketNumberSpace) -> bool { - self.probe[space] + self.probe.contains(space) } /// Determine whether an ACK-only packet should be sent for the given packet @@ -444,7 +445,7 @@ impl PtoState { } pub fn new(space: PacketNumberSpace, probe: PacketNumberSpaceSet) -> Self { - debug_assert!(probe[space]); + debug_assert!(probe.contains(space)); Self { space, count: 1, @@ -454,7 +455,7 @@ impl PtoState { } pub fn pto(&mut self, space: PacketNumberSpace, probe: PacketNumberSpaceSet) { - debug_assert!(probe[space]); + debug_assert!(probe.contains(space)); self.space = space; self.count += 1; self.packets = Self::pto_packet_count(space); @@ -688,7 +689,7 @@ impl LossRecovery { // So maybe fire a PTO. if let Some(pto) = self.pto_time(rtt, PacketNumberSpace::ApplicationData) { if pto < now { - let probes = PacketNumberSpaceSet::from(&[PacketNumberSpace::ApplicationData]); + let probes = enum_set!(PacketNumberSpace::ApplicationData); self.fire_pto(PacketNumberSpace::ApplicationData, probes, now); } } @@ -827,7 +828,7 @@ impl LossRecovery { let mut allow_probes = PacketNumberSpaceSet::default(); for pn_space in PacketNumberSpace::iter() { if let Some(t) = self.pto_time(rtt, pn_space) { - allow_probes[pn_space] = true; + allow_probes.insert(pn_space); if t <= now { qdebug!("[{self}] PTO timer fired for {pn_space}"); if let Some(space) = self.spaces.get_mut(pn_space) { diff --git a/neqo-transport/src/tracking.rs b/neqo-transport/src/tracking.rs index fa51e3462e..e92744ac29 100644 --- a/neqo-transport/src/tracking.rs +++ b/neqo-transport/src/tracking.rs @@ -9,14 +9,14 @@ use std::{ cmp::min, collections::VecDeque, - ops::{Index, IndexMut}, time::{Duration, Instant}, }; use enum_map::{enum_map, Enum, EnumMap}; +use enumset::{EnumSet, EnumSetType}; use neqo_common::{qdebug, qinfo, qtrace, qwarn, IpTosEcn}; use neqo_crypto::Epoch; -use strum::{EnumIter, IntoEnumIterator as _}; +use strum::EnumIter; use crate::{ ecn, @@ -27,7 +27,7 @@ use crate::{ Error, Res, }; -#[derive(Clone, Copy, Debug, PartialEq, PartialOrd, Ord, Eq, Enum, EnumIter)] +#[derive(Debug, PartialOrd, Ord, EnumSetType, Enum, EnumIter)] pub enum PacketNumberSpace { Initial, Handshake, @@ -66,63 +66,7 @@ impl From for PacketNumberSpace { } } -#[derive(Clone, Copy, Default)] -pub struct PacketNumberSpaceSet { - spaces: EnumMap, -} - -impl PacketNumberSpaceSet { - pub fn all() -> Self { - Self { - spaces: enum_map! { - PacketNumberSpace::Initial => true, - PacketNumberSpace::Handshake => true, - PacketNumberSpace::ApplicationData => true, - }, - } - } -} - -impl Index for PacketNumberSpaceSet { - type Output = bool; - - fn index(&self, index: PacketNumberSpace) -> &Self::Output { - &self.spaces[index] - } -} - -impl IndexMut for PacketNumberSpaceSet { - fn index_mut(&mut self, index: PacketNumberSpace) -> &mut Self::Output { - &mut self.spaces[index] - } -} - -impl> From for PacketNumberSpaceSet { - fn from(spaces: T) -> Self { - let mut v = Self::default(); - for sp in spaces.as_ref() { - v[*sp] = true; - } - v - } -} - -impl std::fmt::Debug for PacketNumberSpaceSet { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - let mut first = true; - f.write_str("(")?; - for sp in PacketNumberSpace::iter() { - if self[sp] { - if !first { - f.write_str("+")?; - first = false; - } - std::fmt::Display::fmt(&sp, f)?; - } - } - f.write_str(")") - } -} +pub type PacketNumberSpaceSet = EnumSet; impl std::fmt::Display for PacketNumberSpace { fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { @@ -679,8 +623,8 @@ mod tests { use test_fixture::now; use super::{ - AckTracker, Duration, Instant, PacketNumberSpace, PacketNumberSpaceSet, RecoveryToken, - RecvdPackets, MAX_TRACKED_RANGES, + AckTracker, Duration, Instant, PacketNumberSpace, RecoveryToken, RecvdPackets, + MAX_TRACKED_RANGES, }; use crate::{ frame::Frame, @@ -1114,48 +1058,6 @@ mod tests { ); } - #[test] - fn pnspaceset_default() { - let set = PacketNumberSpaceSet::default(); - assert!(!set[PacketNumberSpace::Initial]); - assert!(!set[PacketNumberSpace::Handshake]); - assert!(!set[PacketNumberSpace::ApplicationData]); - } - - #[test] - fn pnspaceset_from() { - let set = PacketNumberSpaceSet::from(&[PacketNumberSpace::Initial]); - assert!(set[PacketNumberSpace::Initial]); - assert!(!set[PacketNumberSpace::Handshake]); - assert!(!set[PacketNumberSpace::ApplicationData]); - - let set = - PacketNumberSpaceSet::from(&[PacketNumberSpace::Handshake, PacketNumberSpace::Initial]); - assert!(set[PacketNumberSpace::Initial]); - assert!(set[PacketNumberSpace::Handshake]); - assert!(!set[PacketNumberSpace::ApplicationData]); - - let set = PacketNumberSpaceSet::from(&[ - PacketNumberSpace::ApplicationData, - PacketNumberSpace::ApplicationData, - ]); - assert!(!set[PacketNumberSpace::Initial]); - assert!(!set[PacketNumberSpace::Handshake]); - assert!(set[PacketNumberSpace::ApplicationData]); - } - - #[test] - fn pnspaceset_copy() { - let set = PacketNumberSpaceSet::from(&[ - PacketNumberSpace::Handshake, - PacketNumberSpace::ApplicationData, - ]); - let copy = set; - assert!(!copy[PacketNumberSpace::Initial]); - assert!(copy[PacketNumberSpace::Handshake]); - assert!(copy[PacketNumberSpace::ApplicationData]); - } - #[test] fn from_packet_type() { assert_eq!( From d1656d474b7c22606c6592b4adc53cb71efb6282 Mon Sep 17 00:00:00 2001 From: Lars Eggert Date: Sun, 16 Feb 2025 19:01:55 +0200 Subject: [PATCH 3/4] fix: Only try SNI slicing at offset 0 (#2436) * fix: Only try SNI slicing at offset 0 And also check that the SNI length makes sense. This should fix the spurios failures for `compatible_upgrade_large_initial`. * Add test * Update neqo-transport/src/shuffle.rs Co-authored-by: Martin Thomson Signed-off-by: Lars Eggert * Update neqo-transport/src/shuffle.rs Co-authored-by: Martin Thomson Signed-off-by: Lars Eggert * Update neqo-transport/src/shuffle.rs Co-authored-by: Martin Thomson Signed-off-by: Lars Eggert * Suggestions from @martinthomson * Add test * Fixes * Add another test * Rename --------- Signed-off-by: Lars Eggert Co-authored-by: Martin Thomson --- neqo-transport/src/crypto.rs | 73 ++++++++++++----------- neqo-transport/src/lib.rs | 4 +- neqo-transport/src/{shuffle.rs => sni.rs} | 21 +++++-- neqo-transport/tests/sni.rs | 44 ++++++++++++++ 4 files changed, 99 insertions(+), 43 deletions(-) rename neqo-transport/src/{shuffle.rs => sni.rs} (86%) create mode 100644 neqo-transport/tests/sni.rs diff --git a/neqo-transport/src/crypto.rs b/neqo-transport/src/crypto.rs index 68c9ab3411..2552852966 100644 --- a/neqo-transport/src/crypto.rs +++ b/neqo-transport/src/crypto.rs @@ -36,7 +36,7 @@ use crate::{ recovery::RecoveryToken, recv_stream::RxStreamOrderer, send_stream::TxBuffer, - shuffle::find_sni, + sni::find_sni, stats::FrameStats, tparams::{TpZeroRttChecker, TransportParameters, TransportParametersHandler}, tracking::PacketNumberSpace, @@ -1554,9 +1554,8 @@ impl CryptoStreams { (left, _) = left.split_at(limit - right.len()); } else { // Both chunks are too long to fit into one packet. Just send a part of each. - let half_limit = limit / 2; - (left, _) = left.split_at(half_limit); - (right, _) = right.split_at(half_limit); + (left, _) = left.split_at(limit / 2); + (right, _) = right.split_at(limit / 2); } ((left_offset, left), (right_offset, right)) } @@ -1564,41 +1563,43 @@ impl CryptoStreams { let Some(cs) = self.get_mut(space) else { return; }; - let Some((offset, data)) = cs.tx.next_bytes() else { - return; - }; - let written = if sni_slicing { - if let Some(sni) = find_sni(data) { - // Cut the crypto data in two at the midpoint of the SNI and swap the chunks. - let mid = sni.start + (sni.end - sni.start) / 2; - let (left, right) = data.split_at(mid); - - // Truncate the chunks so we can fit them into roughly evenly-filled packets. - let packets_needed = data.len().div_ceil(builder.limit()); - let limit = data.len() / packets_needed; - let ((left_offset, left), (right_offset, right)) = - limit_chunks((offset, left), (offset + mid as u64, right), limit); - ( - write_chunk(right_offset, right, builder), - write_chunk(left_offset, left, builder), - ) + while let Some((offset, data)) = cs.tx.next_bytes() { + let written = if sni_slicing && offset == 0 { + if let Some(sni) = find_sni(data) { + // Cut the crypto data in two at the midpoint of the SNI and swap the chunks. + let mid = sni.start + (sni.end - sni.start) / 2; + let (left, right) = data.split_at(mid); + + // Truncate the chunks so we can fit them into roughly evenly-filled packets. + let packets_needed = data.len().div_ceil(builder.limit()); + let limit = data.len() / packets_needed; + let ((left_offset, left), (right_offset, right)) = + limit_chunks((offset, left), (offset + mid as u64, right), limit); + ( + write_chunk(right_offset, right, builder), + write_chunk(left_offset, left, builder), + ) + } else { + // No SNI found, write the entire data. + (write_chunk(offset, data, builder), None) + } } else { - // No SNI found, write the entire data. + // SNI slicing disabled or data not at offset 0, write the entire data. (write_chunk(offset, data, builder), None) - } - } else { - // SNI slicing disabled, write the entire data. - (write_chunk(offset, data, builder), None) - }; + }; - match written { - (None, None) => (), - (None, Some((offset, len))) | (Some((offset, len)), None) => { - mark_as_sent(cs, space, tokens, offset, len, stats); - } - (Some((offset1, len1)), Some((offset2, len2))) => { - mark_as_sent(cs, space, tokens, offset1, len1, stats); - mark_as_sent(cs, space, tokens, offset2, len2, stats); + match written { + (None, None) => break, + (None, Some((offset, len))) | (Some((offset, len)), None) => { + mark_as_sent(cs, space, tokens, offset, len, stats); + } + (Some((offset1, len1)), Some((offset2, len2))) => { + mark_as_sent(cs, space, tokens, offset1, len1, stats); + mark_as_sent(cs, space, tokens, offset2, len2, stats); + // We only end up in this arm if we successfully sliced above. In that case, + // don't try and fit more crypto data into this packet. + break; + } } } } diff --git a/neqo-transport/src/lib.rs b/neqo-transport/src/lib.rs index 0ba8d77bcc..fc925ca1b3 100644 --- a/neqo-transport/src/lib.rs +++ b/neqo-transport/src/lib.rs @@ -44,7 +44,7 @@ pub mod send_stream; mod send_stream; mod sender; pub mod server; -mod shuffle; +mod sni; mod stats; pub mod stream_id; pub mod streams; @@ -69,7 +69,7 @@ pub use self::{ quic_datagrams::DatagramTracking, recv_stream::{RecvStreamStats, RECV_BUFFER_SIZE}, send_stream::{SendStreamStats, SEND_BUFFER_SIZE}, - shuffle::find_sni, + sni::find_sni, stats::Stats, stream_id::{StreamId, StreamType}, version::Version, diff --git a/neqo-transport/src/shuffle.rs b/neqo-transport/src/sni.rs similarity index 86% rename from neqo-transport/src/shuffle.rs rename to neqo-transport/src/sni.rs index fd81190c27..7b2773315f 100644 --- a/neqo-transport/src/shuffle.rs +++ b/neqo-transport/src/sni.rs @@ -47,7 +47,10 @@ pub fn find_sni(buf: &[u8]) -> Option> { let ext_len: u16 = dec.decode_uint()?; if ext_type == 0 { // SNI! - let sni_len: u16 = dec.decode_uint()?; + let sni_len = dec.decode_uint::()?; + if sni_len < 3 { + return None; + } skip(&mut dec, 3)?; // Skip name_type and host_name length let start = dec.offset(); let end = start + usize::from(sni_len) - 3; @@ -107,10 +110,8 @@ mod tests { // ClientHello without SNI extension let mut buf = Vec::from(&BUF_WITH_SNI[..BUF_WITH_SNI.len() - 39]); let len = buf.len(); - assert!(buf[len - 2] == 0x01 && buf[len - 1] == 0xcb); // Check extensions length - // Set extensions length to 0 - buf[len - 2] = 0x00; - buf[len - 1] = 0x00; + assert_eq!(&buf[len - 2..len], &[0x01, 0xcb], "check extensions length"); + buf[len - 2..len].copy_from_slice(&[0x00, 0x00]); // Set extensions length to 0 assert!(super::find_sni(&buf).is_none()); } @@ -121,6 +122,16 @@ mod tests { assert!(super::find_sni(truncated).is_none()); } + #[test] + fn find_sni_invalid_sni_length() { + // ClientHello with an SNI extension with an invalid length + let mut buf = Vec::from(BUF_WITH_SNI); + let len = buf.len(); + assert_eq!(&buf[len - 23..len - 21], &[0x00, 0x0c], "check SNI length"); + buf[len - 23..len - 21].copy_from_slice(&[0x00, 0x02]); // Set Server Name List length to 2 + assert!(super::find_sni(&buf).is_none()); + } + #[test] fn find_sni_no_ci() { // Not a ClientHello (msg_type != 1) diff --git a/neqo-transport/tests/sni.rs b/neqo-transport/tests/sni.rs new file mode 100644 index 0000000000..916c031b02 --- /dev/null +++ b/neqo-transport/tests/sni.rs @@ -0,0 +1,44 @@ +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +use neqo_transport::ConnectionParameters; +use test_fixture::{default_client, default_server, new_client, now}; + +#[test] +fn sni_no_slicing_at_nonzero_offset() { + let mut client = default_client(); + let mut server = default_server(); + let mut now = now(); + + // This packet will have two CRPYTO frames [y..end] and [0..x], where x < y. + let ch1 = client.process_output(now).dgram(); + assert_eq!(client.stats().frame_tx.crypto, 2); + // This packet will have one CRYPTO frame [x..y]. + let _ch2 = client.process_output(now).dgram(); + assert_eq!(client.stats().frame_tx.crypto, 3); + // We are dropping the second packet and only deliver the first. + let ack = server.process(ch1, now).dgram(); + // Client will now RTX the second packet. + now += client.process(ack, now).callback(); + // Make sure it only has one CRYPTO frame. + _ = client.process_output(now).dgram(); + assert_eq!(client.stats().frame_tx.crypto, 4); +} + +#[test] +fn sni_no_slicing_at_nonzero_offset_no_mlkem() { + let mut client = new_client(ConnectionParameters::default().mlkem(false)); + let mut now = now(); + + // This packet will have two CRPYTO frames [x..end] and [0..x]. + _ = client.process_output(now).dgram(); + assert_eq!(client.stats().frame_tx.crypto, 2); + // Client will now RTX the packet. + now += client.process_output(now).callback(); + // Make sure it has two CRYPTO frames. + _ = client.process_output(now).dgram(); + assert_eq!(client.stats().frame_tx.crypto, 4); +} From 1df2a5a2b321ca4ff692812d11ef6535ecb73e09 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 18 Feb 2025 01:23:15 +0200 Subject: [PATCH 4/4] build(deps): bump lukemathwalker/cargo-chef in /qns (#2445) --- qns/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qns/Dockerfile b/qns/Dockerfile index f44f96cf8d..5719ab36b8 100644 --- a/qns/Dockerfile +++ b/qns/Dockerfile @@ -1,4 +1,4 @@ -FROM lukemathwalker/cargo-chef@sha256:66b16e2261871e522c3c8b2bc2da4222843e03b3581f4f2a98ac8fc59fa3d3ae AS chef +FROM lukemathwalker/cargo-chef@sha256:2c9b2693a474b223a20f1e2bf850816d51848920bd34794fdf7c09afcaaa0fa6 AS chef WORKDIR /app