From 7c42c64d21d390ddad2581f0d9eb1ed4c788499a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 4 Feb 2024 14:50:27 +0100 Subject: [PATCH 01/20] verification: WIP client verification skeleton Signed-off-by: William Woodruff --- docs/x509/verification.rst | 66 ++++++++++++++++++- .../hazmat/bindings/_rust/x509.pyi | 16 +++++ src/cryptography/x509/verification.py | 1 + .../src/policy/extension.rs | 10 +-- .../src/policy/mod.rs | 46 ++++++++++--- src/rust/src/x509/verify.rs | 32 ++++++++- 6 files changed, 154 insertions(+), 17 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index 6afc75f289e5..35c1b2e97131 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -104,6 +104,55 @@ the root of trust: :class:`cryptography.x509.general_name.DNSName`, :class:`cryptography.x509.general_name.IPAddress`. +.. class:: ClientVerifier + + .. versionadded:: 43.0.0 + + A ClientVerifier verifies client certificates. + + It contains and describes various pieces of configurable path + validation logic, such as which subject to expect, how deep prospective + validation chains may go, which signature algorithms are allowed, and + so forth. + + ClientVerifier instances cannot be constructed directly; + :class:`PolicyBuilder` must be used. + + .. attribute:: validation_time + + :type: :class:`datetime.datetime` + + The verifier's validation time. + + .. attribute:: max_chain_depth + + :type: :class:`int` + + The verifier's maximum intermediate CA chain depth. + + .. attribute:: store + + :type: :class:`Store` + + The verifier's trust store. + + .. method:: verify(leaf, intermediates) + + Performs path validation on ``leaf``, returning a valid path + if one exists. The path is returned in leaf-first order: + the first member is ``leaf``, followed by the intermediates used + (if any), followed by a member of the ``store``. + + :param leaf: The leaf :class:`~cryptography.x509.Certificate` to validate + :param intermediates: A :class:`list` of intermediate :class:`~cryptography.x509.Certificate` to attempt to use + + :returns: + A three-tuple of the client certificate's subject, + the client certificate's SAN (or ``None``), and a ``list`` containing + the built chain. + + :raises VerificationError: If a valid chain cannot be constructed + .. class:: ServerVerifier .. versionadded:: 42.0.0 @@ -174,7 +223,8 @@ the root of trust: Sets the verifier's verification time. If not called explicitly, this is set to :meth:`datetime.datetime.now` - when :meth:`build_server_verifier` is called. + when :meth:`build_server_verifier` or :meth:`build_client_verifier` + is called. :param new_time: The :class:`datetime.datetime` to use in the verifier @@ -209,3 +259,17 @@ the root of trust: :param subject: A :class:`Subject` to use in the verifier :returns: An instance of :class:`ServerVerifier` + + .. method:: build_client_verifier() + + .. versionadded:: 43.0.0 + + Builds a verifier for verifying client certificates. + + .. warning:: + + This API is not suitable for website (i.e. server) certificate + verification. You **must** use :meth:`build_server_verifier` + for server verification. + + :returns: An instance of :class:`ClientVerifier` diff --git a/src/cryptography/hazmat/bindings/_rust/x509.pyi b/src/cryptography/hazmat/bindings/_rust/x509.pyi index 418184f8a6fd..9383c5029747 100644 --- a/src/cryptography/hazmat/bindings/_rust/x509.pyi +++ b/src/cryptography/hazmat/bindings/_rust/x509.pyi @@ -62,10 +62,26 @@ class PolicyBuilder: def time(self, new_time: datetime.datetime) -> PolicyBuilder: ... def store(self, new_store: Store) -> PolicyBuilder: ... def max_chain_depth(self, new_max_chain_depth: int) -> PolicyBuilder: ... + def build_client_verifier(self) -> ClientVerifier: ... def build_server_verifier( self, subject: x509.verification.Subject ) -> ServerVerifier: ... +class ClientVerifier: + @property + def validation_time(self) -> datetime.datetime: ... + @property + def store(self) -> Store: ... + @property + def max_chain_depth(self) -> int: ... + def verify( + self, + leaf: x509.Certificate, + intermediates: list[x509.Certificate], + ) -> tuple[ + x509.Name, x509.SubjectAlternativeName | None, list[x509.Certificate] + ]: ... + class ServerVerifier: @property def subject(self) -> x509.verification.Subject: ... diff --git a/src/cryptography/x509/verification.py b/src/cryptography/x509/verification.py index ab1a37ae6b01..a00e994e3fef 100644 --- a/src/cryptography/x509/verification.py +++ b/src/cryptography/x509/verification.py @@ -19,6 +19,7 @@ Store = rust_x509.Store Subject = typing.Union[DNSName, IPAddress] +ClientVerifier = rust_x509.ClientVerifier ServerVerifier = rust_x509.ServerVerifier PolicyBuilder = rust_x509.PolicyBuilder VerificationError = rust_x509.VerificationError diff --git a/src/rust/cryptography-x509-verification/src/policy/extension.rs b/src/rust/cryptography-x509-verification/src/policy/extension.rs index 83d4a5ec1736..9ab88ab5189d 100644 --- a/src/rust/cryptography-x509-verification/src/policy/extension.rs +++ b/src/rust/cryptography-x509-verification/src/policy/extension.rs @@ -599,7 +599,7 @@ mod tests { let cert_pem = v1_cert_pem(); let cert = cert(&cert_pem); let ops = PublicKeyErrorOps {}; - let policy = Policy::new( + let policy = Policy::server( ops, Subject::DNS(DNSName::new("example.com").unwrap()), epoch(), @@ -639,7 +639,7 @@ mod tests { let cert_pem = v1_cert_pem(); let cert = cert(&cert_pem); let ops = PublicKeyErrorOps {}; - let policy = Policy::new( + let policy = Policy::server( ops, Subject::DNS(DNSName::new("example.com").unwrap()), epoch(), @@ -673,7 +673,7 @@ mod tests { let cert_pem = v1_cert_pem(); let cert = cert(&cert_pem); let ops = PublicKeyErrorOps {}; - let policy = Policy::new( + let policy = Policy::server( ops, Subject::DNS(DNSName::new("example.com").unwrap()), epoch(), @@ -704,7 +704,7 @@ mod tests { let cert_pem = v1_cert_pem(); let cert = cert(&cert_pem); let ops = PublicKeyErrorOps {}; - let policy = Policy::new( + let policy = Policy::server( ops, Subject::DNS(DNSName::new("example.com").unwrap()), epoch(), @@ -733,7 +733,7 @@ mod tests { let cert_pem = v1_cert_pem(); let cert = cert(&cert_pem); let ops = PublicKeyErrorOps {}; - let policy = Policy::new( + let policy = Policy::server( ops, Subject::DNS(DNSName::new("example.com").unwrap()), epoch(), diff --git a/src/rust/cryptography-x509-verification/src/policy/mod.rs b/src/rust/cryptography-x509-verification/src/policy/mod.rs index ef270fc79db4..c595bf83aa57 100644 --- a/src/rust/cryptography-x509-verification/src/policy/mod.rs +++ b/src/rust/cryptography-x509-verification/src/policy/mod.rs @@ -234,18 +234,17 @@ pub struct Policy<'a, B: CryptoOps> { } impl<'a, B: CryptoOps> Policy<'a, B> { - /// Create a new policy with defaults for the certificate profile defined in - /// the CA/B Forum's Basic Requirements. - pub fn new( + fn new( ops: B, - subject: Subject<'a>, + subject: Option>, time: asn1::DateTime, max_chain_depth: Option, ) -> Self { + let has_subject = subject.is_some(); Self { ops, max_chain_depth: max_chain_depth.unwrap_or(DEFAULT_MAX_CHAIN_DEPTH), - subject: Some(subject), + subject, validation_time: time, extended_key_usage: EKU_SERVER_AUTH_OID.clone(), minimum_rsa_modulus: WEBPKI_MINIMUM_RSA_MODULUS, @@ -315,11 +314,17 @@ impl<'a, B: CryptoOps> Policy<'a, B> { Criticality::Agnostic, Some(ee::key_usage), ), - // CA/B 7.1.2.7.12 Subscriber Certificate Subject Alternative Name - subject_alternative_name: ExtensionValidator::present( - Criticality::Agnostic, - Some(ee::subject_alternative_name), - ), + subject_alternative_name: match has_subject { + // CA/B 7.1.2.7.12 Subscriber Certificate Subject Alternative Name + true => ExtensionValidator::present( + Criticality::Agnostic, + Some(ee::subject_alternative_name), + ), + false => ExtensionValidator::MaybePresent { + criticality: Criticality::Agnostic, + validator: None, + }, + }, // 5280 4.2.1.9: Basic Constraints basic_constraints: ExtensionValidator::maybe_present( Criticality::Agnostic, @@ -337,6 +342,27 @@ impl<'a, B: CryptoOps> Policy<'a, B> { } } + /// Create a new policy with suitable defaults for client certification + /// validation. + /// + /// **IMPORTANT**: This is **not** the appropriate API for verifying + /// website (i.e. server) certificates. For that, you **must** use + /// [`Policy::server`]. + pub fn client(ops: B, time: asn1::DateTime, max_chain_depth: Option) -> Self { + Self::new(ops, None, time, max_chain_depth) + } + + /// Create a new policy with defaults for the server certificate profile + /// defined in the CA/B Forum's Basic Requirements. + pub fn server( + ops: B, + subject: Subject<'a>, + time: asn1::DateTime, + max_chain_depth: Option, + ) -> Self { + Self::new(ops, Some(subject), time, max_chain_depth) + } + fn permits_basic(&self, cert: &Certificate<'_>) -> Result<(), ValidationError> { // CA/B 7.1.1: // Certificates MUST be of type X.509 v3. diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 8cd9cfdf964b..6ac1fb8078ff 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -118,6 +118,25 @@ impl PolicyBuilder { }) } + fn build_client_verifier(&self, _py: pyo3::Python<'_>) -> CryptographyResult { + // let store = match self.store.as_ref() { + // Some(s) => s.clone_ref(py), + // None => { + // return Err(CryptographyError::from( + // pyo3::exceptions::PyValueError::new_err( + // "A client verifier must have a trust store.", + // ), + // )); + // } + // }; + + // let time = match self.time.as_ref() { + // Some(t) => t.clone(), + // None => datetime_now(py)?, + // }; + todo!() + } + fn build_server_verifier( &self, py: pyo3::Python<'_>, @@ -142,7 +161,7 @@ impl PolicyBuilder { let policy = OwnedPolicy::try_new(subject_owner, |subject_owner| { let subject = build_subject(py, subject_owner)?; - Ok::, pyo3::PyErr>(PyCryptoPolicy(Policy::new( + Ok::, pyo3::PyErr>(PyCryptoPolicy(Policy::server( PyCryptoOps {}, subject, time, @@ -180,6 +199,17 @@ self_cell::self_cell!( } ); +#[pyo3::pyclass( + frozen, + name = "ClientVerifier", + module = "cryptography.hazmat.bindings._rust.x509" +)] +struct PyClientVerifier { + _policy: OwnedPolicy, + #[pyo3(get)] + store: pyo3::Py, +} + #[pyo3::pyclass( frozen, name = "ServerVerifier", From 543f055c778087340b3c5ba0381d019a45a2b727 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 4 Feb 2024 15:24:12 +0100 Subject: [PATCH 02/20] verify: fill in build_client_verifier Signed-off-by: William Woodruff --- src/rust/src/x509/verify.rs | 48 ++++++++++++++++++++++++------------- 1 file changed, 31 insertions(+), 17 deletions(-) diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 6ac1fb8078ff..3649206745f8 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -118,23 +118,35 @@ impl PolicyBuilder { }) } - fn build_client_verifier(&self, _py: pyo3::Python<'_>) -> CryptographyResult { - // let store = match self.store.as_ref() { - // Some(s) => s.clone_ref(py), - // None => { - // return Err(CryptographyError::from( - // pyo3::exceptions::PyValueError::new_err( - // "A client verifier must have a trust store.", - // ), - // )); - // } - // }; - - // let time = match self.time.as_ref() { - // Some(t) => t.clone(), - // None => datetime_now(py)?, - // }; - todo!() + fn build_client_verifier(&self, py: pyo3::Python<'_>) -> CryptographyResult { + let store = match self.store.as_ref() { + Some(s) => s.clone_ref(py), + None => { + return Err(CryptographyError::from( + pyo3::exceptions::PyValueError::new_err( + "A client verifier must have a trust store.", + ), + )); + } + }; + + let time = match self.time.as_ref() { + Some(t) => t.clone(), + None => datetime_now(py)?, + }; + + let policy = OwnedPolicy::try_new(SubjectOwner::None, |_| { + Ok::, pyo3::PyErr>(PyCryptoPolicy(Policy::client( + PyCryptoOps {}, + time, + self.max_chain_depth, + ))) + })?; + + Ok(PyClientVerifier { + _policy: policy, + store, + }) } fn build_server_verifier( @@ -188,6 +200,7 @@ enum SubjectOwner { // so, which was only stabilized with 3.10. DNSName(String), IPAddress(pyo3::Py), + None, } self_cell::self_cell!( @@ -317,6 +330,7 @@ fn build_subject<'a>( Ok(Subject::IP(ip_addr)) } + SubjectOwner::None => Err(pyo3::exceptions::PyValueError::new_err("missing subject")), } } From 8a540d319cd2920b7db0ce32c743870da63318f3 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 8 Feb 2024 13:02:45 +0000 Subject: [PATCH 03/20] implement ClientVerifier.verify Signed-off-by: William Woodruff --- docs/x509/verification.rst | 13 ++- .../hazmat/bindings/_rust/x509.pyi | 4 +- src/cryptography/x509/verification.py | 5 + .../src/policy/mod.rs | 8 +- src/rust/src/x509/verify.rs | 95 +++++++++++++++++-- tests/x509/verification/test_verification.py | 25 +++++ 6 files changed, 132 insertions(+), 18 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index 35c1b2e97131..637ca970a58f 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -104,6 +104,15 @@ the root of trust: :class:`cryptography.x509.general_name.DNSName`, :class:`cryptography.x509.general_name.IPAddress`. +.. class:: VerifiedClient + + .. versionadded:: 43.0.0 + + Type alias: A tuple of :class:`~cryptography.x509.Extension` and + a list of :class:`~cryptography.x509.Certificate`. The extension contains + a :class:`~cryptography.x509.SubjectAlternativeName` with the client's + SAN. + .. class:: ClientVerifier .. versionadded:: 43.0.0 @@ -147,9 +156,7 @@ the root of trust: :param intermediates: A :class:`list` of intermediate :class:`~cryptography.x509.Certificate` to attempt to use :returns: - A three-tuple of the client certificate's subject, - the client certificate's SAN (or ``None``), and a ``list`` containing - the built chain. + A new instance of :class:`VerifiedClient` :raises VerificationError: If a valid chain cannot be constructed diff --git a/src/cryptography/hazmat/bindings/_rust/x509.pyi b/src/cryptography/hazmat/bindings/_rust/x509.pyi index 9383c5029747..101e4f299584 100644 --- a/src/cryptography/hazmat/bindings/_rust/x509.pyi +++ b/src/cryptography/hazmat/bindings/_rust/x509.pyi @@ -78,9 +78,7 @@ class ClientVerifier: self, leaf: x509.Certificate, intermediates: list[x509.Certificate], - ) -> tuple[ - x509.Name, x509.SubjectAlternativeName | None, list[x509.Certificate] - ]: ... + ) -> x509.verification.VerifiedClient: ... class ServerVerifier: @property diff --git a/src/cryptography/x509/verification.py b/src/cryptography/x509/verification.py index a00e994e3fef..6e0ba3bbe7b2 100644 --- a/src/cryptography/x509/verification.py +++ b/src/cryptography/x509/verification.py @@ -7,11 +7,15 @@ import typing from cryptography.hazmat.bindings._rust import x509 as rust_x509 +from cryptography.x509.base import Certificate +from cryptography.x509.extensions import Extension, SubjectAlternativeName from cryptography.x509.general_name import DNSName, IPAddress __all__ = [ "Store", "Subject", + "VerifiedClient", + "ClientVerifier", "ServerVerifier", "PolicyBuilder", "VerificationError", @@ -19,6 +23,7 @@ Store = rust_x509.Store Subject = typing.Union[DNSName, IPAddress] +VerifiedClient = tuple[Extension[SubjectAlternativeName], list[Certificate]] ClientVerifier = rust_x509.ClientVerifier ServerVerifier = rust_x509.ServerVerifier PolicyBuilder = rust_x509.PolicyBuilder diff --git a/src/rust/cryptography-x509-verification/src/policy/mod.rs b/src/rust/cryptography-x509-verification/src/policy/mod.rs index c595bf83aa57..6bfa3875c925 100644 --- a/src/rust/cryptography-x509-verification/src/policy/mod.rs +++ b/src/rust/cryptography-x509-verification/src/policy/mod.rs @@ -320,10 +320,10 @@ impl<'a, B: CryptoOps> Policy<'a, B> { Criticality::Agnostic, Some(ee::subject_alternative_name), ), - false => ExtensionValidator::MaybePresent { - criticality: Criticality::Agnostic, - validator: None, - }, + // Under client validation, we return the SAN rather than verifying + // it directly against the profile. As such, we require it here + // but don't supply any custom validator logic. + false => ExtensionValidator::present(Criticality::Agnostic, None), }, // 5280 4.2.1.9: Basic Constraints basic_constraints: ExtensionValidator::maybe_present( diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 3649206745f8..e217535061be 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -2,20 +2,26 @@ // 2.0, and the BSD License. See the LICENSE file in the root of this repository // for complete details. -use cryptography_x509::certificate::Certificate; +use cryptography_x509::{ + certificate::Certificate, extensions::SubjectAlternativeName, oid::SUBJECT_ALTERNATIVE_NAME_OID, +}; use cryptography_x509_verification::{ ops::{CryptoOps, VerificationCertificate}, policy::{Policy, Subject}, trust_store::Store, types::{DNSName, IPAddress}, }; +use pyo3::IntoPy; use crate::backend::keys; -use crate::error::{CryptographyError, CryptographyResult}; -use crate::types; use crate::x509::certificate::Certificate as PyCertificate; use crate::x509::common::{datetime_now, datetime_to_py, py_to_datetime}; use crate::x509::sign; +use crate::{asn1::oid_to_py_oid, types}; +use crate::{ + error::{CryptographyError, CryptographyResult}, + x509, +}; pub(crate) struct PyCryptoOps {} @@ -143,10 +149,7 @@ impl PolicyBuilder { ))) })?; - Ok(PyClientVerifier { - _policy: policy, - store, - }) + Ok(PyClientVerifier { policy, store }) } fn build_server_verifier( @@ -218,11 +221,86 @@ self_cell::self_cell!( module = "cryptography.hazmat.bindings._rust.x509" )] struct PyClientVerifier { - _policy: OwnedPolicy, + policy: OwnedPolicy, #[pyo3(get)] store: pyo3::Py, } +impl PyClientVerifier { + fn as_policy(&self) -> &Policy<'_, PyCryptoOps> { + &self.policy.borrow_dependent().0 + } +} + +#[pyo3::pymethods] +impl PyClientVerifier { + #[getter] + fn validation_time<'p>(&self, py: pyo3::Python<'p>) -> pyo3::PyResult<&'p pyo3::PyAny> { + datetime_to_py(py, &self.as_policy().validation_time) + } + + #[getter] + fn max_chain_depth(&self) -> u8 { + self.as_policy().max_chain_depth + } + + fn verify( + &self, + py: pyo3::Python<'_>, + leaf: pyo3::Py, + intermediates: Vec>, + ) -> CryptographyResult> { + let policy = self.as_policy(); + let store = self.store.get(); + + let chain = cryptography_x509_verification::verify( + &VerificationCertificate::new( + leaf.get().raw.borrow_dependent().clone(), + leaf.clone_ref(py), + ), + intermediates.iter().map(|i| { + VerificationCertificate::new( + i.get().raw.borrow_dependent().clone(), + i.clone_ref(py), + ) + }), + policy, + store.raw.borrow_dependent(), + ) + .map_err(|e| VerificationError::new_err(format!("validation failed: {e:?}")))?; + + let py_chain = pyo3::types::PyList::empty(py); + for c in &chain { + py_chain.append(c.extra())?; + } + + // NOTE: These `unwrap()` cannot fail, since the underlying policy + // enforces the presence of a SAN and the well-formedness of the + // extension set. + let leaf_san = &chain[0] + .certificate() + .extensions() + .unwrap() + .get_extension(&SUBJECT_ALTERNATIVE_NAME_OID) + .unwrap(); + + let py_san = + types::SUBJECT_ALTERNATIVE_NAME + .get(py)? + .call1((x509::parse_general_names( + py, + &leaf_san.value::>()?, + )?,))?; + + let py_oid = oid_to_py_oid(py, &leaf_san.extn_id)?; + let py_san_ext = types::EXTENSION + .get(py)? + .call1((py_oid, leaf_san.critical, py_san))?; + + Ok((py_san_ext, py_chain).into_py(py)) + } +} + #[pyo3::pyclass( frozen, name = "ServerVerifier", @@ -377,6 +455,7 @@ impl PyStore { } pub(crate) fn add_to_module(module: &pyo3::prelude::PyModule) -> pyo3::PyResult<()> { + module.add_class::()?; module.add_class::()?; module.add_class::()?; module.add_class::()?; diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index 8c2be7054227..ee611952020f 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -105,6 +105,31 @@ def test_store_rejects_non_certificates(self): Store(["not a cert"]) # type: ignore[list-item] +class TestClientVerifier: + def test_verify(self): + # expires 2018-11-16 01:15:03 UTC + leaf = _load_cert( + os.path.join("x509", "cryptography.io.pem"), + x509.load_pem_x509_certificate, + ) + + store = Store([leaf]) + + builder = PolicyBuilder().store(store) + builder = builder.time( + datetime.datetime.fromisoformat("2018-11-16T00:00:00+00:00") + ) + verifier = builder.build_client_verifier() + + san, chain = verifier.verify(leaf, []) + assert isinstance(san, x509.Extension) + assert isinstance(san.value, x509.SubjectAlternativeName) + assert chain == [leaf] + + dns_names = san.value.get_values_for_type(x509.DNSName) + assert set(dns_names) == {"www.cryptography.io", "cryptography.io"} + + class TestServerVerifier: @pytest.mark.parametrize( ("validation_time", "valid"), From b7761d62b84263a07eabfe0db083a3700ae1d492 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 8 Feb 2024 13:07:09 +0000 Subject: [PATCH 04/20] verification: make Python 3.8 happy Signed-off-by: William Woodruff --- src/cryptography/x509/verification.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/cryptography/x509/verification.py b/src/cryptography/x509/verification.py index 6e0ba3bbe7b2..9ccff9957f08 100644 --- a/src/cryptography/x509/verification.py +++ b/src/cryptography/x509/verification.py @@ -23,7 +23,9 @@ Store = rust_x509.Store Subject = typing.Union[DNSName, IPAddress] -VerifiedClient = tuple[Extension[SubjectAlternativeName], list[Certificate]] +VerifiedClient = typing.Tuple[ + Extension[SubjectAlternativeName], typing.List[Certificate] +] ClientVerifier = rust_x509.ClientVerifier ServerVerifier = rust_x509.ServerVerifier PolicyBuilder = rust_x509.PolicyBuilder From a9f24a83e2f2e9c09f21ff31750e897fb4f7fce8 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Thu, 8 Feb 2024 14:04:11 +0000 Subject: [PATCH 05/20] switch to a full VerifiedClient type Signed-off-by: William Woodruff --- docs/x509/verification.rst | 18 +++++++++++++---- .../hazmat/bindings/_rust/x509.pyi | 8 +++++++- src/cryptography/x509/verification.py | 6 +----- src/rust/src/x509/verify.rs | 20 +++++++++++++++++-- tests/x509/verification/test_verification.py | 14 ++++++++----- 5 files changed, 49 insertions(+), 17 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index 637ca970a58f..0c0444b0c265 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -108,10 +108,20 @@ the root of trust: .. versionadded:: 43.0.0 - Type alias: A tuple of :class:`~cryptography.x509.Extension` and - a list of :class:`~cryptography.x509.Certificate`. The extension contains - a :class:`~cryptography.x509.SubjectAlternativeName` with the client's - SAN. + .. attribute:: subject + + :type: :class:`~cryptography.x509.Extension` + + The subject of the verified client certificate, as presented in the + :class:`~cryptography.x509.SubjectAlternativeName` extension. + + .. attribute:: chain + + :type: A list of :class:`~cryptography.x509.Certificate`, in leaf-first order + + The chain of certificates that forms the valid chain to the client + certificate. + .. class:: ClientVerifier diff --git a/src/cryptography/hazmat/bindings/_rust/x509.pyi b/src/cryptography/hazmat/bindings/_rust/x509.pyi index 101e4f299584..14b4f14261dd 100644 --- a/src/cryptography/hazmat/bindings/_rust/x509.pyi +++ b/src/cryptography/hazmat/bindings/_rust/x509.pyi @@ -67,6 +67,12 @@ class PolicyBuilder: self, subject: x509.verification.Subject ) -> ServerVerifier: ... +class VerifiedClient: + @property + def subject(self) -> x509.Extension[x509.SubjectAlternativeName]: ... + @property + def chain(self) -> list[x509.Certificate]: ... + class ClientVerifier: @property def validation_time(self) -> datetime.datetime: ... @@ -78,7 +84,7 @@ class ClientVerifier: self, leaf: x509.Certificate, intermediates: list[x509.Certificate], - ) -> x509.verification.VerifiedClient: ... + ) -> VerifiedClient: ... class ServerVerifier: @property diff --git a/src/cryptography/x509/verification.py b/src/cryptography/x509/verification.py index 9ccff9957f08..191705e8352b 100644 --- a/src/cryptography/x509/verification.py +++ b/src/cryptography/x509/verification.py @@ -7,8 +7,6 @@ import typing from cryptography.hazmat.bindings._rust import x509 as rust_x509 -from cryptography.x509.base import Certificate -from cryptography.x509.extensions import Extension, SubjectAlternativeName from cryptography.x509.general_name import DNSName, IPAddress __all__ = [ @@ -23,9 +21,7 @@ Store = rust_x509.Store Subject = typing.Union[DNSName, IPAddress] -VerifiedClient = typing.Tuple[ - Extension[SubjectAlternativeName], typing.List[Certificate] -] +VerifiedClient = rust_x509.VerifiedClient ClientVerifier = rust_x509.ClientVerifier ServerVerifier = rust_x509.ServerVerifier PolicyBuilder = rust_x509.PolicyBuilder diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index e217535061be..e6e26b1802ab 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -215,6 +215,18 @@ self_cell::self_cell!( } ); +#[pyo3::pyclass( + frozen, + name = "VerifiedClient", + module = "cryptography.hazmat.bindings._rust.x509" +)] +struct PyVerifiedClient { + #[pyo3(get)] + subject: pyo3::Py, + #[pyo3(get)] + chain: pyo3::Py, +} + #[pyo3::pyclass( frozen, name = "ClientVerifier", @@ -249,7 +261,7 @@ impl PyClientVerifier { py: pyo3::Python<'_>, leaf: pyo3::Py, intermediates: Vec>, - ) -> CryptographyResult> { + ) -> CryptographyResult { let policy = self.as_policy(); let store = self.store.get(); @@ -297,7 +309,10 @@ impl PyClientVerifier { .get(py)? .call1((py_oid, leaf_san.critical, py_san))?; - Ok((py_san_ext, py_chain).into_py(py)) + Ok(PyVerifiedClient { + subject: py_san_ext.into_py(py), + chain: py_chain.into_py(py), + }) } } @@ -455,6 +470,7 @@ impl PyStore { } pub(crate) fn add_to_module(module: &pyo3::prelude::PyModule) -> pyo3::PyResult<()> { + module.add_class::()?; module.add_class::()?; module.add_class::()?; module.add_class::()?; diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index ee611952020f..b05298d0d279 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -121,12 +121,16 @@ def test_verify(self): ) verifier = builder.build_client_verifier() - san, chain = verifier.verify(leaf, []) - assert isinstance(san, x509.Extension) - assert isinstance(san.value, x509.SubjectAlternativeName) - assert chain == [leaf] + verified_client = verifier.verify(leaf, []) + assert isinstance(verified_client.subject, x509.Extension) + assert isinstance( + verified_client.subject.value, x509.SubjectAlternativeName + ) + assert verified_client.chain == [leaf] - dns_names = san.value.get_values_for_type(x509.DNSName) + dns_names = verified_client.subject.value.get_values_for_type( + x509.DNSName + ) assert set(dns_names) == {"www.cryptography.io", "cryptography.io"} From 6e09ac18501002436d486165761b809cab019af9 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 11 Feb 2024 11:20:23 +0000 Subject: [PATCH 06/20] remove the SubjectOwner::None hack Signed-off-by: William Woodruff --- src/rust/src/x509/verify.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index e6e26b1802ab..061e8e01bb5d 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -141,13 +141,7 @@ impl PolicyBuilder { None => datetime_now(py)?, }; - let policy = OwnedPolicy::try_new(SubjectOwner::None, |_| { - Ok::, pyo3::PyErr>(PyCryptoPolicy(Policy::client( - PyCryptoOps {}, - time, - self.max_chain_depth, - ))) - })?; + let policy = PyCryptoPolicy(Policy::client(PyCryptoOps {}, time, self.max_chain_depth)); Ok(PyClientVerifier { policy, store }) } @@ -203,7 +197,6 @@ enum SubjectOwner { // so, which was only stabilized with 3.10. DNSName(String), IPAddress(pyo3::Py), - None, } self_cell::self_cell!( @@ -233,14 +226,14 @@ struct PyVerifiedClient { module = "cryptography.hazmat.bindings._rust.x509" )] struct PyClientVerifier { - policy: OwnedPolicy, + policy: PyCryptoPolicy<'static>, #[pyo3(get)] store: pyo3::Py, } impl PyClientVerifier { fn as_policy(&self) -> &Policy<'_, PyCryptoOps> { - &self.policy.borrow_dependent().0 + &self.policy.0 } } @@ -423,7 +416,6 @@ fn build_subject<'a>( Ok(Subject::IP(ip_addr)) } - SubjectOwner::None => Err(pyo3::exceptions::PyValueError::new_err("missing subject")), } } From 0b92cb57b5e85079c4ea559ecd15df3c3dc0e3ad Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 11 Feb 2024 11:25:33 +0000 Subject: [PATCH 07/20] docs: fix ClientVerifier Signed-off-by: William Woodruff --- docs/x509/verification.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index 0c0444b0c265..cd504e956805 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -130,9 +130,8 @@ the root of trust: A ClientVerifier verifies client certificates. It contains and describes various pieces of configurable path - validation logic, such as which subject to expect, how deep prospective - validation chains may go, which signature algorithms are allowed, and - so forth. + validation logic, such as how deep prospective validation chains may go, + which signature algorithms are allowed, and so forth. ClientVerifier instances cannot be constructed directly; :class:`PolicyBuilder` must be used. From 00c0c6f53affc4fe5fad9a3e22f0b9f55146e906 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 11 Feb 2024 11:27:04 +0000 Subject: [PATCH 08/20] verification: replace match with if Signed-off-by: William Woodruff --- .../cryptography-x509-verification/src/policy/mod.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/rust/cryptography-x509-verification/src/policy/mod.rs b/src/rust/cryptography-x509-verification/src/policy/mod.rs index 6bfa3875c925..55b843d6e1ab 100644 --- a/src/rust/cryptography-x509-verification/src/policy/mod.rs +++ b/src/rust/cryptography-x509-verification/src/policy/mod.rs @@ -314,16 +314,17 @@ impl<'a, B: CryptoOps> Policy<'a, B> { Criticality::Agnostic, Some(ee::key_usage), ), - subject_alternative_name: match has_subject { + subject_alternative_name: if has_subject { // CA/B 7.1.2.7.12 Subscriber Certificate Subject Alternative Name - true => ExtensionValidator::present( + ExtensionValidator::present( Criticality::Agnostic, Some(ee::subject_alternative_name), - ), + ) + } else { // Under client validation, we return the SAN rather than verifying // it directly against the profile. As such, we require it here // but don't supply any custom validator logic. - false => ExtensionValidator::present(Criticality::Agnostic, None), + ExtensionValidator::present(Criticality::Agnostic, None) }, // 5280 4.2.1.9: Basic Constraints basic_constraints: ExtensionValidator::maybe_present( From aeaceb24775de3d01bb3bf7adf9f6e48b448fbe9 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 11 Feb 2024 14:30:18 +0000 Subject: [PATCH 09/20] return GNs directly, not whole extension Signed-off-by: William Woodruff --- docs/x509/verification.rst | 8 ++++---- .../hazmat/bindings/_rust/x509.pyi | 2 +- src/rust/src/x509/verify.rs | 20 +++++-------------- tests/x509/verification/test_verification.py | 11 +++------- 4 files changed, 13 insertions(+), 28 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index cd504e956805..af69a6946c45 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -108,12 +108,12 @@ the root of trust: .. versionadded:: 43.0.0 - .. attribute:: subject + .. attribute:: subjects - :type: :class:`~cryptography.x509.Extension` + :type: list of :class:`~cryptography.x509.GeneralName` - The subject of the verified client certificate, as presented in the - :class:`~cryptography.x509.SubjectAlternativeName` extension. + The subjects presented in the verified client's Subject Alternative Name + extension. .. attribute:: chain diff --git a/src/cryptography/hazmat/bindings/_rust/x509.pyi b/src/cryptography/hazmat/bindings/_rust/x509.pyi index 14b4f14261dd..aa85657fcfd8 100644 --- a/src/cryptography/hazmat/bindings/_rust/x509.pyi +++ b/src/cryptography/hazmat/bindings/_rust/x509.pyi @@ -69,7 +69,7 @@ class PolicyBuilder: class VerifiedClient: @property - def subject(self) -> x509.Extension[x509.SubjectAlternativeName]: ... + def subjects(self) -> list[x509.GeneralName]: ... @property def chain(self) -> list[x509.Certificate]: ... diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 061e8e01bb5d..2da79e318d34 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -14,10 +14,10 @@ use cryptography_x509_verification::{ use pyo3::IntoPy; use crate::backend::keys; +use crate::types; use crate::x509::certificate::Certificate as PyCertificate; use crate::x509::common::{datetime_now, datetime_to_py, py_to_datetime}; use crate::x509::sign; -use crate::{asn1::oid_to_py_oid, types}; use crate::{ error::{CryptographyError, CryptographyResult}, x509, @@ -215,7 +215,7 @@ self_cell::self_cell!( )] struct PyVerifiedClient { #[pyo3(get)] - subject: pyo3::Py, + subjects: pyo3::Py, #[pyo3(get)] chain: pyo3::Py, } @@ -289,21 +289,11 @@ impl PyClientVerifier { .get_extension(&SUBJECT_ALTERNATIVE_NAME_OID) .unwrap(); - let py_san = - types::SUBJECT_ALTERNATIVE_NAME - .get(py)? - .call1((x509::parse_general_names( - py, - &leaf_san.value::>()?, - )?,))?; - - let py_oid = oid_to_py_oid(py, &leaf_san.extn_id)?; - let py_san_ext = types::EXTENSION - .get(py)? - .call1((py_oid, leaf_san.critical, py_san))?; + let py_gns = + x509::parse_general_names(py, &leaf_san.value::>()?)?; Ok(PyVerifiedClient { - subject: py_san_ext.into_py(py), + subjects: py_gns, chain: py_chain.into_py(py), }) } diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index b05298d0d279..e90538c11d52 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -122,16 +122,11 @@ def test_verify(self): verifier = builder.build_client_verifier() verified_client = verifier.verify(leaf, []) - assert isinstance(verified_client.subject, x509.Extension) - assert isinstance( - verified_client.subject.value, x509.SubjectAlternativeName - ) assert verified_client.chain == [leaf] - dns_names = verified_client.subject.value.get_values_for_type( - x509.DNSName - ) - assert set(dns_names) == {"www.cryptography.io", "cryptography.io"} + assert x509.DNSName("www.cryptography.io") in verified_client.subjects + assert x509.DNSName("cryptography.io") in verified_client.subjects + assert len(verified_client.subjects) == 2 class TestServerVerifier: From dd8948978a0d51cfa5214e0f9643c1f62345d100 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 11 Feb 2024 14:34:22 +0000 Subject: [PATCH 10/20] docs/verification: document UnsupportedGeneralNameType raise Signed-off-by: William Woodruff --- docs/x509/verification.rst | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index af69a6946c45..ab360417b482 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -169,6 +169,8 @@ the root of trust: :raises VerificationError: If a valid chain cannot be constructed + :raises UnsupportedGeneralNameType: If a valid chain exists, but contains an unsupported general name type + .. class:: ServerVerifier .. versionadded:: 42.0.0 From 0a49f047d2f9724d0dbea1854d2ab01ddc7d2ff9 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 28 Feb 2024 20:36:05 -0500 Subject: [PATCH 11/20] lib: RFC822 checks on NCs --- src/rust/cryptography-x509-verification/src/lib.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/rust/cryptography-x509-verification/src/lib.rs b/src/rust/cryptography-x509-verification/src/lib.rs index 5ded892d5cbb..a119b3a67d4e 100644 --- a/src/rust/cryptography-x509-verification/src/lib.rs +++ b/src/rust/cryptography-x509-verification/src/lib.rs @@ -19,6 +19,7 @@ use cryptography_x509::{ name::GeneralName, oid::{NAME_CONSTRAINTS_OID, SUBJECT_ALTERNATIVE_NAME_OID}, }; +use types::{RFC822Constraint, RFC822Name}; use crate::certificate::cert_is_self_issued; use crate::ops::{CryptoOps, VerificationCertificate}; @@ -136,6 +137,19 @@ impl<'a, 'chain> NameChain<'a, 'chain> { ))), } } + (GeneralName::RFC822Name(pattern), GeneralName::RFC822Name(name)) => { + match (RFC822Constraint::new(pattern.0), RFC822Name::new(name.0)) { + (Some(pattern), Some(name)) => Ok(Applied(pattern.matches(&name))), + (_, None) => Err(ValidationError::Other(format!( + "unsatisfiable RFC822 name constraint: malformed SAN {:?}", + name.0, + ))), + (None, _) => Err(ValidationError::Other(format!( + "malformed RFC822 name constraints: {:?}", + pattern.0 + ))), + } + } _ => Ok(Skipped), } } From 83fedfd9e59707a56bef5264342242a1cba26928 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 4 Mar 2024 21:50:23 -0500 Subject: [PATCH 12/20] test_limbo: enable client tests --- tests/x509/verification/test_limbo.py | 36 +++++++++++++++++++-------- 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/tests/x509/verification/test_limbo.py b/tests/x509/verification/test_limbo.py index c745bdbe5729..0c89be55b55f 100644 --- a/tests/x509/verification/test_limbo.py +++ b/tests/x509/verification/test_limbo.py @@ -13,6 +13,7 @@ from cryptography.x509 import load_pem_x509_certificate from cryptography.x509.verification import ( PolicyBuilder, + ServerVerifier, Store, VerificationError, ) @@ -78,12 +79,14 @@ def _get_limbo_peer(expected_peer): kind = expected_peer["kind"] - assert kind in ("DNS", "IP") + assert kind in ("DNS", "IP", "RFC822") value = expected_peer["value"] if kind == "DNS": return x509.DNSName(value) - else: + elif kind == "IP": return x509.IPAddress(ipaddress.ip_address(value)) + else: + return x509.RFC822Name(value) def _limbo_testcase(id_, testcase): @@ -95,14 +98,10 @@ def _limbo_testcase(id_, testcase): if unsupported: pytest.skip(f"explicitly skipped features: {unsupported}") - if testcase["validation_kind"] != "SERVER": - pytest.skip("non-SERVER testcase") - assert testcase["signature_algorithms"] == [] assert testcase["extended_key_usage"] == [] or testcase[ "extended_key_usage" ] == ["serverAuth"] - assert testcase["expected_peer_names"] == [] trusted_certs = [ load_pem_x509_certificate(cert.encode()) @@ -115,7 +114,6 @@ def _limbo_testcase(id_, testcase): peer_certificate = load_pem_x509_certificate( testcase["peer_certificate"].encode() ) - peer_name = _get_limbo_peer(testcase["expected_peer_name"]) validation_time = testcase["validation_time"] validation_time = ( datetime.datetime.fromisoformat(validation_time) @@ -131,12 +129,28 @@ def _limbo_testcase(id_, testcase): if max_chain_depth is not None: builder = builder.max_chain_depth(max_chain_depth) - verifier = builder.build_server_verifier(peer_name) + if testcase["validation_kind"] == "SERVER": + peer_name = _get_limbo_peer(testcase["expected_peer_name"]) + verifier = builder.build_server_verifier(peer_name) + else: + verifier = builder.build_client_verifier() if should_pass: - built_chain = verifier.verify( - peer_certificate, untrusted_intermediates - ) + if isinstance(verifier, ServerVerifier): + built_chain = verifier.verify( + peer_certificate, untrusted_intermediates + ) + else: + verified_client = verifier.verify( + peer_certificate, untrusted_intermediates + ) + + expected_subjects = { + _get_limbo_peer(p) for p in testcase["expected_peer_names"] + } + assert expected_subjects.issubset(verified_client.subjects) + + built_chain = verified_client.chain # Assert that the verifier returns chains in [EE, ..., TA] order. assert built_chain[0] == peer_certificate From 63a8b3fdb8ae3fed0d5842ccfca74863d0ed0dbb Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 4 Mar 2024 21:53:19 -0500 Subject: [PATCH 13/20] tests: flake --- tests/x509/verification/test_limbo.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/x509/verification/test_limbo.py b/tests/x509/verification/test_limbo.py index 0c89be55b55f..e2d64e7ce132 100644 --- a/tests/x509/verification/test_limbo.py +++ b/tests/x509/verification/test_limbo.py @@ -12,6 +12,7 @@ from cryptography import x509 from cryptography.x509 import load_pem_x509_certificate from cryptography.x509.verification import ( + ClientVerifier, PolicyBuilder, ServerVerifier, Store, @@ -129,6 +130,7 @@ def _limbo_testcase(id_, testcase): if max_chain_depth is not None: builder = builder.max_chain_depth(max_chain_depth) + verifier: ServerVerifier | ClientVerifier if testcase["validation_kind"] == "SERVER": peer_name = _get_limbo_peer(testcase["expected_peer_name"]) verifier = builder.build_server_verifier(peer_name) From 12a5452c7e4351b2ffb78c9d920a2f4db10268cc Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Mon, 4 Mar 2024 22:08:41 -0500 Subject: [PATCH 14/20] test_verification: more Python API coverage --- tests/x509/verification/test_verification.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index e90538c11d52..e8c280fce0e6 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -106,6 +106,12 @@ def test_store_rejects_non_certificates(self): class TestClientVerifier: + def test_build_client_verifier_missing_store(self): + with pytest.raises( + ValueError, match="A client verifier must have a trust store" + ): + PolicyBuilder().build_client_verifier() + def test_verify(self): # expires 2018-11-16 01:15:03 UTC leaf = _load_cert( @@ -115,12 +121,16 @@ def test_verify(self): store = Store([leaf]) - builder = PolicyBuilder().store(store) - builder = builder.time( - datetime.datetime.fromisoformat("2018-11-16T00:00:00+00:00") + validation_time = datetime.datetime.fromisoformat( + "2018-11-16T00:00:00+00:00" ) + builder = PolicyBuilder().store(store) + builder = builder.time(validation_time).max_chain_depth(16) verifier = builder.build_client_verifier() + assert verifier.validation_time == validation_time.replace(tzinfo=None) + assert verifier.max_chain_depth == 16 + verified_client = verifier.verify(leaf, []) assert verified_client.chain == [leaf] From eeda511b8018c1420c061d95cde6b4b2ecbfee6b Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 10 Mar 2024 16:50:58 -0400 Subject: [PATCH 15/20] verification: filter GNs by NC support --- docs/x509/verification.rst | 5 ++++- src/rust/src/x509/verify.rs | 31 ++++++++++++++++++++++--------- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index ab360417b482..94b7ed8b65d7 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -113,7 +113,10 @@ the root of trust: :type: list of :class:`~cryptography.x509.GeneralName` The subjects presented in the verified client's Subject Alternative Name - extension. + extension. This list only contains subjects that currently have + Name Constraint support, meaning that it only contains DNS names, + email addresses and IP addresses even if the Subject Alternative Name + contains additional types. .. attribute:: chain diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 2da79e318d34..02f2825817d9 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -3,7 +3,8 @@ // for complete details. use cryptography_x509::{ - certificate::Certificate, extensions::SubjectAlternativeName, oid::SUBJECT_ALTERNATIVE_NAME_OID, + certificate::Certificate, extensions::SubjectAlternativeName, name::GeneralName, + oid::SUBJECT_ALTERNATIVE_NAME_OID, }; use cryptography_x509_verification::{ ops::{CryptoOps, VerificationCertificate}, @@ -14,14 +15,13 @@ use cryptography_x509_verification::{ use pyo3::IntoPy; use crate::backend::keys; +use crate::error::{CryptographyError, CryptographyResult}; use crate::types; use crate::x509::certificate::Certificate as PyCertificate; use crate::x509::common::{datetime_now, datetime_to_py, py_to_datetime}; use crate::x509::sign; -use crate::{ - error::{CryptographyError, CryptographyResult}, - x509, -}; + +use super::parse_general_name; pub(crate) struct PyCryptoOps {} @@ -279,7 +279,7 @@ impl PyClientVerifier { py_chain.append(c.extra())?; } - // NOTE: These `unwrap()` cannot fail, since the underlying policy + // NOTE: These `unwrap()`s cannot fail, since the underlying policy // enforces the presence of a SAN and the well-formedness of the // extension set. let leaf_san = &chain[0] @@ -289,11 +289,24 @@ impl PyClientVerifier { .get_extension(&SUBJECT_ALTERNATIVE_NAME_OID) .unwrap(); - let py_gns = - x509::parse_general_names(py, &leaf_san.value::>()?)?; + let leaf_gns = leaf_san.value::>()?; + + // Instead of returning all general names, we return only ones + // that we currently have name constraint implementations for. + let filtered_gns = leaf_gns.filter(|gn| { + matches!( + gn, + GeneralName::DNSName(_) | GeneralName::IPAddress(_) | GeneralName::RFC822Name(_) + ) + }); + + let filtered_py_gns = pyo3::types::PyList::empty(py); + for filtered_gn in filtered_gns { + filtered_py_gns.append(parse_general_name(py, filtered_gn)?)?; + } Ok(PyVerifiedClient { - subjects: py_gns, + subjects: filtered_py_gns.into(), chain: py_chain.into_py(py), }) } From b3fa9cf37ded45d3fe00c68e46b8823daf9713ce Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sun, 10 Mar 2024 17:36:55 -0400 Subject: [PATCH 16/20] verification: forbid unsupported NC GNs This is what we should have been doing originally, per RFC 5280 4.2.1.10: > If a name constraints extension that is marked as critical > imposes constraints on a particular name form, and an instance of > that name form appears in the subject field or subjectAltName > extension of a subsequent certificate, then the application MUST > either process the constraint or reject the certificate. --- .../cryptography-x509-verification/src/lib.rs | 12 ++++++++++ src/rust/src/x509/verify.rs | 22 ++++--------------- 2 files changed, 16 insertions(+), 18 deletions(-) diff --git a/src/rust/cryptography-x509-verification/src/lib.rs b/src/rust/cryptography-x509-verification/src/lib.rs index 1c468be67b72..036e9dcd1b0f 100644 --- a/src/rust/cryptography-x509-verification/src/lib.rs +++ b/src/rust/cryptography-x509-verification/src/lib.rs @@ -151,6 +151,18 @@ impl<'a, 'chain> NameChain<'a, 'chain> { ))), } } + // All other matching pairs of (constraint, name) are currently unsupported. + (GeneralName::OtherName(_), GeneralName::OtherName(_)) + | (GeneralName::X400Address(_), GeneralName::X400Address(_)) + | (GeneralName::DirectoryName(_), GeneralName::DirectoryName(_)) + | (GeneralName::EDIPartyName(_), GeneralName::EDIPartyName(_)) + | ( + GeneralName::UniformResourceIdentifier(_), + GeneralName::UniformResourceIdentifier(_), + ) + | (GeneralName::RegisteredID(_), GeneralName::RegisteredID(_)) => Err( + ValidationError::Other("unsupported name constraint".to_string()), + ), _ => Ok(Skipped), } } diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 02f2825817d9..2c65f6327103 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -3,8 +3,7 @@ // for complete details. use cryptography_x509::{ - certificate::Certificate, extensions::SubjectAlternativeName, name::GeneralName, - oid::SUBJECT_ALTERNATIVE_NAME_OID, + certificate::Certificate, extensions::SubjectAlternativeName, oid::SUBJECT_ALTERNATIVE_NAME_OID, }; use cryptography_x509_verification::{ ops::{CryptoOps, VerificationCertificate}, @@ -21,7 +20,7 @@ use crate::x509::certificate::Certificate as PyCertificate; use crate::x509::common::{datetime_now, datetime_to_py, py_to_datetime}; use crate::x509::sign; -use super::parse_general_name; +use super::parse_general_names; pub(crate) struct PyCryptoOps {} @@ -290,23 +289,10 @@ impl PyClientVerifier { .unwrap(); let leaf_gns = leaf_san.value::>()?; - - // Instead of returning all general names, we return only ones - // that we currently have name constraint implementations for. - let filtered_gns = leaf_gns.filter(|gn| { - matches!( - gn, - GeneralName::DNSName(_) | GeneralName::IPAddress(_) | GeneralName::RFC822Name(_) - ) - }); - - let filtered_py_gns = pyo3::types::PyList::empty(py); - for filtered_gn in filtered_gns { - filtered_py_gns.append(parse_general_name(py, filtered_gn)?)?; - } + let py_gns = parse_general_names(py, &leaf_gns)?; Ok(PyVerifiedClient { - subjects: filtered_py_gns.into(), + subjects: py_gns, chain: py_chain.into_py(py), }) } From df2d3ef9db8fc57056619b5ead79f44c5c4d9a0a Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 13 Mar 2024 16:09:43 -0400 Subject: [PATCH 17/20] docs/verification: remove old sentence Signed-off-by: William Woodruff --- docs/x509/verification.rst | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index 94b7ed8b65d7..ab360417b482 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -113,10 +113,7 @@ the root of trust: :type: list of :class:`~cryptography.x509.GeneralName` The subjects presented in the verified client's Subject Alternative Name - extension. This list only contains subjects that currently have - Name Constraint support, meaning that it only contains DNS names, - email addresses and IP addresses even if the Subject Alternative Name - contains additional types. + extension. .. attribute:: chain From 5e2cfe997f9dd1beaacf3e513d1784f9d086d536 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 15 Mar 2024 11:26:04 -0400 Subject: [PATCH 18/20] verification: ensure the right EKU for client/server paths Signed-off-by: William Woodruff --- .../src/policy/mod.rs | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/src/rust/cryptography-x509-verification/src/policy/mod.rs b/src/rust/cryptography-x509-verification/src/policy/mod.rs index 679c4e05e803..4be4b2faa724 100644 --- a/src/rust/cryptography-x509-verification/src/policy/mod.rs +++ b/src/rust/cryptography-x509-verification/src/policy/mod.rs @@ -19,7 +19,8 @@ use cryptography_x509::common::{ use cryptography_x509::extensions::{BasicConstraints, Extensions, SubjectAlternativeName}; use cryptography_x509::name::GeneralName; use cryptography_x509::oid::{ - BASIC_CONSTRAINTS_OID, EC_SECP256R1, EC_SECP384R1, EC_SECP521R1, EKU_SERVER_AUTH_OID, + BASIC_CONSTRAINTS_OID, EC_SECP256R1, EC_SECP384R1, EC_SECP521R1, EKU_CLIENT_AUTH_OID, + EKU_SERVER_AUTH_OID, }; use once_cell::sync::Lazy; @@ -239,6 +240,7 @@ impl<'a, B: CryptoOps> Policy<'a, B> { subject: Option>, time: asn1::DateTime, max_chain_depth: Option, + extended_key_usage: ObjectIdentifier, ) -> Self { let has_subject = subject.is_some(); Self { @@ -246,7 +248,7 @@ impl<'a, B: CryptoOps> Policy<'a, B> { max_chain_depth: max_chain_depth.unwrap_or(DEFAULT_MAX_CHAIN_DEPTH), subject, validation_time: time, - extended_key_usage: EKU_SERVER_AUTH_OID.clone(), + extended_key_usage, minimum_rsa_modulus: WEBPKI_MINIMUM_RSA_MODULUS, permitted_public_key_algorithms: Arc::clone(&*WEBPKI_PERMITTED_SPKI_ALGORITHMS), permitted_signature_algorithms: Arc::clone(&*WEBPKI_PERMITTED_SIGNATURE_ALGORITHMS), @@ -350,7 +352,13 @@ impl<'a, B: CryptoOps> Policy<'a, B> { /// website (i.e. server) certificates. For that, you **must** use /// [`Policy::server`]. pub fn client(ops: B, time: asn1::DateTime, max_chain_depth: Option) -> Self { - Self::new(ops, None, time, max_chain_depth) + Self::new( + ops, + None, + time, + max_chain_depth, + EKU_CLIENT_AUTH_OID.clone(), + ) } /// Create a new policy with defaults for the server certificate profile @@ -361,7 +369,13 @@ impl<'a, B: CryptoOps> Policy<'a, B> { time: asn1::DateTime, max_chain_depth: Option, ) -> Self { - Self::new(ops, Some(subject), time, max_chain_depth) + Self::new( + ops, + Some(subject), + time, + max_chain_depth, + EKU_SERVER_AUTH_OID.clone(), + ) } fn permits_basic(&self, cert: &Certificate<'_>) -> Result<(), ValidationError> { From 4c9ac45f00ebe3782ea41d45064d64eda59fa211 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 19 Mar 2024 21:05:49 -0400 Subject: [PATCH 19/20] test_limbo: fixup EKU assertion --- tests/x509/verification/test_limbo.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/x509/verification/test_limbo.py b/tests/x509/verification/test_limbo.py index e2d64e7ce132..ec0b782d8fa1 100644 --- a/tests/x509/verification/test_limbo.py +++ b/tests/x509/verification/test_limbo.py @@ -100,9 +100,6 @@ def _limbo_testcase(id_, testcase): pytest.skip(f"explicitly skipped features: {unsupported}") assert testcase["signature_algorithms"] == [] - assert testcase["extended_key_usage"] == [] or testcase[ - "extended_key_usage" - ] == ["serverAuth"] trusted_certs = [ load_pem_x509_certificate(cert.encode()) @@ -132,9 +129,13 @@ def _limbo_testcase(id_, testcase): verifier: ServerVerifier | ClientVerifier if testcase["validation_kind"] == "SERVER": + assert testcase["extended_key_usage"] == [] or testcase[ + "extended_key_usage" + ] == ["serverAuth"] peer_name = _get_limbo_peer(testcase["expected_peer_name"]) verifier = builder.build_server_verifier(peer_name) else: + assert testcase["extended_key_usage"] == ["clientAuth"] verifier = builder.build_client_verifier() if should_pass: From 26f800a4a292b85acbb36191035d8de09129453e Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 20 Mar 2024 20:45:37 -0400 Subject: [PATCH 20/20] verification: feedback --- .../src/policy/extension.rs | 20 ++++++++++-------- .../src/policy/mod.rs | 21 +++++++------------ tests/x509/verification/test_limbo.py | 6 +++--- 3 files changed, 22 insertions(+), 25 deletions(-) diff --git a/src/rust/cryptography-x509-verification/src/policy/extension.rs b/src/rust/cryptography-x509-verification/src/policy/extension.rs index 9ab88ab5189d..a707b0d8d65f 100644 --- a/src/rust/cryptography-x509-verification/src/policy/extension.rs +++ b/src/rust/cryptography-x509-verification/src/policy/extension.rs @@ -303,15 +303,17 @@ pub(crate) mod ee { _ => (), }; - let san: SubjectAlternativeName<'_> = extn.value()?; - if !policy - .subject - .as_ref() - .map_or_else(|| false, |sub| sub.matches(&san)) - { - return Err(ValidationError::Other( - "leaf certificate has no matching subjectAltName".into(), - )); + // 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::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 4be4b2faa724..22f5a13dc0aa 100644 --- a/src/rust/cryptography-x509-verification/src/policy/mod.rs +++ b/src/rust/cryptography-x509-verification/src/policy/mod.rs @@ -242,7 +242,6 @@ impl<'a, B: CryptoOps> Policy<'a, B> { max_chain_depth: Option, extended_key_usage: ObjectIdentifier, ) -> Self { - let has_subject = subject.is_some(); Self { ops, max_chain_depth: max_chain_depth.unwrap_or(DEFAULT_MAX_CHAIN_DEPTH), @@ -316,18 +315,14 @@ impl<'a, B: CryptoOps> Policy<'a, B> { Criticality::Agnostic, Some(ee::key_usage), ), - subject_alternative_name: if has_subject { - // CA/B 7.1.2.7.12 Subscriber Certificate Subject Alternative Name - ExtensionValidator::present( - Criticality::Agnostic, - Some(ee::subject_alternative_name), - ) - } else { - // Under client validation, we return the SAN rather than verifying - // it directly against the profile. As such, we require it here - // but don't supply any custom validator logic. - ExtensionValidator::present(Criticality::Agnostic, None) - }, + // 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. + subject_alternative_name: ExtensionValidator::present( + Criticality::Agnostic, + Some(ee::subject_alternative_name), + ), // 5280 4.2.1.9: Basic Constraints basic_constraints: ExtensionValidator::maybe_present( Criticality::Agnostic, diff --git a/tests/x509/verification/test_limbo.py b/tests/x509/verification/test_limbo.py index ec0b782d8fa1..2675ca735475 100644 --- a/tests/x509/verification/test_limbo.py +++ b/tests/x509/verification/test_limbo.py @@ -148,10 +148,10 @@ def _limbo_testcase(id_, testcase): peer_certificate, untrusted_intermediates ) - expected_subjects = { + expected_subjects = [ _get_limbo_peer(p) for p in testcase["expected_peer_names"] - } - assert expected_subjects.issubset(verified_client.subjects) + ] + assert expected_subjects == verified_client.subjects built_chain = verified_client.chain