diff --git a/crates/uv-client/src/tls.rs b/crates/uv-client/src/tls.rs index 4f483712892d7..132d2fde9364e 100644 --- a/crates/uv-client/src/tls.rs +++ b/crates/uv-client/src/tls.rs @@ -41,22 +41,71 @@ pub(crate) struct DiagnosticCertificate(CertificateDer<'static>); impl DiagnosticCertificate { fn parse(&self) -> Option> { - let (_, certificate) = X509Certificate::from_der(self.0.as_ref()).ok()?; - Some(certificate) + match X509Certificate::from_der(self.0.as_ref()) { + Ok((_, certificate)) => Some(certificate), + Err(err) => { + debug!("Failed to parse certificate for improved validation message: {err:?}"); + None + } + } } } #[derive(Debug)] -pub(crate) enum TlsConfigurationError { - UnsupportedCriticalExtension { - source: CertificateSource, - certificate: DiagnosticCertificate, - }, - InvalidTrustAnchor { - source: CertificateSource, - certificate: DiagnosticCertificate, - error: WebPkiError, - }, +pub(crate) struct TlsConfigurationError { + source: CertificateSource, + certificate: DiagnosticCertificate, + reason: InvalidTrustAnchorReason, +} + +#[derive(Debug)] +pub(crate) enum InvalidTrustAnchorReason { + UnsupportedCriticalExtension, + BadDer, + BadDerTime, + EmptyEkuExtension, + ExtensionValueInvalid, + MalformedExtensions, + TrailingData, + UnsupportedCertVersion, + Other(WebPkiError), +} + +impl InvalidTrustAnchorReason { + fn from_webpki_error(error: WebPkiError) -> Self { + match error { + WebPkiError::UnsupportedCriticalExtension => Self::UnsupportedCriticalExtension, + WebPkiError::BadDer => Self::BadDer, + WebPkiError::BadDerTime => Self::BadDerTime, + WebPkiError::EmptyEkuExtension => Self::EmptyEkuExtension, + WebPkiError::ExtensionValueInvalid => Self::ExtensionValueInvalid, + WebPkiError::MalformedExtensions => Self::MalformedExtensions, + WebPkiError::TrailingData(_) => Self::TrailingData, + WebPkiError::UnsupportedCertVersion => Self::UnsupportedCertVersion, + error => Self::Other(error), + } + } + + fn message(&self) -> Option<&'static str> { + match self { + Self::UnsupportedCriticalExtension => None, + Self::BadDer => Some("malformed DER certificate"), + Self::BadDerTime => Some("malformed certificate time"), + Self::EmptyEkuExtension => Some("empty extended key usage extension"), + Self::ExtensionValueInvalid => Some("invalid certificate extension value"), + Self::MalformedExtensions => Some("malformed certificate extensions"), + Self::TrailingData => Some("trailing data in DER certificate"), + Self::UnsupportedCertVersion => Some("unsupported certificate version"), + Self::Other(_) => None, + } + } + + fn error(&self) -> Option<&WebPkiError> { + match self { + Self::Other(error) => Some(error), + _ => None, + } + } } impl TlsConfigurationError { @@ -65,88 +114,90 @@ impl TlsConfigurationError { error: WebPkiError, cert: &CertificateDer<'_>, ) -> Self { - let certificate = DiagnosticCertificate(cert.clone().into_owned()); - match error { - WebPkiError::UnsupportedCriticalExtension => Self::UnsupportedCriticalExtension { - source, - certificate, - }, - error => Self::InvalidTrustAnchor { - source, - certificate, - error, - }, + Self { + source, + certificate: DiagnosticCertificate(cert.clone().into_owned()), + reason: InvalidTrustAnchorReason::from_webpki_error(error), } } } +fn format_trust_anchor_detail( + reason: &InvalidTrustAnchorReason, + certificate: Option<&X509Certificate<'_>>, +) -> Option { + match reason { + InvalidTrustAnchorReason::UnsupportedCertVersion => certificate.map(|certificate| { + format!( + "unsupported certificate version `{}`", + certificate.version() + ) + }), + InvalidTrustAnchorReason::ExtensionValueInvalid => None, + _ => None, + } +} + impl Display for TlsConfigurationError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - match self { - Self::UnsupportedCriticalExtension { - source, - certificate, - } => { - write!( - f, - "certificate in `{}` (from `{}`) uses an unsupported critical extension", - source.path().simplified_display(), - source.env_var() - )?; - if let Some(certificate) = certificate.parse() { - let subject = certificate.subject(); - if subject.iter_attributes().next().is_some() { - write!(f, " on certificate `{subject}`")?; - } - let critical_extensions = certificate - .iter_extensions() - .filter(|extension| extension.critical) - .map(|extension| extension.oid.to_owned()) - .collect::>(); - if let [critical_extension] = critical_extensions.as_slice() { - write!(f, "; critical extension: `{critical_extension}`")?; - } else if !critical_extensions.is_empty() { - write!( - f, - "; critical extensions: {}", - critical_extensions - .iter() - .map(|oid| format!("`{oid}`")) - .join(", ") - )?; - } - } - Ok(()) + write!( + f, + "certificate in `{}` (from `{}`) ", + self.source.path().simplified_display(), + self.source.env_var() + )?; + match &self.reason { + InvalidTrustAnchorReason::UnsupportedCriticalExtension => { + write!(f, "uses an unsupported critical extension")?; + } + _ => { + write!(f, "could not be used as a trust anchor")?; } - Self::InvalidTrustAnchor { - source, - certificate, - .. - } => { - write!( - f, - "certificate in `{}` (from `{}`) could not be used as a trust anchor", - source.path().simplified_display(), - source.env_var() - )?; - if let Some(certificate) = certificate.parse() { - let subject = certificate.subject(); - if subject.iter_attributes().next().is_some() { - write!(f, " on certificate `{subject}`")?; - } + } + + let parsed_certificate = self.certificate.parse(); + if let Some(certificate) = parsed_certificate.as_ref() { + let subject = certificate.subject(); + if subject.iter_attributes().next().is_some() { + // Avoid rendering empty subject DNs. + write!(f, " on certificate `{subject}`")?; + } + if let InvalidTrustAnchorReason::UnsupportedCriticalExtension = &self.reason { + let critical_extensions = certificate + .iter_extensions() + .filter(|extension| extension.critical) + .map(|extension| extension.oid.to_owned()) + .collect::>(); + if let [critical_extension] = critical_extensions.as_slice() { + write!(f, "; critical extension: `{critical_extension}`")?; + } else if !critical_extensions.is_empty() { + write!( + f, + "; critical extensions: {}", + critical_extensions + .iter() + .map(|oid| format!("`{oid}`")) + .join(", ") + )?; } - Ok(()) } } + + let detailed_reason = format_trust_anchor_detail(&self.reason, parsed_certificate.as_ref()) + .or_else(|| self.reason.message().map(str::to_owned)); + if let Some(detailed_reason) = detailed_reason { + write!(f, ": {detailed_reason}")?; + } + + Ok(()) } } impl std::error::Error for TlsConfigurationError { fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { - match self { - Self::UnsupportedCriticalExtension { .. } => None, - Self::InvalidTrustAnchor { error, .. } => Some(error), - } + self.reason + .error() + .map(|error| error as &(dyn std::error::Error + 'static)) } } diff --git a/crates/uv-client/tests/it/ssl_certs.rs b/crates/uv-client/tests/it/ssl_certs.rs index be874aa706568..dd714ee57d274 100644 --- a/crates/uv-client/tests/it/ssl_certs.rs +++ b/crates/uv-client/tests/it/ssl_certs.rs @@ -509,7 +509,7 @@ Caused by: certificate in `[TMP]/ca.pem` (from `SSL_CERT_FILE`) uses an unsuppor } /// An invalid trust anchor in `SSL_CERT_FILE` returns a builder error with a -/// generic trust-anchor message. +/// more specific validation message. #[tokio::test] async fn test_ssl_cert_file_invalid_trust_anchor_returns_error() -> Result<()> { let cert = TestCertificate::new_with_duplicate_basic_constraints_ca_extension()?; @@ -524,16 +524,14 @@ async fn test_ssl_cert_file_invalid_trust_anchor_returns_error() -> Result<()> { .expect_err("expected client build to fail"); let source = err.source().expect("expected client build error source"); - let next_source = source.source().expect("expected trust anchor validation cause"); - let display = format!("{err}\nCaused by: {source}\nCaused by: {next_source}"); + let display = format!("{err}\nCaused by: {source}"); with_settings!({ filters => vec![(temp_dir_filter.as_str(), "[TMP]/")] }, { assert_snapshot!(display, @r#" failed to build HTTP client -Caused by: certificate in `[TMP]/ca.pem` (from `SSL_CERT_FILE`) could not be used as a trust anchor on certificate `CN=uv-test-ca, O=Astral Software Inc.` -Caused by: ExtensionValueInvalid +Caused by: certificate in `[TMP]/ca.pem` (from `SSL_CERT_FILE`) could not be used as a trust anchor on certificate `CN=uv-test-ca, O=Astral Software Inc.`: invalid certificate extension value "#); }); })