Improve fmt::Display for common error variants

src/crl/types.rs

@@ -150,7 +150,7 @@ impl CertRevocationList<'_> {
         };
 
         if time >= next_update {
-            return Err(Error::CrlExpired);
+            return Err(Error::CrlExpired { time, next_update });
         }
 
         Ok(())
@@ -1254,8 +1254,10 @@ mod tests {
         let crl = CertRevocationList::from(BorrowedCertRevocationList::from_der(&crl[..]).unwrap());
         //  Friday, February 2, 2024 8:26:19 PM GMT
         let time = UnixTime::since_unix_epoch(Duration::from_secs(1_706_905_579));
-
-        assert!(matches!(crl.check_expiration(time), Err(Error::CrlExpired)));
+        assert!(matches!(
+            crl.check_expiration(time),
+            Err(Error::CrlExpired { .. })
+        ));
     }
 
     #[test]

src/error.rs

@@ -21,6 +21,7 @@ use core::ops::ControlFlow;
 
 #[cfg(feature = "alloc")]
 use pki_types::ServerName;
+use pki_types::UnixTime;
 
 /// An error that occurs during certificate validation or name validation.
 #[derive(Clone, Debug, PartialEq, Eq)]
@@ -37,21 +38,36 @@ pub enum Error {
 
     /// The certificate is expired; i.e. the time it is being validated for is
     /// later than the certificate's notAfter time.
-    CertExpired,
+    CertExpired {
+        /// The validation time.
+        time: UnixTime,
+        /// The notAfter time of the certificate.
+        not_after: UnixTime,
+    },
 
     /// The certificate is not valid for the name it is being validated for.
     CertNotValidForName(InvalidNameContext),
 
     /// The certificate is not valid yet; i.e. the time it is being validated
     /// for is earlier than the certificate's notBefore time.
-    CertNotValidYet,
+    CertNotValidYet {
+        /// The validation time.
+        time: UnixTime,
+        /// The notBefore time of the certificate.
+        not_before: UnixTime,
+    },
 
     /// The certificate, or one of its issuers, has been revoked.
     CertRevoked,
 
     /// The CRL is expired; i.e. the verification time is not before the time
     /// in the CRL nextUpdate field.
-    CrlExpired,
+    CrlExpired {
+        /// The validation time.
+        time: UnixTime,
+        /// The nextUpdate time of the CRL.
+        next_update: UnixTime,
+    },
 
     /// An end-entity certificate is being used as a CA certificate.
     EndEntityUsedAsCa,
@@ -222,9 +238,9 @@ impl Error {
     pub(crate) fn rank(&self) -> u32 {
         match &self {
             // Errors related to certificate validity
-            Self::CertNotValidYet | Self::CertExpired => 290,
+            Self::CertNotValidYet { .. } | Self::CertExpired { .. } => 290,
             Self::CertNotValidForName(_) => 280,
-            Self::CertRevoked | Self::UnknownRevocationStatus | Self::CrlExpired => 270,
+            Self::CertRevoked | Self::UnknownRevocationStatus | Self::CrlExpired { .. } => 270,
             Self::InvalidCrlSignatureForPublicKey | Self::InvalidSignatureForPublicKey => 260,
             Self::SignatureAlgorithmMismatch => 250,
             Self::RequiredEkuNotFound => 240,
@@ -300,7 +316,75 @@ impl From<Error> for ControlFlow<Error, Error> {
 
 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(all(feature = "alloc", feature = "std"))]
+            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)"
+                    ),
+                    [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.iter().enumerate() {
+                            write!(f, "{}", name)?;
+                            if i < n - 2 {
+                                write!(f, ", ")?;
+                            }
+                        }
+                        write!(f, " or {}", last)
+                    }
+                }
+            }
+
+            other => write!(f, "{:?}", other),
+        }
     }
 }
 
@@ -355,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(all(feature = "alloc", feature = "std"))]
+        {
+            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());
+    }
+}

src/verify_cert.rs

@@ -378,10 +378,10 @@ fn check_validity(input: &mut untrusted::Reader<'_>, time: UnixTime) -> Result<(
         return Err(Error::InvalidCertValidity);
     }
     if time < not_before {
-        return Err(Error::CertNotValidYet);
+        return Err(Error::CertNotValidYet { time, not_before });
     }
     if time > not_after {
-        return Err(Error::CertExpired);
+        return Err(Error::CertExpired { time, not_after });
     }
 
     // TODO: mozilla::pkix allows the TrustDomain to check not_before and

