From 609118b249f56ac0f2b99b094ff8706672eed1d2 Mon Sep 17 00:00:00 2001 From: Frank Denis Date: Thu, 15 May 2025 19:14:48 +0200 Subject: [PATCH 1/2] Improve TLS certificate loading, handling and validation 1. Improved Certificate Loading: - Now fails explicitly when no certificates can be loaded - Properly reports when CA certificates are unavailable 2. Enhanced Error Handling: - Added new error types for specific TLS/certificate validation issues - Improved error messages for better diagnostics 3. Better Certificate Validation: - Implemented certificate validation checks during connection establishment - Added basic validation of certificate data to ensure certificates aren't empty - Added proper error handling for invalid DNS names 4. Proper TLS Connection Validation: - Added explicit error handling for TLS connection failures - Added logging for certificate validation failures for easier debugging --- lib/src/component/error.rs | 7 +- lib/src/config/backends/client_cert_info.rs | 17 +++- lib/src/error.rs | 20 ++++ lib/src/upstream.rs | 100 +++++++++++++++++--- 4 files changed, 129 insertions(+), 15 deletions(-) diff --git a/lib/src/component/error.rs b/lib/src/component/error.rs index adeb56f5..f53ded02 100644 --- a/lib/src/component/error.rs +++ b/lib/src/component/error.rs @@ -259,7 +259,12 @@ impl From for types::Error { | Error::InvalidAlpnRepsonse { .. } | Error::DeviceDetectionError(_) | Error::Again - | Error::SharedMemory => types::Error::GenericError, + | Error::SharedMemory + | Error::TlsNoCertsAdded + | Error::TlsNoCAAvailable + | Error::TlsNoValidCACerts + | Error::TlsInvalidHost + | Error::TlsCertificateValidationFailed => types::Error::GenericError, } } } diff --git a/lib/src/config/backends/client_cert_info.rs b/lib/src/config/backends/client_cert_info.rs index 57c71feb..adfb425b 100644 --- a/lib/src/config/backends/client_cert_info.rs +++ b/lib/src/config/backends/client_cert_info.rs @@ -26,6 +26,8 @@ pub enum ClientCertError { InvalidToml, #[error("No certificates found in client cert definition")] NoCertsFound, + #[error("Invalid certificate data: {0}")] + InvalidCertificateData(String), #[error("Expected a string value for key {0}, got something else")] InvalidTomlData(&'static str), } @@ -42,7 +44,15 @@ impl ClientCertInfo { for item in cert_info.into_iter().chain(key_info) { match item { - rustls_pemfile::Item::X509Certificate(x) => certificates.push(Certificate(x)), + rustls_pemfile::Item::X509Certificate(x) => { + // Basic validation of certificate data + if x.is_empty() { + return Err(ClientCertError::InvalidCertificateData( + "Empty certificate data".to_string(), + )); + } + certificates.push(Certificate(x)) + } rustls_pemfile::Item::RSAKey(x) => keys.push(PrivateKey(x)), rustls_pemfile::Item::PKCS8Key(x) => keys.push(PrivateKey(x)), rustls_pemfile::Item::ECKey(x) => keys.push(PrivateKey(x)), @@ -50,6 +60,11 @@ impl ClientCertInfo { } } + // Ensure certificates were found + if certificates.is_empty() { + return Err(ClientCertError::NoCertsFound); + } + let key = if keys.is_empty() { return Err(ClientCertError::NoKeysFound); } else if keys.len() > 1 { diff --git a/lib/src/error.rs b/lib/src/error.rs index 3cdcc6ea..5bc8b86f 100644 --- a/lib/src/error.rs +++ b/lib/src/error.rs @@ -116,6 +116,21 @@ pub enum Error { #[error("Could not load native certificates: {0}")] BadCerts(std::io::Error), + #[error("No valid CA certificates could be added")] + TlsNoCertsAdded, + + #[error("No CA certificates available")] + TlsNoCAAvailable, + + #[error("No valid CA certificates found in provided certificate bundle")] + TlsNoValidCACerts, + + #[error("Invalid or missing host for TLS connection")] + TlsInvalidHost, + + #[error("TLS certificate validation failed")] + TlsCertificateValidationFailed, + #[error("Could not generate new backend name from '{0}'")] BackendNameRegistryError(String), @@ -197,6 +212,11 @@ impl Error { Error::AbiVersionMismatch | Error::BackendUrl(_) | Error::BadCerts(_) + | Error::TlsNoCertsAdded + | Error::TlsNoCAAvailable + | Error::TlsNoValidCACerts + | Error::TlsInvalidHost + | Error::TlsCertificateValidationFailed | Error::DownstreamRequestError(_) | Error::DownstreamRespSending | Error::FastlyConfig(_) diff --git a/lib/src/upstream.rs b/lib/src/upstream.rs index c6705210..613deb18 100644 --- a/lib/src/upstream.rs +++ b/lib/src/upstream.rs @@ -48,16 +48,24 @@ impl TlsConfig { let mut roots = rustls::RootCertStore::empty(); match rustls_native_certs::load_native_certs() { Ok(certs) => { + let mut cert_added = false; for cert in certs { - if let Err(e) = roots.add(&rustls::Certificate(cert.0)) { - warn!("failed to load certificate: {e}"); + match roots.add(&rustls::Certificate(cert.0)) { + Ok(_) => cert_added = true, + Err(e) => { + // Log but continue trying other certs + warn!("failed to load certificate: {e}"); + } } } + if !cert_added { + return Err(Error::TlsNoCertsAdded); + } } Err(err) => return Err(Error::BadCerts(err)), } if roots.is_empty() { - warn!("no CA certificates available"); + return Err(Error::TlsNoCAAvailable); } let partial_config = rustls::ClientConfig::builder().with_safe_defaults(); @@ -143,6 +151,13 @@ impl hyper::service::Service for BackendConnector { ignored ); } + if added == 0 && !self.backend.ca_certs.is_empty() { + return Box::pin(async { + Err(Box::::from(Box::new( + Error::TlsNoValidCACerts, + ))) + }); + } let config = if self.backend.ca_certs.is_empty() { config .partial_config @@ -153,9 +168,26 @@ impl hyper::service::Service for BackendConnector { }; Box::pin(async move { - let tcp = connect_fut.await.map_err(Box::new)?; + let tcp = match connect_fut.await { + Ok(t) => t, + Err(e) => { + return Err(Box::::from(Box::new( + Error::IoError(std::io::Error::new( + std::io::ErrorKind::Other, + format!("TCP connection error: {}", e), + )), + ))) + } + }; - let remote_addr = tcp.peer_addr()?; + let remote_addr = match tcp.peer_addr() { + Ok(addr) => addr, + Err(e) => { + return Err(Box::::from(Box::new( + Error::IoError(e), + ))) + } + }; let metadata = ConnMetadata { direct_pass: false, remote_addr, @@ -163,7 +195,18 @@ impl hyper::service::Service for BackendConnector { let conn = if backend.uri.scheme_str() == Some("https") { let mut config = if let Some(certed_key) = &backend.client_cert { - config.with_client_auth_cert(certed_key.certs(), certed_key.key())? + match config.with_client_auth_cert(certed_key.certs(), certed_key.key()) { + Ok(cfg) => cfg, + Err(_) => { + return Err(Box::::from(Box::new( + Error::InvalidClientCert( + crate::config::ClientCertError::InvalidCertificateData( + "Client certificate validation failed".to_string(), + ), + ), + ))) + } + } } else { config.with_no_client_auth() }; @@ -173,14 +216,45 @@ impl hyper::service::Service for BackendConnector { } let connector = TlsConnector::from(Arc::new(config)); - let cert_host = backend - .cert_host - .as_deref() - .or_else(|| backend.uri.host()) - .unwrap_or_default(); - let dnsname = ServerName::try_from(cert_host).map_err(Box::new)?; + let cert_host = match backend.cert_host.as_deref().or_else(|| backend.uri.host()) { + Some(host) => host, + None => { + return Err(Box::::from(Box::new( + Error::TlsInvalidHost, + ))) + } + }; + + let dnsname = match ServerName::try_from(cert_host) { + Ok(name) => name, + Err(_) => { + let err_msg = format!("Invalid DNS name: {}", cert_host); + tracing::error!("{}", err_msg); + return Err(Box::::from(Box::new( + Error::TlsInvalidHost, + ))); + } + }; - let tls = connector.connect(dnsname, tcp).await.map_err(Box::new)?; + // Connect with proper validation + let tls = match connector.connect(dnsname, tcp).await { + Ok(conn) => conn, + Err(e) => { + // Log detailed error information for certificate issues + tracing::error!("TLS certificate validation failed: {}", e); + if e.to_string().contains("certificate validation failed") { + return Err(Box::::from( + Box::new(Error::TlsCertificateValidationFailed), + )); + } + return Err(Box::::from(Box::new( + Error::IoError(std::io::Error::new( + std::io::ErrorKind::Other, + format!("TLS connection error: {}", e), + )), + ))); + } + }; if backend.grpc { let (_, tls_state) = tls.get_ref(); From b7b90dd4f78115daee1a678c58948dcaea1a53d3 Mon Sep 17 00:00:00 2001 From: Charles Eckman Date: Wed, 28 May 2025 16:08:55 -0400 Subject: [PATCH 2/2] Use Rust error transformations and early-returns (map_err, ?) --- lib/src/upstream.rs | 104 +++++++++++++++++--------------------------- 1 file changed, 39 insertions(+), 65 deletions(-) diff --git a/lib/src/upstream.rs b/lib/src/upstream.rs index 613deb18..a44cbf4a 100644 --- a/lib/src/upstream.rs +++ b/lib/src/upstream.rs @@ -152,11 +152,9 @@ impl hyper::service::Service for BackendConnector { ); } if added == 0 && !self.backend.ca_certs.is_empty() { - return Box::pin(async { - Err(Box::::from(Box::new( - Error::TlsNoValidCACerts, - ))) - }); + return Box::pin(std::future::ready(Err( + Box::new(Error::TlsNoValidCACerts).into() + ))); } let config = if self.backend.ca_certs.is_empty() { config @@ -168,26 +166,14 @@ impl hyper::service::Service for BackendConnector { }; Box::pin(async move { - let tcp = match connect_fut.await { - Ok(t) => t, - Err(e) => { - return Err(Box::::from(Box::new( - Error::IoError(std::io::Error::new( - std::io::ErrorKind::Other, - format!("TCP connection error: {}", e), - )), - ))) - } - }; + let tcp = connect_fut.await.map_err(|e| { + std::io::Error::new( + std::io::ErrorKind::Other, + format!("TCP connection error: {}", e), + ) + })?; - let remote_addr = match tcp.peer_addr() { - Ok(addr) => addr, - Err(e) => { - return Err(Box::::from(Box::new( - Error::IoError(e), - ))) - } - }; + let remote_addr = tcp.peer_addr()?; let metadata = ConnMetadata { direct_pass: false, remote_addr, @@ -195,18 +181,15 @@ impl hyper::service::Service for BackendConnector { let conn = if backend.uri.scheme_str() == Some("https") { let mut config = if let Some(certed_key) = &backend.client_cert { - match config.with_client_auth_cert(certed_key.certs(), certed_key.key()) { - Ok(cfg) => cfg, - Err(_) => { - return Err(Box::::from(Box::new( - Error::InvalidClientCert( - crate::config::ClientCertError::InvalidCertificateData( - "Client certificate validation failed".to_string(), - ), + config + .with_client_auth_cert(certed_key.certs(), certed_key.key()) + .map_err(|_| { + Error::InvalidClientCert( + crate::config::ClientCertError::InvalidCertificateData( + "Client certificate validation failed".to_string(), ), - ))) - } - } + ) + })? } else { config.with_no_client_auth() }; @@ -216,45 +199,36 @@ impl hyper::service::Service for BackendConnector { } let connector = TlsConnector::from(Arc::new(config)); - let cert_host = match backend.cert_host.as_deref().or_else(|| backend.uri.host()) { - Some(host) => host, - None => { - return Err(Box::::from(Box::new( - Error::TlsInvalidHost, - ))) - } - }; + let cert_host = backend + .cert_host + .as_deref() + .or_else(|| backend.uri.host()) + .ok_or(Error::TlsInvalidHost)?; - let dnsname = match ServerName::try_from(cert_host) { - Ok(name) => name, - Err(_) => { - let err_msg = format!("Invalid DNS name: {}", cert_host); - tracing::error!("{}", err_msg); - return Err(Box::::from(Box::new( - Error::TlsInvalidHost, - ))); - } - }; + let dnsname = ServerName::try_from(cert_host).map_err(|_| { + let err_msg = format!("Invalid DNS name: {}", cert_host); + tracing::error!("{}", err_msg); + Error::TlsInvalidHost + })?; // Connect with proper validation - let tls = match connector.connect(dnsname, tcp).await { - Ok(conn) => conn, - Err(e) => { + let tls = connector + .connect(dnsname, tcp) + .await + .inspect_err(|e| { // Log detailed error information for certificate issues tracing::error!("TLS certificate validation failed: {}", e); + }) + .map_err(|e| { if e.to_string().contains("certificate validation failed") { - return Err(Box::::from( - Box::new(Error::TlsCertificateValidationFailed), - )); - } - return Err(Box::::from(Box::new( + Error::TlsCertificateValidationFailed + } else { Error::IoError(std::io::Error::new( std::io::ErrorKind::Other, format!("TLS connection error: {}", e), - )), - ))); - } - }; + )) + } + })?; if backend.grpc { let (_, tls_state) = tls.get_ref();