Remove CustomPolicyBuilder in favor of extending PolicyBuilder.

deivse · deivse · commit de53b3e8e563 · 2024-09-29T15:33:42.000+02:00
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -74,88 +74,15 @@ pub(crate) struct PolicyBuilder {
     time: Option<asn1::DateTime>,
     store: Option<pyo3::Py<PyStore>>,
     max_chain_depth: Option<u8>,
-}
-
-#[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<PolicyBuilder> {
-        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<PyStore>) -> CryptographyResult<PolicyBuilder> {
-        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<PolicyBuilder> {
-        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<PyClientVerifier> {
-        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<PyServerVerifier> {
-        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<asn1::DateTime>,
-    store: Option<pyo3::Py<PyStore>>,
-    max_chain_depth: Option<u8>,
     ca_ext_policy: Option<ExtensionPolicy<PyCryptoOps>>,
     ee_ext_policy: Option<ExtensionPolicy<PyCryptoOps>>,
 }
 
-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<CustomPolicyBuilder> {
+    ) -> CryptographyResult<PolicyBuilder> {
         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<PyStore>,
-    ) -> CryptographyResult<CustomPolicyBuilder> {
+    ) -> CryptographyResult<PolicyBuilder> {
         policy_builder_set_once_check!(self, store, "trust store");
 
-        Ok(CustomPolicyBuilder {
+        Ok(PolicyBuilder {
             store: Some(new_store),
             ..self.py_clone(py)
         })
@@ -208,100 +135,80 @@ impl CustomPolicyBuilder {
         &self,
         py: pyo3::Python<'_>,
         new_max_chain_depth: u8,
-    ) -> CryptographyResult<CustomPolicyBuilder> {
+    ) -> CryptographyResult<PolicyBuilder> {
         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<PyClientVerifier> {
-        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(
         &self,
         py: pyo3::Python<'_>,
         subject: pyo3::PyObject,
     ) -> CryptographyResult<PyServerVerifier> {
-        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::<PyCryptoPolicy<'_>, 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<pyo3::Py<PyStore>>,
-    time: &Option<asn1::DateTime>,
-    subject: pyo3::PyObject,
-    make_policy: impl Fn(Subject<'_>, asn1::DateTime) -> PyCryptoPolicy<'_>,
-) -> CryptographyResult<PyServerVerifier> {
-    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::<PyCryptoPolicy<'_>, 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<pyo3::Py<PyStore>>,
-    time: &Option<asn1::DateTime>,
-    make_policy: impl Fn(asn1::DateTime) -> PyCryptoPolicy<'static>,
-) -> CryptographyResult<PyClientVerifier> {
-    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
diff --git a/tests/x509/verification/test_verification.py b/tests/x509/verification/test_verification.py
