Plumb errors through rustls implementation

Author: johnhurt

.bleep

@@ -1 +1 @@
-3919de32fe7a184847dd6ce7da98247ec9e1eb86
+7c7f575484b512ad279084c427adf66069ec2619

pingora-core/Cargo.toml

@@ -68,6 +68,7 @@ tokio-test = "0.4"
 zstd = "0"
 httpdate = "1"
 x509-parser = { version = "0.16.0", optional = true }
+ouroboros = { version = "0.18.4", optional = true }
 
 [target.'cfg(unix)'.dependencies]
 daemonize = "0.5.0"
@@ -92,7 +93,7 @@ jemallocator = "0.5"
 default = []
 openssl = ["pingora-openssl", "openssl_derived",]
 boringssl = ["pingora-boringssl", "openssl_derived",]
-rustls = ["pingora-rustls", "any_tls", "dep:x509-parser"]
+rustls = ["pingora-rustls", "any_tls", "dep:x509-parser", "ouroboros"]
 patched_http1 = ["pingora-http/patched_http1"]
 openssl_derived = ["any_tls"]
 any_tls = []

pingora-core/src/connectors/tls/rustls/mod.rs

@@ -21,8 +21,8 @@ use pingora_error::{
     OrErr, Result,
 };
 use pingora_rustls::{
-    load_ca_file_into_store, load_certs_key_file, load_platform_certs_incl_env_into_store, version,
-    CertificateDer, ClientConfig as RusTlsClientConfig, PrivateKeyDer, RootCertStore,
+    load_ca_file_into_store, load_certs_and_key_files, load_platform_certs_incl_env_into_store,
+    version, CertificateDer, ClientConfig as RusTlsClientConfig, PrivateKeyDer, RootCertStore,
     TlsConnector as RusTlsConnector,
 };
 
@@ -37,18 +37,21 @@ pub struct Connector {
 }
 
 impl Connector {
+    /// Create a new connector based on the optional configurations. If no
+    /// configurations are provided, no customized certificates or keys will be
+    /// used
     pub fn new(config_opt: Option<ConnectorOptions>) -> Self {
-        TlsConnector::build_connector(config_opt)
+        TlsConnector::build_connector(config_opt).unwrap()
     }
 }
 
 pub(crate) struct TlsConnector {
-    config: RusTlsClientConfig,
-    ca_certs: RootCertStore,
+    config: Arc<RusTlsClientConfig>,
+    ca_certs: Arc<RootCertStore>,
 }
 
 impl TlsConnector {
-    pub(crate) fn build_connector(options: Option<ConnectorOptions>) -> Connector
+    pub(crate) fn build_connector(options: Option<ConnectorOptions>) -> Result<Connector>
     where
         Self: Sized,
     {
@@ -65,16 +68,16 @@ impl TlsConnector {
 
             if let Some(conf) = options.as_ref() {
                 if let Some(ca_file_path) = conf.ca_file.as_ref() {
-                    load_ca_file_into_store(ca_file_path, &mut ca_certs);
+                    load_ca_file_into_store(ca_file_path, &mut ca_certs)?;
                 } else {
-                    load_platform_certs_incl_env_into_store(&mut ca_certs);
+                    load_platform_certs_incl_env_into_store(&mut ca_certs)?;
                 }
                 if let Some((cert, key)) = conf.cert_key_file.as_ref() {
-                    certs_key = load_certs_key_file(cert, key);
+                    certs_key = load_certs_and_key_files(cert, key)?;
                 }
                 // TODO: support SSLKEYLOGFILE
             } else {
-                load_platform_certs_incl_env_into_store(&mut ca_certs);
+                load_platform_certs_incl_env_into_store(&mut ca_certs)?;
             }
 
             (ca_certs, certs_key)
@@ -92,19 +95,19 @@ impl TlsConnector {
                     Err(err) => {
                         // TODO: is there a viable alternative to the panic?
                         // falling back to no client auth... does not seem to be reasonable.
-                        panic!(
-                            "{}",
-                            format!("Failed to configure client auth cert/key. Error: {}", err)
-                        );
+                        panic!("Failed to configure client auth cert/key. Error: {}", err);
                     }
                 }
             }
             None => builder.with_no_client_auth(),
         };
 
-        Connector {
-            ctx: Arc::new(TlsConnector { config, ca_certs }),
-        }
+        Ok(Connector {
+            ctx: Arc::new(TlsConnector {
+                config: Arc::new(config),
+                ca_certs: Arc::new(ca_certs),
+            }),
+        })
     }
 }
 
