From 1835639dba91085236bce44c94e0e75e229c9f7a Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Fri, 14 Feb 2025 10:43:31 +0100 Subject: [PATCH 1/4] Ensure SAN is checked for servers, irrespective of ExtensionPolicy. --- .../src/policy/extension.rs | 71 ++++++++++---- .../src/policy/mod.rs | 7 +- tests/x509/verification/test_verification.py | 92 +++++++++++++------ 3 files changed, 121 insertions(+), 49 deletions(-) diff --git a/src/rust/cryptography-x509-verification/src/policy/extension.rs b/src/rust/cryptography-x509-verification/src/policy/extension.rs index 8e6f858e128b..0e960791ede7 100644 --- a/src/rust/cryptography-x509-verification/src/policy/extension.rs +++ b/src/rust/cryptography-x509-verification/src/policy/extension.rs @@ -4,7 +4,7 @@ use std::sync::Arc; -use cryptography_x509::extensions::{Extension, Extensions}; +use cryptography_x509::extensions::{Extension, Extensions, SubjectAlternativeName}; use cryptography_x509::oid::{ AUTHORITY_INFORMATION_ACCESS_OID, AUTHORITY_KEY_IDENTIFIER_OID, BASIC_CONSTRAINTS_OID, EXTENDED_KEY_USAGE_OID, KEY_USAGE_OID, NAME_CONSTRAINTS_OID, SUBJECT_ALTERNATIVE_NAME_OID, @@ -16,6 +16,13 @@ use crate::{ ops::CryptoOps, policy::Policy, ValidationError, ValidationErrorKind, ValidationResult, }; +use super::Subject; + +pub(crate) enum CertificateType { + EE, + CA, +} + #[derive(Clone)] pub struct ExtensionPolicy<'cb, B: CryptoOps> { pub authority_information_access: ExtensionValidator<'cb, B>, @@ -121,7 +128,7 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { // validation case. subject_alternative_name: ExtensionValidator::present( Criticality::Agnostic, - Some(Arc::new(ee::subject_alternative_name)), + Some(Arc::new(ee::subject_alternative_name_criticality)), ), // 5280 4.2.1.9: Basic Constraints basic_constraints: ExtensionValidator::maybe_present( @@ -142,6 +149,7 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { pub(crate) fn permits<'chain>( &self, policy: &Policy<'_, B>, + certificate_type: CertificateType, cert: &VerificationCertificate<'chain, B>, extensions: &Extensions<'_>, ) -> ValidationResult<'chain, (), B> { @@ -180,6 +188,12 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { subject_alternative_name_seen = true; self.subject_alternative_name .permits(policy, cert, Some(&ext))?; + + if let CertificateType::EE = certificate_type { + // This ensures that even custom ExtensionPolicies will always + // check the SAN against the policy's subject + validate_subject_alternative_name_match(&policy.subject, &ext)?; + } } BASIC_CONSTRAINTS_OID => { basic_constraints_seen = true; @@ -231,10 +245,39 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { self.extended_key_usage.permits(policy, cert, None)?; } + if let CertificateType::EE = certificate_type { + // SAN is always required if the policy contains a specific subject (server profile). + if policy.subject.is_some() && !subject_alternative_name_seen { + return Err(ValidationError::new(ValidationErrorKind::Other( + "leaf server certificate has no subjectAltName".into(), + ))); + } + } + Ok(()) } } +pub(crate) fn validate_subject_alternative_name_match<'chain, B: CryptoOps>( + subject: &Option>, + extn: &Extension<'_>, +) -> ValidationResult<'chain, (), B> { + // NOTE: We only verify the SAN against the policy's subject if the + // policy actually contains one. This allows us to handle both client and server + // profiles, **with the expectation** that + // server profile construction requires a subject to be present. + if let Some(sub) = subject { + let san: SubjectAlternativeName<'_> = extn.value()?; + if !sub.matches(&san) { + return Err(ValidationError::new(ValidationErrorKind::Other( + "leaf certificate has no matching subjectAltName".into(), + ))); + } + } + + Ok(()) +} + /// Represents different criticality states for an extension. #[derive(Clone)] pub enum Criticality { @@ -389,9 +432,7 @@ impl<'cb, B: CryptoOps> ExtensionValidator<'cb, B> { } mod ee { - use cryptography_x509::extensions::{ - BasicConstraints, ExtendedKeyUsage, Extension, KeyUsage, SubjectAlternativeName, - }; + use cryptography_x509::extensions::{BasicConstraints, ExtendedKeyUsage, Extension, KeyUsage}; use crate::ops::{CryptoOps, VerificationCertificate}; use crate::policy::{Policy, ValidationError, ValidationErrorKind, ValidationResult}; @@ -414,8 +455,11 @@ mod ee { Ok(()) } - pub(crate) fn subject_alternative_name<'chain, B: CryptoOps>( - policy: &Policy<'_, B>, + /// This only checks the criticality of the SAN extension. Matching the SAN + /// against the subject is handled in `validate_subject_alternative_name_match` + /// which is always called, irrespective of how the ExtensionPolicy is configured. + pub(crate) fn subject_alternative_name_criticality<'chain, B: CryptoOps>( + _: &Policy<'_, B>, cert: &VerificationCertificate<'chain, B>, extn: &Extension<'_>, ) -> ValidationResult<'chain, (), B> { @@ -435,19 +479,6 @@ mod ee { _ => (), }; - // NOTE: We only verify the SAN against the policy's subject if the - // policy actually contains one. This enables both client and server - // profiles to use this validator, **with the expectation** that - // server profile construction requires a subject to be present. - if let Some(sub) = policy.subject.as_ref() { - let san: SubjectAlternativeName<'_> = extn.value()?; - if !sub.matches(&san) { - return Err(ValidationError::new(ValidationErrorKind::Other( - "leaf certificate has no matching subjectAltName".into(), - ))); - } - } - Ok(()) } diff --git a/src/rust/cryptography-x509-verification/src/policy/mod.rs b/src/rust/cryptography-x509-verification/src/policy/mod.rs index e1c850c59c46..d38ed134b82d 100644 --- a/src/rust/cryptography-x509-verification/src/policy/mod.rs +++ b/src/rust/cryptography-x509-verification/src/policy/mod.rs @@ -22,6 +22,7 @@ use cryptography_x509::oid::{ BASIC_CONSTRAINTS_OID, EC_SECP256R1, EC_SECP384R1, EC_SECP521R1, EKU_CLIENT_AUTH_OID, EKU_SERVER_AUTH_OID, }; +use extension::CertificateType; use once_cell::sync::Lazy; use crate::ops::CryptoOps; @@ -424,7 +425,8 @@ impl<'a, B: CryptoOps> Policy<'a, B> { } } - self.ca_extension_policy.permits(self, cert, extensions)?; + self.ca_extension_policy + .permits(self, CertificateType::CA, cert, extensions)?; Ok(()) } @@ -437,7 +439,8 @@ impl<'a, B: CryptoOps> Policy<'a, B> { ) -> ValidationResult<'chain, (), B> { self.permits_basic(cert.certificate())?; - self.ee_extension_policy.permits(self, cert, extensions)?; + self.ee_extension_policy + .permits(self, CertificateType::EE, cert, extensions)?; Ok(()) } diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index 3cd2da736722..af87a44c547a 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -214,33 +214,6 @@ def test_verify_fails_renders_oid(self): ): verifier.verify(leaf, []) - def test_custom_ext_policy_no_san(self): - leaf = _load_cert( - os.path.join("x509", "custom", "no_sans.pem"), - x509.load_pem_x509_certificate, - ) - - store = Store([leaf]) - validation_time = datetime.datetime.fromisoformat( - "2025-04-14T00:00:00+00:00" - ) - - builder = PolicyBuilder().store(store) - builder = builder.time(validation_time) - - with pytest.raises( - VerificationError, - match="missing required extension", - ): - builder.build_client_verifier().verify(leaf, []) - - builder = builder.extension_policies( - ExtensionPolicy.webpki_defaults_ca(), ExtensionPolicy.permit_all() - ) - - verified_client = builder.build_client_verifier().verify(leaf, []) - assert verified_client.subjects is None - class TestServerVerifier: @pytest.mark.parametrize( @@ -553,3 +526,68 @@ def validator(*_): match="Python validator must return None.", ): verifier.verify(self.leaf, []) + + def test_no_subject_alt_name(self): + leaf = _load_cert( + os.path.join("x509", "custom", "no_sans.pem"), + x509.load_pem_x509_certificate, + ) + + store = Store([leaf]) + validation_time = datetime.datetime.fromisoformat( + "2025-04-14T00:00:00+00:00" + ) + + builder = PolicyBuilder().store(store) + builder = builder.time(validation_time) + + with pytest.raises( + VerificationError, + match="missing required extension", + ): + builder.build_client_verifier().verify(leaf, []) + with pytest.raises( + VerificationError, + match="missing required extension", + ): + builder.build_server_verifier(DNSName("example.com")).verify( + leaf, [] + ) + + builder = builder.extension_policies( + ExtensionPolicy.webpki_defaults_ca(), ExtensionPolicy.permit_all() + ) + + verified_client = builder.build_client_verifier().verify(leaf, []) + assert verified_client.subjects is None + + # For ServerVerifier, SAN must be present, + # even if the ExtensionPolicy permits it's absence. + with pytest.raises( + VerificationError, + match="leaf server certificate has no subjectAltName", + ): + # SAN must be present for ServerVerifier, + # + builder.build_server_verifier(DNSName("example.com")).verify( + leaf, [] + ) + + def test_wrong_subject_alt_name(self): + builder = PolicyBuilder().store(self.store) + builder = builder.time(self.validation_time) + builder = builder.extension_policies( + ExtensionPolicy.webpki_defaults_ca(), ExtensionPolicy.permit_all() + ) + + builder.build_client_verifier().verify(self.leaf, []) + + # For ServerVerifier, SAN must be matched against the subject + # even if the extension policy permits any SANs. + with pytest.raises( + VerificationError, + match="leaf certificate has no matching subjectAltName", + ): + builder.build_server_verifier(DNSName("wrong.io")).verify( + self.leaf, [] + ) From ae50694c6efa1ae52ca0531fd31ca7cff6be9547 Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Fri, 14 Feb 2025 10:54:55 +0100 Subject: [PATCH 2/4] Update comments to reflect code changes. --- .../src/policy/extension.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/rust/cryptography-x509-verification/src/policy/extension.rs b/src/rust/cryptography-x509-verification/src/policy/extension.rs index 0e960791ede7..a4a445142806 100644 --- a/src/rust/cryptography-x509-verification/src/policy/extension.rs +++ b/src/rust/cryptography-x509-verification/src/policy/extension.rs @@ -123,9 +123,10 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { Some(Arc::new(ee::key_usage)), ), // CA/B 7.1.2.7.12 Subscriber Certificate Subject Alternative Name - // This validator handles both client and server cases by only matching against - // the SAN if the profile contains a subject, which it won't in the client - // validation case. + // This validator only handles the criticality checks. Matching + // SANs against the subject in the profile is handled by + // `validate_subject_alternative_name_match` which is + // invoked for all EE certificates, irrespective of this field's contents. subject_alternative_name: ExtensionValidator::present( Criticality::Agnostic, Some(Arc::new(ee::subject_alternative_name_criticality)), @@ -258,14 +259,13 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { } } +/// This only verifies the SAN against `subject` if `subject` is not None. +/// This allows us to handle both client and server profiles, +/// **with the expectation** that `subject` is always set for server profiles. pub(crate) fn validate_subject_alternative_name_match<'chain, B: CryptoOps>( subject: &Option>, extn: &Extension<'_>, ) -> ValidationResult<'chain, (), B> { - // NOTE: We only verify the SAN against the policy's subject if the - // policy actually contains one. This allows us to handle both client and server - // profiles, **with the expectation** that - // server profile construction requires a subject to be present. if let Some(sub) = subject { let san: SubjectAlternativeName<'_> = extn.value()?; if !sub.matches(&san) { @@ -455,9 +455,6 @@ mod ee { Ok(()) } - /// This only checks the criticality of the SAN extension. Matching the SAN - /// against the subject is handled in `validate_subject_alternative_name_match` - /// which is always called, irrespective of how the ExtensionPolicy is configured. pub(crate) fn subject_alternative_name_criticality<'chain, B: CryptoOps>( _: &Policy<'_, B>, cert: &VerificationCertificate<'chain, B>, From 90ee3cf772f2bafd705cfaf9cc102b73c2c2ba9e Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Fri, 14 Feb 2025 14:36:22 +0100 Subject: [PATCH 3/4] Improve naming, add comment. --- .../cryptography-x509-verification/src/policy/extension.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/rust/cryptography-x509-verification/src/policy/extension.rs b/src/rust/cryptography-x509-verification/src/policy/extension.rs index a4a445142806..9410305b2e3e 100644 --- a/src/rust/cryptography-x509-verification/src/policy/extension.rs +++ b/src/rust/cryptography-x509-verification/src/policy/extension.rs @@ -129,7 +129,7 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { // invoked for all EE certificates, irrespective of this field's contents. subject_alternative_name: ExtensionValidator::present( Criticality::Agnostic, - Some(Arc::new(ee::subject_alternative_name_criticality)), + Some(Arc::new(ee::subject_alternative_name)), ), // 5280 4.2.1.9: Basic Constraints basic_constraints: ExtensionValidator::maybe_present( @@ -455,7 +455,7 @@ mod ee { Ok(()) } - pub(crate) fn subject_alternative_name_criticality<'chain, B: CryptoOps>( + pub(crate) fn subject_alternative_name<'chain, B: CryptoOps>( _: &Policy<'_, B>, cert: &VerificationCertificate<'chain, B>, extn: &Extension<'_>, @@ -476,6 +476,9 @@ mod ee { _ => (), }; + // NOTE: policy.subject is checked against SAN elsewhere (see `ExtensionPolicy::permits`) + // since we want to check that even if a custom ExtensionPolicy with a lax validator is used. + Ok(()) } From cd50e9aa65ab7b22dda4d13b9b8de08b7115dfa6 Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Fri, 14 Feb 2025 14:37:47 +0100 Subject: [PATCH 4/4] Improve comment wording. --- src/rust/cryptography-x509-verification/src/policy/extension.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rust/cryptography-x509-verification/src/policy/extension.rs b/src/rust/cryptography-x509-verification/src/policy/extension.rs index 9410305b2e3e..88433101ed70 100644 --- a/src/rust/cryptography-x509-verification/src/policy/extension.rs +++ b/src/rust/cryptography-x509-verification/src/policy/extension.rs @@ -477,7 +477,7 @@ mod ee { }; // NOTE: policy.subject is checked against SAN elsewhere (see `ExtensionPolicy::permits`) - // since we want to check that even if a custom ExtensionPolicy with a lax validator is used. + // since we always want to check that, even if a custom ExtensionPolicy with a lax validator is used. Ok(()) }