From 4a3e7dcc977cc3f9091154c15e6ecdcee3b1d00d Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Wed, 20 Mar 2024 21:00:00 -0400 Subject: verification: client verification APIs (#10345) * verification: WIP client verification skeleton Signed-off-by: William Woodruff * verify: fill in build_client_verifier Signed-off-by: William Woodruff * implement ClientVerifier.verify Signed-off-by: William Woodruff * verification: make Python 3.8 happy Signed-off-by: William Woodruff * switch to a full VerifiedClient type Signed-off-by: William Woodruff * remove the SubjectOwner::None hack Signed-off-by: William Woodruff * docs: fix ClientVerifier Signed-off-by: William Woodruff * verification: replace match with if Signed-off-by: William Woodruff * return GNs directly, not whole extension Signed-off-by: William Woodruff * docs/verification: document UnsupportedGeneralNameType raise Signed-off-by: William Woodruff * lib: RFC822 checks on NCs * test_limbo: enable client tests * tests: flake * test_verification: more Python API coverage * verification: filter GNs by NC support * 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. * docs/verification: remove old sentence Signed-off-by: William Woodruff * verification: ensure the right EKU for client/server paths Signed-off-by: William Woodruff * test_limbo: fixup EKU assertion * verification: feedback --------- Signed-off-by: William Woodruff --- docs/x509/verification.rst | 84 +++++++++++++- src/cryptography/hazmat/bindings/_rust/x509.pyi | 20 ++++ src/cryptography/x509/verification.py | 4 + src/rust/cryptography-x509-verification/src/lib.rs | 14 +++ .../src/policy/extension.rs | 20 ++-- .../src/policy/mod.rs | 50 +++++++-- src/rust/src/x509/verify.rs | 122 ++++++++++++++++++++- tests/x509/verification/test_limbo.py | 45 +++++--- tests/x509/verification/test_verification.py | 34 ++++++ 9 files changed, 361 insertions(+), 32 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index 6afc75f..ab36041 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -104,6 +104,73 @@ the root of trust: :class:`cryptography.x509.general_name.DNSName`, :class:`cryptography.x509.general_name.IPAddress`. +.. class:: VerifiedClient + + .. versionadded:: 43.0.0 + + .. attribute:: subjects + + :type: list of :class:`~cryptography.x509.GeneralName` + + The subjects presented in the verified client's Subject Alternative Name + 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 + + .. versionadded:: 43.0.0 + + A ClientVerifier verifies client certificates. + + It contains and describes various pieces of configurable path + 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. + + .. 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 new instance of :class:`VerifiedClient` + + :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 @@ -174,7 +241,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 +277,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 418184f..aa85657 100644 --- a/src/cryptography/hazmat/bindings/_rust/x509.pyi +++ b/src/cryptography/hazmat/bindings/_rust/x509.pyi @@ -62,10 +62,30 @@ 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 VerifiedClient: + @property + def subjects(self) -> list[x509.GeneralName]: ... + @property + def chain(self) -> list[x509.Certificate]: ... + +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], + ) -> VerifiedClient: ... + class ServerVerifier: @property def subject(self) -> x509.verification.Subject: ... diff --git a/src/cryptography/x509/verification.py b/src/cryptography/x509/verification.py index ab1a37a..191705e 100644 --- a/src/cryptography/x509/verification.py +++ b/src/cryptography/x509/verification.py @@ -12,6 +12,8 @@ from cryptography.x509.general_name import DNSName, IPAddress __all__ = [ "Store", "Subject", + "VerifiedClient", + "ClientVerifier", "ServerVerifier", "PolicyBuilder", "VerificationError", @@ -19,6 +21,8 @@ __all__ = [ Store = rust_x509.Store Subject = typing.Union[DNSName, IPAddress] +VerifiedClient = rust_x509.VerifiedClient +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/lib.rs b/src/rust/cryptography-x509-verification/src/lib.rs index 01bc76a..036e9dc 100644 --- a/src/rust/cryptography-x509-verification/src/lib.rs +++ b/src/rust/cryptography-x509-verification/src/lib.rs @@ -20,6 +20,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}; @@ -137,6 +138,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 + ))), + } + } // All other matching pairs of (constraint, name) are currently unsupported. (GeneralName::OtherName(_), GeneralName::OtherName(_)) | (GeneralName::X400Address(_), GeneralName::X400Address(_)) diff --git a/src/rust/cryptography-x509-verification/src/policy/extension.rs b/src/rust/cryptography-x509-verification/src/policy/extension.rs index 9ab88ab..a707b0d 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 8f704a3..22f5a13 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; @@ -234,20 +235,19 @@ pub struct Policy<'a, B: CryptoOps> { } impl<'a, B: CryptoOps> Policy<'a, B> { - /// Create a new policy with defaults for the server certificate profile - /// defined in the CA/B Forum's Basic Requirements. - pub fn server( + fn new( ops: B, - subject: Subject<'a>, + subject: Option>, time: asn1::DateTime, max_chain_depth: Option, + extended_key_usage: ObjectIdentifier, ) -> Self { 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(), + 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), @@ -316,6 +316,9 @@ impl<'a, B: CryptoOps> Policy<'a, B> { Some(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. subject_alternative_name: ExtensionValidator::present( Criticality::Agnostic, Some(ee::subject_alternative_name), @@ -337,6 +340,39 @@ 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, + EKU_CLIENT_AUTH_OID.clone(), + ) + } + + /// 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, + EKU_SERVER_AUTH_OID.clone(), + ) + } + 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 d35c3a6..2c65f63 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -2,13 +2,16 @@ // 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}; @@ -17,6 +20,8 @@ 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_names; + pub(crate) struct PyCryptoOps {} impl CryptoOps for PyCryptoOps { @@ -118,6 +123,28 @@ 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)?, + }; + + let policy = PyCryptoPolicy(Policy::client(PyCryptoOps {}, time, self.max_chain_depth)); + + Ok(PyClientVerifier { policy, store }) + } + fn build_server_verifier( &self, py: pyo3::Python<'_>, @@ -182,6 +209,97 @@ self_cell::self_cell!( #[pyo3::pyclass( frozen, + name = "VerifiedClient", + module = "cryptography.hazmat.bindings._rust.x509" +)] +struct PyVerifiedClient { + #[pyo3(get)] + subjects: pyo3::Py, + #[pyo3(get)] + chain: pyo3::Py, +} + +#[pyo3::pyclass( + frozen, + name = "ClientVerifier", + module = "cryptography.hazmat.bindings._rust.x509" +)] +struct PyClientVerifier { + policy: PyCryptoPolicy<'static>, + #[pyo3(get)] + store: pyo3::Py, +} + +impl PyClientVerifier { + fn as_policy(&self) -> &Policy<'_, PyCryptoOps> { + &self.policy.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()`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] + .certificate() + .extensions() + .unwrap() + .get_extension(&SUBJECT_ALTERNATIVE_NAME_OID) + .unwrap(); + + let leaf_gns = leaf_san.value::>()?; + let py_gns = parse_general_names(py, &leaf_gns)?; + + Ok(PyVerifiedClient { + subjects: py_gns, + chain: py_chain.into_py(py), + }) + } +} + +#[pyo3::pyclass( + frozen, name = "ServerVerifier", module = "cryptography.hazmat.bindings._rust.x509" )] @@ -333,6 +451,8 @@ 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::()?; module.add_class::()?; diff --git a/tests/x509/verification/test_limbo.py b/tests/x509/verification/test_limbo.py index c745bdb..2675ca7 100644 --- a/tests/x509/verification/test_limbo.py +++ b/tests/x509/verification/test_limbo.py @@ -12,7 +12,9 @@ import pytest from cryptography import x509 from cryptography.x509 import load_pem_x509_certificate from cryptography.x509.verification import ( + ClientVerifier, PolicyBuilder, + ServerVerifier, Store, VerificationError, ) @@ -78,12 +80,14 @@ LIMBO_SKIP_TESTCASES = { 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 +99,7 @@ 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 +112,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 +127,33 @@ 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) + 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: - 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 == verified_client.subjects + + built_chain = verified_client.chain # Assert that the verifier returns chains in [EE, ..., TA] order. assert built_chain[0] == peer_certificate diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index 8c2be70..e8c280f 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -105,6 +105,40 @@ class TestStore: Store(["not a cert"]) # type: ignore[list-item] +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( + os.path.join("x509", "cryptography.io.pem"), + x509.load_pem_x509_certificate, + ) + + store = Store([leaf]) + + 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] + + 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: @pytest.mark.parametrize( ("validation_time", "valid"), -- cgit v1.1