diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 3d285b638ae2..138dd3a27f87 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -26,6 +26,12 @@ Changelog :class:`~cryptography.hazmat.primitives.hpke.KEM` so callers can split the encapsulated key from the ciphertext returned by :meth:`~cryptography.hazmat.primitives.hpke.Suite.encrypt`. +* :meth:`~cryptography.x509.verification.ExtensionPolicy.require_present`, + :meth:`~cryptography.x509.verification.ExtensionPolicy.may_be_present`, and + :meth:`~cryptography.x509.verification.ExtensionPolicy.require_not_present` + now accept any extension type. Previously only a fixed set of extension + types was supported, which made it impossible to account for otherwise + unrecognized critical extensions during path validation. * Added support for using :class:`~cryptography.x509.Certificate`, :class:`~cryptography.x509.CertificateSigningRequest`, and :class:`~cryptography.x509.CertificateRevocationList` as field types in diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index f2c0afa77e07..8b3bb488f9c1 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -315,15 +315,12 @@ the root of trust: .. note:: Calling any of the builder methods (:meth:`require_not_present`, :meth:`may_be_present`, or :meth:`require_present`) multiple times with the same extension type will raise an exception. - .. note:: Currently only the following extension types are supported in the ExtensionPolicy API: - :class:`~cryptography.x509.AuthorityInformationAccess`, - :class:`~cryptography.x509.AuthorityKeyIdentifier`, - :class:`~cryptography.x509.SubjectKeyIdentifier`, - :class:`~cryptography.x509.KeyUsage`, - :class:`~cryptography.x509.SubjectAlternativeName`, - :class:`~cryptography.x509.BasicConstraints`, - :class:`~cryptography.x509.NameConstraints`, - :class:`~cryptography.x509.ExtendedKeyUsage`. + .. versionchanged:: 49.0.0 + Any extension type may now be used with the builder methods. + + .. note:: If a present extension's value cannot be parsed into a known + Python object, it is passed to the validator callback as an + :class:`~cryptography.x509.UnrecognizedExtension`. .. staticmethod:: permit_all() diff --git a/src/rust/cryptography-x509-verification/src/policy/extension.rs b/src/rust/cryptography-x509-verification/src/policy/extension.rs index 73dd14fb1048..17d694bd094c 100644 --- a/src/rust/cryptography-x509-verification/src/policy/extension.rs +++ b/src/rust/cryptography-x509-verification/src/policy/extension.rs @@ -2,6 +2,7 @@ // 2.0, and the BSD License. See the LICENSE file in the root of this repository // for complete details. +use std::collections::HashSet; use std::sync::Arc; use cryptography_x509::extensions::{Extension, Extensions}; @@ -15,7 +16,6 @@ use crate::ops::{CryptoOps, VerificationCertificate}; use crate::policy::Policy; use crate::{ValidationError, ValidationErrorKind, ValidationResult}; -#[derive(Clone)] pub struct ExtensionPolicy<'cb, B: CryptoOps> { pub authority_information_access: ExtensionValidator<'cb, B>, pub authority_key_identifier: ExtensionValidator<'cb, B>, @@ -25,6 +25,33 @@ pub struct ExtensionPolicy<'cb, B: CryptoOps> { pub basic_constraints: ExtensionValidator<'cb, B>, pub name_constraints: ExtensionValidator<'cb, B>, pub extended_key_usage: ExtensionValidator<'cb, B>, + // Validators for any other extensions, i.e. those that don't have a + // dedicated field above. There is at most one validator per OID. + additional_extensions: Vec>, + // OIDs that have been explicitly configured via `with_validator`. Used to + // reject reconfiguring the same extension more than once. This is empty for + // the policies returned by the constructors, so their built-in validators + // can still be overridden once. + configured_oids: HashSet, +} + +// NOTE: Derived `Clone` would impose an unnecessary `B: Clone` bound, so we +// implement it manually (the contents are `Clone` regardless of `B`). +impl Clone for ExtensionPolicy<'_, B> { + fn clone(&self) -> Self { + ExtensionPolicy { + authority_information_access: self.authority_information_access.clone(), + authority_key_identifier: self.authority_key_identifier.clone(), + subject_key_identifier: self.subject_key_identifier.clone(), + key_usage: self.key_usage.clone(), + subject_alternative_name: self.subject_alternative_name.clone(), + basic_constraints: self.basic_constraints.clone(), + name_constraints: self.name_constraints.clone(), + extended_key_usage: self.extended_key_usage.clone(), + additional_extensions: self.additional_extensions.clone(), + configured_oids: self.configured_oids.clone(), + } + } } impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { @@ -50,6 +77,8 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { basic_constraints: make_permissive_validator(BASIC_CONSTRAINTS_OID), name_constraints: make_permissive_validator(NAME_CONSTRAINTS_OID), extended_key_usage: make_permissive_validator(EXTENDED_KEY_USAGE_OID), + additional_extensions: Vec::new(), + configured_oids: HashSet::new(), } } @@ -112,6 +141,8 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { Criticality::NonCritical, Some(Arc::new(ca::extended_key_usage)), ), + additional_extensions: Vec::new(), + configured_oids: HashSet::new(), } } @@ -168,9 +199,46 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { Criticality::NonCritical, Some(Arc::new(ee::extended_key_usage)), ), + additional_extensions: Vec::new(), + configured_oids: HashSet::new(), } } + /// Returns a new `ExtensionPolicy` with the given validator assigned to + /// the extension identified by the validator's OID. + /// + /// Extensions with a dedicated field are assigned to that field, + /// overwriting the built-in validator (e.g. the one from a default + /// policy); all others are stored in `additional_extensions`. + /// + /// Returns `Err` with the offending OID if this policy has already been + /// explicitly configured for that OID via a previous call, so the same + /// extension cannot be configured more than once. + pub fn with_validator( + &self, + validator: ExtensionValidator<'cb, B>, + ) -> Result { + let oid = validator.oid().clone(); + if self.configured_oids.contains(&oid) { + return Err(oid); + } + + let mut policy = self.clone(); + match oid { + AUTHORITY_INFORMATION_ACCESS_OID => policy.authority_information_access = validator, + AUTHORITY_KEY_IDENTIFIER_OID => policy.authority_key_identifier = validator, + SUBJECT_KEY_IDENTIFIER_OID => policy.subject_key_identifier = validator, + KEY_USAGE_OID => policy.key_usage = validator, + SUBJECT_ALTERNATIVE_NAME_OID => policy.subject_alternative_name = validator, + BASIC_CONSTRAINTS_OID => policy.basic_constraints = validator, + NAME_CONSTRAINTS_OID => policy.name_constraints = validator, + EXTENDED_KEY_USAGE_OID => policy.extended_key_usage = validator, + _ => policy.additional_extensions.push(validator), + } + policy.configured_oids.insert(oid); + Ok(policy) + } + pub(crate) fn permits<'chain>( &self, policy: &Policy<'_, B>, @@ -185,6 +253,7 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { let mut basic_constraints_seen = false; let mut name_constraints_seen = false; let mut extended_key_usage_seen = false; + let mut additional_extensions_seen = HashSet::new(); // Iterate over each extension and run its policy. for ext in extensions.iter() { @@ -225,13 +294,21 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { extended_key_usage_seen = true; self.extended_key_usage.permits(policy, cert, Some(&ext))?; } - _ if ext.critical => { - return Err(ValidationError::new(ValidationErrorKind::ExtensionError { - oid: ext.extn_id, - reason: "certificate contains unaccounted-for critical extensions", - })); + _ => { + if let Some(validator) = self + .additional_extensions + .iter() + .find(|validator| validator.oid() == &ext.extn_id) + { + additional_extensions_seen.insert(ext.extn_id.clone()); + validator.permits(policy, cert, Some(&ext))?; + } else if ext.critical { + return Err(ValidationError::new(ValidationErrorKind::ExtensionError { + oid: ext.extn_id, + reason: "certificate contains unaccounted-for critical extensions", + })); + } } - _ => {} } } @@ -262,6 +339,11 @@ impl<'cb, B: CryptoOps + 'cb> ExtensionPolicy<'cb, B> { if !extended_key_usage_seen { self.extended_key_usage.permits(policy, cert, None)?; } + for validator in &self.additional_extensions { + if !additional_extensions_seen.contains(validator.oid()) { + validator.permits(policy, cert, None)?; + } + } Ok(()) } @@ -313,7 +395,6 @@ pub type MaybeExtensionValidatorCallback<'cb, B> = Arc< >; /// Represents different validation states for an extension. -#[derive(Clone)] pub enum ExtensionValidator<'cb, B: CryptoOps> { /// The extension MUST NOT be present. NotPresent { oid: asn1::ObjectIdentifier }, @@ -335,7 +416,45 @@ pub enum ExtensionValidator<'cb, B: CryptoOps> { }, } +// NOTE: Derived `Clone` would impose an unnecessary `B: Clone` bound, so we +// implement it manually (the contents are `Clone` regardless of `B`). +impl Clone for ExtensionValidator<'_, B> { + fn clone(&self) -> Self { + match self { + ExtensionValidator::NotPresent { oid } => { + ExtensionValidator::NotPresent { oid: oid.clone() } + } + ExtensionValidator::Present { + oid, + criticality, + validator, + } => ExtensionValidator::Present { + oid: oid.clone(), + criticality: criticality.clone(), + validator: validator.clone(), + }, + ExtensionValidator::MaybePresent { + oid, + criticality, + validator, + } => ExtensionValidator::MaybePresent { + oid: oid.clone(), + criticality: criticality.clone(), + validator: validator.clone(), + }, + } + } +} + impl<'cb, B: CryptoOps> ExtensionValidator<'cb, B> { + pub fn oid(&self) -> &asn1::ObjectIdentifier { + match self { + ExtensionValidator::NotPresent { oid } => oid, + ExtensionValidator::Present { oid, .. } => oid, + ExtensionValidator::MaybePresent { oid, .. } => oid, + } + } + pub(crate) fn not_present(oid: asn1::ObjectIdentifier) -> Self { Self::NotPresent { oid } } diff --git a/src/rust/src/x509/verify/extension_policy.rs b/src/rust/src/x509/verify/extension_policy.rs index 805889a11c59..a0666f418c8a 100644 --- a/src/rust/src/x509/verify/extension_policy.rs +++ b/src/rust/src/x509/verify/extension_policy.rs @@ -1,12 +1,6 @@ -use std::collections::HashSet; use std::sync::Arc; use cryptography_x509::extensions::Extension; -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, - SUBJECT_KEY_IDENTIFIER_OID, -}; use cryptography_x509_verification::ops::VerificationCertificate; use cryptography_x509_verification::policy::{ Criticality, ExtensionPolicy, ExtensionValidator, MaybeExtensionValidatorCallback, Policy, @@ -17,7 +11,8 @@ use pyo3::types::{PyAnyMethods, PyTypeMethods}; use pyo3::{intern, PyResult}; use super::PyCryptoOps; -use crate::asn1::py_oid_to_oid; +use crate::asn1::{oid_to_py_oid, py_oid_to_oid}; +use crate::error::CryptographyError; use crate::types; use crate::x509::certificate::parse_cert_ext; @@ -55,7 +50,6 @@ impl From for Criticality { )] pub(crate) struct PyExtensionPolicy { inner_policy: ExtensionPolicy<'static, PyCryptoOps>, - already_set_oids: HashSet, } impl PyExtensionPolicy { @@ -64,51 +58,19 @@ impl PyExtensionPolicy { } fn new(inner_policy: ExtensionPolicy<'static, PyCryptoOps>) -> Self { - PyExtensionPolicy { - inner_policy, - already_set_oids: HashSet::new(), - } + PyExtensionPolicy { inner_policy } } fn with_assigned_validator( &self, validator: ExtensionValidator<'static, PyCryptoOps>, ) -> PyResult { - let oid = match &validator { - ExtensionValidator::NotPresent { oid } => oid, - ExtensionValidator::MaybePresent { oid, .. } => oid, - ExtensionValidator::Present { oid, .. } => oid, - } - .clone(); - if self.already_set_oids.contains(&oid) { - return Err(pyo3::exceptions::PyValueError::new_err(format!( + let inner_policy = self.inner_policy.with_validator(validator).map_err(|oid| { + pyo3::exceptions::PyValueError::new_err(format!( "ExtensionPolicy already configured for extension with OID {oid}" - ))); - } - - let mut policy = self.inner_policy.clone(); - match oid { - AUTHORITY_INFORMATION_ACCESS_OID => policy.authority_information_access = validator, - AUTHORITY_KEY_IDENTIFIER_OID => policy.authority_key_identifier = validator, - SUBJECT_KEY_IDENTIFIER_OID => policy.subject_key_identifier = validator, - KEY_USAGE_OID => policy.key_usage = validator, - SUBJECT_ALTERNATIVE_NAME_OID => policy.subject_alternative_name = validator, - BASIC_CONSTRAINTS_OID => policy.basic_constraints = validator, - NAME_CONSTRAINTS_OID => policy.name_constraints = validator, - EXTENDED_KEY_USAGE_OID => policy.extended_key_usage = validator, - _ => { - return Err(pyo3::exceptions::PyValueError::new_err(format!( - "Unsupported extension OID: {oid}", - ))) - } - } - - let mut already_set_oids = self.already_set_oids.clone(); - already_set_oids.insert(oid); - Ok(PyExtensionPolicy { - inner_policy: policy, - already_set_oids, - }) + )) + })?; + Ok(PyExtensionPolicy { inner_policy }) } } @@ -232,13 +194,29 @@ fn make_py_extension<'chain, 'p>( py: pyo3::Python<'p>, ext: Option<&Extension<'p>>, ) -> ValidationResult<'chain, Option>, PyCryptoOps> { + let conversion_error = |e: CryptographyError| { + ValidationError::new(ValidationErrorKind::Other(format!( + "{e} (while converting Extension to Python object)" + ))) + }; + Ok(match ext { None => None, - Some(ext) => parse_cert_ext(py, ext).map_err(|e| { - ValidationError::new(ValidationErrorKind::Other(format!( - "{e} (while converting Extension to Python object)" - ))) - })?, + Some(ext) => match parse_cert_ext(py, ext).map_err(conversion_error)? { + Some(parsed) => Some(parsed), + // The extension is present but its value can't be parsed into a + // known Python object. Mirror `Certificate.extensions` and surface + // it as an `UnrecognizedExtension` rather than dropping it to None. + None => { + let oid = + oid_to_py_oid(py, &ext.extn_id).map_err(|e| conversion_error(e.into()))?; + let unrecognized = types::UNRECOGNIZED_EXTENSION + .get(py) + .and_then(|ty| ty.call1((oid, ext.extn_value))) + .map_err(|e| conversion_error(e.into()))?; + Some(unrecognized) + } + }, }) } diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index a37b932cff88..7d7401ad97aa 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -12,8 +12,11 @@ from cryptography import x509 from cryptography.hazmat._oid import ExtendedKeyUsageOID +from cryptography.hazmat.primitives import hashes +from cryptography.hazmat.primitives.asymmetric import ec from cryptography.x509 import ExtensionType from cryptography.x509.general_name import DNSName, IPAddress +from cryptography.x509.oid import NameOID from cryptography.x509.verification import ( Criticality, ExtensionPolicy, @@ -314,20 +317,236 @@ class _Extension: None, ) - def test_unsupported_extension(self): - ext_policy = ExtensionPolicy.permit_all() - pattern = ( - f"Unsupported extension OID: {x509.Admissions.oid.dotted_string}" + def test_arbitrary_extension_supported(self): + # Extension types that aren't among the handful with built-in + # validators can still be added to a policy. See GH #14850. + for extension_type in (x509.Admissions, x509.CertificatePolicies): + ext_policy = ExtensionPolicy.permit_all() + ext_policy.may_be_present( + extension_type, Criticality.AGNOSTIC, None + ) + ext_policy.require_present( + extension_type, Criticality.AGNOSTIC, None + ) + ext_policy.require_not_present(extension_type) + + @staticmethod + def _chain_with_leaf_extension(leaf_extension, critical=True): + # Builds a (ca, leaf) chain where the leaf additionally carries the + # given extension. Used to exercise extensions that aren't among those + # with a built-in validator. See GH #14850. + ca_key = ec.generate_private_key(ec.SECP256R1()) + leaf_key = ec.generate_private_key(ec.SECP256R1()) + + not_before = datetime.datetime(2024, 1, 1) + not_after = datetime.datetime(2034, 1, 1) + validation_time = datetime.datetime(2025, 1, 1) + + ca_name = x509.Name( + [x509.NameAttribute(NameOID.COMMON_NAME, "Test CA")] + ) + ca = ( + x509.CertificateBuilder() + .subject_name(ca_name) + .issuer_name(ca_name) + .public_key(ca_key.public_key()) + .serial_number(x509.random_serial_number()) + .not_valid_before(not_before) + .not_valid_after(not_after) + .add_extension( + x509.BasicConstraints(ca=True, path_length=None), + critical=True, + ) + .add_extension( + x509.KeyUsage( + digital_signature=False, + content_commitment=False, + key_encipherment=False, + data_encipherment=False, + key_agreement=False, + key_cert_sign=True, + crl_sign=True, + encipher_only=False, + decipher_only=False, + ), + critical=True, + ) + .sign(ca_key, hashes.SHA256()) + ) + + leaf_name = x509.Name( + [x509.NameAttribute(NameOID.COMMON_NAME, "example.com")] + ) + leaf = ( + x509.CertificateBuilder() + .subject_name(leaf_name) + .issuer_name(ca_name) + .public_key(leaf_key.public_key()) + .serial_number(x509.random_serial_number()) + .not_valid_before(not_before) + .not_valid_after(not_after) + .add_extension( + x509.SubjectAlternativeName([x509.DNSName("example.com")]), + critical=False, + ) + .add_extension( + x509.AuthorityKeyIdentifier.from_issuer_public_key( + ca_key.public_key() + ), + critical=False, + ) + .add_extension( + x509.KeyUsage( + digital_signature=True, + content_commitment=False, + key_encipherment=False, + data_encipherment=False, + key_agreement=False, + key_cert_sign=False, + crl_sign=False, + encipher_only=False, + decipher_only=False, + ), + critical=False, + ) + .add_extension(leaf_extension, critical=critical) + .sign(ca_key, hashes.SHA256()) + ) + + return ca, leaf, validation_time + + def test_critical_unaccounted_extension_fails(self): + ca, leaf, validation_time = self._chain_with_leaf_extension( + x509.CertificatePolicies( + [ + x509.PolicyInformation( + x509.ObjectIdentifier("1.2.3.4"), None + ) + ] + ) ) + builder = PolicyBuilder().store(Store([ca])).time(validation_time) with pytest.raises( - ValueError, - match=pattern, + VerificationError, + match="unaccounted-for critical extensions", ): - ext_policy.may_be_present( - x509.Admissions, - Criticality.AGNOSTIC, - None, + builder.build_client_verifier().verify(leaf, []) + + def test_critical_extension_accounted_for(self): + ca, leaf, validation_time = self._chain_with_leaf_extension( + x509.CertificatePolicies( + [ + x509.PolicyInformation( + x509.ObjectIdentifier("1.2.3.4"), None + ) + ] + ) + ) + + seen = [] + + def validator(policy, cert, ext): + assert isinstance(policy, Policy) + assert isinstance(cert, x509.Certificate) + assert isinstance(ext, x509.CertificatePolicies) + seen.append(ext) + + ee_policy = ExtensionPolicy.webpki_defaults_ee().require_present( + x509.CertificatePolicies, Criticality.CRITICAL, validator + ) + builder = ( + PolicyBuilder() + .store(Store([ca])) + .time(validation_time) + .extension_policies( + ca_policy=ExtensionPolicy.webpki_defaults_ca(), + ee_policy=ee_policy, ) + ) + builder.build_client_verifier().verify(leaf, []) + assert len(seen) == 1 + + def test_unparseable_extension_passed_as_unrecognized(self): + # An extension whose value can't be parsed into a known Python object + # is delivered to the validator callback as an UnrecognizedExtension, + # rather than as None. + custom_oid = x509.ObjectIdentifier("1.2.3.4.5.6.7.8") + ca, leaf, validation_time = self._chain_with_leaf_extension( + x509.UnrecognizedExtension(custom_oid, b"\x04\x03abc"), + critical=True, + ) + + seen = [] + + def validator(policy, cert, ext): + assert isinstance(ext, x509.UnrecognizedExtension) + assert ext.oid == custom_oid + assert ext.value == b"\x04\x03abc" + seen.append(ext) + + # A custom extension type carrying the OID, so it can be registered. + class CustomExtension(x509.ExtensionType): + oid = custom_oid + + ee_policy = ExtensionPolicy.webpki_defaults_ee().require_present( + CustomExtension, Criticality.CRITICAL, validator + ) + builder = ( + PolicyBuilder() + .store(Store([ca])) + .time(validation_time) + .extension_policies( + ca_policy=ExtensionPolicy.webpki_defaults_ca(), + ee_policy=ee_policy, + ) + ) + builder.build_client_verifier().verify(leaf, []) + assert len(seen) == 1 + + def test_additional_extension_absent(self): + # Covers the required-but-absent handling for an additional (arbitrary) + # extension, i.e. one without a dedicated field. self.leaf does not + # carry this extension. + custom_oid = x509.ObjectIdentifier("1.2.3.4.5.6.7.8") + + class CustomExtension(x509.ExtensionType): + oid = custom_oid + + default_builder = ( + PolicyBuilder().store(self.store).time(self.validation_time) + ) + + # require_present for an absent additional extension fails. + require_present_builder = default_builder.extension_policies( + ca_policy=ExtensionPolicy.webpki_defaults_ca(), + ee_policy=ExtensionPolicy.webpki_defaults_ee().require_present( + CustomExtension, Criticality.AGNOSTIC, None + ), + ) + with pytest.raises( + VerificationError, + match="missing required extension", + ): + require_present_builder.build_client_verifier().verify( + self.leaf, [] + ) + + # may_be_present for an absent additional extension still invokes the + # callback, with ext=None. + seen: list[Optional[ExtensionType]] = [] + + def validator(policy, cert, ext): + assert ext is None + seen.append(ext) + + may_be_present_builder = default_builder.extension_policies( + ca_policy=ExtensionPolicy.webpki_defaults_ca(), + ee_policy=ExtensionPolicy.webpki_defaults_ee().may_be_present( + CustomExtension, Criticality.AGNOSTIC, validator + ), + ) + may_be_present_builder.build_client_verifier().verify(self.leaf, []) + assert seen == [None] @staticmethod def _make_validator_cb(extension_type: type[ExtensionType]):