Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

X509 CustomPolicyBuilder foundation. #11559

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/spelling_wordlist.txt
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ unencrypted
unicode
unpadded
unpadding
validator
Ventura
verifier
Verifier
Expand Down
121 changes: 118 additions & 3 deletions docs/x509/verification.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, usage of subject for anything is strongly discouraged at this point, because it's not type safe.

Moreoever, you don't really need the validator to do anything special for you, this is always cert.subject.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agreed. This is the thing that I pointed out in my previous comment .

I think having just the subjects field but making it optional is fine. There could potentially be two VerifiedClient classes, one used with PolicyBuilder, and the other used with CustomPolicyBuilder, since subjects only needs to be optional in the first case, but personally I think the increased implementation complexity is just not worth it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed subject and renamed sans back to subjects (but kept it optional). I'm leaving the conversation unresolved in case there are some other concerns related to this.


The subjects presented in the verified client's Subject Alternative Name
extension.
extension or `None` if the extension is not present.

.. attribute:: chain

Expand All @@ -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.

Expand Down Expand Up @@ -156,6 +168,18 @@ the root of trust:
:type: :class:`Store`

The verifier's trust store.

.. attribute:: eku
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest pulling EKU handling out into its own PR, which can be reviewed on its own.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing this PR adds is the CustomPolicyBuilder.eku(...) setter and the getters on ClientVerifier and ServerVerifier. The eku is not passed to the underlying Policy at all at this point.

Just wanted to point that out. If you still want me to break it out, I'll do that of course.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah if you wouldn't mind pulling it out that's an easy review that we can land entirely independently of the larger conversation and it makes future reviews of this PR easier. 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I'll create the EKU PR once we are done with this one.


: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)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -294,3 +330,82 @@ the root of trust:
for server verification.

:returns: An instance of :class:`ClientVerifier`

.. class:: CustomPolicyBuilder
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need a new CustomPolicyBuilder? Can they just be methods on the existing PolicyBuilder?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I also thought about that. That's definitely a possibility, but I prefer this version since it kind of follows the "make it hard to do insecure things" part of cryptography's philosophy. In this case it would be more like "make insecure things explicit", e.g. this would make using any of the "easier to get wrong" features more visible in a code review.

The first paragraph from this comment of mine describes my motivation pretty well too. There's also a comment from @woodruffw where he describes why he prefers the CustomPolicyBuilder API.

In the end, I would be fine with both APIs, but I have grown to like the idea of a separate CustomPolicyBuilder. (Although I obviously can't be fully objective since it was my idea)


.. 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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this, I think this could be in its own PR.

Copy link
Author

@deivse deivse Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit surprised that you want this in it's own PR to be honest, since this functionality is already present on PolicyBuilder, so this is pretty much a glorified copy-paste job. But it's also very possible that I'm just not used to this review style and there's a very valid reason for this.

In any case, I'll wait for your reply for now. If you still want this in it's own PR, I would appreciate it if you could describe some of the motivation behind that, just so we are on the same page and I can scope the next PRs better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this was confusion by me -- I'd forgotten we had max_chain_depth on the existing policy builder, so I thought this was net new functionality, not just repeating it on the custom builder.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The general motivation we have is to minimize the set of changes in all PRs so that we can focus heavily on the novel parts and iterate without the cognitive load of larger changes (where we need to ignore the things that don't matter). So yes, it may be a glorified copy-paste, but if it's uncontroversial we can then review/land it quickly and focus on the meatier bits 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm down to break this out as well, but getting mixed signals here, I will err on the side of leaving this in for now since other methods that are already on PolicyBuilder aren't getting their own PR.


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`
20 changes: 19 additions & 1 deletion src/cryptography/hazmat/bindings/_rust/x509.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -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]: ...

Expand All @@ -80,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,
Expand All @@ -95,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,
Expand Down
2 changes: 2 additions & 0 deletions src/cryptography/x509/verification.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
__all__ = [
"ClientVerifier",
"PolicyBuilder",
"CustomPolicyBuilder",
"ServerVerifier",
"Store",
"Subject",
Expand All @@ -25,4 +26,5 @@
ClientVerifier = rust_x509.ClientVerifier
ServerVerifier = rust_x509.ServerVerifier
PolicyBuilder = rust_x509.PolicyBuilder
CustomPolicyBuilder = rust_x509.CustomPolicyBuilder
VerificationError = rust_x509.VerificationError
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ use cryptography_x509::{

use crate::{ops::CryptoOps, policy::Policy, ValidationError};

pub(crate) struct ExtensionPolicy<B: CryptoOps> {
#[derive(Clone)]
pub struct ExtensionPolicy<B: CryptoOps> {
pub(crate) authority_information_access: ExtensionValidator<B>,
pub(crate) authority_key_identifier: ExtensionValidator<B>,
pub(crate) subject_key_identifier: ExtensionValidator<B>,
Expand Down Expand Up @@ -123,6 +124,7 @@ impl<B: CryptoOps> ExtensionPolicy<B> {
}

/// Represents different criticality states for an extension.
#[derive(Clone)]
pub(crate) enum Criticality {
/// The extension MUST be marked as critical.
Critical,
Expand Down Expand Up @@ -151,6 +153,7 @@ type MaybeExtensionValidatorCallback<B> =
fn(&Policy<'_, B>, &Certificate<'_>, Option<&Extension<'_>>) -> Result<(), ValidationError>;

/// Represents different validation states for an extension.
#[derive(Clone)]
pub(crate) enum ExtensionValidator<B: CryptoOps> {
/// The extension MUST NOT be present.
NotPresent,
Expand Down
4 changes: 3 additions & 1 deletion src/rust/cryptography-x509-verification/src/policy/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
11 changes: 11 additions & 0 deletions src/rust/cryptography-x509/src/oid.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 2 additions & 2 deletions src/rust/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
}

Expand Down
Loading
Loading