fix(s2n-quic-rustls): mark re-exported types as deprecated (#2176)

* fix(s2n-quic-rustls): mark re-exported types as deprecated * allow deprecated * add with_prefer_server_cipher_suite_order --------- Co-authored-by: Apoorv Kothari <[email protected]> Co-authored-by: Wesley Rosenblum <[email protected]>
aws · Apr 5, 2024 · 5afe1e1 · 5afe1e1
1 parent 343fa0d
commit 5afe1e1
Show file tree

Hide file tree

Showing 6 changed files with 73 additions and 39 deletions.
diff --git a/examples/rustls-mtls/src/lib.rs b/examples/rustls-mtls/src/lib.rs
@@ -4,7 +4,9 @@
 use rustls::{
     cipher_suite, ClientConfig, Error, RootCertStore, ServerConfig, SupportedCipherSuite,
 };
-use s2n_quic::provider::{tls, tls::rustls::rustls};
+use s2n_quic::provider::tls;
+#[allow(deprecated)]
+use s2n_quic::provider::tls::rustls::rustls;
 use std::{io::Cursor, path::Path, sync::Arc};
 use tokio::{fs::File, io::AsyncReadExt};
 use tracing::Level;

diff --git a/quic/s2n-quic-qns/src/tls.rs b/quic/s2n-quic-qns/src/tls.rs
@@ -116,15 +116,18 @@ impl Client {
             use ::rustls::{version, ClientConfig, KeyLogFile};
             use std::sync::Arc;
 
+            #[allow(deprecated)]
+            let cipher_suites = rustls::DEFAULT_CIPHERSUITES;
             let mut config = ClientConfig::builder()
-                .with_cipher_suites(rustls::DEFAULT_CIPHERSUITES)
+                .with_cipher_suites(cipher_suites)
                 .with_safe_default_kx_groups()
                 .with_protocol_versions(&[&version::TLS13])?
                 .with_custom_certificate_verifier(Arc::new(rustls::DisabledVerifier))
                 .with_no_client_auth();
             config.max_fragment_size = None;
             config.alpn_protocols = alpns.iter().map(|p| p.as_bytes().to_vec()).collect();
             config.key_log = Arc::new(KeyLogFile::new());
+            #[allow(deprecated)]
             rustls::Client::new(config)
         } else {
             rustls::Client::builder()
@@ -265,9 +268,11 @@ pub mod s2n_tls {
 
 pub mod rustls {
     use super::*;
+    #[allow(deprecated)]
+    pub use s2n_quic::provider::tls::rustls::DEFAULT_CIPHERSUITES;
     pub use s2n_quic::provider::tls::rustls::{
         certificate::{Certificate, IntoCertificate, IntoPrivateKey, PrivateKey},
-        Client, Server, DEFAULT_CIPHERSUITES,
+        Client, Server,
     };
 
     pub fn ca(ca: Option<&PathBuf>) -> Result<Certificate> {

diff --git a/quic/s2n-quic-rustls/src/certificate.rs b/quic/s2n-quic-rustls/src/certificate.rs
@@ -1,9 +1,7 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-#![allow(dead_code)]
-
-use rustls::Error;
+use crate::Error;
 
 macro_rules! cert_type {
     ($name:ident, $trait:ident, $method:ident, $inner:ty) => {
@@ -57,13 +55,11 @@ macro_rules! cert_type {
             fn $method(self) -> Result<$name, Error> {
                 match self.extension() {
                     Some(ext) if ext == "der" => {
-                        let pem =
-                            std::fs::read(self).map_err(|err| Error::General(err.to_string()))?;
+                        let pem = std::fs::read(self)?;
                         pem.$method()
                     }
                     _ => {
-                        let pem = std::fs::read_to_string(self)
-                            .map_err(|err| Error::General(err.to_string()))?;
+                        let pem = std::fs::read_to_string(self)?;
                         pem.$method()
                     }
                 }
@@ -91,8 +87,7 @@ mod pem {
     pub fn into_certificate(contents: &[u8]) -> Result<Vec<rustls::Certificate>, Error> {
         let mut cursor = std::io::Cursor::new(contents);
         let certs = rustls_pemfile::certs(&mut cursor)
-            .map(|certs| certs.into_iter().map(rustls::Certificate).collect())
-            .map_err(|_| Error::General("Could not read certificate".to_string()))?;
+            .map(|certs| certs.into_iter().map(rustls::Certificate).collect())?;
         Ok(certs)
     }
 
@@ -114,19 +109,18 @@ mod pem {
                     return Ok(rustls::PrivateKey(keys.pop().unwrap()))
                 }
                 Ok(keys) => {
-                    return Err(Error::General(format!(
+                    return Err(rustls::Error::General(format!(
                         "Unexpected number of keys: {} (only 1 supported)",
                         keys.len()
-                    )));
+                    ))
+                    .into());
                 }
                 // try the next parser
                 Err(_) => continue,
             }
         }
 
-        Err(Error::General(
-            "could not load any valid private keys".to_string(),
-        ))
+        Err(rustls::Error::General("could not load any valid private keys".to_string()).into())
     }
 }
 

diff --git a/quic/s2n-quic-rustls/src/client.rs b/quic/s2n-quic-rustls/src/client.rs
@@ -1,7 +1,7 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-use crate::{certificate, session::Session};
+use crate::{certificate, session::Session, Error};
 use core::convert::TryFrom;
 use rustls::ClientConfig;
 use s2n_codec::EncoderValue;
@@ -14,6 +14,7 @@ pub struct Client {
 }
 
 impl Client {
+    #[deprecated = "client and server builders should be used instead"]
     pub fn new(config: ClientConfig) -> Self {
         Self {
             config: Arc::new(config),
@@ -34,12 +35,14 @@ impl Default for Client {
     }
 }
 
+// TODO this should be removed after removing deprecated re-exports
 impl From<ClientConfig> for Client {
     fn from(config: ClientConfig) -> Self {
-        Self::new(config)
+        Self::from(Arc::new(config))
     }
 }
 
+// TODO this should be removed after removing deprecated re-exports
 impl From<Arc<ClientConfig>> for Client {
     fn from(config: Arc<ClientConfig>) -> Self {
         Self { config }
@@ -108,7 +111,7 @@ impl Builder {
     pub fn with_certificate<C: certificate::IntoCertificate>(
         mut self,
         certificate: C,
-    ) -> Result<Self, rustls::Error> {
+    ) -> Result<Self, Error> {
         let certificates = certificate.into_certificate()?;
         let root_certificate = certificates.0.first().ok_or_else(|| {
             rustls::Error::General("Certificate chain needs to have at least one entry".to_string())
@@ -119,7 +122,7 @@ impl Builder {
         Ok(self)
     }
 
-    pub fn with_max_cert_chain_depth(self, len: u16) -> Result<Self, rustls::Error> {
+    pub fn with_max_cert_chain_depth(self, len: u16) -> Result<Self, Error> {
         // TODO is there a way to configure this?
         let _ = len;
         Ok(self)
@@ -133,19 +136,19 @@ impl Builder {
         Ok(self)
     }
 
-    pub fn with_key_logging(mut self) -> Result<Self, rustls::Error> {
+    pub fn with_key_logging(mut self) -> Result<Self, Error> {
         self.key_log = Some(Arc::new(rustls::KeyLogFile::new()));
         Ok(self)
     }
 
-    pub fn build(self) -> Result<Client, rustls::Error> {
+    pub fn build(self) -> Result<Client, Error> {
         // TODO load system root store?
         if self.cert_store.is_empty() {
             //= https://www.rfc-editor.org/rfc/rfc9001#section-4.4
             //# A client MUST authenticate the identity of the server.
-            return Err(rustls::Error::General(
-                "missing trusted root certificate(s)".to_string(),
-            ));
+            return Err(
+                rustls::Error::General("missing trusted root certificate(s)".to_string()).into(),
+            );
         }
 
         let mut config = ClientConfig::builder()
@@ -162,6 +165,7 @@ impl Builder {
             config.key_log = key_log;
         }
 
+        #[allow(deprecated)]
         Ok(Client::new(config))
     }
 }
diff --git a/quic/s2n-quic-rustls/src/lib.rs b/quic/s2n-quic-rustls/src/lib.rs
@@ -3,7 +3,17 @@
 
 #![forbid(unsafe_code)]
 
-pub use rustls::{self, Certificate, PrivateKey};
+/// *WARNING*: These are deprecated and should not be used.
+#[deprecated = "client and server builders should be used instead"]
+pub use ::rustls::{Certificate, PrivateKey};
+
+#[deprecated = "client and server builders should be used instead"]
+pub mod rustls {
+    pub use ::rustls::*;
+}
+
+/// Wrap error types in Box to avoid leaking rustls types
+type Error = Box<dyn std::error::Error + Send + Sync + 'static>;
 
 mod cipher_suite;
 mod error;
@@ -13,7 +23,10 @@ pub mod certificate;
 pub mod client;
 pub mod server;
 
-pub use cipher_suite::DEFAULT_CIPHERSUITES;
+#[deprecated = "client and server builders should be used instead"]
+pub static DEFAULT_CIPHERSUITES: &[rustls::SupportedCipherSuite] =
+    cipher_suite::DEFAULT_CIPHERSUITES;
+
 pub use client::Client;
 pub use server::Server;
 

diff --git a/quic/s2n-quic-rustls/src/server.rs b/quic/s2n-quic-rustls/src/server.rs
@@ -1,7 +1,7 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-use crate::{certificate, session::Session};
+use crate::{certificate, session::Session, Error};
 use rustls::ServerConfig;
 use s2n_codec::EncoderValue;
 use s2n_quic_core::{application::ServerName, crypto::tls};
@@ -13,6 +13,7 @@ pub struct Server {
 }
 
 impl Server {
+    #[deprecated = "client and server builders should be used instead"]
     pub fn new(config: ServerConfig) -> Self {
         Self {
             config: Arc::new(config),
@@ -32,12 +33,14 @@ impl Default for Server {
     }
 }
 
+// TODO this should be removed after removing deprecated re-exports
 impl From<ServerConfig> for Server {
     fn from(config: ServerConfig) -> Self {
-        Self::new(config)
+        Self::from(Arc::new(config))
     }
 }
 
+// TODO this should be removed after removing deprecated re-exports
 impl From<Arc<ServerConfig>> for Server {
     fn from(config: Arc<ServerConfig>) -> Self {
         Self { config }
@@ -82,6 +85,7 @@ pub struct Builder {
     cert_resolver: Option<Arc<dyn rustls::server::ResolvesServerCert>>,
     application_protocols: Vec<Vec<u8>>,
     key_log: Option<Arc<dyn rustls::KeyLog>>,
+    prefer_server_cipher_suite_order: bool,
 }
 
 impl Default for Builder {
@@ -96,43 +100,53 @@ impl Builder {
             cert_resolver: None,
             application_protocols: vec![b"h3".to_vec()],
             key_log: None,
+            prefer_server_cipher_suite_order: true,
         }
     }
 
     pub fn with_certificate<C: certificate::IntoCertificate, PK: certificate::IntoPrivateKey>(
-        self,
+        mut self,
         certificate: C,
         private_key: PK,
-    ) -> Result<Self, rustls::Error> {
+    ) -> Result<Self, Error> {
         let certificate = certificate.into_certificate()?;
         let private_key = private_key.into_private_key()?;
         let resolver = AlwaysResolvesChain::new(certificate, private_key)?;
         let resolver = Arc::new(resolver);
-        self.with_cert_resolver(resolver)
+        self.cert_resolver = Some(resolver);
+        Ok(self)
     }
 
+    #[deprecated = "client and server builders should be used instead"]
     pub fn with_cert_resolver(
         mut self,
         cert_resolver: Arc<dyn rustls::server::ResolvesServerCert>,
-    ) -> Result<Self, rustls::Error> {
+    ) -> Result<Self, Error> {
         self.cert_resolver = Some(cert_resolver);
         Ok(self)
     }
 
     pub fn with_application_protocols<P: Iterator<Item = I>, I: AsRef<[u8]>>(
         mut self,
         protocols: P,
-    ) -> Result<Self, rustls::Error> {
+    ) -> Result<Self, Error> {
         self.application_protocols = protocols.map(|p| p.as_ref().to_vec()).collect();
         Ok(self)
     }
 
-    pub fn with_key_logging(mut self) -> Result<Self, rustls::Error> {
+    pub fn with_key_logging(mut self) -> Result<Self, Error> {
         self.key_log = Some(Arc::new(rustls::KeyLogFile::new()));
         Ok(self)
     }
 
-    pub fn build(self) -> Result<Server, rustls::Error> {
+    /// If enabled, the cipher suite order of the client is ignored, and the top cipher suite
+    /// in the server list that the client supports is chosen (default: true)
+    pub fn with_prefer_server_cipher_suite_order(mut self, enabled: bool) -> Result<Self, Error> {
+        self.prefer_server_cipher_suite_order = enabled;
+        Ok(self)
+    }
+
+    pub fn build(self) -> Result<Server, Error> {
         let builder = ServerConfig::builder()
             .with_cipher_suites(crate::cipher_suite::DEFAULT_CIPHERSUITES)
             .with_safe_default_kx_groups()
@@ -144,17 +158,19 @@ impl Builder {
         } else {
             return Err(rustls::Error::General(
                 "Missing certificate or certificate resolver".to_string(),
-            ));
+            )
+            .into());
         };
 
-        config.ignore_client_order = true;
+        config.ignore_client_order = self.prefer_server_cipher_suite_order;
         config.max_fragment_size = None;
         config.alpn_protocols = self.application_protocols;
 
         if let Some(key_log) = self.key_log {
             config.key_log = key_log;
         }
 
+        #[allow(deprecated)]
         Ok(Server::new(config))
     }
 }