policy/extension: improve extension policy errors (pyca#11162)

* policy/extension: improve extension policy errors * verification: ValidationError::ExtensionError variant Begin cleaning things up. * policy/extension: remove redundant clone * ensure that we render the ext OID * lib: coverage for other display arms * relocate custom vector * test-vectors: typo
woodruffw-forks · Jun 26, 2024 · f370b09 · f370b09
1 parent ae3b2a0
commit f370b09
Show file tree

Hide file tree

Showing 6 changed files with 143 additions and 32 deletions.
diff --git a/docs/development/test-vectors.rst b/docs/development/test-vectors.rst
@@ -528,6 +528,8 @@ Custom X.509 Vectors
   algorithm parameters. This encoding is invalid, but was generated by Java 11.
 * ``dsa_null_alg_params.pem`` - A certificate with a DSA signature with ``NULL``
   algorithm parameters. This encoding is invalid, but was generated by Java 20.
+* ``ekucrit-testuser-cert.pem`` - A leaf certificate containing a critical EKU.
+  This is an invalid certificate per CA/B 7.1.2.7.6.
 
 Custom X.509 Request Vectors
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

diff --git a/src/rust/cryptography-x509-verification/src/lib.rs b/src/rust/cryptography-x509-verification/src/lib.rs
@@ -12,8 +12,10 @@ pub mod policy;
 pub mod trust_store;
 pub mod types;
 
+use std::fmt::Display;
 use std::vec;
 
+use asn1::ObjectIdentifier;
 use cryptography_x509::extensions::{DuplicateExtensionsError, Extensions};
 use cryptography_x509::{
     extensions::{NameConstraints, SubjectAlternativeName},
@@ -35,10 +37,45 @@ pub enum ValidationError {
     CandidatesExhausted(Box<ValidationError>),
     Malformed(asn1::ParseError),
     DuplicateExtension(DuplicateExtensionsError),
+    ExtensionError {
+        oid: ObjectIdentifier,
+        reason: &'static str,
+    },
     FatalError(&'static str),
     Other(String),
 }
 
+impl From<asn1::ParseError> for ValidationError {
+    fn from(value: asn1::ParseError) -> Self {
+        Self::Malformed(value)
+    }
+}
+
+impl From<DuplicateExtensionsError> for ValidationError {
+    fn from(value: DuplicateExtensionsError) -> Self {
+        Self::DuplicateExtension(value)
+    }
+}
+
+impl Display for ValidationError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            ValidationError::CandidatesExhausted(inner) => {
+                write!(f, "candidates exhausted: {inner}")
+            }
+            ValidationError::Malformed(err) => err.fmt(f),
+            ValidationError::DuplicateExtension(DuplicateExtensionsError(oid)) => {
+                write!(f, "malformed certificate: duplicate extension: {oid}")
+            }
+            ValidationError::ExtensionError { oid, reason } => {
+                write!(f, "invalid extension: {oid}: {reason}")
+            }
+            ValidationError::FatalError(err) => write!(f, "fatal error: {err}"),
+            ValidationError::Other(err) => write!(f, "{err}"),
+        }
+    }
+}
+
 struct Budget {
     name_constraint_checks: usize,
 }
@@ -64,18 +101,6 @@ impl Budget {
     }
 }
 
-impl From<asn1::ParseError> for ValidationError {
-    fn from(value: asn1::ParseError) -> Self {
-        Self::Malformed(value)
-    }
-}
-
-impl From<DuplicateExtensionsError> for ValidationError {
-    fn from(value: DuplicateExtensionsError) -> Self {
-        Self::DuplicateExtension(value)
-    }
-}
-
 struct NameChain<'a, 'chain> {
     child: Option<&'a NameChain<'a, 'chain>>,
     sans: SubjectAlternativeName<'chain>,
@@ -412,3 +437,30 @@ impl<'a, 'chain: 'a, B: CryptoOps> ChainBuilder<'a, 'chain, B> {
         Ok(chain)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use asn1::ParseError;
+    use cryptography_x509::{
+        extensions::DuplicateExtensionsError, oid::SUBJECT_ALTERNATIVE_NAME_OID,
+    };
+
+    use crate::ValidationError;
+
+    #[test]
+    fn test_validationerror_display() {
+        let err = ValidationError::Malformed(ParseError::new(asn1::ParseErrorKind::InvalidLength));
+        assert_eq!(err.to_string(), "ASN.1 parsing error: invalid length");
+
+        let err = ValidationError::DuplicateExtension(DuplicateExtensionsError(
+            SUBJECT_ALTERNATIVE_NAME_OID,
+        ));
+        assert_eq!(
+            err.to_string(),
+            "malformed certificate: duplicate extension: 2.5.29.17"
+        );
+
+        let err = ValidationError::FatalError("oops");
+        assert_eq!(err.to_string(), "fatal error: oops");
+    }
+}
diff --git a/src/rust/cryptography-x509-verification/src/policy/extension.rs b/src/rust/cryptography-x509-verification/src/policy/extension.rs
@@ -81,10 +81,10 @@ impl<B: CryptoOps> ExtensionPolicy<B> {
                     self.extended_key_usage.permits(policy, cert, Some(&ext))?;
                 }
                 _ if ext.critical => {
-                    return Err(ValidationError::Other(format!(
-                        "certificate contains unaccounted-for critical extensions: {}",
-                        ext.extn_id
-                    )));
+                    return Err(ValidationError::ExtensionError {
+                        oid: ext.extn_id,
+                        reason: "certificate contains unaccounted-for critical extensions",
+                    });
                 }
                 _ => {}
             }
