From 72dabe1d8577b090a6cd7b0560ce8f4bae6c396c Mon Sep 17 00:00:00 2001 From: Christopher Patton Date: Mon, 29 Sep 2025 17:16:57 -0700 Subject: [PATCH] Remove the "kx-*" features The "kx-*" features control default key exchange preferences. Its implementation requires disabling APIs for manually setting curve preferences via `set_curves()` or `set_curves_list()`. In practice, most teams need to be able to override default preferences at runtime anyway, which means these features were never really used. This commit gets rid of them, thereby reducing some complexity in the API. --- .github/workflows/ci.yml | 2 -- boring/Cargo.toml | 22 ---------------- boring/src/ssl/mod.rs | 54 -------------------------------------- boring/src/ssl/test/mod.rs | 25 ------------------ 4 files changed, 103 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8ee8b46c..46e621e7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -363,8 +363,6 @@ jobs: name: Run `underscore-wildcards` tests - run: cargo test --features pq-experimental,rpk name: Run `pq-experimental,rpk` tests - - run: cargo test --features kx-safe-default,pq-experimental - name: Run `kx-safe-default` tests - run: cargo test --features pq-experimental,underscore-wildcards name: Run `pq-experimental,underscore-wildcards` tests - run: cargo test --features rpk,underscore-wildcards diff --git a/boring/Cargo.toml b/boring/Cargo.toml index caac99f2..bc9dba22 100644 --- a/boring/Cargo.toml +++ b/boring/Cargo.toml @@ -44,28 +44,6 @@ pq-experimental = ["boring-sys/pq-experimental"] # those for `pq-experimental` feature apply. underscore-wildcards = ["boring-sys/underscore-wildcards"] -# Controlling key exchange preferences at compile time - -# Choose key exchange preferences at compile time. This prevents the user from -# choosing their own preferences. -kx-safe-default = [] - -# Support PQ key exchange. The client will prefer classical key exchange, but -# will upgrade to PQ key exchange if requested by the server. This is the -# safest option if you don't know if the peer supports PQ key exchange. This -# feature implies "kx-safe-default". -kx-client-pq-supported = ["kx-safe-default"] - -# Prefer PQ key exchange. The client will prefer PQ exchange, but fallback to -# classical key exchange if requested by the server. This is the best option if -# you know the peer supports PQ key exchange. This feature implies -# "kx-safe-default" and "kx-client-pq-supported". -kx-client-pq-preferred = ["kx-safe-default", "kx-client-pq-supported"] - -# Disable key exchange involving non-NIST key exchange on the client side. -# Implies "kx-safe-default". -kx-client-nist-required = ["kx-safe-default"] - [dependencies] bitflags = { workspace = true } foreign-types = { workspace = true } diff --git a/boring/src/ssl/mod.rs b/boring/src/ssl/mod.rs index 49788c3e..302f99ba 100644 --- a/boring/src/ssl/mod.rs +++ b/boring/src/ssl/mod.rs @@ -747,16 +747,12 @@ impl SslCurve { } } - // We need to allow dead_code here because `SslRef::set_curves` is conditionally compiled - // against the absence of the `kx-safe-default` feature and thus this function is never used. - // // **NOTE**: This function only exists because the version of boringssl we currently use does // not expose SSL_CTX_set1_group_ids. Because `SslRef::curve()` returns the public SSL_GROUP id // as opposed to the internal NID, but `SslContextBuilder::set_curves()` requires the internal // NID, we need this mapping in place to avoid breaking changes to the public API. Once the // underlying boringssl version is upgraded, this should be removed in favor of the new // SSL_CTX_set1_group_ids API. - #[allow(dead_code)] pub fn nid(&self) -> Option { match self.0 { ffi::SSL_GROUP_SECP224R1 => Some(ffi::NID_secp224r1), @@ -2017,11 +2013,6 @@ impl SslContextBuilder { } /// Sets the context's supported curves. - // - // If the "kx-*" flags are used to set key exchange preference, then don't allow the user to - // set them here. This ensures we don't override the user's preference without telling them: - // when the flags are used, the preferences are set just before connecting or accepting. - #[cfg(not(feature = "kx-safe-default"))] #[corresponds(SSL_CTX_set1_curves_list)] pub fn set_curves_list(&mut self, curves: &str) -> Result<(), ErrorStack> { let curves = CString::new(curves).map_err(ErrorStack::internal_error)?; @@ -2035,12 +2026,7 @@ impl SslContextBuilder { } /// Sets the context's supported curves. - // - // If the "kx-*" flags are used to set key exchange preference, then don't allow the user to - // set them here. This ensures we don't override the user's preference without telling them: - // when the flags are used, the preferences are set just before connecting or accepting. #[corresponds(SSL_CTX_set1_curves)] - #[cfg(not(feature = "kx-safe-default"))] pub fn set_curves(&mut self, curves: &[SslCurve]) -> Result<(), ErrorStack> { let curves: Vec = curves .iter() @@ -2915,40 +2901,6 @@ impl SslRef { } } - #[cfg(feature = "kx-safe-default")] - fn client_set_default_curves_list(&mut self) { - let curves = if cfg!(feature = "kx-client-pq-preferred") { - if cfg!(feature = "kx-client-nist-required") { - "P256Kyber768Draft00:P-256:P-384:P-521" - } else { - "X25519MLKEM768:X25519Kyber768Draft00:X25519:P256Kyber768Draft00:P-256:P-384:P-521" - } - } else if cfg!(feature = "kx-client-pq-supported") { - if cfg!(feature = "kx-client-nist-required") { - "P-256:P-384:P-521:P256Kyber768Draft00" - } else { - "X25519:P-256:P-384:P-521:X25519MLKEM768:X25519Kyber768Draft00:P256Kyber768Draft00" - } - } else { - if cfg!(feature = "kx-client-nist-required") { - "P-256:P-384:P-521" - } else { - "X25519:P-256:P-384:P-521" - } - }; - - self.set_curves_list(curves) - .expect("invalid default client curves list"); - } - - #[cfg(feature = "kx-safe-default")] - fn server_set_default_curves_list(&mut self) { - self.set_curves_list( - "X25519MLKEM768:X25519Kyber768Draft00:P256Kyber768Draft00:X25519:P-256:P-384", - ) - .expect("invalid default server curves list"); - } - /// Returns the [`SslCurve`] used for this `SslRef`. #[corresponds(SSL_get_curve_id)] #[must_use] @@ -4341,9 +4293,6 @@ where pub fn setup_connect(mut self) -> MidHandshakeSslStream { self.set_connect_state(); - #[cfg(feature = "kx-safe-default")] - self.inner.ssl.client_set_default_curves_list(); - MidHandshakeSslStream { stream: self.inner, error: Error { @@ -4373,9 +4322,6 @@ where pub fn setup_accept(mut self) -> MidHandshakeSslStream { self.set_accept_state(); - #[cfg(feature = "kx-safe-default")] - self.inner.ssl.server_set_default_curves_list(); - MidHandshakeSslStream { stream: self.inner, error: Error { diff --git a/boring/src/ssl/test/mod.rs b/boring/src/ssl/test/mod.rs index 6ac6ca75..0a4f6243 100644 --- a/boring/src/ssl/test/mod.rs +++ b/boring/src/ssl/test/mod.rs @@ -952,30 +952,6 @@ fn sni_callback_swapped_ctx() { assert!(CALLED_BACK.load(Ordering::SeqCst)); } -#[cfg(feature = "kx-safe-default")] -#[test] -fn client_set_default_curves_list() { - let ssl_ctx = crate::ssl::SslContextBuilder::new(SslMethod::tls()) - .unwrap() - .build(); - let mut ssl = Ssl::new(&ssl_ctx).unwrap(); - - // Panics if Kyber768 missing in boringSSL. - ssl.client_set_default_curves_list(); -} - -#[cfg(feature = "kx-safe-default")] -#[test] -fn server_set_default_curves_list() { - let ssl_ctx = crate::ssl::SslContextBuilder::new(SslMethod::tls()) - .unwrap() - .build(); - let mut ssl = Ssl::new(&ssl_ctx).unwrap(); - - // Panics if Kyber768 missing in boringSSL. - ssl.server_set_default_curves_list(); -} - #[test] fn get_curve() { let server = Server::builder().build(); @@ -994,7 +970,6 @@ fn get_curve_name() { assert_eq!(SslCurve::X25519.name(), Some("X25519")); } -#[cfg(not(feature = "kx-safe-default"))] #[test] fn set_curves() { let mut ctx = SslContext::builder(SslMethod::tls()).unwrap();