@@ -118,96 +121,81 @@ where
     T: IO,
     P: Peer + Send + Sync,
 {
-    let mut config = tls_ctx.config.clone();
+    let config = &tls_ctx.config;
 
     // TODO: setup CA/verify cert store from peer
-    // looks like the fields are always None
-    // peer.get_ca()
-
+    // peer.get_ca() returns None by default. It must be replaced by the
+    // implementation of `peer`
     let key_pair = peer.get_client_cert_key();
-    let updated_config: Option<RusTlsClientConfig> = match key_pair {
+    let mut updated_config_opt: Option<RusTlsClientConfig> = match key_pair {
         None => None,
         Some(key_arc) => {
             debug!("setting client cert and key");
 
             let mut cert_chain = vec![];
             debug!("adding leaf certificate to mTLS cert chain");
-            cert_chain.push(key_arc.leaf().to_owned());
+            cert_chain.push(key_arc.leaf());
 
             debug!("adding intermediate certificates to mTLS cert chain");
             key_arc
                 .intermediates()
                 .to_owned()
                 .iter()
-                .map(|i| i.to_vec())
+                .copied()
                 .for_each(|i| cert_chain.push(i));
 
-            let certs: Vec<CertificateDer> = cert_chain
-                .into_iter()
-                .map(|c| c.as_slice().to_owned().into())
-                .collect();
+            let certs: Vec<CertificateDer> = cert_chain.into_iter().map(|c| c.into()).collect();
             let private_key: PrivateKeyDer =
                 key_arc.key().as_slice().to_owned().try_into().unwrap();
 
             let builder = RusTlsClientConfig::builder_with_protocol_versions(&[
                 &version::TLS12,
                 &version::TLS13,
             ])
-            .with_root_certificates(tls_ctx.ca_certs.clone());
-
-            let updated_config = builder
-                .with_client_auth_cert(certs, private_key)
-                .explain_err(InvalidCert, |e| {
-                    format!(
-                        "Failed to use peer cert/key to update Rustls config: {:?}",
-                        e
-                    )
-                })?;
+            .with_root_certificates(Arc::clone(&tls_ctx.ca_certs));
+            debug!("added root ca certificates");
+
+            let updated_config = builder.with_client_auth_cert(certs, private_key).or_err(
+                InvalidCert,
+                "Failed to use peer cert/key to update Rustls config",
+            )?;
             Some(updated_config)
         }
     };
 
     if let Some(alpn) = alpn_override.as_ref().or(peer.get_alpn()) {
-        config.alpn_protocols = alpn.to_wire_protocols();
+        let alpn_protocols = alpn.to_wire_protocols();
+        if let Some(updated_config) = updated_config_opt.as_mut() {
+            updated_config.alpn_protocols = alpn_protocols;
+        } else {
+            let mut updated_config = RusTlsClientConfig::clone(config);
+            updated_config.alpn_protocols = alpn_protocols;
+            updated_config_opt = Some(updated_config);
+        }
     }
 
     // TODO: curve setup from peer
     // - second key share from peer, currently only used in boringssl with PQ features
 
-    let tls_conn = if let Some(cfg) = updated_config {
+    let tls_conn = if let Some(cfg) = updated_config_opt {
         RusTlsConnector::from(Arc::new(cfg))
     } else {
-        RusTlsConnector::from(Arc::new(config))
+        RusTlsConnector::from(Arc::clone(config))
     };
 
-    // TODO: for consistent behaviour between TLS providers some additions are required
+    // TODO: for consistent behavior between TLS providers some additions are required
     // - allowing to disable verification
-    // - the validation/replace logic would need adjustments to match the boringssl/openssl behaviour
-    //   implementing a custom certificate_verifier could be used to achieve matching behaviour
+    // - the validation/replace logic would need adjustments to match the boringssl/openssl behavior
+    //   implementing a custom certificate_verifier could be used to achieve matching behavior
     //let d_conf = config.dangerous();
     //d_conf.set_certificate_verifier(...);
 
     let mut domain = peer.sni().to_string();
-    if peer.sni().is_empty() {
-        // use ip in case SNI is not present
-        // TODO: disable validation
-        domain = peer.address().as_inet().unwrap().ip().to_string()
-    }
-
     if peer.verify_cert() && peer.verify_hostname() {
         // TODO: streamline logic with replacing first underscore within TLS implementations
         if let Some(sni_s) = replace_leftmost_underscore(peer.sni()) {
             domain = sni_s;
         }
-        if let Some(alt_cn) = peer.alternative_cn() {
-            if !alt_cn.is_empty() {
-                domain = alt_cn.to_string();
-                // TODO: streamline logic with replacing first underscore within TLS implementations
-                if let Some(alt_cn_s) = replace_leftmost_underscore(alt_cn) {
-                    domain = alt_cn_s;
-                }
-            }
-        }
     }
 
     let connect_future = handshake(&tls_conn, &domain, stream);

pingora-core/src/listeners/tls/rustls/mod.rs

@@ -18,8 +18,8 @@ use crate::listeners::TlsAcceptCallbacks;
 use crate::protocols::tls::{server::handshake, server::handshake_with_callback, TlsStream};
 use log::debug;
 use pingora_error::ErrorType::InternalError;
-use pingora_error::{Error, ErrorSource, ImmutStr, OrErr, Result};
-use pingora_rustls::load_certs_key_file;
+use pingora_error::{Error, OrErr, Result};
+use pingora_rustls::load_certs_and_key_files;
 use pingora_rustls::ServerConfig;
 use pingora_rustls::{version, TlsAcceptor as RusTlsAcceptor};
 
@@ -38,21 +38,29 @@ pub struct Acceptor {
 }
 
 impl TlsSettings {
+    /// Create a Rustls acceptor based on the current setting for certificates,
+    /// keys, and protocols.
+    ///
+    /// _NOTE_ This function will panic if there is an error in loading
+    /// certificate files or constructing the builder
+    ///
+    /// Todo: Return a result instead of panicking XD
     pub fn build(self) -> Acceptor {
-        let (certs, key) =
-            load_certs_key_file(&self.cert_path, &self.key_path).unwrap_or_else(|| {
-                panic!(
-                    "Failed to load provided certificates \"{}\" or key \"{}\".",
-                    self.cert_path, self.key_path
-                )
-            });
+        let Ok(Some((certs, key))) = load_certs_and_key_files(&self.cert_path, &self.key_path)
+        else {
+            panic!(
+                "Failed to load provided certificates \"{}\" or key \"{}\".",
+                self.cert_path, self.key_path
+            )
+        };
 
+        // TODO - Add support for client auth & custom CA support
         let mut config =
             ServerConfig::builder_with_protocol_versions(&[&version::TLS12, &version::TLS13])
                 .with_no_client_auth()
                 .with_single_cert(certs, key)
                 .explain_err(InternalError, |e| {
-                    format!("Failed to create server listener config: {}", e)
+                    format!("Failed to create server listener config: {e}")
                 })
                 .unwrap();
 
@@ -92,14 +100,10 @@ impl TlsSettings {
         Self: Sized,
     {
         // TODO: verify if/how callback in handshake can be done using Rustls
-        Err(Error::create(
+        Error::e_explain(
             InternalError,
-            ErrorSource::Internal,
-            Some(ImmutStr::from(
-                "Certificate callbacks are not supported with feature \"rustls\".",
-            )),
-            None,
-        ))
+            "Certificate callbacks are not supported with feature \"rustls\".",
+        )
     }
 }
 

pingora-core/src/protocols/tls/rustls/client.rs

@@ -28,9 +28,7 @@ pub async fn handshake<S: IO>(
 ) -> Result<TlsStream<S>> {
     let mut stream = TlsStream::from_connector(connector, domain, io)
         .await
-        .explain_err(TLSHandshakeFailure, |e| {
-            format!("tip: tls stream error: {e}")
-        })?;
+        .or_err(TLSHandshakeFailure, "tls stream error")?;
 
     let handshake_result = stream.connect().await;
     match handshake_result {

pingora-core/src/protocols/tls/rustls/mod.rs

@@ -17,20 +17,9 @@ pub mod server;
 mod stream;
 
 pub use stream::*;
-use x509_parser::prelude::FromDer;
 
-pub type CaType = [Box<CertWrapper>];
+use crate::utils::tls::WrappedX509;
 
-#[derive(Debug)]
-#[repr(transparent)]
-pub struct CertWrapper(pub [u8]);
-
-impl CertWrapper {
-    pub fn not_after(&self) -> String {
-        let (_, x509cert) = x509_parser::certificate::X509Certificate::from_der(&self.0)
-            .expect("Failed to parse certificate from DER format.");
-        x509cert.validity.not_after.to_string()
-    }
-}
+pub type CaType = [WrappedX509];
 
 pub struct TlsRef;

pingora-core/src/protocols/tls/rustls/server.rs

@@ -80,10 +80,9 @@ pub async fn handshake_with_callback<S: IO>(
             .resume_accept()
             .await
             .explain_err(TLSHandshakeFailure, |e| format!("TLS accept() failed: {e}"))?;
-        Ok(tls_stream)
-    } else {
-        Ok(tls_stream)
     }
+
+    Ok(tls_stream)
 }
 
 #[async_trait]