From 37f4cec115da9f5490ef1a0787a78f0d1a4fb583 Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Fri, 6 Sep 2024 17:20:35 +0200 Subject: [PATCH 1/9] Add CustomPolicyBuilder foundation. --- .../hazmat/bindings/_rust/x509.pyi | 16 +- src/cryptography/x509/verification.py | 2 + .../src/policy/extension.rs | 5 +- .../src/policy/mod.rs | 4 +- src/rust/cryptography-x509/src/oid.rs | 11 + src/rust/src/lib.rs | 4 +- src/rust/src/x509/verify.rs | 295 ++++++++++++++---- tests/x509/verification/test_limbo.py | 17 +- tests/x509/verification/test_verification.py | 125 ++++++-- 9 files changed, 372 insertions(+), 107 deletions(-) diff --git a/src/cryptography/hazmat/bindings/_rust/x509.pyi b/src/cryptography/hazmat/bindings/_rust/x509.pyi index aa85657fcfd8..d689c485b683 100644 --- a/src/cryptography/hazmat/bindings/_rust/x509.pyi +++ b/src/cryptography/hazmat/bindings/_rust/x509.pyi @@ -67,9 +67,23 @@ class PolicyBuilder: self, subject: x509.verification.Subject ) -> ServerVerifier: ... +class CustomPolicyBuilder: + def time(self, new_time: datetime.datetime) -> CustomPolicyBuilder: ... + def store(self, new_store: Store) -> CustomPolicyBuilder: ... + def max_chain_depth( + self, new_max_chain_depth: int + ) -> CustomPolicyBuilder: ... + def eku(self, new_eku: x509.ObjectIdentifier) -> CustomPolicyBuilder: ... + 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]: ... + def subject(self) -> x509.Name: ... + @property + def sans(self) -> list[x509.GeneralName] | None: ... @property def chain(self) -> list[x509.Certificate]: ... diff --git a/src/cryptography/x509/verification.py b/src/cryptography/x509/verification.py index b83650681237..fe0153581f85 100644 --- a/src/cryptography/x509/verification.py +++ b/src/cryptography/x509/verification.py @@ -12,6 +12,7 @@ __all__ = [ "ClientVerifier", "PolicyBuilder", + "CustomPolicyBuilder", "ServerVerifier", "Store", "Subject", @@ -25,4 +26,5 @@ ClientVerifier = rust_x509.ClientVerifier ServerVerifier = rust_x509.ServerVerifier PolicyBuilder = rust_x509.PolicyBuilder +CustomPolicyBuilder = rust_x509.CustomPolicyBuilder 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 a01eb490122b..df53c50507a5 100644 --- a/src/rust/cryptography-x509-verification/src/policy/extension.rs +++ b/src/rust/cryptography-x509-verification/src/policy/extension.rs @@ -14,7 +14,8 @@ use cryptography_x509::{ use crate::{ops::CryptoOps, policy::Policy, ValidationError}; -pub(crate) struct ExtensionPolicy { +#[derive(Clone)] +pub struct ExtensionPolicy { pub(crate) authority_information_access: ExtensionValidator, pub(crate) authority_key_identifier: ExtensionValidator, pub(crate) subject_key_identifier: ExtensionValidator, @@ -123,6 +124,7 @@ impl ExtensionPolicy { } /// Represents different criticality states for an extension. +#[derive(Clone)] pub(crate) enum Criticality { /// The extension MUST be marked as critical. Critical, @@ -151,6 +153,7 @@ type MaybeExtensionValidatorCallback = fn(&Policy<'_, B>, &Certificate<'_>, Option<&Extension<'_>>) -> Result<(), ValidationError>; /// Represents different validation states for an extension. +#[derive(Clone)] pub(crate) enum ExtensionValidator { /// The extension MUST NOT be present. NotPresent, diff --git a/src/rust/cryptography-x509-verification/src/policy/mod.rs b/src/rust/cryptography-x509-verification/src/policy/mod.rs index 5616a83a8ceb..a89511fd6d69 100644 --- a/src/rust/cryptography-x509-verification/src/policy/mod.rs +++ b/src/rust/cryptography-x509-verification/src/policy/mod.rs @@ -25,10 +25,12 @@ use cryptography_x509::oid::{ use once_cell::sync::Lazy; use crate::ops::CryptoOps; -use crate::policy::extension::{ca, common, ee, Criticality, ExtensionPolicy, ExtensionValidator}; +use crate::policy::extension::{ca, common, ee, Criticality, ExtensionValidator}; use crate::types::{DNSName, DNSPattern, IPAddress}; use crate::{ValidationError, VerificationCertificate}; +pub use crate::policy::extension::ExtensionPolicy; + // RSA key constraints, as defined in CA/B 6.1.5. static WEBPKI_MINIMUM_RSA_MODULUS: usize = 2048; diff --git a/src/rust/cryptography-x509/src/oid.rs b/src/rust/cryptography-x509/src/oid.rs index fbc440eea122..c29cd42de2ee 100644 --- a/src/rust/cryptography-x509/src/oid.rs +++ b/src/rust/cryptography-x509/src/oid.rs @@ -148,6 +148,17 @@ pub const EKU_ANY_KEY_USAGE_OID: asn1::ObjectIdentifier = asn1::oid!(2, 5, 29, 3 pub const EKU_CERTIFICATE_TRANSPARENCY_OID: asn1::ObjectIdentifier = asn1::oid!(1, 3, 6, 1, 4, 1, 11129, 2, 4, 4); +pub const ALL_EKU_OIDS: [asn1::ObjectIdentifier; 8] = [ + EKU_SERVER_AUTH_OID, + EKU_CLIENT_AUTH_OID, + EKU_CODE_SIGNING_OID, + EKU_EMAIL_PROTECTION_OID, + EKU_TIME_STAMPING_OID, + EKU_OCSP_SIGNING_OID, + EKU_ANY_KEY_USAGE_OID, + EKU_CERTIFICATE_TRANSPARENCY_OID, +]; + pub const PBES2_OID: asn1::ObjectIdentifier = asn1::oid!(1, 2, 840, 113549, 1, 5, 13); pub const PBKDF2_OID: asn1::ObjectIdentifier = asn1::oid!(1, 2, 840, 113549, 1, 5, 12); diff --git a/src/rust/src/lib.rs b/src/rust/src/lib.rs index e15fffa6d32e..c53a948b40aa 100644 --- a/src/rust/src/lib.rs +++ b/src/rust/src/lib.rs @@ -132,8 +132,8 @@ mod _rust { use crate::x509::sct::Sct; #[pymodule_export] use crate::x509::verify::{ - PolicyBuilder, PyClientVerifier, PyServerVerifier, PyStore, PyVerifiedClient, - VerificationError, + CustomPolicyBuilder, PolicyBuilder, PyClientVerifier, PyServerVerifier, PyStore, + PyVerifiedClient, VerificationError, }; } diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index dbe95a494267..045c2d430a8e 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -3,25 +3,31 @@ // for complete details. use cryptography_x509::{ - certificate::Certificate, extensions::SubjectAlternativeName, oid::SUBJECT_ALTERNATIVE_NAME_OID, + certificate::Certificate, + extensions::SubjectAlternativeName, + oid::{ALL_EKU_OIDS, SUBJECT_ALTERNATIVE_NAME_OID}, }; use cryptography_x509_verification::{ ops::{CryptoOps, VerificationCertificate}, - policy::{Policy, Subject}, + policy::{ExtensionPolicy, Policy, Subject}, trust_store::Store, types::{DNSName, IPAddress}, }; -use pyo3::types::{PyAnyMethods, PyListMethods}; +use pyo3::{ + types::{PyAnyMethods, PyListMethods}, + ToPyObject, +}; -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::py_oid_to_oid, backend::keys}; use super::parse_general_names; +#[derive(Clone)] pub(crate) struct PyCryptoOps {} impl CryptoOps for PyCryptoOps { @@ -125,25 +131,128 @@ 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.", - ), - )); - } - }; + build_client_verifier_impl(py, &self.store, &self.time, |time| { + Policy::client(PyCryptoOps {}, time, self.max_chain_depth) + }) + } + + fn build_server_verifier( + &self, + py: pyo3::Python<'_>, + subject: pyo3::PyObject, + ) -> CryptographyResult { + build_server_verifier_impl(py, &self.store, &self.time, subject, |subject, time| { + Policy::server(PyCryptoOps {}, subject, time, self.max_chain_depth) + }) + } +} + +#[pyo3::pyclass(frozen, module = "cryptography.x509.verification")] +pub(crate) struct CustomPolicyBuilder { + time: Option, + store: Option>, + max_chain_depth: Option, + eku: Option, + ca_ext_policy: Option>, + ee_ext_policy: Option>, +} + +impl CustomPolicyBuilder { + /// Clones the builder, requires the GIL token to increase + /// reference count for `self.store`. + fn py_clone(&self, py: pyo3::Python<'_>) -> CustomPolicyBuilder { + CustomPolicyBuilder { + time: self.time.clone(), + store: self.store.as_ref().map(|s| s.clone_ref(py)), + max_chain_depth: self.max_chain_depth, + eku: self.eku.clone(), + ca_ext_policy: self.ca_ext_policy.clone(), + ee_ext_policy: self.ee_ext_policy.clone(), + } + } +} + +#[pyo3::pymethods] +impl CustomPolicyBuilder { + #[new] + fn new() -> CustomPolicyBuilder { + CustomPolicyBuilder { + time: None, + store: None, + max_chain_depth: None, + eku: None, + ca_ext_policy: None, + ee_ext_policy: None, + } + } + + fn time( + &self, + py: pyo3::Python<'_>, + new_time: pyo3::Bound<'_, pyo3::PyAny>, + ) -> CryptographyResult { + policy_builder_set_once_check!(self, time, "validation time"); + + Ok(CustomPolicyBuilder { + time: Some(py_to_datetime(py, new_time)?), + ..self.py_clone(py) + }) + } + + fn store( + &self, + py: pyo3::Python<'_>, + new_store: pyo3::Py, + ) -> CryptographyResult { + policy_builder_set_once_check!(self, store, "trust store"); - let time = match self.time.as_ref() { - Some(t) => t.clone(), - None => datetime_now(py)?, - }; + Ok(CustomPolicyBuilder { + store: Some(new_store), + ..self.py_clone(py) + }) + } + + fn max_chain_depth( + &self, + py: pyo3::Python<'_>, + new_max_chain_depth: u8, + ) -> CryptographyResult { + policy_builder_set_once_check!(self, max_chain_depth, "maximum chain depth"); + + Ok(CustomPolicyBuilder { + max_chain_depth: Some(new_max_chain_depth), + ..self.py_clone(py) + }) + } + + fn eku( + &self, + py: pyo3::Python<'_>, + new_eku: pyo3::Bound<'_, pyo3::PyAny>, + ) -> CryptographyResult { + policy_builder_set_once_check!(self, eku, "EKU"); + + let oid = py_oid_to_oid(new_eku)?; + + if !ALL_EKU_OIDS.contains(&oid) { + return Err(CryptographyError::from( + pyo3::exceptions::PyValueError::new_err( + "Unknown EKU OID. Only EKUs from x509.ExtendedKeyUsageOID are supported.", + ), + )); + } - let policy = PyCryptoPolicy(Policy::client(PyCryptoOps {}, time, self.max_chain_depth)); + Ok(CustomPolicyBuilder { + eku: Some(oid), + ..self.py_clone(py) + }) + } - Ok(PyClientVerifier { policy, store }) + fn build_client_verifier(&self, py: pyo3::Python<'_>) -> CryptographyResult { + build_client_verifier_impl(py, &self.store, &self.time, |time| { + // TODO: Replace with a custom policy once it's implemented in cryptography-x509-verification + Policy::client(PyCryptoOps {}, time, self.max_chain_depth) + }) } fn build_server_verifier( @@ -151,42 +260,80 @@ impl PolicyBuilder { py: pyo3::Python<'_>, subject: pyo3::PyObject, ) -> 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 server verifier must have a trust store.", - ), - )); - } - }; - - let time = match self.time.as_ref() { - Some(t) => t.clone(), - None => datetime_now(py)?, - }; - let subject_owner = build_subject_owner(py, &subject)?; - - let policy = OwnedPolicy::try_new(subject_owner, |subject_owner| { - let subject = build_subject(py, subject_owner)?; - Ok::, pyo3::PyErr>(PyCryptoPolicy(Policy::server( - PyCryptoOps {}, - subject, - time, - self.max_chain_depth, - ))) - })?; - - Ok(PyServerVerifier { - py_subject: subject, - policy, - store, + build_server_verifier_impl(py, &self.store, &self.time, subject, |subject, time| { + // TODO: Replace with a custom policy once it's implemented in cryptography-x509-verification + Policy::server(PyCryptoOps {}, subject, time, self.max_chain_depth) }) } } -struct PyCryptoPolicy<'a>(Policy<'a, PyCryptoOps>); +/// This is a helper to avoid code duplication between PolicyBuilder and CustomPolicyBuilder. +fn build_server_verifier_impl( + py: pyo3::Python<'_>, + store: &Option>, + time: &Option, + subject: pyo3::PyObject, + make_policy: impl Fn(Subject<'_>, asn1::DateTime) -> PyCryptoPolicy<'_>, +) -> CryptographyResult { + let store = match store { + Some(s) => s.clone_ref(py), + None => { + return Err(CryptographyError::from( + pyo3::exceptions::PyValueError::new_err( + "A server verifier must have a trust store.", + ), + )); + } + }; + + let time = match time.as_ref() { + Some(t) => t.clone(), + None => datetime_now(py)?, + }; + let subject_owner = build_subject_owner(py, &subject)?; + + let policy = OwnedPolicy::try_new(subject_owner, |subject_owner| { + let subject = build_subject(py, subject_owner)?; + Ok::, pyo3::PyErr>(make_policy(subject, time)) + })?; + + Ok(PyServerVerifier { + py_subject: subject, + policy, + store, + }) +} + +/// This is a helper to avoid code duplication between PolicyBuilder and CustomPolicyBuilder. +fn build_client_verifier_impl( + py: pyo3::Python<'_>, + store: &Option>, + time: &Option, + make_policy: impl Fn(asn1::DateTime) -> PyCryptoPolicy<'static>, +) -> CryptographyResult { + let store = match 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 time.as_ref() { + Some(t) => t.clone(), + None => datetime_now(py)?, + }; + + Ok(PyClientVerifier { + policy: make_policy(time), + store, + }) +} + +type PyCryptoPolicy<'a> = Policy<'a, PyCryptoOps>; /// This enum exists solely to provide heterogeneously typed ownership for `OwnedPolicy`. enum SubjectOwner { @@ -215,7 +362,9 @@ self_cell::self_cell!( )] pub(crate) struct PyVerifiedClient { #[pyo3(get)] - subjects: pyo3::Py, + subject: pyo3::Py, + #[pyo3(get)] + sans: Option>, #[pyo3(get)] chain: pyo3::Py, } @@ -233,7 +382,7 @@ pub(crate) struct PyClientVerifier { impl PyClientVerifier { fn as_policy(&self) -> &Policy<'_, PyCryptoOps> { - &self.policy.0 + &self.policy } } @@ -290,22 +439,32 @@ impl PyClientVerifier { 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() - .ok() - .unwrap() - .get_extension(&SUBJECT_ALTERNATIVE_NAME_OID) - .unwrap(); + let cert = &chain[0].certificate(); + + let py_sans = || -> pyo3::PyResult> { + let leaf_san_ext = cert + .extensions() + .ok() + .unwrap() + .get_extension(&SUBJECT_ALTERNATIVE_NAME_OID); + + match leaf_san_ext { + Some(leaf_san) => { + let leaf_gns = leaf_san + .value::>() + .map_err(|e| -> CryptographyError { e.into() })?; + let py_gns = parse_general_names(py, &leaf_gns)?; + Ok(Some(py_gns)) + } + None => Ok(None), + } + }()?; - let leaf_gns = leaf_san.value::>()?; - let py_gns = parse_general_names(py, &leaf_gns)?; + let py_subject = crate::x509::parse_name(py, cert.subject())?; Ok(PyVerifiedClient { - subjects: py_gns, + subject: py_subject.to_object(py), + sans: py_sans, chain: py_chain.unbind(), }) } @@ -326,7 +485,7 @@ pub(crate) struct PyServerVerifier { impl PyServerVerifier { fn as_policy(&self) -> &Policy<'_, PyCryptoOps> { - &self.policy.borrow_dependent().0 + self.policy.borrow_dependent() } } diff --git a/tests/x509/verification/test_limbo.py b/tests/x509/verification/test_limbo.py index d0402c4ce30a..114405f2393e 100644 --- a/tests/x509/verification/test_limbo.py +++ b/tests/x509/verification/test_limbo.py @@ -6,6 +6,7 @@ import ipaddress import json import os +from typing import Type, Union import pytest @@ -13,6 +14,7 @@ from cryptography.x509 import load_pem_x509_certificate from cryptography.x509.verification import ( ClientVerifier, + CustomPolicyBuilder, PolicyBuilder, ServerVerifier, Store, @@ -96,7 +98,11 @@ def _get_limbo_peer(expected_peer): return x509.RFC822Name(value) -def _limbo_testcase(id_, testcase): +def _limbo_testcase( + id_, + testcase, + builder_type: Union[Type[PolicyBuilder], Type[CustomPolicyBuilder]], +): if id_ in LIMBO_SKIP_TESTCASES: pytest.skip(f"explicitly skipped testcase: {id_}") @@ -127,7 +133,7 @@ def _limbo_testcase(id_, testcase): max_chain_depth = testcase["max_chain_depth"] should_pass = testcase["expected_result"] == "SUCCESS" - builder = PolicyBuilder().store(Store(trusted_certs)) + builder = builder_type().store(Store(trusted_certs)) if validation_time is not None: builder = builder.time(validation_time) if max_chain_depth is not None: @@ -163,7 +169,7 @@ def _limbo_testcase(id_, testcase): expected_subjects = [ _get_limbo_peer(p) for p in testcase["expected_peer_names"] ] - assert expected_subjects == verified_client.subjects + assert expected_subjects == verified_client.sans built_chain = verified_client.chain @@ -177,7 +183,8 @@ def _limbo_testcase(id_, testcase): verifier.verify(peer_certificate, untrusted_intermediates) -def test_limbo(subtests, pytestconfig): +@pytest.mark.parametrize("builder_type", [PolicyBuilder, CustomPolicyBuilder]) +def test_limbo(subtests, pytestconfig, builder_type): limbo_root = pytestconfig.getoption("--x509-limbo-root", skip=True) limbo_path = os.path.join(limbo_root, "limbo.json") with open(limbo_path, mode="rb") as limbo_file: @@ -187,4 +194,4 @@ def test_limbo(subtests, pytestconfig): with subtests.test(): # NOTE: Pass in the id separately to make pytest # error renderings slightly nicer. - _limbo_testcase(testcase["id"], testcase) + _limbo_testcase(testcase["id"], testcase, builder_type) diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index f5e70bab3538..c9875b930b12 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -6,12 +6,18 @@ import os from functools import lru_cache from ipaddress import IPv4Address +from typing import Type, Union import pytest from cryptography import x509 from cryptography.x509.general_name import DNSName, IPAddress +from cryptography.x509.oid import ( + AuthorityInformationAccessOID, + ExtendedKeyUsageOID, +) from cryptography.x509.verification import ( + CustomPolicyBuilder, PolicyBuilder, Store, VerificationError, @@ -28,60 +34,71 @@ def dummy_store() -> Store: return Store([cert]) -class TestPolicyBuilder: - def test_time_already_set(self): +AnyPolicyBuilder = Union[PolicyBuilder, CustomPolicyBuilder] + + +@pytest.mark.parametrize("builder_type", [PolicyBuilder, CustomPolicyBuilder]) +class TestPolicyBuilderCommon: + """ + Tests functionality that is identical between + PolicyBuilder and CustomPolicyBuilder. + """ + + def test_time_already_set(self, builder_type: Type[AnyPolicyBuilder]): with pytest.raises(ValueError): - PolicyBuilder().time(datetime.datetime.now()).time( + builder_type().time(datetime.datetime.now()).time( datetime.datetime.now() ) - def test_store_already_set(self): + def test_store_already_set(self, builder_type: Type[AnyPolicyBuilder]): with pytest.raises(ValueError): - PolicyBuilder().store(dummy_store()).store(dummy_store()) + builder_type().store(dummy_store()).store(dummy_store()) - def test_max_chain_depth_already_set(self): + def test_max_chain_depth_already_set( + self, builder_type: Type[AnyPolicyBuilder] + ): with pytest.raises(ValueError): - PolicyBuilder().max_chain_depth(8).max_chain_depth(9) + builder_type().max_chain_depth(8).max_chain_depth(9) - def test_ipaddress_subject(self): + def test_ipaddress_subject(self, builder_type: Type[AnyPolicyBuilder]): policy = ( - PolicyBuilder() + builder_type() .store(dummy_store()) .build_server_verifier(IPAddress(IPv4Address("0.0.0.0"))) ) assert policy.subject == IPAddress(IPv4Address("0.0.0.0")) - def test_dnsname_subject(self): + def test_dnsname_subject(self, builder_type: Type[AnyPolicyBuilder]): policy = ( - PolicyBuilder() + builder_type() .store(dummy_store()) .build_server_verifier(DNSName("cryptography.io")) ) assert policy.subject == DNSName("cryptography.io") - def test_subject_bad_types(self): + def test_subject_bad_types(self, builder_type: Type[AnyPolicyBuilder]): # Subject must be a supported GeneralName type with pytest.raises(TypeError): - PolicyBuilder().store(dummy_store()).build_server_verifier( + builder_type().store(dummy_store()).build_server_verifier( "cryptography.io" # type: ignore[arg-type] ) with pytest.raises(TypeError): - PolicyBuilder().store(dummy_store()).build_server_verifier( + builder_type().store(dummy_store()).build_server_verifier( "0.0.0.0" # type: ignore[arg-type] ) with pytest.raises(TypeError): - PolicyBuilder().store(dummy_store()).build_server_verifier( + builder_type().store(dummy_store()).build_server_verifier( IPv4Address("0.0.0.0") # type: ignore[arg-type] ) with pytest.raises(TypeError): - PolicyBuilder().store(dummy_store()).build_server_verifier(None) # type: ignore[arg-type] + builder_type().store(dummy_store()).build_server_verifier(None) # type: ignore[arg-type] - def test_builder_pattern(self): + def test_builder_pattern(self, builder_type: Type[AnyPolicyBuilder]): now = datetime.datetime.now().replace(microsecond=0) store = dummy_store() max_chain_depth = 16 - builder = PolicyBuilder() + builder = builder_type() builder = builder.time(now) builder = builder.store(store) builder = builder.max_chain_depth(max_chain_depth) @@ -92,11 +109,29 @@ def test_builder_pattern(self): assert verifier.store == store assert verifier.max_chain_depth == max_chain_depth - def test_build_server_verifier_missing_store(self): + def test_build_server_verifier_missing_store( + self, builder_type: Type[AnyPolicyBuilder] + ): with pytest.raises( ValueError, match="A server verifier must have a trust store" ): - PolicyBuilder().build_server_verifier(DNSName("cryptography.io")) + builder_type().build_server_verifier(DNSName("cryptography.io")) + + +class TestCustomPolicyBuilder: + def test_eku_already_set(self): + with pytest.raises(ValueError): + CustomPolicyBuilder().eku(ExtendedKeyUsageOID.IPSEC_IKE).eku( + ExtendedKeyUsageOID.IPSEC_IKE + ) + + def test_eku_bad_type(self): + with pytest.raises(TypeError): + CustomPolicyBuilder().eku("not an OID") # type: ignore[arg-type] + + def test_eku_non_eku_oid(self): + with pytest.raises(ValueError): + CustomPolicyBuilder().eku(AuthorityInformationAccessOID.OCSP) class TestStore: @@ -109,14 +144,23 @@ def test_store_rejects_non_certificates(self): Store(["not a cert"]) # type: ignore[list-item] +@pytest.mark.parametrize( + "builder_type", + [ + PolicyBuilder, + CustomPolicyBuilder, + ], +) class TestClientVerifier: - def test_build_client_verifier_missing_store(self): + def test_build_client_verifier_missing_store( + self, builder_type: Type[AnyPolicyBuilder] + ): with pytest.raises( ValueError, match="A client verifier must have a trust store" ): - PolicyBuilder().build_client_verifier() + builder_type().build_client_verifier() - def test_verify(self): + def test_verify(self, builder_type: Type[AnyPolicyBuilder]): # expires 2018-11-16 01:15:03 UTC leaf = _load_cert( os.path.join("x509", "cryptography.io.pem"), @@ -128,7 +172,7 @@ def test_verify(self): validation_time = datetime.datetime.fromisoformat( "2018-11-16T00:00:00+00:00" ) - builder = PolicyBuilder().store(store) + builder = builder_type().store(store) builder = builder.time(validation_time).max_chain_depth(16) verifier = builder.build_client_verifier() @@ -139,11 +183,34 @@ def test_verify(self): 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 + expected_subject = x509.Name( + [ + x509.NameAttribute( + x509.NameOID.ORGANIZATIONAL_UNIT_NAME, "GT48742965" + ), + x509.NameAttribute( + x509.NameOID.ORGANIZATIONAL_UNIT_NAME, + "See www.rapidssl.com/resources/cps (c)14", + ), + x509.NameAttribute( + x509.NameOID.ORGANIZATIONAL_UNIT_NAME, + "Domain Control Validated - RapidSSL(R)", + ), + x509.NameAttribute( + x509.NameOID.COMMON_NAME, "www.cryptography.io" + ), + ] + ) + assert verified_client.subject == expected_subject + assert verified_client.sans is not None + assert x509.DNSName("www.cryptography.io") in verified_client.sans + assert x509.DNSName("cryptography.io") in verified_client.sans + + assert len(verified_client.sans) == 2 - def test_verify_fails_renders_oid(self): + def test_verify_fails_renders_oid( + self, builder_type: Type[AnyPolicyBuilder] + ): leaf = _load_cert( os.path.join("x509", "custom", "ekucrit-testuser-cert.pem"), x509.load_pem_x509_certificate, @@ -155,7 +222,7 @@ def test_verify_fails_renders_oid(self): "2024-06-26T00:00:00+00:00" ) - builder = PolicyBuilder().store(store) + builder = builder_type().store(store) builder = builder.time(validation_time) verifier = builder.build_client_verifier() From f40b86345ada19246a3d48bb5f0cd38a9f87caf4 Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Sat, 7 Sep 2024 10:05:19 +0200 Subject: [PATCH 2/9] Add EKU getters to ClientVerifier and ServerVerifier. --- src/cryptography/hazmat/bindings/_rust/x509.pyi | 4 ++++ src/rust/src/x509/verify.rs | 17 ++++++++++++++++- tests/x509/verification/test_verification.py | 2 ++ 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/cryptography/hazmat/bindings/_rust/x509.pyi b/src/cryptography/hazmat/bindings/_rust/x509.pyi index d689c485b683..bad8c9c285cc 100644 --- a/src/cryptography/hazmat/bindings/_rust/x509.pyi +++ b/src/cryptography/hazmat/bindings/_rust/x509.pyi @@ -94,6 +94,8 @@ class ClientVerifier: def store(self) -> Store: ... @property def max_chain_depth(self) -> int: ... + @property + def eku(self) -> x509.ObjectIdentifier: ... def verify( self, leaf: x509.Certificate, @@ -109,6 +111,8 @@ class ServerVerifier: def store(self) -> Store: ... @property def max_chain_depth(self) -> int: ... + @property + def eku(self) -> x509.ObjectIdentifier: ... def verify( self, leaf: x509.Certificate, diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 045c2d430a8e..7a8026b51865 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -18,11 +18,14 @@ use pyo3::{ ToPyObject, }; -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, + error::{CryptographyError, CryptographyResult}, +}; use crate::{asn1::py_oid_to_oid, backend::keys}; use super::parse_general_names; @@ -401,6 +404,12 @@ impl PyClientVerifier { self.as_policy().max_chain_depth } + #[getter] + fn eku(&self, py: pyo3::Python<'_>) -> pyo3::PyResult> { + let eku = &self.as_policy().extended_key_usage; + return Ok(oid_to_py_oid(py, eku)?.as_unbound().clone_ref(py)); + } + fn verify( &self, py: pyo3::Python<'_>, @@ -504,6 +513,12 @@ impl PyServerVerifier { self.as_policy().max_chain_depth } + #[getter] + fn eku(&self, py: pyo3::Python<'_>) -> pyo3::PyResult> { + let eku = &self.as_policy().extended_key_usage; + return Ok(oid_to_py_oid(py, eku)?.as_unbound().clone_ref(py)); + } + fn verify<'p>( &self, py: pyo3::Python<'p>, diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index c9875b930b12..8b89112b6654 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -108,6 +108,7 @@ def test_builder_pattern(self, builder_type: Type[AnyPolicyBuilder]): assert verifier.validation_time == now assert verifier.store == store assert verifier.max_chain_depth == max_chain_depth + assert verifier.eku == ExtendedKeyUsageOID.SERVER_AUTH def test_build_server_verifier_missing_store( self, builder_type: Type[AnyPolicyBuilder] @@ -179,6 +180,7 @@ def test_verify(self, builder_type: Type[AnyPolicyBuilder]): assert verifier.validation_time == validation_time.replace(tzinfo=None) assert verifier.max_chain_depth == 16 assert verifier.store is store + assert verifier.eku == ExtendedKeyUsageOID.CLIENT_AUTH verified_client = verifier.verify(leaf, []) assert verified_client.chain == [leaf] From 42c00a7b37bdf0f3ed834e8bb9a18aa8afec9dc4 Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Sat, 7 Sep 2024 10:58:36 +0200 Subject: [PATCH 3/9] Document the implemented part of custom verification. --- docs/spelling_wordlist.txt | 1 + docs/x509/verification.rst | 121 ++++++++++++++++++++++++++++++++++++- 2 files changed, 119 insertions(+), 3 deletions(-) diff --git a/docs/spelling_wordlist.txt b/docs/spelling_wordlist.txt index 6a0282266821..f8e6d4232ae0 100644 --- a/docs/spelling_wordlist.txt +++ b/docs/spelling_wordlist.txt @@ -140,6 +140,7 @@ unencrypted unicode unpadded unpadding +validator Ventura verifier Verifier diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index b0e1daee2994..36ca253e7930 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -111,12 +111,22 @@ the root of trust: .. versionadded:: 43.0.0 - .. attribute:: subjects + .. versionchanged:: 44.0.0 + Renamed `subjects` to :attr:`sans`. + Made `sans` optional, added :attr:`subject`. - :type: list of :class:`~cryptography.x509.GeneralName` + .. attribute:: subject + + :type: :class:`~cryptography.x509.Name` + + The subject presented in the verified client's certificate. + + .. attribute:: sans + + :type: list of :class:`~cryptography.x509.GeneralName` or None The subjects presented in the verified client's Subject Alternative Name - extension. + extension or `None` if the extension is not present. .. attribute:: chain @@ -129,6 +139,8 @@ the root of trust: .. class:: ClientVerifier .. versionadded:: 43.0.0 + .. versionchanged:: 44.0.0 + Added :attr:`eku`. A ClientVerifier verifies client certificates. @@ -156,6 +168,18 @@ the root of trust: :type: :class:`Store` The verifier's trust store. + + .. attribute:: eku + + :type: :class:`~cryptography.x509.ObjectIdentifier` or None + + The value of the Extended Key Usage extension required by this verifier + If the verifier was built using :meth:`PolicyBuilder.build_client_verifier`, + this will always be :attr:`~cryptography.x509.oid.ExtendedKeyUsageOID.CLIENT_AUTH`. + + :note: + See :meth:`CustomPolicyBuilder.eku` documentation for how verification is affected + when changing the required EKU or using a custom extension policy. .. method:: verify(leaf, intermediates) @@ -212,6 +236,18 @@ the root of trust: The verifier's trust store. + .. attribute:: eku + + :type: :class:`~cryptography.x509.ObjectIdentifier` + + The value of the Extended Key Usage extension required by this verifier + If the verifier was built using :meth:`PolicyBuilder.build_server_verifier`, + this will always be :attr:`~cryptography.x509.oid.ExtendedKeyUsageOID.SERVER_AUTH`. + + :note: + See :meth:`CustomPolicyBuilder.eku` documentation for how verification is affected + when changing the required EKU or using a custom extension policy. + .. method:: verify(leaf, intermediates) Performs path validation on ``leaf``, returning a valid path @@ -294,3 +330,82 @@ the root of trust: for server verification. :returns: An instance of :class:`ClientVerifier` + +.. class:: CustomPolicyBuilder + + .. versionadded:: 44.0.0 + + A CustomPolicyBuilder provides a builder-style interface for constructing a + Verifier, but provides additional control over the verification policy compared to :class:`PolicyBuilder`. + + .. method:: time(new_time) + + Sets the verifier's verification time. + + If not called explicitly, this is set to :meth:`datetime.datetime.now` + when :meth:`build_server_verifier` or :meth:`build_client_verifier` + is called. + + :param new_time: The :class:`datetime.datetime` to use in the verifier + + :returns: A new instance of :class:`PolicyBuilder` + + .. method:: store(new_store) + + Sets the verifier's trust store. + + :param new_store: The :class:`Store` to use in the verifier + + :returns: A new instance of :class:`PolicyBuilder` + + .. method:: max_chain_depth(new_max_chain_depth) + + Sets the verifier's maximum chain building depth. + + This depth behaves tracks the length of the intermediate CA + chain: a maximum depth of zero means that the leaf must be directly + issued by a member of the store, a depth of one means no more than + one intermediate CA, and so forth. Note that self-issued intermediates + don't count against the chain depth, per RFC 5280. + + :param new_max_chain_depth: The maximum depth to allow in the verifier + + :returns: A new instance of :class:`PolicyBuilder` + + .. method:: eku(new_eku) + + Sets the Extended Key Usage required by the verifier's policy. + + If this method is not called, the EKU defaults to :attr:`~cryptography.x509.oid.ExtendedKeyUsageOID.SERVER_AUTH` + if :meth:`build_server_verifier` is called, and :attr:`~cryptography.x509.oid.ExtendedKeyUsageOID.CLIENT_AUTH` if + :meth:`build_client_verifier` is called. + + When using the default extension policies, only certificates + with the Extended Key Usage extension containing the specified value + will be accepted. To accept more than one EKU or any EKU, use an extension policy + with a custom validator. The EKU set via this method is accessible to custom extension validator + callbacks via the `policy` argument. + + :param ~cryptography.x509.ObjectIdentifier new_eku: + + :returns: A new instance of :class:`PolicyBuilder` + + .. method:: build_server_verifier(subject) + + Builds a verifier for verifying server certificates. + + :param subject: A :class:`Subject` to use in the verifier + + :returns: An instance of :class:`ServerVerifier` + + .. method:: build_client_verifier() + + 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` From eb4050b3b4b2fac3f4fee91fea2089266aa5a05b Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Thu, 19 Sep 2024 09:00:40 +0200 Subject: [PATCH 4/9] Remove `subject` field from VerifiedClient, rename `sans` back to `subjects`. --- docs/x509/verification.rst | 11 ++----- .../hazmat/bindings/_rust/x509.pyi | 4 +-- src/rust/src/x509/verify.rs | 29 +++++++------------ tests/x509/verification/test_limbo.py | 2 +- tests/x509/verification/test_verification.py | 28 +++--------------- 5 files changed, 19 insertions(+), 55 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index 36ca253e7930..777644ece87c 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -112,16 +112,9 @@ the root of trust: .. versionadded:: 43.0.0 .. versionchanged:: 44.0.0 - Renamed `subjects` to :attr:`sans`. - Made `sans` optional, added :attr:`subject`. + Made `subjects` optional with the addition of custom extension policies. - .. attribute:: subject - - :type: :class:`~cryptography.x509.Name` - - The subject presented in the verified client's certificate. - - .. attribute:: sans + .. attribute:: subjects :type: list of :class:`~cryptography.x509.GeneralName` or None diff --git a/src/cryptography/hazmat/bindings/_rust/x509.pyi b/src/cryptography/hazmat/bindings/_rust/x509.pyi index bad8c9c285cc..02145af308c7 100644 --- a/src/cryptography/hazmat/bindings/_rust/x509.pyi +++ b/src/cryptography/hazmat/bindings/_rust/x509.pyi @@ -81,9 +81,7 @@ class CustomPolicyBuilder: class VerifiedClient: @property - def subject(self) -> x509.Name: ... - @property - def sans(self) -> list[x509.GeneralName] | None: ... + def subjects(self) -> list[x509.GeneralName] | None: ... @property def chain(self) -> list[x509.Certificate]: ... diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 7a8026b51865..7406e07d422b 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -13,10 +13,7 @@ use cryptography_x509_verification::{ trust_store::Store, types::{DNSName, IPAddress}, }; -use pyo3::{ - types::{PyAnyMethods, PyListMethods}, - ToPyObject, -}; +use pyo3::types::{PyAnyMethods, PyListMethods}; use crate::types; use crate::x509::certificate::Certificate as PyCertificate; @@ -365,9 +362,7 @@ self_cell::self_cell!( )] pub(crate) struct PyVerifiedClient { #[pyo3(get)] - subject: pyo3::Py, - #[pyo3(get)] - sans: Option>, + subjects: Option>, #[pyo3(get)] chain: pyo3::Py, } @@ -448,32 +443,30 @@ impl PyClientVerifier { py_chain.append(c.extra())?; } - let cert = &chain[0].certificate(); - - let py_sans = || -> pyo3::PyResult> { - let leaf_san_ext = cert + let subjects = { + // NOTE: The `unwrap()` cannot fail, since the underlying policy + // enforces the well-formedness of the extension set. + let leaf_san_ext = &chain[0] + .certificate() .extensions() .ok() .unwrap() .get_extension(&SUBJECT_ALTERNATIVE_NAME_OID); match leaf_san_ext { + None => None, Some(leaf_san) => { let leaf_gns = leaf_san .value::>() .map_err(|e| -> CryptographyError { e.into() })?; let py_gns = parse_general_names(py, &leaf_gns)?; - Ok(Some(py_gns)) + Some(py_gns) } - None => Ok(None), } - }()?; - - let py_subject = crate::x509::parse_name(py, cert.subject())?; + }; Ok(PyVerifiedClient { - subject: py_subject.to_object(py), - sans: py_sans, + subjects, chain: py_chain.unbind(), }) } diff --git a/tests/x509/verification/test_limbo.py b/tests/x509/verification/test_limbo.py index 114405f2393e..c3298dad4a2e 100644 --- a/tests/x509/verification/test_limbo.py +++ b/tests/x509/verification/test_limbo.py @@ -169,7 +169,7 @@ def _limbo_testcase( expected_subjects = [ _get_limbo_peer(p) for p in testcase["expected_peer_names"] ] - assert expected_subjects == verified_client.sans + assert expected_subjects == verified_client.subjects built_chain = verified_client.chain diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index 8b89112b6654..09dc14f2a1b7 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -185,30 +185,10 @@ def test_verify(self, builder_type: Type[AnyPolicyBuilder]): verified_client = verifier.verify(leaf, []) assert verified_client.chain == [leaf] - expected_subject = x509.Name( - [ - x509.NameAttribute( - x509.NameOID.ORGANIZATIONAL_UNIT_NAME, "GT48742965" - ), - x509.NameAttribute( - x509.NameOID.ORGANIZATIONAL_UNIT_NAME, - "See www.rapidssl.com/resources/cps (c)14", - ), - x509.NameAttribute( - x509.NameOID.ORGANIZATIONAL_UNIT_NAME, - "Domain Control Validated - RapidSSL(R)", - ), - x509.NameAttribute( - x509.NameOID.COMMON_NAME, "www.cryptography.io" - ), - ] - ) - assert verified_client.subject == expected_subject - assert verified_client.sans is not None - assert x509.DNSName("www.cryptography.io") in verified_client.sans - assert x509.DNSName("cryptography.io") in verified_client.sans - - assert len(verified_client.sans) == 2 + assert verified_client.subjects is not None + 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 def test_verify_fails_renders_oid( self, builder_type: Type[AnyPolicyBuilder] From 36adbe937c184d77bc5c09ca8cf8a26157f53fe7 Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Thu, 19 Sep 2024 09:12:50 +0200 Subject: [PATCH 5/9] Remove EKU-related setters, getters and documentation from this PR. --- docs/x509/verification.rst | 44 ----------------- .../hazmat/bindings/_rust/x509.pyi | 5 -- src/rust/cryptography-x509/src/oid.rs | 11 ----- src/rust/src/x509/verify.rs | 49 ++----------------- tests/x509/verification/test_verification.py | 22 --------- 5 files changed, 3 insertions(+), 128 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index 777644ece87c..1cbad5526aa2 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -132,8 +132,6 @@ the root of trust: .. class:: ClientVerifier .. versionadded:: 43.0.0 - .. versionchanged:: 44.0.0 - Added :attr:`eku`. A ClientVerifier verifies client certificates. @@ -161,18 +159,6 @@ the root of trust: :type: :class:`Store` The verifier's trust store. - - .. attribute:: eku - - :type: :class:`~cryptography.x509.ObjectIdentifier` or None - - The value of the Extended Key Usage extension required by this verifier - If the verifier was built using :meth:`PolicyBuilder.build_client_verifier`, - this will always be :attr:`~cryptography.x509.oid.ExtendedKeyUsageOID.CLIENT_AUTH`. - - :note: - See :meth:`CustomPolicyBuilder.eku` documentation for how verification is affected - when changing the required EKU or using a custom extension policy. .. method:: verify(leaf, intermediates) @@ -229,18 +215,6 @@ the root of trust: The verifier's trust store. - .. attribute:: eku - - :type: :class:`~cryptography.x509.ObjectIdentifier` - - The value of the Extended Key Usage extension required by this verifier - If the verifier was built using :meth:`PolicyBuilder.build_server_verifier`, - this will always be :attr:`~cryptography.x509.oid.ExtendedKeyUsageOID.SERVER_AUTH`. - - :note: - See :meth:`CustomPolicyBuilder.eku` documentation for how verification is affected - when changing the required EKU or using a custom extension policy. - .. method:: verify(leaf, intermediates) Performs path validation on ``leaf``, returning a valid path @@ -365,24 +339,6 @@ the root of trust: :returns: A new instance of :class:`PolicyBuilder` - .. method:: eku(new_eku) - - Sets the Extended Key Usage required by the verifier's policy. - - If this method is not called, the EKU defaults to :attr:`~cryptography.x509.oid.ExtendedKeyUsageOID.SERVER_AUTH` - if :meth:`build_server_verifier` is called, and :attr:`~cryptography.x509.oid.ExtendedKeyUsageOID.CLIENT_AUTH` if - :meth:`build_client_verifier` is called. - - When using the default extension policies, only certificates - with the Extended Key Usage extension containing the specified value - will be accepted. To accept more than one EKU or any EKU, use an extension policy - with a custom validator. The EKU set via this method is accessible to custom extension validator - callbacks via the `policy` argument. - - :param ~cryptography.x509.ObjectIdentifier new_eku: - - :returns: A new instance of :class:`PolicyBuilder` - .. method:: build_server_verifier(subject) Builds a verifier for verifying server certificates. diff --git a/src/cryptography/hazmat/bindings/_rust/x509.pyi b/src/cryptography/hazmat/bindings/_rust/x509.pyi index 02145af308c7..684c60db23b6 100644 --- a/src/cryptography/hazmat/bindings/_rust/x509.pyi +++ b/src/cryptography/hazmat/bindings/_rust/x509.pyi @@ -73,7 +73,6 @@ class CustomPolicyBuilder: def max_chain_depth( self, new_max_chain_depth: int ) -> CustomPolicyBuilder: ... - def eku(self, new_eku: x509.ObjectIdentifier) -> CustomPolicyBuilder: ... def build_client_verifier(self) -> ClientVerifier: ... def build_server_verifier( self, subject: x509.verification.Subject @@ -92,8 +91,6 @@ class ClientVerifier: def store(self) -> Store: ... @property def max_chain_depth(self) -> int: ... - @property - def eku(self) -> x509.ObjectIdentifier: ... def verify( self, leaf: x509.Certificate, @@ -109,8 +106,6 @@ class ServerVerifier: def store(self) -> Store: ... @property def max_chain_depth(self) -> int: ... - @property - def eku(self) -> x509.ObjectIdentifier: ... def verify( self, leaf: x509.Certificate, diff --git a/src/rust/cryptography-x509/src/oid.rs b/src/rust/cryptography-x509/src/oid.rs index c29cd42de2ee..fbc440eea122 100644 --- a/src/rust/cryptography-x509/src/oid.rs +++ b/src/rust/cryptography-x509/src/oid.rs @@ -148,17 +148,6 @@ pub const EKU_ANY_KEY_USAGE_OID: asn1::ObjectIdentifier = asn1::oid!(2, 5, 29, 3 pub const EKU_CERTIFICATE_TRANSPARENCY_OID: asn1::ObjectIdentifier = asn1::oid!(1, 3, 6, 1, 4, 1, 11129, 2, 4, 4); -pub const ALL_EKU_OIDS: [asn1::ObjectIdentifier; 8] = [ - EKU_SERVER_AUTH_OID, - EKU_CLIENT_AUTH_OID, - EKU_CODE_SIGNING_OID, - EKU_EMAIL_PROTECTION_OID, - EKU_TIME_STAMPING_OID, - EKU_OCSP_SIGNING_OID, - EKU_ANY_KEY_USAGE_OID, - EKU_CERTIFICATE_TRANSPARENCY_OID, -]; - pub const PBES2_OID: asn1::ObjectIdentifier = asn1::oid!(1, 2, 840, 113549, 1, 5, 13); pub const PBKDF2_OID: asn1::ObjectIdentifier = asn1::oid!(1, 2, 840, 113549, 1, 5, 12); diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 7406e07d422b..055e816d05e6 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -3,9 +3,7 @@ // for complete details. use cryptography_x509::{ - certificate::Certificate, - extensions::SubjectAlternativeName, - oid::{ALL_EKU_OIDS, SUBJECT_ALTERNATIVE_NAME_OID}, + certificate::Certificate, extensions::SubjectAlternativeName, oid::SUBJECT_ALTERNATIVE_NAME_OID, }; use cryptography_x509_verification::{ ops::{CryptoOps, VerificationCertificate}, @@ -15,15 +13,12 @@ use cryptography_x509_verification::{ }; use pyo3::types::{PyAnyMethods, PyListMethods}; +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, - error::{CryptographyError, CryptographyResult}, -}; -use crate::{asn1::py_oid_to_oid, backend::keys}; use super::parse_general_names; @@ -152,7 +147,6 @@ pub(crate) struct CustomPolicyBuilder { time: Option, store: Option>, max_chain_depth: Option, - eku: Option, ca_ext_policy: Option>, ee_ext_policy: Option>, } @@ -165,7 +159,6 @@ impl CustomPolicyBuilder { time: self.time.clone(), store: self.store.as_ref().map(|s| s.clone_ref(py)), max_chain_depth: self.max_chain_depth, - eku: self.eku.clone(), ca_ext_policy: self.ca_ext_policy.clone(), ee_ext_policy: self.ee_ext_policy.clone(), } @@ -180,7 +173,6 @@ impl CustomPolicyBuilder { time: None, store: None, max_chain_depth: None, - eku: None, ca_ext_policy: None, ee_ext_policy: None, } @@ -225,29 +217,6 @@ impl CustomPolicyBuilder { }) } - fn eku( - &self, - py: pyo3::Python<'_>, - new_eku: pyo3::Bound<'_, pyo3::PyAny>, - ) -> CryptographyResult { - policy_builder_set_once_check!(self, eku, "EKU"); - - let oid = py_oid_to_oid(new_eku)?; - - if !ALL_EKU_OIDS.contains(&oid) { - return Err(CryptographyError::from( - pyo3::exceptions::PyValueError::new_err( - "Unknown EKU OID. Only EKUs from x509.ExtendedKeyUsageOID are supported.", - ), - )); - } - - Ok(CustomPolicyBuilder { - eku: Some(oid), - ..self.py_clone(py) - }) - } - fn build_client_verifier(&self, py: pyo3::Python<'_>) -> CryptographyResult { build_client_verifier_impl(py, &self.store, &self.time, |time| { // TODO: Replace with a custom policy once it's implemented in cryptography-x509-verification @@ -399,12 +368,6 @@ impl PyClientVerifier { self.as_policy().max_chain_depth } - #[getter] - fn eku(&self, py: pyo3::Python<'_>) -> pyo3::PyResult> { - let eku = &self.as_policy().extended_key_usage; - return Ok(oid_to_py_oid(py, eku)?.as_unbound().clone_ref(py)); - } - fn verify( &self, py: pyo3::Python<'_>, @@ -506,12 +469,6 @@ impl PyServerVerifier { self.as_policy().max_chain_depth } - #[getter] - fn eku(&self, py: pyo3::Python<'_>) -> pyo3::PyResult> { - let eku = &self.as_policy().extended_key_usage; - return Ok(oid_to_py_oid(py, eku)?.as_unbound().clone_ref(py)); - } - fn verify<'p>( &self, py: pyo3::Python<'p>, diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index 09dc14f2a1b7..f8dc6049c166 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -12,10 +12,6 @@ from cryptography import x509 from cryptography.x509.general_name import DNSName, IPAddress -from cryptography.x509.oid import ( - AuthorityInformationAccessOID, - ExtendedKeyUsageOID, -) from cryptography.x509.verification import ( CustomPolicyBuilder, PolicyBuilder, @@ -108,7 +104,6 @@ def test_builder_pattern(self, builder_type: Type[AnyPolicyBuilder]): assert verifier.validation_time == now assert verifier.store == store assert verifier.max_chain_depth == max_chain_depth - assert verifier.eku == ExtendedKeyUsageOID.SERVER_AUTH def test_build_server_verifier_missing_store( self, builder_type: Type[AnyPolicyBuilder] @@ -119,22 +114,6 @@ def test_build_server_verifier_missing_store( builder_type().build_server_verifier(DNSName("cryptography.io")) -class TestCustomPolicyBuilder: - def test_eku_already_set(self): - with pytest.raises(ValueError): - CustomPolicyBuilder().eku(ExtendedKeyUsageOID.IPSEC_IKE).eku( - ExtendedKeyUsageOID.IPSEC_IKE - ) - - def test_eku_bad_type(self): - with pytest.raises(TypeError): - CustomPolicyBuilder().eku("not an OID") # type: ignore[arg-type] - - def test_eku_non_eku_oid(self): - with pytest.raises(ValueError): - CustomPolicyBuilder().eku(AuthorityInformationAccessOID.OCSP) - - class TestStore: def test_store_rejects_empty_list(self): with pytest.raises(ValueError): @@ -180,7 +159,6 @@ def test_verify(self, builder_type: Type[AnyPolicyBuilder]): assert verifier.validation_time == validation_time.replace(tzinfo=None) assert verifier.max_chain_depth == 16 assert verifier.store is store - assert verifier.eku == ExtendedKeyUsageOID.CLIENT_AUTH verified_client = verifier.verify(leaf, []) assert verified_client.chain == [leaf] From 624be1d0c1b3715e8b337fc7abac2867a69f0651 Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Sat, 28 Sep 2024 10:29:38 +0200 Subject: [PATCH 6/9] Use double backticks in reStructuredText. --- docs/x509/verification.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index 1cbad5526aa2..459c398a1479 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -112,14 +112,14 @@ the root of trust: .. versionadded:: 43.0.0 .. versionchanged:: 44.0.0 - Made `subjects` optional with the addition of custom extension policies. + Made ``subjects`` optional with the addition of custom extension policies. .. attribute:: subjects :type: list of :class:`~cryptography.x509.GeneralName` or None The subjects presented in the verified client's Subject Alternative Name - extension or `None` if the extension is not present. + extension or ``None`` if the extension is not present. .. attribute:: chain From 3138e8e898b24f4b82fd097e7cb81593de09d786 Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Sun, 29 Sep 2024 15:30:20 +0200 Subject: [PATCH 7/9] Remove CustomPolicyBuilder in favor of extending PolicyBuilder. --- docs/x509/verification.rst | 61 ----- .../hazmat/bindings/_rust/x509.pyi | 11 - src/cryptography/x509/verification.py | 2 - src/rust/src/lib.rs | 4 +- src/rust/src/x509/verify.rs | 223 +++++------------- tests/x509/verification/test_limbo.py | 15 +- tests/x509/verification/test_verification.py | 78 ++---- 7 files changed, 97 insertions(+), 297 deletions(-) diff --git a/docs/x509/verification.rst b/docs/x509/verification.rst index 459c398a1479..70aafd48f94c 100644 --- a/docs/x509/verification.rst +++ b/docs/x509/verification.rst @@ -297,64 +297,3 @@ the root of trust: for server verification. :returns: An instance of :class:`ClientVerifier` - -.. class:: CustomPolicyBuilder - - .. versionadded:: 44.0.0 - - A CustomPolicyBuilder provides a builder-style interface for constructing a - Verifier, but provides additional control over the verification policy compared to :class:`PolicyBuilder`. - - .. method:: time(new_time) - - Sets the verifier's verification time. - - If not called explicitly, this is set to :meth:`datetime.datetime.now` - when :meth:`build_server_verifier` or :meth:`build_client_verifier` - is called. - - :param new_time: The :class:`datetime.datetime` to use in the verifier - - :returns: A new instance of :class:`PolicyBuilder` - - .. method:: store(new_store) - - Sets the verifier's trust store. - - :param new_store: The :class:`Store` to use in the verifier - - :returns: A new instance of :class:`PolicyBuilder` - - .. method:: max_chain_depth(new_max_chain_depth) - - Sets the verifier's maximum chain building depth. - - This depth behaves tracks the length of the intermediate CA - chain: a maximum depth of zero means that the leaf must be directly - issued by a member of the store, a depth of one means no more than - one intermediate CA, and so forth. Note that self-issued intermediates - don't count against the chain depth, per RFC 5280. - - :param new_max_chain_depth: The maximum depth to allow in the verifier - - :returns: A new instance of :class:`PolicyBuilder` - - .. method:: build_server_verifier(subject) - - Builds a verifier for verifying server certificates. - - :param subject: A :class:`Subject` to use in the verifier - - :returns: An instance of :class:`ServerVerifier` - - .. method:: build_client_verifier() - - 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 684c60db23b6..983200df5e45 100644 --- a/src/cryptography/hazmat/bindings/_rust/x509.pyi +++ b/src/cryptography/hazmat/bindings/_rust/x509.pyi @@ -67,17 +67,6 @@ class PolicyBuilder: self, subject: x509.verification.Subject ) -> ServerVerifier: ... -class CustomPolicyBuilder: - def time(self, new_time: datetime.datetime) -> CustomPolicyBuilder: ... - def store(self, new_store: Store) -> CustomPolicyBuilder: ... - def max_chain_depth( - self, new_max_chain_depth: int - ) -> CustomPolicyBuilder: ... - 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] | None: ... diff --git a/src/cryptography/x509/verification.py b/src/cryptography/x509/verification.py index fe0153581f85..b83650681237 100644 --- a/src/cryptography/x509/verification.py +++ b/src/cryptography/x509/verification.py @@ -12,7 +12,6 @@ __all__ = [ "ClientVerifier", "PolicyBuilder", - "CustomPolicyBuilder", "ServerVerifier", "Store", "Subject", @@ -26,5 +25,4 @@ ClientVerifier = rust_x509.ClientVerifier ServerVerifier = rust_x509.ServerVerifier PolicyBuilder = rust_x509.PolicyBuilder -CustomPolicyBuilder = rust_x509.CustomPolicyBuilder VerificationError = rust_x509.VerificationError diff --git a/src/rust/src/lib.rs b/src/rust/src/lib.rs index c53a948b40aa..e15fffa6d32e 100644 --- a/src/rust/src/lib.rs +++ b/src/rust/src/lib.rs @@ -132,8 +132,8 @@ mod _rust { use crate::x509::sct::Sct; #[pymodule_export] use crate::x509::verify::{ - CustomPolicyBuilder, PolicyBuilder, PyClientVerifier, PyServerVerifier, PyStore, - PyVerifiedClient, VerificationError, + PolicyBuilder, PyClientVerifier, PyServerVerifier, PyStore, PyVerifiedClient, + VerificationError, }; } diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 055e816d05e6..23cdc566c0b0 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -74,88 +74,15 @@ pub(crate) struct PolicyBuilder { time: Option, store: Option>, max_chain_depth: Option, -} - -#[pyo3::pymethods] -impl PolicyBuilder { - #[new] - fn new() -> PolicyBuilder { - PolicyBuilder { - time: None, - store: None, - max_chain_depth: None, - } - } - - fn time( - &self, - py: pyo3::Python<'_>, - new_time: pyo3::Bound<'_, pyo3::PyAny>, - ) -> CryptographyResult { - policy_builder_set_once_check!(self, time, "validation time"); - - Ok(PolicyBuilder { - time: Some(py_to_datetime(py, new_time)?), - store: self.store.as_ref().map(|s| s.clone_ref(py)), - max_chain_depth: self.max_chain_depth, - }) - } - - fn store(&self, new_store: pyo3::Py) -> CryptographyResult { - policy_builder_set_once_check!(self, store, "trust store"); - - Ok(PolicyBuilder { - time: self.time.clone(), - store: Some(new_store), - max_chain_depth: self.max_chain_depth, - }) - } - - fn max_chain_depth( - &self, - py: pyo3::Python<'_>, - new_max_chain_depth: u8, - ) -> CryptographyResult { - policy_builder_set_once_check!(self, max_chain_depth, "maximum chain depth"); - - Ok(PolicyBuilder { - time: self.time.clone(), - store: self.store.as_ref().map(|s| s.clone_ref(py)), - max_chain_depth: Some(new_max_chain_depth), - }) - } - - fn build_client_verifier(&self, py: pyo3::Python<'_>) -> CryptographyResult { - build_client_verifier_impl(py, &self.store, &self.time, |time| { - Policy::client(PyCryptoOps {}, time, self.max_chain_depth) - }) - } - - fn build_server_verifier( - &self, - py: pyo3::Python<'_>, - subject: pyo3::PyObject, - ) -> CryptographyResult { - build_server_verifier_impl(py, &self.store, &self.time, subject, |subject, time| { - Policy::server(PyCryptoOps {}, subject, time, self.max_chain_depth) - }) - } -} - -#[pyo3::pyclass(frozen, module = "cryptography.x509.verification")] -pub(crate) struct CustomPolicyBuilder { - time: Option, - store: Option>, - max_chain_depth: Option, ca_ext_policy: Option>, ee_ext_policy: Option>, } -impl CustomPolicyBuilder { +impl PolicyBuilder { /// Clones the builder, requires the GIL token to increase /// reference count for `self.store`. - fn py_clone(&self, py: pyo3::Python<'_>) -> CustomPolicyBuilder { - CustomPolicyBuilder { + fn py_clone(&self, py: pyo3::Python<'_>) -> PolicyBuilder { + PolicyBuilder { time: self.time.clone(), store: self.store.as_ref().map(|s| s.clone_ref(py)), max_chain_depth: self.max_chain_depth, @@ -166,10 +93,10 @@ impl CustomPolicyBuilder { } #[pyo3::pymethods] -impl CustomPolicyBuilder { +impl PolicyBuilder { #[new] - fn new() -> CustomPolicyBuilder { - CustomPolicyBuilder { + fn new() -> PolicyBuilder { + PolicyBuilder { time: None, store: None, max_chain_depth: None, @@ -182,10 +109,10 @@ impl CustomPolicyBuilder { &self, py: pyo3::Python<'_>, new_time: pyo3::Bound<'_, pyo3::PyAny>, - ) -> CryptographyResult { + ) -> CryptographyResult { policy_builder_set_once_check!(self, time, "validation time"); - Ok(CustomPolicyBuilder { + Ok(PolicyBuilder { time: Some(py_to_datetime(py, new_time)?), ..self.py_clone(py) }) @@ -195,10 +122,10 @@ impl CustomPolicyBuilder { &self, py: pyo3::Python<'_>, new_store: pyo3::Py, - ) -> CryptographyResult { + ) -> CryptographyResult { policy_builder_set_once_check!(self, store, "trust store"); - Ok(CustomPolicyBuilder { + Ok(PolicyBuilder { store: Some(new_store), ..self.py_clone(py) }) @@ -208,20 +135,36 @@ impl CustomPolicyBuilder { &self, py: pyo3::Python<'_>, new_max_chain_depth: u8, - ) -> CryptographyResult { + ) -> CryptographyResult { policy_builder_set_once_check!(self, max_chain_depth, "maximum chain depth"); - Ok(CustomPolicyBuilder { + Ok(PolicyBuilder { max_chain_depth: Some(new_max_chain_depth), ..self.py_clone(py) }) } fn build_client_verifier(&self, py: pyo3::Python<'_>) -> CryptographyResult { - build_client_verifier_impl(py, &self.store, &self.time, |time| { - // TODO: Replace with a custom policy once it's implemented in cryptography-x509-verification - Policy::client(PyCryptoOps {}, time, self.max_chain_depth) - }) + 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: Pass extension policies here once implemented in cryptography-x509-verification. + let policy = Policy::client(PyCryptoOps {}, time, self.max_chain_depth); + + Ok(PyClientVerifier { policy, store }) } fn build_server_verifier( @@ -229,79 +172,43 @@ impl CustomPolicyBuilder { py: pyo3::Python<'_>, subject: pyo3::PyObject, ) -> CryptographyResult { - build_server_verifier_impl(py, &self.store, &self.time, subject, |subject, time| { - // TODO: Replace with a custom policy once it's implemented in cryptography-x509-verification - Policy::server(PyCryptoOps {}, subject, time, self.max_chain_depth) + let store = match self.store.as_ref() { + Some(s) => s.clone_ref(py), + None => { + return Err(CryptographyError::from( + pyo3::exceptions::PyValueError::new_err( + "A server verifier must have a trust store.", + ), + )); + } + }; + + let time = match self.time.as_ref() { + Some(t) => t.clone(), + None => datetime_now(py)?, + }; + let subject_owner = build_subject_owner(py, &subject)?; + + let policy = OwnedPolicy::try_new(subject_owner, |subject_owner| { + let subject = build_subject(py, subject_owner)?; + + // TODO: Pass extension policies here once implemented in cryptography-x509-verification. + Ok::, pyo3::PyErr>(Policy::server( + PyCryptoOps {}, + subject, + time, + self.max_chain_depth, + )) + })?; + + Ok(PyServerVerifier { + py_subject: subject, + policy, + store, }) } } -/// This is a helper to avoid code duplication between PolicyBuilder and CustomPolicyBuilder. -fn build_server_verifier_impl( - py: pyo3::Python<'_>, - store: &Option>, - time: &Option, - subject: pyo3::PyObject, - make_policy: impl Fn(Subject<'_>, asn1::DateTime) -> PyCryptoPolicy<'_>, -) -> CryptographyResult { - let store = match store { - Some(s) => s.clone_ref(py), - None => { - return Err(CryptographyError::from( - pyo3::exceptions::PyValueError::new_err( - "A server verifier must have a trust store.", - ), - )); - } - }; - - let time = match time.as_ref() { - Some(t) => t.clone(), - None => datetime_now(py)?, - }; - let subject_owner = build_subject_owner(py, &subject)?; - - let policy = OwnedPolicy::try_new(subject_owner, |subject_owner| { - let subject = build_subject(py, subject_owner)?; - Ok::, pyo3::PyErr>(make_policy(subject, time)) - })?; - - Ok(PyServerVerifier { - py_subject: subject, - policy, - store, - }) -} - -/// This is a helper to avoid code duplication between PolicyBuilder and CustomPolicyBuilder. -fn build_client_verifier_impl( - py: pyo3::Python<'_>, - store: &Option>, - time: &Option, - make_policy: impl Fn(asn1::DateTime) -> PyCryptoPolicy<'static>, -) -> CryptographyResult { - let store = match 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 time.as_ref() { - Some(t) => t.clone(), - None => datetime_now(py)?, - }; - - Ok(PyClientVerifier { - policy: make_policy(time), - store, - }) -} - type PyCryptoPolicy<'a> = Policy<'a, PyCryptoOps>; /// This enum exists solely to provide heterogeneously typed ownership for `OwnedPolicy`. diff --git a/tests/x509/verification/test_limbo.py b/tests/x509/verification/test_limbo.py index c3298dad4a2e..d0402c4ce30a 100644 --- a/tests/x509/verification/test_limbo.py +++ b/tests/x509/verification/test_limbo.py @@ -6,7 +6,6 @@ import ipaddress import json import os -from typing import Type, Union import pytest @@ -14,7 +13,6 @@ from cryptography.x509 import load_pem_x509_certificate from cryptography.x509.verification import ( ClientVerifier, - CustomPolicyBuilder, PolicyBuilder, ServerVerifier, Store, @@ -98,11 +96,7 @@ def _get_limbo_peer(expected_peer): return x509.RFC822Name(value) -def _limbo_testcase( - id_, - testcase, - builder_type: Union[Type[PolicyBuilder], Type[CustomPolicyBuilder]], -): +def _limbo_testcase(id_, testcase): if id_ in LIMBO_SKIP_TESTCASES: pytest.skip(f"explicitly skipped testcase: {id_}") @@ -133,7 +127,7 @@ def _limbo_testcase( max_chain_depth = testcase["max_chain_depth"] should_pass = testcase["expected_result"] == "SUCCESS" - builder = builder_type().store(Store(trusted_certs)) + builder = PolicyBuilder().store(Store(trusted_certs)) if validation_time is not None: builder = builder.time(validation_time) if max_chain_depth is not None: @@ -183,8 +177,7 @@ def _limbo_testcase( verifier.verify(peer_certificate, untrusted_intermediates) -@pytest.mark.parametrize("builder_type", [PolicyBuilder, CustomPolicyBuilder]) -def test_limbo(subtests, pytestconfig, builder_type): +def test_limbo(subtests, pytestconfig): limbo_root = pytestconfig.getoption("--x509-limbo-root", skip=True) limbo_path = os.path.join(limbo_root, "limbo.json") with open(limbo_path, mode="rb") as limbo_file: @@ -194,4 +187,4 @@ def test_limbo(subtests, pytestconfig, builder_type): with subtests.test(): # NOTE: Pass in the id separately to make pytest # error renderings slightly nicer. - _limbo_testcase(testcase["id"], testcase, builder_type) + _limbo_testcase(testcase["id"], testcase) diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py index f8dc6049c166..1d2f9261c57d 100644 --- a/tests/x509/verification/test_verification.py +++ b/tests/x509/verification/test_verification.py @@ -6,14 +6,12 @@ import os from functools import lru_cache from ipaddress import IPv4Address -from typing import Type, Union import pytest from cryptography import x509 from cryptography.x509.general_name import DNSName, IPAddress from cryptography.x509.verification import ( - CustomPolicyBuilder, PolicyBuilder, Store, VerificationError, @@ -30,71 +28,60 @@ def dummy_store() -> Store: return Store([cert]) -AnyPolicyBuilder = Union[PolicyBuilder, CustomPolicyBuilder] - - -@pytest.mark.parametrize("builder_type", [PolicyBuilder, CustomPolicyBuilder]) -class TestPolicyBuilderCommon: - """ - Tests functionality that is identical between - PolicyBuilder and CustomPolicyBuilder. - """ - - def test_time_already_set(self, builder_type: Type[AnyPolicyBuilder]): +class TestPolicyBuilder: + def test_time_already_set(self): with pytest.raises(ValueError): - builder_type().time(datetime.datetime.now()).time( + PolicyBuilder().time(datetime.datetime.now()).time( datetime.datetime.now() ) - def test_store_already_set(self, builder_type: Type[AnyPolicyBuilder]): + def test_store_already_set(self): with pytest.raises(ValueError): - builder_type().store(dummy_store()).store(dummy_store()) + PolicyBuilder().store(dummy_store()).store(dummy_store()) - def test_max_chain_depth_already_set( - self, builder_type: Type[AnyPolicyBuilder] - ): + def test_max_chain_depth_already_set(self): with pytest.raises(ValueError): - builder_type().max_chain_depth(8).max_chain_depth(9) + PolicyBuilder().max_chain_depth(8).max_chain_depth(9) - def test_ipaddress_subject(self, builder_type: Type[AnyPolicyBuilder]): + def test_ipaddress_subject(self): policy = ( - builder_type() + PolicyBuilder() .store(dummy_store()) .build_server_verifier(IPAddress(IPv4Address("0.0.0.0"))) ) assert policy.subject == IPAddress(IPv4Address("0.0.0.0")) - def test_dnsname_subject(self, builder_type: Type[AnyPolicyBuilder]): + def test_dnsname_subject(self): policy = ( - builder_type() + PolicyBuilder() .store(dummy_store()) .build_server_verifier(DNSName("cryptography.io")) ) assert policy.subject == DNSName("cryptography.io") - def test_subject_bad_types(self, builder_type: Type[AnyPolicyBuilder]): + def test_subject_bad_types(self): # Subject must be a supported GeneralName type with pytest.raises(TypeError): - builder_type().store(dummy_store()).build_server_verifier( + PolicyBuilder().store(dummy_store()).build_server_verifier( "cryptography.io" # type: ignore[arg-type] ) with pytest.raises(TypeError): - builder_type().store(dummy_store()).build_server_verifier( + PolicyBuilder().store(dummy_store()).build_server_verifier( "0.0.0.0" # type: ignore[arg-type] ) with pytest.raises(TypeError): - builder_type().store(dummy_store()).build_server_verifier( + PolicyBuilder().store(dummy_store()).build_server_verifier( IPv4Address("0.0.0.0") # type: ignore[arg-type] ) with pytest.raises(TypeError): - builder_type().store(dummy_store()).build_server_verifier(None) # type: ignore[arg-type] + PolicyBuilder().store(dummy_store()).build_server_verifier(None) # type: ignore[arg-type] - def test_builder_pattern(self, builder_type: Type[AnyPolicyBuilder]): + def test_builder_pattern(self): now = datetime.datetime.now().replace(microsecond=0) store = dummy_store() max_chain_depth = 16 - builder = builder_type() + builder = PolicyBuilder() builder = builder.time(now) builder = builder.store(store) builder = builder.max_chain_depth(max_chain_depth) @@ -105,13 +92,11 @@ def test_builder_pattern(self, builder_type: Type[AnyPolicyBuilder]): assert verifier.store == store assert verifier.max_chain_depth == max_chain_depth - def test_build_server_verifier_missing_store( - self, builder_type: Type[AnyPolicyBuilder] - ): + def test_build_server_verifier_missing_store(self): with pytest.raises( ValueError, match="A server verifier must have a trust store" ): - builder_type().build_server_verifier(DNSName("cryptography.io")) + PolicyBuilder().build_server_verifier(DNSName("cryptography.io")) class TestStore: @@ -124,23 +109,14 @@ def test_store_rejects_non_certificates(self): Store(["not a cert"]) # type: ignore[list-item] -@pytest.mark.parametrize( - "builder_type", - [ - PolicyBuilder, - CustomPolicyBuilder, - ], -) class TestClientVerifier: - def test_build_client_verifier_missing_store( - self, builder_type: Type[AnyPolicyBuilder] - ): + def test_build_client_verifier_missing_store(self): with pytest.raises( ValueError, match="A client verifier must have a trust store" ): - builder_type().build_client_verifier() + PolicyBuilder().build_client_verifier() - def test_verify(self, builder_type: Type[AnyPolicyBuilder]): + def test_verify(self): # expires 2018-11-16 01:15:03 UTC leaf = _load_cert( os.path.join("x509", "cryptography.io.pem"), @@ -152,7 +128,7 @@ def test_verify(self, builder_type: Type[AnyPolicyBuilder]): validation_time = datetime.datetime.fromisoformat( "2018-11-16T00:00:00+00:00" ) - builder = builder_type().store(store) + builder = PolicyBuilder().store(store) builder = builder.time(validation_time).max_chain_depth(16) verifier = builder.build_client_verifier() @@ -168,9 +144,7 @@ def test_verify(self, builder_type: Type[AnyPolicyBuilder]): assert x509.DNSName("cryptography.io") in verified_client.subjects assert len(verified_client.subjects) == 2 - def test_verify_fails_renders_oid( - self, builder_type: Type[AnyPolicyBuilder] - ): + def test_verify_fails_renders_oid(self): leaf = _load_cert( os.path.join("x509", "custom", "ekucrit-testuser-cert.pem"), x509.load_pem_x509_certificate, @@ -182,7 +156,7 @@ def test_verify_fails_renders_oid( "2024-06-26T00:00:00+00:00" ) - builder = builder_type().store(store) + builder = PolicyBuilder().store(store) builder = builder.time(validation_time) verifier = builder.build_client_verifier() From 866dffaec609be9f47d7577897e9b2946d300d46 Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Tue, 8 Oct 2024 10:54:36 +0200 Subject: [PATCH 8/9] Code style improvements. --- src/rust/src/x509/verify.rs | 34 +++++++++++++--------------------- 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 23cdc566c0b0..97e04e545f2f 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -79,8 +79,6 @@ pub(crate) struct PolicyBuilder { } impl PolicyBuilder { - /// Clones the builder, requires the GIL token to increase - /// reference count for `self.store`. fn py_clone(&self, py: pyo3::Python<'_>) -> PolicyBuilder { PolicyBuilder { time: self.time.clone(), @@ -313,26 +311,20 @@ impl PyClientVerifier { py_chain.append(c.extra())?; } - let subjects = { - // NOTE: The `unwrap()` cannot fail, since the underlying policy - // enforces the well-formedness of the extension set. - let leaf_san_ext = &chain[0] - .certificate() - .extensions() - .ok() - .unwrap() - .get_extension(&SUBJECT_ALTERNATIVE_NAME_OID); - - match leaf_san_ext { - None => None, - Some(leaf_san) => { - let leaf_gns = leaf_san - .value::>() - .map_err(|e| -> CryptographyError { e.into() })?; - let py_gns = parse_general_names(py, &leaf_gns)?; - Some(py_gns) - } + // NOTE: The `unwrap()` cannot fail, since the underlying policy + // enforces the well-formedness of the extension set. + let subjects = match &chain[0] + .certificate() + .extensions() + .ok() + .unwrap() + .get_extension(&SUBJECT_ALTERNATIVE_NAME_OID) + { + Some(leaf_san) => { + let leaf_gns = leaf_san.value::>()?; + Some(parse_general_names(py, &leaf_gns)?) } + None => None, }; Ok(PyVerifiedClient { From a5b9a42e266dafaa0597b09d0b4804597bec2508 Mon Sep 17 00:00:00 2001 From: Ivan Desiatov Date: Tue, 8 Oct 2024 13:24:44 +0200 Subject: [PATCH 9/9] Resolve coverage issues. --- .../src/policy/extension.rs | 5 +--- .../src/policy/mod.rs | 4 +-- src/rust/src/x509/verify.rs | 29 +++++++------------ 3 files changed, 12 insertions(+), 26 deletions(-) diff --git a/src/rust/cryptography-x509-verification/src/policy/extension.rs b/src/rust/cryptography-x509-verification/src/policy/extension.rs index df53c50507a5..a01eb490122b 100644 --- a/src/rust/cryptography-x509-verification/src/policy/extension.rs +++ b/src/rust/cryptography-x509-verification/src/policy/extension.rs @@ -14,8 +14,7 @@ use cryptography_x509::{ use crate::{ops::CryptoOps, policy::Policy, ValidationError}; -#[derive(Clone)] -pub struct ExtensionPolicy { +pub(crate) struct ExtensionPolicy { pub(crate) authority_information_access: ExtensionValidator, pub(crate) authority_key_identifier: ExtensionValidator, pub(crate) subject_key_identifier: ExtensionValidator, @@ -124,7 +123,6 @@ impl ExtensionPolicy { } /// Represents different criticality states for an extension. -#[derive(Clone)] pub(crate) enum Criticality { /// The extension MUST be marked as critical. Critical, @@ -153,7 +151,6 @@ type MaybeExtensionValidatorCallback = fn(&Policy<'_, B>, &Certificate<'_>, Option<&Extension<'_>>) -> Result<(), ValidationError>; /// Represents different validation states for an extension. -#[derive(Clone)] pub(crate) enum ExtensionValidator { /// The extension MUST NOT be present. NotPresent, diff --git a/src/rust/cryptography-x509-verification/src/policy/mod.rs b/src/rust/cryptography-x509-verification/src/policy/mod.rs index a89511fd6d69..5616a83a8ceb 100644 --- a/src/rust/cryptography-x509-verification/src/policy/mod.rs +++ b/src/rust/cryptography-x509-verification/src/policy/mod.rs @@ -25,12 +25,10 @@ use cryptography_x509::oid::{ use once_cell::sync::Lazy; use crate::ops::CryptoOps; -use crate::policy::extension::{ca, common, ee, Criticality, ExtensionValidator}; +use crate::policy::extension::{ca, common, ee, Criticality, ExtensionPolicy, ExtensionValidator}; use crate::types::{DNSName, DNSPattern, IPAddress}; use crate::{ValidationError, VerificationCertificate}; -pub use crate::policy::extension::ExtensionPolicy; - // RSA key constraints, as defined in CA/B 6.1.5. static WEBPKI_MINIMUM_RSA_MODULUS: usize = 2048; diff --git a/src/rust/src/x509/verify.rs b/src/rust/src/x509/verify.rs index 97e04e545f2f..face9acf674f 100644 --- a/src/rust/src/x509/verify.rs +++ b/src/rust/src/x509/verify.rs @@ -7,7 +7,7 @@ use cryptography_x509::{ }; use cryptography_x509_verification::{ ops::{CryptoOps, VerificationCertificate}, - policy::{ExtensionPolicy, Policy, Subject}, + policy::{Policy, Subject}, trust_store::Store, types::{DNSName, IPAddress}, }; @@ -22,7 +22,6 @@ use crate::x509::sign; use super::parse_general_names; -#[derive(Clone)] pub(crate) struct PyCryptoOps {} impl CryptoOps for PyCryptoOps { @@ -74,8 +73,6 @@ pub(crate) struct PolicyBuilder { time: Option, store: Option>, max_chain_depth: Option, - ca_ext_policy: Option>, - ee_ext_policy: Option>, } impl PolicyBuilder { @@ -84,8 +81,6 @@ impl PolicyBuilder { time: self.time.clone(), store: self.store.as_ref().map(|s| s.clone_ref(py)), max_chain_depth: self.max_chain_depth, - ca_ext_policy: self.ca_ext_policy.clone(), - ee_ext_policy: self.ee_ext_policy.clone(), } } } @@ -98,8 +93,6 @@ impl PolicyBuilder { time: None, store: None, max_chain_depth: None, - ca_ext_policy: None, - ee_ext_policy: None, } } @@ -311,24 +304,22 @@ impl PyClientVerifier { py_chain.append(c.extra())?; } - // NOTE: The `unwrap()` cannot fail, since the underlying policy - // enforces the well-formedness of the extension set. - let subjects = match &chain[0] + // 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() .ok() .unwrap() .get_extension(&SUBJECT_ALTERNATIVE_NAME_OID) - { - Some(leaf_san) => { - let leaf_gns = leaf_san.value::>()?; - Some(parse_general_names(py, &leaf_gns)?) - } - None => None, - }; + .unwrap(); + + let leaf_gns = leaf_san.value::>()?; + let py_gns = parse_general_names(py, &leaf_gns)?; Ok(PyVerifiedClient { - subjects, + subjects: Some(py_gns), chain: py_chain.unbind(), }) }