Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- Rename `Error::InvalidAlpnRepsonse` to correct a typo ([#612](https://github.com/fastly/Viceroy/pull/612))
- Upgrade to Rust 1.95 ([#604](https://github.com/fastly/Viceroy/pull/604))
- Add options for experimenting with wasm gc and exceptions. ([#601](https://github.com/fastly/Viceroy/pull/601))
- Improve TLS certificate loading, handling and validation ([#478](https://github.com/fastly/Viceroy/pull/478))

## 0.16.5 (2026-03-23)

Expand Down
6 changes: 5 additions & 1 deletion src/component/compute/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,11 @@ impl From<error::Error> for types::Error {
| Error::ToStr(_)
| Error::InvalidAlpnResponse { .. }
| Error::DeviceDetectionError(_)
| Error::SharedMemory => types::Error::GenericError,
| Error::SharedMemory
| Error::TlsNoCAAvailable
| Error::TlsNoValidCACerts
| Error::TlsInvalidHost
| Error::TlsCertificateValidationFailed => types::Error::GenericError,
}
}
}
Expand Down
17 changes: 16 additions & 1 deletion src/config/backends/client_cert_info.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}
Expand All @@ -42,14 +44,27 @@ 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)),
_ => {}
}
}

// 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 {
Expand Down
16 changes: 16 additions & 0 deletions src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,18 @@ pub enum Error {
#[error("Could not load native certificates: {0}")]
BadCerts(std::io::Error),

#[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),

Expand Down Expand Up @@ -205,6 +217,10 @@ impl Error {
Error::AbiVersionMismatch
| Error::BackendUrl(_)
| Error::BadCerts(_)
| Error::TlsNoCAAvailable
| Error::TlsNoValidCACerts
| Error::TlsInvalidHost
| Error::TlsCertificateValidationFailed
| Error::DownstreamRequestError(_)
| Error::DownstreamRespSending
| Error::FastlyConfig(_)
Expand Down
66 changes: 50 additions & 16 deletions src/upstream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,18 @@ pub struct TlsConfig {

impl TlsConfig {
pub fn new() -> Result<TlsConfig, Error> {
let certs = rustls_native_certs::load_native_certs().map_err(Error::BadCerts)?;
let mut roots = rustls::RootCertStore::empty();
match rustls_native_certs::load_native_certs() {
Ok(certs) => {
for cert in certs {
if let Err(e) = roots.add(&rustls::Certificate(cert.0)) {
warn!("failed to load certificate: {e}");
}
}
}
Err(err) => return Err(Error::BadCerts(err)),
let (added, failed) =
roots.add_parsable_certificates(&certs.into_iter().map(|c| c.0).collect::<Vec<_>>());
if failed > 0 {
warn!(
"failed to load {} certificate(s). attempting to continue with {} available certificate(s)",
failed, added
);
}
if roots.is_empty() {
warn!("no CA certificates available");
return Err(Error::TlsNoCAAvailable);
}
Comment on lines 50 to 61
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know some of this was pre-existing but I believe we can simplify to:

let certs = rustls_native_certs::load_native_certs().map_err(Error::BadCerts)?;
let mut roots = rustls::RootCertStore::empty();
let (added, failed) = roots.add_parsable_certificates(&certs.into_iter().map(|c| c.0).collect::<Vec<_>>());
if failed > 0 {
    warn!("failed to load {} certificate(s). attempting to continue with {} available certificate(s)", failed, added);
}
if roots.is_empty() {
    return Err(Error::TlsNoCAAvailable);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also means the TlsNoCertsAdded error variant can be removed


let partial_config = rustls::ClientConfig::builder().with_safe_defaults();
Expand Down Expand Up @@ -144,6 +143,11 @@ impl hyper::service::Service<Uri> for BackendConnector {
ignored
);
}
if added == 0 && !self.backend.ca_certs.is_empty() {
return Box::pin(std::future::ready(Err(
Box::new(Error::TlsNoValidCACerts).into()
)));
}
let config = if self.backend.ca_certs.is_empty() {
config
.partial_config
Expand All @@ -154,7 +158,7 @@ impl hyper::service::Service<Uri> for BackendConnector {
};

Box::pin(async move {
let tcp = connect_fut.await.map_err(Box::new)?;
let tcp = connect_fut.await?;

let remote_addr = tcp.peer_addr()?;
let metadata = ConnMetadata {
Expand All @@ -164,7 +168,15 @@ impl hyper::service::Service<Uri> 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())?
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()
};
Expand All @@ -178,10 +190,32 @@ impl hyper::service::Service<Uri> for BackendConnector {
.cert_host
.as_deref()
.or_else(|| backend.uri.host())
.unwrap_or_default();
let dnsname = ServerName::try_from(cert_host).map_err(Box::new)?;

let tls = connector.connect(dnsname, tcp).await.map_err(Box::new)?;
.ok_or(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 = 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") {
Error::TlsCertificateValidationFailed
} else {
Error::IoError(std::io::Error::other(format!(
"TLS connection error: {}",
e
)))
}
})?;

if backend.grpc {
let (_, tls_state) = tls.get_ref();
Expand Down
Loading