From db9c299aad2d55a0c39b30da4a703e725845ecfd Mon Sep 17 00:00:00 2001 From: Joe Birr-Pixton Date: Tue, 28 Jan 2025 11:50:59 +0000 Subject: [PATCH] Improve `fmt::Display` for common error variants --- src/error.rs | 167 +++++++++++++++++++++++++++++++++++++- tests/tls_server_certs.rs | 9 +- 2 files changed, 174 insertions(+), 2 deletions(-) diff --git a/src/error.rs b/src/error.rs index fb10e758..b2e0aae7 100644 --- a/src/error.rs +++ b/src/error.rs @@ -316,7 +316,75 @@ impl From for ControlFlow { impl fmt::Display for Error { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{:?}", self) + match self { + Self::CertExpired { time, not_after } => write!( + f, + "certificate expired: current time is {}, \ + but certificate is not valid after {} \ + ({} seconds ago)", + time.as_secs(), + not_after.as_secs(), + time.as_secs().saturating_sub(not_after.as_secs()) + ), + + Self::CertNotValidYet { time, not_before } => write!( + f, + "certificate not valid yet: current time is {}, \ + but certificate is not valid before {} \ + ({} seconds in future)", + time.as_secs(), + not_before.as_secs(), + not_before.as_secs().saturating_sub(time.as_secs()) + ), + + Self::CrlExpired { time, next_update } => write!( + f, + "certificate revocation list expired: \ + current time is {}, \ + but CRL is not valid after {} \ + ({} seconds ago)", + time.as_secs(), + next_update.as_secs(), + time.as_secs().saturating_sub(next_update.as_secs()) + ), + + #[cfg(feature = "alloc")] + Self::CertNotValidForName(InvalidNameContext { + expected, + presented, + }) => { + write!( + f, + "certificate not valid for name: {:?} is required, but certificate ", + expected.to_str() + )?; + + match presented.as_slice() { + &[] => write!( + f, + "is not valid for any names (according to its subjectAltName extension)" + ), + &[ref one] => write!(f, "is only valid for {}", one), + many => { + write!(f, "is only valid for ")?; + + let n = many.len(); + let all_but_last = &many[..n - 1]; + let last = &many[n - 1]; + + for (i, name) in all_but_last.into_iter().enumerate() { + write!(f, "{}", name)?; + if i < n - 2 { + write!(f, ", ")?; + } + } + write!(f, " or {}", last) + } + } + } + + other => write!(f, "{:?}", other), + } } } @@ -371,3 +439,100 @@ pub enum DerTypeId { RevokedCertEntry, IssuingDistributionPoint, } + +#[cfg(test)] +mod tests { + use super::*; + use std::string::ToString; + use std::time::Duration; + + #[test] + fn error_display() { + assert_eq!( + "certificate expired: current time is 320, \ + but certificate is not valid after 300 (20 seconds ago)", + Error::CertExpired { + time: UnixTime::since_unix_epoch(Duration::from_secs(320)), + not_after: UnixTime::since_unix_epoch(Duration::from_secs(300)), + } + .to_string(), + ); + + assert_eq!( + "certificate not valid yet: current time is 300, \ + but certificate is not valid before 320 (20 seconds in future)", + Error::CertNotValidYet { + time: UnixTime::since_unix_epoch(Duration::from_secs(300)), + not_before: UnixTime::since_unix_epoch(Duration::from_secs(320)), + } + .to_string(), + ); + + assert_eq!( + "certificate revocation list expired: current time \ + is 320, but CRL is not valid after 300 (20 seconds ago)", + Error::CrlExpired { + time: UnixTime::since_unix_epoch(Duration::from_secs(320)), + next_update: UnixTime::since_unix_epoch(Duration::from_secs(300)), + } + .to_string(), + ); + + // name errors + #[cfg(feature = "alloc")] + { + assert_eq!( + "certificate not valid for name: \"example.com\" is required, \ + but certificate is not valid for any names (according to its \ + subjectAltName extension)", + Error::CertNotValidForName(InvalidNameContext { + expected: "example.com".try_into().unwrap(), + presented: vec![], + }) + .to_string(), + ); + + assert_eq!( + "certificate not valid for name: \"example.com\" is required, \ + but certificate is only valid for DnsName(\"foo.com\")", + Error::CertNotValidForName(InvalidNameContext { + expected: "example.com".try_into().unwrap(), + presented: vec!["DnsName(\"foo.com\")".to_string(),], + }) + .to_string(), + ); + + assert_eq!( + "certificate not valid for name: \"example.com\" is required, \ + but certificate is only valid for DnsName(\"foo.com\") \ + or DnsName(\"cowboy.org\")", + Error::CertNotValidForName(InvalidNameContext { + expected: "example.com".try_into().unwrap(), + presented: vec![ + "DnsName(\"foo.com\")".to_string(), + "DnsName(\"cowboy.org\")".to_string(), + ], + }) + .to_string(), + ); + + assert_eq!( + "certificate not valid for name: \"example.com\" is required, \ + but certificate is only valid for DnsName(\"foo.com\"), \ + DnsName(\"cowboy.org\") or IpAddress(127.0.0.1)", + Error::CertNotValidForName(InvalidNameContext { + expected: "example.com".try_into().unwrap(), + presented: vec![ + "DnsName(\"foo.com\")".to_string(), + "DnsName(\"cowboy.org\")".to_string(), + "IpAddress(127.0.0.1)".to_string() + ], + }) + .to_string(), + ); + } + + // other errors (fall back to fmt::Debug) + assert_eq!("BadDerTime", Error::BadDerTime.to_string()); + } +} diff --git a/tests/tls_server_certs.rs b/tests/tls_server_certs.rs index 721861c8..a2cf93d3 100644 --- a/tests/tls_server_certs.rs +++ b/tests/tls_server_certs.rs @@ -50,7 +50,7 @@ fn check_cert( for invalid in invalid_names { let name = ServerName::try_from(*invalid).unwrap(); assert_eq!( - cert.verify_is_valid_for_subject_name(&name), + display_error(cert.verify_is_valid_for_subject_name(&name)), Err(webpki::Error::CertNotValidForName(InvalidNameContext { expected: name.to_owned(), presented: presented_names.iter().map(|n| n.to_string()).collect(), @@ -61,6 +61,13 @@ fn check_cert( Ok(()) } +fn display_error(r: Result<(), webpki::Error>) -> Result<(), webpki::Error> { + if let Some(error) = r.as_ref().err() { + println!("name error: {}", error); + } + r +} + // DO NOT EDIT BELOW: generated by tests/generate.py #[test]