tests/client_auth_revocation.rs

@@ -1662,7 +1662,10 @@ fn expired_crl_enforce_expiration() {
     let revocation = Some(builder.build());
     assert_eq!(
         check_cert(ee, intermediates, ca, revocation),
-        Err(webpki::Error::CrlExpired)
+        Err(webpki::Error::CrlExpired {
+            time: UnixTime::since_unix_epoch(Duration::from_secs(0x1fed_f00d)),
+            next_update: UnixTime::since_unix_epoch(Duration::from_secs(0x1fed_f00d - 10)),
+        })
     );
 }
 
@@ -1691,6 +1694,9 @@ fn expired_crl_enforce_expiration_owned() {
     let revocation = Some(builder.build());
     assert_eq!(
         check_cert(ee, intermediates, ca, revocation),
-        Err(webpki::Error::CrlExpired)
+        Err(webpki::Error::CrlExpired {
+            time: UnixTime::since_unix_epoch(Duration::from_secs(0x1fed_f00d)),
+            next_update: UnixTime::since_unix_epoch(Duration::from_secs(0x1fed_f00d - 10)),
+        })
     );
 }

tests/generate.py

@@ -2246,14 +2246,20 @@ def _expired_crl_enforce_expiration() -> None:
         )
 
         # Providing a CRL that's expired should error if the expiration policy is set to enforce.
+        expected_error = """
+        CrlExpired {
+          time: UnixTime::since_unix_epoch(Duration::from_secs(0x1fed_f00d)),
+          next_update: UnixTime::since_unix_epoch(Duration::from_secs(0x1fed_f00d - 10)),
+        }
+        """
         _revocation_test(
             test_name=test_name,
             chain=no_ku_chain,
             crl_paths=[ee_not_revoked_crl_path],
             depth=ChainDepth.CHAIN,
             policy=StatusRequirement.ALLOW_UNKNOWN,
             expiration=ExpirationPolicy.ENFORCE,
-            expected_error="CrlExpired",
+            expected_error=expected_error,
         )
 
     with trim_top("client_auth_revocation.rs") as output:

tests/integration.rs

@@ -320,3 +320,56 @@ fn expect_cert_dns_names<'name>(
 
     assert!(cert.valid_dns_names().eq(expected_names))
 }
+
+#[cfg(feature = "alloc")]
+#[test]
+fn cert_time_validity() {
+    let ee: &[u8] = include_bytes!("netflix/ee.der");
+    let inter = CertificateDer::from(&include_bytes!("netflix/inter.der")[..]);
+    let ca = CertificateDer::from(&include_bytes!("netflix/ca.der")[..]);
+
+    let anchors = [anchor_from_trusted_cert(&ca).unwrap()];
+
+    let not_before = UnixTime::since_unix_epoch(Duration::from_secs(1_478_563_200));
+    let not_after = UnixTime::since_unix_epoch(Duration::from_secs(1_541_203_199));
+
+    let just_before = UnixTime::since_unix_epoch(Duration::from_secs(not_before.as_secs() - 1));
+    let just_after = UnixTime::since_unix_epoch(Duration::from_secs(not_after.as_secs() + 1));
+
+    let ee = CertificateDer::from(ee);
+    let cert = webpki::EndEntityCert::try_from(&ee).unwrap();
+
+    assert_eq!(
+        cert.verify_for_usage(
+            webpki::ALL_VERIFICATION_ALGS,
+            &anchors,
+            &[inter.clone()],
+            just_before,
+            KeyUsage::server_auth(),
+            None,
+            None,
+        )
+        .err(),
+        Some(webpki::Error::CertNotValidYet {
+            time: just_before,
+            not_before
+        })
+    );
+
+    assert_eq!(
+        cert.verify_for_usage(
+            webpki::ALL_VERIFICATION_ALGS,
+            &anchors,
+            &[inter],
+            just_after,
+            KeyUsage::server_auth(),
+            None,
+            None,
+        )
+        .err(),
+        Some(webpki::Error::CertExpired {
+            time: just_after,
+            not_after
+        })
+    );
+}

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]