@@ -205,9 +205,10 @@ impl<B: CryptoOps> ExtensionValidator<B> {
             // Extension MUST NOT be present and isn't; OK.
             (ExtensionValidator::NotPresent, None) => Ok(()),
             // Extension MUST NOT be present but is; NOT OK.
-            (ExtensionValidator::NotPresent, Some(_)) => Err(ValidationError::Other(
-                "Certificate contains prohibited extension".to_string(),
-            )),
+            (ExtensionValidator::NotPresent, Some(extn)) => Err(ValidationError::ExtensionError {
+                oid: extn.extn_id.clone(),
+                reason: "Certificate contains prohibited extension",
+            }),
             // Extension MUST be present but is not; NOT OK.
             (ExtensionValidator::Present { .. }, None) => Err(ValidationError::Other(
                 "Certificate is missing required extension".to_string(),
@@ -221,9 +222,10 @@ impl<B: CryptoOps> ExtensionValidator<B> {
                 Some(extn),
             ) => {
                 if !criticality.permits(extn.critical) {
-                    return Err(ValidationError::Other(
-                        "Certificate extension has incorrect criticality".to_string(),
-                    ));
+                    return Err(ValidationError::ExtensionError {
+                        oid: extn.extn_id.clone(),
+                        reason: "Certificate extension has incorrect criticality",
+                    });
                 }
 
                 // If a custom validator is supplied, apply it.
@@ -237,15 +239,17 @@ impl<B: CryptoOps> ExtensionValidator<B> {
                 },
                 extn,
             ) => {
-                // If the extension is present, apply our criticality check.
-                if extn.map_or(false, |extn| !criticality.permits(extn.critical)) {
-                    return Err(ValidationError::Other(
-                        "Certificate extension has incorrect criticality".to_string(),
-                    ));
+                match extn {
+                    // If the extension is present, apply our criticality check.
+                    Some(extn) if !criticality.permits(extn.critical) => {
+                        Err(ValidationError::ExtensionError {
+                            oid: extn.extn_id.clone(),
+                            reason: "Certificate extension has incorrect criticality",
+                        })
+                    }
+                    // If a custom validator is supplied, apply it.
+                    _ => validator.map_or(Ok(()), |v| v(policy, cert, extn)),
                 }
-
-                // If a custom validator is supplied, apply it.
-                validator.map_or(Ok(()), |v| v(policy, cert, extn))
             }
         }
     }

diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs
@@ -282,7 +282,7 @@ impl PyClientVerifier {
             policy,
             store.raw.borrow_dependent(),
         )
-        .map_err(|e| VerificationError::new_err(format!("validation failed: {e:?}")))?;
+        .map_err(|e| VerificationError::new_err(format!("validation failed: {e}")))?;
 
         let py_chain = pyo3::types::PyList::empty_bound(py);
         for c in &chain {

diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py
@@ -11,7 +11,11 @@
 
 from cryptography import x509
 from cryptography.x509.general_name import DNSName, IPAddress
-from cryptography.x509.verification import PolicyBuilder, Store
+from cryptography.x509.verification import (
+    PolicyBuilder,
+    Store,
+    VerificationError,
+)
 from tests.x509.test_x509 import _load_cert
 
 
@@ -139,6 +143,32 @@ def test_verify(self):
         assert x509.DNSName("cryptography.io") in verified_client.subjects
         assert len(verified_client.subjects) == 2
 
+    def test_verify_fails_renders_oid(self):
+        leaf = _load_cert(
+            os.path.join("x509", "custom", "ekucrit-testuser-cert.pem"),
+            x509.load_pem_x509_certificate,
+        )
+
+        store = Store([leaf])
+
+        validation_time = datetime.datetime.fromisoformat(
+            "2024-06-26T00:00:00+00:00"
+        )
+
+        builder = PolicyBuilder().store(store)
+        builder = builder.time(validation_time)
+        verifier = builder.build_client_verifier()
+
+        pattern = (
+            r"invalid extension: 2\.5\.29\.37: "
+            r"Certificate extension has incorrect criticality"
+        )
+        with pytest.raises(
+            VerificationError,
+            match=pattern,
+        ):
+            verifier.verify(leaf, [])
+
 
 class TestServerVerifier:
     @pytest.mark.parametrize(

diff --git a/vectors/cryptography_vectors/x509/custom/ekucrit-testuser-cert.pem b/vectors/cryptography_vectors/x509/custom/ekucrit-testuser-cert.pem
@@ -0,0 +1,23 @@
+-----BEGIN CERTIFICATE-----
+MIIDyTCCArGgAwIBAgIUQWZSqoDvybWdo39pxRgeN0bLh8QwDQYJKoZIhvcNAQEL
+BQAwLDEUMBIGA1UECgwLVGVzdCBJc3N1ZXIxFDASBgNVBAMMC2V4YW1wbGUubmV0
+MB4XDTI0MDYyNTIyNTY0MFoXDTI0MDkyMzIyNTY0MFowLzEtMCsGA1UEAwwkZTBk
+Y2JmNTEtMDIyNC00MzYzLWI3NWUtYjZjZmIxODE3NzUzMIIBIjANBgkqhkiG9w0B
+AQEFAAOCAQ8AMIIBCgKCAQEAwq9wRSIpDGjEfRSOHxcfaOQmi1QR2AV0m1Exu8RW
+WwE+SycflSQOcPxNWn1B0dvVAIAmp5fSBram+6fdB+qgP/fz9/mHBBvP1+J7lLue
+1CUUDkci6P136HQ+kSsEDqrwMXzPESVNJk6b0FusF0gCEGTe01pgHKd82mpXK62W
+tSYFOYEFV4kB7u0ckkWEhiKGTKQ+zI5GSeApy23ao8q+oHDdBcD91ViYwgoWwKMY
+mYhZyLFZHh4D7axi275HjqVZZ1AmCy0bSLMgxwgHKEeFRmR3Yaoz3TkTi0fAUs4e
+w6Rdtor/PMecunp6atiHVUj9FWraAafGzVrM8Wfj6t88FwIDAQABo4HfMIHcMGQG
+A1UdEQRdMFuGKnVybjpwdWJsaWNpZDpJRE4rZXhhbXBsZS5uZXQrdXNlcit0ZXN0
+dXNlcoYtdXJuOnV1aWQ6ZTBkY2JmNTEtMDIyNC00MzYzLWI3NWUtYjZjZmIxODE3
+NzUzMA4GA1UdDwEB/wQEAwIHgDAWBgNVHSUBAf8EDDAKBggrBgEFBQcDAjAMBgNV
+HRMBAf8EAjAAMB0GA1UdDgQWBBQOeL5d5FUOQeZD99n1nxTvFMmN6DAfBgNVHSME
+GDAWgBQOeL5d5FUOQeZD99n1nxTvFMmN6DANBgkqhkiG9w0BAQsFAAOCAQEAjL4c
+TUCEYWDWW03AWskf7GGeUb2wehWOoH7cw5dtZa4UC1JghuPs+HbMLxRvy6/NsnrV
+7ZzzXiutTQEbE5EBQBhJAjuh34uogNe1itRvCFq8xUTQ+e8xP1nXCfZ2UMD0rb1F
+kvpqm4cFpX9AizjhnwOi4X7/svnv79yovfwGKPgUMfVb3Vbnd6aMeZbBh34hSSBn
+Emigl7tmS2KOs/eD+O2zQFu4NgUe4HH+jdE0+FDBkYwIOhLPGL2pCmdb7kM60Oo4
+W4yvwiQSJkfn1u4xvBoONsp8lNVkpYfFHWotuwCrHchVgCyaXcp7fEFUrl6mb+CY
+s4x++eieNDpxzcFsuw==
+-----END CERTIFICATE-----