From 111d5359f361d0b6b87c0715ec1f25ea55c7c2aa Mon Sep 17 00:00:00 2001 From: Matt Mastracci Date: Tue, 25 Feb 2025 09:45:26 -0700 Subject: [PATCH] Fix macOS system validator w/self-signed certs (#387) When using the system validator on macOS, self-signed certs will fail with -67901 ("The validity period in the certificate exceeds the maximum allowed"). If we have additional roots, we instead want to validate them with the webpki validator. If we have no additional roots, convert that error into UnknownIssuer as no macOS system root should ever issue invalid certs. We may want to move to the system validator at some point but we'll need to make sure we don't subtly break user installations. --- gel-stream/src/common/rustls.rs | 228 ++++++++++++++++++++++++++++---- gel-stream/tests/tls.rs | 60 ++++++++- 2 files changed, 262 insertions(+), 26 deletions(-) diff --git a/gel-stream/src/common/rustls.rs b/gel-stream/src/common/rustls.rs index 9261eb2f..fbb0b6b3 100644 --- a/gel-stream/src/common/rustls.rs +++ b/gel-stream/src/common/rustls.rs @@ -229,6 +229,24 @@ impl TlsDriver for RustlsDriver { } } +fn make_roots( + root_certs: &[CertificateDer<'static>], + webpki: bool, +) -> Result { + let mut roots = RootCertStore::empty(); + if webpki { + let webpki_roots = webpki_roots::TLS_SERVER_ROOTS; + roots.extend(webpki_roots.iter().cloned()); + } + let (loaded, ignored) = roots.add_parsable_certificates(root_certs.iter().cloned()); + if !root_certs.is_empty() && (loaded == 0 || ignored > 0) { + return Err( + std::io::Error::new(std::io::ErrorKind::InvalidInput, "Invalid certificate").into(), + ); + } + Ok(roots) +} + fn make_verifier( server_cert_verify: &TlsServerCertVerify, root_cert: &TlsCert, @@ -242,22 +260,12 @@ fn make_verifier( root_cert, TlsCert::Webpki | TlsCert::WebpkiPlus(_) | TlsCert::Custom(_) ) { - let mut roots = RootCertStore::empty(); - if matches!(root_cert, TlsCert::Webpki | TlsCert::WebpkiPlus(_)) { - let webpki_roots = webpki_roots::TLS_SERVER_ROOTS; - roots.extend(webpki_roots.iter().cloned()); - } - - if let TlsCert::Custom(root) = root_cert { - let (loaded, ignored) = roots.add_parsable_certificates(root.iter().cloned()); - if loaded == 0 || ignored > 0 { - return Err(std::io::Error::new( - std::io::ErrorKind::InvalidInput, - "Invalid certificate", - ) - .into()); - } - } + let roots = match root_cert { + TlsCert::Webpki => make_roots(&[], true), + TlsCert::Custom(roots) => make_roots(roots, false), + TlsCert::WebpkiPlus(roots) => make_roots(roots, true), + _ => unreachable!(), + }?; let verifier = WebPkiServerVerifier::builder(Arc::new(roots)) .with_crls(crls) @@ -268,17 +276,25 @@ fn make_verifier( return Ok(verifier); } - let verifier = if let TlsCert::SystemPlus(roots) = root_cert { - Verifier::new_with_extra_roots(roots.iter().cloned())? + // We need to work around macOS returning `certificate is not standards compliant: -67901` + // when using the system verifier. + let verifier: Arc = if let TlsCert::SystemPlus(roots) = root_cert { + let roots = make_roots(roots, false)?; + let v1 = WebPkiServerVerifier::builder(Arc::new(roots)) + .with_crls(crls) + .build()?; + let v2 = Arc::new(Verifier::new()); + Arc::new(ChainingVerifier::new(v1, v2)) } else { - Verifier::new() + Arc::new(ErrorFilteringVerifier::new(Arc::new(Verifier::new()))) }; - let verifier = if *server_cert_verify == TlsServerCertVerify::IgnoreHostname { - Arc::new(IgnoreHostnameVerifier::new(Arc::new(verifier))) as Arc - } else { - Arc::new(verifier) - }; + let verifier: Arc = + if *server_cert_verify == TlsServerCertVerify::IgnoreHostname { + Arc::new(IgnoreHostnameVerifier::new(verifier)) + } else { + verifier + }; Ok(verifier) } @@ -335,6 +351,98 @@ impl ServerCertVerifier for IgnoreHostnameVerifier { } } +#[derive(Debug)] +struct ChainingVerifier { + verifier1: Arc, + verifier2: Arc, +} + +impl ChainingVerifier { + fn new(verifier1: Arc, verifier2: Arc) -> Self { + Self { + verifier1, + verifier2, + } + } +} + +impl ServerCertVerifier for ChainingVerifier { + fn verify_server_cert( + &self, + end_entity: &CertificateDer<'_>, + intermediates: &[CertificateDer<'_>], + server_name: &ServerName, + ocsp_response: &[u8], + now: UnixTime, + ) -> Result { + let res = self.verifier1.verify_server_cert( + end_entity, + intermediates, + server_name, + ocsp_response, + now, + ); + if let Ok(res) = res { + return Ok(res); + } + + let res2 = self.verifier2.verify_server_cert( + end_entity, + intermediates, + server_name, + ocsp_response, + now, + ); + if let Ok(res) = res2 { + return Ok(res); + } + + res + } + + fn verify_tls12_signature( + &self, + message: &[u8], + cert: &CertificateDer<'_>, + dss: &DigitallySignedStruct, + ) -> Result { + let res = self.verifier1.verify_tls12_signature(message, cert, dss); + if let Ok(res) = res { + return Ok(res); + } + + let res2 = self.verifier2.verify_tls12_signature(message, cert, dss); + if let Ok(res) = res2 { + return Ok(res); + } + + res + } + + fn verify_tls13_signature( + &self, + message: &[u8], + cert: &CertificateDer<'_>, + dss: &DigitallySignedStruct, + ) -> Result { + let res = self.verifier1.verify_tls13_signature(message, cert, dss); + if let Ok(res) = res { + return Ok(res); + } + + let res2 = self.verifier2.verify_tls13_signature(message, cert, dss); + if let Ok(res) = res2 { + return Ok(res); + } + + res + } + + fn supported_verify_schemes(&self) -> Vec { + self.verifier1.supported_verify_schemes() + } +} + #[derive(Debug)] struct NullVerifier; @@ -387,3 +495,75 @@ impl ServerCertVerifier for NullVerifier { ] } } + +#[derive(Debug)] +struct ErrorFilteringVerifier { + verifier: Arc, +} + +impl ErrorFilteringVerifier { + fn new(verifier: Arc) -> Self { + Self { verifier } + } + + fn filter_err(res: Result) -> Result { + match res { + Ok(res) => Ok(res), + // On macOS, the system verifier returns `certificate is not + // standards compliant: -67901` for self-signed certificates that + // have too long of a validity period. It's probably better if we + // eventually have the WebPki verifier handle certs as a fallback to + // ensure a better error is returned. + #[cfg(target_vendor = "apple")] + Err(rustls::Error::InvalidCertificate(rustls::CertificateError::Other(e))) + if e.to_string().contains("-67901") => + { + Err(rustls::Error::InvalidCertificate( + rustls::CertificateError::UnknownIssuer, + )) + } + Err(e) => Err(e), + } + } +} + +impl ServerCertVerifier for ErrorFilteringVerifier { + fn verify_server_cert( + &self, + end_entity: &CertificateDer<'_>, + intermediates: &[CertificateDer<'_>], + server_name: &ServerName, + ocsp_response: &[u8], + now: UnixTime, + ) -> Result { + Self::filter_err(self.verifier.verify_server_cert( + end_entity, + intermediates, + server_name, + ocsp_response, + now, + )) + } + + fn verify_tls12_signature( + &self, + message: &[u8], + cert: &CertificateDer<'_>, + dss: &DigitallySignedStruct, + ) -> Result { + Self::filter_err(self.verifier.verify_tls12_signature(message, cert, dss)) + } + + fn verify_tls13_signature( + &self, + message: &[u8], + cert: &CertificateDer<'_>, + dss: &DigitallySignedStruct, + ) -> Result { + Self::filter_err(self.verifier.verify_tls13_signature(message, cert, dss)) + } + + fn supported_verify_schemes(&self) -> Vec { + self.verifier.supported_verify_schemes() + } +} diff --git a/gel-stream/tests/tls.rs b/gel-stream/tests/tls.rs index 5b4e3d3b..bb0e0170 100644 --- a/gel-stream/tests/tls.rs +++ b/gel-stream/tests/tls.rs @@ -201,7 +201,6 @@ macro_rules! tls_test ( tls_test! { /// The certificate is not valid for 127.0.0.1, so the connection should fail. - #[cfg(not(target_vendor = "apple"))] #[tokio::test] #[ntest::timeout(30_000)] async fn test_target_tcp_tls_verify_full_fails() -> Result<(), ConnectionError> { @@ -229,6 +228,35 @@ tls_test! { Ok(()) } + /// The certificate is not valid for 127.0.0.1, so the connection should fail. + #[tokio::test] + #[ntest::timeout(30_000)] + async fn test_target_tcp_tls_verify_full_fails_webpki() -> Result<(), ConnectionError> { + let (addr, accept_task) = + spawn_tls_server::(None, TlsAlpn::default(), None, TlsClientCertVerify::Ignore).await?; + + let connect_task = tokio::spawn(async move { + let target = Target::new_resolved_tls( + addr, // Raw IP + TlsParameters { + root_cert: TlsCert::Webpki, + ..Default::default() + }, + ); + let stm = Connector::::new_explicit(target).unwrap().connect().await; + assert!( + matches!(&stm, Err(ConnectionError::SslError(ssl)) if ssl.common_error() == Some(CommonError::InvalidIssuer)), + "{stm:?}" + ); + Ok::<_, std::io::Error>(()) + }); + + accept_task.await.unwrap().unwrap_err(); + connect_task.await.unwrap().unwrap(); + + Ok(()) + } + /// The certificate is not valid for 127.0.0.1, so the connection should fail. #[tokio::test] #[ntest::timeout(30_000)] @@ -258,6 +286,35 @@ tls_test! { Ok(()) } + /// The certificate is not valid for 127.0.0.1, so the connection should fail. + #[tokio::test] + #[ntest::timeout(30_000)] + async fn test_target_tcp_tls_verify_full_fails_name_system_plus() -> Result<(), ConnectionError> { + let (addr, accept_task) = + spawn_tls_server::(None, TlsAlpn::default(), None, TlsClientCertVerify::Ignore).await?; + + let connect_task = tokio::spawn(async move { + let target = Target::new_resolved_tls( + addr, // Raw IP + TlsParameters { + root_cert: TlsCert::SystemPlus(vec![load_test_ca()]), + ..Default::default() + }, + ); + let stm = Connector::::new_explicit(target).unwrap().connect().await; + assert!( + matches!(&stm, Err(ConnectionError::SslError(ssl)) if ssl.common_error() == Some(CommonError::InvalidCertificateForName)), + "{stm:?}" + ); + Ok::<_, std::io::Error>(()) + }); + + accept_task.await.unwrap().unwrap_err(); + connect_task.await.unwrap().unwrap(); + + Ok(()) + } + /// The certificate is valid for "localhost", so the connection should succeed. #[tokio::test] #[ntest::timeout(30_000)] @@ -291,7 +348,6 @@ tls_test! { } /// The certificate is valid for "localhost", so the connection should succeed. - #[cfg(not(target_vendor = "apple"))] #[tokio::test] #[ntest::timeout(30_000)] async fn test_target_tcp_tls_verify_full_addl_certs_ok() -> Result<(), ConnectionError> {