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 Verification: Extension policies documentation. #11800

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

deivse
Copy link
Contributor

@deivse deivse commented Oct 19, 2024

Finally coming back with another PR. The idea is that this one will serve to confirm the user-facing API for custom extension policy support, and the next one will have the first parts of the implementation.

@deivse deivse changed the title Add documentation related to custom x509 verification extension policies. x509 Verification: Extension policies documentation. Oct 19, 2024
@deivse deivse force-pushed the x509_verification_extension_policy_builder_docs branch from d7c5ebd to 649dd50 Compare October 19, 2024 10:00
Comment on lines +279 to +292
.. method:: ca_extension_policy(new_policy)

.. versionadded:: 44.0.0

Sets the CA extension policy for the verifier.
If this method is not called, the default CA extension policy that
follows the CA/B Forum guidelines is used.

:param ExtensionPolicy new_policy: The CA extension policy to use.
Use :class:`ExtensionPolicyBuilder` to create the policy.

:returns: A new instance of :class:`PolicyBuilder`

.. method:: ee_extension_policy(new_policy)
Copy link
Member

Choose a reason for hiding this comment

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

Does it actually make sense for these to be two separate methods? What's the cirucmstance where you want to set one but not the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, my use case is to relax some of the extension policy requirements for EE certificates, namely - allow different extended key usages and to allow certificates without SANs since we don't always have them. All our CA certificates however comply to the default extension policy, so there's no need to adjust that for me.

A single set_extension_policies(ee_ext_policy, ca_ext_policy) method would work, but would make things a bit more verbose when you only want to set one of the two: set_extension_policies(custom_policy, ExtensionPolicyBuilder.webpki_defaults_ca().build()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do consider the single-method option I would also consider changing

ExtensionPolicyBuilder.webpki_defaults_ca().require_not_present(SubjectAlternativeName.oid).build()

to

ExtensionPolicy.webpki_defaults_ca().require_not_present(SubjectAlternativeName.oid)

Implementation-wise the build method is only there to make the API make sense from an outside perspective, as internally the builder itself is just a policy wrapper with some methods. However I understand that this might theoretically make the api a bit more confusing as it still uses the builder pattern without explicitly signalling that in the type name.

Comment on lines +341 to +357
.. staticmethod:: webpki_defaults_ca()

Creates an ExtensionPolicyBuilder initialized with a
CA extension policy based on CA/B Forum guidelines.

This is the CA extension policy used by :class:`PolicyBuilder`.

:returns: An instance of :class:`ExtensionPolicyBuilder`

.. staticmethod:: webpki_defaults_ee()

Creates an ExtensionPolicyBuilder initialized with an
EE extension policy based on CA/B Forum guidelines.

This is the EE extension policy used by :class:`PolicyBuilder`.

:returns: An instance of :class:`ExtensionPolicyBuilder`
Copy link
Member

Choose a reason for hiding this comment

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

@woodruffw right now our extension policies are basically "CABF, but with some things loosened where necessary in practice", is there a good way of articulating that for users?


:returns: An instance of :class:`ExtensionPolicyBuilder`

.. method:: not_present(oid)
Copy link
Member

Choose a reason for hiding this comment

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

I think these names should be require_not_present() or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think something like require_not_present, may_be_present and require_present would work, yes. Let me know if that sounds good to you.

Pass the created policy to :meth:`PolicyBuilder.ca_extension_policy` or :meth:`PolicyBuilder.ee_extension_policy`
to set the policies used during verification.

.. class:: Policy
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this feels duplicative of our existing types, which makes me feel that something is off. But I don't have a concrete suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this does indeed closely duplicate the rust Policy struct in cryptography-x509-verification. I couldn't really come up with any way to avoid this since that module is supposed to be completely python-agnostic, and we need to pass the policy parameters to the python extension validator callback.

I definitely welcome any suggestions on how to make this better, this is definitely something I would like to avoid, but I think it might not be possible without dropping the constraint of cryptography-x509-verification being python-agnostic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants