-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
FR: Allow clients to set extension policies for x.509 verification #11165
Comments
For xrefs: #10276 (comment) and #11042 are similar requests. |
Hello, just want to bump this, since this is something we also require where I work. I haven't contributed to cryptography before, but I think I may be able to contribute this functionality with some direction from more experienced contributors. From what I read, the main issue most people have with this is the EKU extension being required and validated. A |
We are interested in expanding these APIs, but it's a very large design
space, so before you dive into implementing, I think it'd be helpful for
you to sketch out the API that you think makes sense.
…On Sun, Jul 21, 2024 at 5:25 AM Ivan Desiatov ***@***.***> wrote:
Hello, just want to bump this, since this is something we also require at
my company.
I was hoping the 43.0.0 ClientVerifier would solve my issues, and it does
to an extent by removing the subject specification requirement, but
unfortunately only some of our certificates have the EKU extension with
correct values (those actually used for client/server authentication in the
traditional sense.)
I haven't contributed to cryptography before, but I think I may be able to
contribute this functionality with some direction from more experienced
contributors. The minimal feature set resolving the issue most people have
with this is the EKU extension being required and validated. A
make_generic_verifier as was suggested in some other discussion on this
topic would solve this. A potential extension of this functionality would
then be to allow users to define custom "extension policies".
If this is something that is desired, I can discuss contributing with my
manager.
—
Reply to this email directly, view it on GitHub
<#11165 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBAWMQDNRI6BIHFXSDLZNN47XAVCNFSM6AAAAABJ42EE3GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBRGU2DCOBUGU>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
At a high level, at least, what I was imagining was a way to instantiate I think that matches the pattern being used now, and it makes sense to me, but if you think some other entry point makes more sense that's also perfectly fine. I'm more interested in having the functionality than caring about whether it is my preferred style. I'm working up a usage example that I'll hopefully post in a bit. |
My preference would definitely be for this, in terms of misuse-resistance: |
Something like this:
Obviously the namespaces for new objects are fungible. The same pattern would follow for |
Additionally to a general "here-be-dragons"
|
I like the python API that @nbastin suggests, but I think since we are trying to allow customizing verification to this extent, it shouldn't be limited to extension policies, and we should allow customization of most values in the rust I also think that the CA/B extension policies are a good default even in the case of custom verification, so they can be used unless supplied explicitly (or a different default can be applied, but I wouldn't be able to come up with one): CustomPolicyBuilder().store().eku(x509.ExtendedKeyUsageOID.CODE_SIGNING) # this still uses the current default extension policies Extension policies on the python side could either be implemented as ExtensionPolicy objects with fixed supported extensions (currently how it's done on the Rust side), or a python dict that would map arbitrary x509 extension OIDs to ee_policy = ExtensionPolicy.ee_default()
# Could accept both cryptography extension classes and oids
ee_policy[x509.KeyUsage.oid] = ExtensionValidator.present(Criticality.Critical, my_ku_validator) Regarding custom validator callbacks, I think they shouldn't accept the policy as a parameter to avoid exposing the actual rust # The signature is simplified compared to nbastin's initial suggestion, doesn't expose the policy object.
def my_ku_validator (cert, extension):
...
if not_validated:
# input on what exception type(s) should be allowed here appreciated.
raise ValueError("...") Finally, here's all of it together: from cryptography import x509
from cryptography.x509.verification import Criticality, ExtensionValidator, PolicyBuilder, ExtensionPolicy
def key_usage_validator (cert, extension):
...
ee_extension_policy = ExtensionPolicy.ee_default()
ee_extension_policy[x509.KeyUsage] = ExtensionValidator.present(Criticality.Critical, key_usage_validator)
pb = CustomPolicyBuilder() \
.store(store) \
.max_chain_depth(4) \
.ee_extension_policy(ee_extension_policy) \
.eku(x509.ExtendedKeyUsageOID.CODE_SIGNING)
vf = pb.build_client_verifier() # Current API with client and server verifier applicable depending on if subject name verification is desired. I still haven't given much thought to how this could be implemented on the Rust side, so feel free to point out any issues that you think might arise since this will be my first time using pyo3. @alex, sorry for the delayed response. What do you think about this? |
I'm not sure how to handle the built-int extension validators for the default policies on the python side, but I think they can be a different breed of |
Extension validation is not context free - validators must have access to the entire policy object or you can't do meaningful validation. |
For folks looking into this: keep in mind that the core validation handling in PyCA Cryptography is pure Rust, i.e. isn't aware of Python objects (even via PyO3). That means that any callback-style approach for custom extension handling needs to ensure that the core validator can handle callbacks without needing to obtain the GIL or access Python objects. |
Yes, I agree. I just thought of a different approach where any required context would be taken from the scope where the python custom validator is defined. I now realise that that's not ideal for multiple reasons (e.g. accessing policy properties that have not been explicitly set by the user) I am currently playing around with implementation, not as a final version, just to get accustomed to working with cryptography internals. Does anyone have thoughts on the proposed API? By the way, I would be fine limiting the scope of the proposal to custom extension policies for now, without allowing to change the algorithms permitted by the policy. However, I think a separate |
I currently have a working prototype of this feature with the API as outlined in the code snippet below. I would appreciate advice on what the next steps should be for gradually getting this reviewed and merged. Should I open a PR that contains only the .pyi interface stubs so the external python API can be finalised first? def validator_cb(policy, cert, ext):
# Any exception from the python validator is treated as failure.
raise ValueError("...")
ee_ext_policy = ExtensionPolicy.web_pki_defaults_ee()
# I ended up mirroring the rust ExtensionPolicy struct here instead of using a dict
# since only a predefined set of extensions is supported by the underlying
# cryptography_x509_verification implementation, and a dict
# would evoke the impression that arbitrary extension types are supported.
ee_ext_policy.basic_constraints = ExtensionValidator.maybe_present(
Criticality.AGNOSTIC, validator_cb
)
policy_builder = CustomPolicyBuilder().store(store)\
.max_chain_depth(10)\
.ee_extension_policy(ee_ext_policy)\
.ca_extension_policy(ca_ext_policy)\
# Setting the eku is optional.
# If the eku is not set, the default extension policies accept any eku.
# but otherwise keep the original requirements (criticality, present/non-present)
.eku(x509.ExtendedKeyUsageOID.CLIENT_AUTH)
verifier = policy_builder.build_client_verifier()
# A custom policy may not require a SANs extesnion so this may be None.
verifier.sans
# This is an always present x509.Name instance, although this may be ommitted,
# since accessing the subject of the leaf certificate does not require as
# much extra code from the user compared to getting the SANs
verifier.subject |
I think for the policy, it'd be better to have a builder. And I think (?) we do want to support arbitrary extensions (@woodruffw I believe you have a use case)? Doing a .pyi for the API sounds like a reasonable place for the extension bits. Pieces like max_chain_depth and EKU are probably straightforward enough that they could go directly to PR now. Thanks for looking at this. |
Do you mean that you prefer the API with a separate I will include the stubs, and a cut down version of the Regarding arbitrary extension support, it doesn't seem like it would be hard to implement, and might actually improve some of the code in |
If there is a branch available for testing, I'd be happy to test it out to compare it against what we can do currently with the pyopenssl X509Store ( see #10393 ). |
I have a branch in my fork of cryptography (disclaimer: wip, lackluster tests and no docs). I haven't worked with pyopenssl before, but it seems that the only way to adjust how extensions are handled is with flags, namely |
He can correct me if I misunderstood, but I think @alex meant something like: ca_exts_policy = ExtensionPolicyBuilder().subject_alternative_name(...).basic_constraints(...).build() This would also enable classmethods to extend/augment reasonable baseline policies, e.g.: ca_exts_policy = ExtensionPolicyBuilder.ca_webpki_policies().whatever(...).build() |
I like this API, as long as we don't allow to specify validators for arbitrary extension types, which @alex mentioned you might have a use case for, and might be useful in general. If we do support arbitrary extension types I think the initial proposition, where the extension policy is just a dict mapping extension OIDs to validators might make more sense. Although, doing something like @woodruffw, I'd also appreciate your input on adding a |
Yeah, I think this would be my preference -- I think custom extension support would ideally be (We can model that as an OID -> validator dictionary under the hood if doing so makes sense! But IMO the builder API is maximally consistent with PyCA's other X.509 APIs.)
I think I prefer (My actual use case is validating custom extensions that are only present in the Sigstore PKI.) |
@woodruffw I was thinking about the details of the ExtensionPolicyBuilder API and there's a slight problem with having a While it's possible, I don't like the idea of having a "dynamic function signature" based on the first parameter. By this I mean having both Instead I suggest the following interface: ext_policy_builder.not_present(oid)
ext_policy_builder.maybe_present(oid, criticality, validator)
ext_policy_builder.must_be_present(oid, criticality, validator) I think this has many positives as it reduces the number of boilerplate rust methods (no new method for each common extension), and provides a rigid API that would also make the resulting code quite readable in my opinion. PS: |
Given that the default policy seems to generally be to verify against the CA/B ruleset (a reasonable default), it would be nice for non-internet PKI users if we could pass our own extension policy rules into the verifier.
The text was updated successfully, but these errors were encountered: