Inline choose_ciphersuite_preferring_client and co

Test the behaviour of `ServerConfig::ignore_client_order` at the public API level.
rustls · Feb 19, 2024 · 59532da · 59532da
1 parent 0125301
commit 59532da
Show file tree

Hide file tree

Showing 3 changed files with 54 additions and 80 deletions.
diff --git a/rustls/src/server/hs.rs b/rustls/src/server/hs.rs
@@ -465,7 +465,7 @@ impl ExpectClientHello {
             return Err(PeerIncompatible::NoKxGroupsInCommon);
         }
 
-        let suitable_suites = self
+        let mut suitable_suites_iter = self
             .config
             .provider
             .cipher_suites
@@ -480,9 +480,7 @@ impl ExpectClientHello {
                 // And key exchange groups
                 && (!ecdhe_possible || suite.usable_for_kx_algorithm(KeyExchangeAlgorithm::ECDHE))
                 && (!ffdhe_possible || suite.usable_for_kx_algorithm(KeyExchangeAlgorithm::DHE))
-            })
-            .copied()
-            .collect::<Vec<_>>();
+            });
 
         // RFC 7919 (https://datatracker.ietf.org/doc/html/rfc7919#section-4) requires us to send
         // the InsufficientSecurity alert in case we don't recognize client's FFDHE groups (i.e.,
@@ -491,9 +489,17 @@ impl ExpectClientHello {
         // and continue to send HandshakeFailure.
 
         let suite = if self.config.ignore_client_order {
-            suites::choose_ciphersuite_preferring_server(client_suites, &suitable_suites)
+            suitable_suites_iter.find(|suite| client_suites.contains(&suite.suite()))
         } else {
-            suites::choose_ciphersuite_preferring_client(client_suites, &suitable_suites)
+            let suitable_suites = suitable_suites_iter.collect::<Vec<_>>();
+            client_suites
+                .iter()
+                .find_map(|client_suite| {
+                    suitable_suites
+                        .iter()
+                        .find(|x| *client_suite == x.suite())
+                })
+                .copied()
         }
         .ok_or(PeerIncompatible::NoCipherSuitesInCommon)?;
 
@@ -510,18 +516,18 @@ impl ExpectClientHello {
 
         if selected_version == ProtocolVersion::TLSv1_3 {
             // This unwrap is structurally guaranteed by the early return for `!ffdhe_possible && !ecdhe_possible`
-            return Ok((suite, *maybe_skxg.unwrap()));
+            return Ok((*suite, *maybe_skxg.unwrap()));
         }
 
         // For TLS1.2, the server can unilaterally choose a DHE group if it has one and
         // there was no better option.
         match maybe_skxg {
-            Some(skxg) => Ok((suite, *skxg)),
+            Some(skxg) => Ok((*suite, *skxg)),
             None if suite.usable_for_kx_algorithm(KeyExchangeAlgorithm::DHE) => {
                 // If kx for the selected cipher suite is DHE and no DHE groups are specified in the extension,
                 // the server is free to choose DHE params, we choose the first DHE kx group of the provider.
                 if let Some(server_selected_ffdhe_skxg) = first_supported_dhe_kxg {
-                    Ok((suite, *server_selected_ffdhe_skxg))
+                    Ok((*suite, *server_selected_ffdhe_skxg))
                 } else {
                     Err(PeerIncompatible::NoKxGroupsInCommon)
                 }

diff --git a/rustls/src/suites.rs b/rustls/src/suites.rs
@@ -167,37 +167,6 @@ impl fmt::Debug for SupportedCipherSuite {
     }
 }
 
-// These both O(N^2)!
-pub(crate) fn choose_ciphersuite_preferring_client(
-    client_suites: &[CipherSuite],
-    server_suites: &[SupportedCipherSuite],
-) -> Option<SupportedCipherSuite> {
-    for client_suite in client_suites {
-        if let Some(selected) = server_suites
-            .iter()
-            .find(|x| *client_suite == x.suite())
-        {
-            return Some(*selected);
-        }
-    }
-
-    None
-}
-
-pub(crate) fn choose_ciphersuite_preferring_server(
-    client_suites: &[CipherSuite],
-    server_suites: &[SupportedCipherSuite],
-) -> Option<SupportedCipherSuite> {
-    if let Some(selected) = server_suites
-        .iter()
-        .find(|x| client_suites.contains(&x.suite()))
-    {
-        return Some(*selected);
-    }
-
-    None
-}
-
 /// Return true if `sigscheme` is usable by any of the given suites.
 pub(crate) fn compatible_sigscheme_for_suites(
     sigscheme: SignatureScheme,
@@ -264,48 +233,8 @@ pub enum ConnectionTrafficSecrets {
 }
 
 test_for_each_provider! {
-    use super::*;
-    use crate::enums::CipherSuite;
     use provider::tls13::*;
 
-    #[test]
-    fn test_client_pref() {
-        let client = vec![
-            CipherSuite::TLS13_AES_128_GCM_SHA256,
-            CipherSuite::TLS13_AES_256_GCM_SHA384,
-        ];
-        let server = vec![TLS13_AES_256_GCM_SHA384, TLS13_AES_128_GCM_SHA256];
-        let chosen = choose_ciphersuite_preferring_client(&client, &server);
-        assert!(chosen.is_some());
-        assert_eq!(chosen.unwrap(), TLS13_AES_128_GCM_SHA256);
-    }
-
-    #[test]
-    fn test_server_pref() {
-        let client = vec![
-            CipherSuite::TLS13_AES_128_GCM_SHA256,
-            CipherSuite::TLS13_AES_256_GCM_SHA384,
-        ];
-        let server = vec![TLS13_AES_256_GCM_SHA384, TLS13_AES_128_GCM_SHA256];
-        let chosen = choose_ciphersuite_preferring_server(&client, &server);
-        assert!(chosen.is_some());
-        assert_eq!(chosen.unwrap(), TLS13_AES_256_GCM_SHA384);
-    }
-
-    #[test]
-    fn test_pref_fails() {
-        assert!(choose_ciphersuite_preferring_client(
-            &[CipherSuite::TLS_NULL_WITH_NULL_NULL],
-            provider::ALL_CIPHER_SUITES
-        )
-        .is_none());
-        assert!(choose_ciphersuite_preferring_server(
-            &[CipherSuite::TLS_NULL_WITH_NULL_NULL],
-            provider::ALL_CIPHER_SUITES
-        )
-        .is_none());
-    }
-
     #[test]
     fn test_scs_is_debug() {
         println!("{:?}", provider::ALL_CIPHER_SUITES);

diff --git a/rustls/tests/api.rs b/rustls/tests/api.rs
@@ -3011,6 +3011,45 @@ fn negotiated_ciphersuite_server() {
     }
 }
 
+#[test]
+fn negotiated_ciphersuite_server_ignoring_client_preference() {
+    for (version, kt, suite) in test_ciphersuites() {
+        let scs = find_suite(suite);
+        let scs_other = if scs.suite() == CipherSuite::TLS13_AES_256_GCM_SHA384 {
+            find_suite(CipherSuite::TLS13_AES_128_GCM_SHA256)
+        } else {
+            find_suite(CipherSuite::TLS13_AES_256_GCM_SHA384)
+        };
+        let mut server_config = finish_server_config(
+            kt,
+            ServerConfig::builder_with_provider(
+                CryptoProvider {
+                    cipher_suites: vec![scs, scs_other],
+                    ..provider::default_provider()
+                }
+                .into(),
+            )
+            .with_protocol_versions(&[version])
+            .unwrap(),
+        );
+        server_config.ignore_client_order = true;
+
+        let client_config = finish_client_config(
+            kt,
+            ClientConfig::builder_with_provider(
+                CryptoProvider {
+                cipher_suites: vec![ scs_other, scs ],
+                ..provider::default_provider()
+            }.into(),
+        )
+        .with_safe_default_protocol_versions()
+        .unwrap());
+
+        do_suite_test(client_config, server_config, scs, version.version);
+    }
+
+}
+
 #[derive(Debug, PartialEq)]
 struct KeyLogItem {
     label: String,