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

Easily unsafe APIs in ExtensinoPolicy #12531

Open
alex opened this issue Feb 28, 2025 · 1 comment
Open

Easily unsafe APIs in ExtensinoPolicy #12531

alex opened this issue Feb 28, 2025 · 1 comment

Comments

@alex
Copy link
Member

alex commented Feb 28, 2025

In main we added an ExtensionPolicy API which lets you control how extensions are validated on certificates, both EE and CA. In the process of developing that, I noticed that because we (originally) did subject validation in the SAN handler, if we allowed overriding that SAN extension policy, we'd silently ignore subject validation. Bad!

On further review however, there's actually a repeated pattern, and right now you can disable an awful lot of sensitive policies without an obvious way to turn them back on. Specifically:

  • EE: EKU
  • CA: KU
  • CA: BasicConstraints
  • CA: EKU

This indicates we need to a) decide its ok for these to be disabled, b) if not, make sure they're always on, c) decide if the API we have is too dangerous and there's changes we need to make.

This must be resolved before we release.

cc: @woodruffw, @deivse

@alex alex added this to the Forty Fifth Release milestone Feb 28, 2025
@deivse
Copy link
Contributor

deivse commented Feb 28, 2025

So from my pov, SAN was a bit of a special case, because of the distinction between a ServerVerifier and a ClientVerifier. Basically, setting a custom SAN handler would allow you to completely bypass ExtensionPolicy-level subject match checks. For which there are valid use cases, but, that's what the ClientVerifier API is for. So for me, that was a case of forcing consumers to use the ClientVerifier API in such cases, ensuring their code is explicit in it's intent.

Regarding the big picture, I always thought of this API as dangerous, I think in one of the very first discussions I may have suggested to consider putting this into hazmat. However I think it's a necessary evil in a sense. Should someone use a CA
certificate without BasicConstraints.CA set? No. Will there be cases where someone will be working with some god awful legacy system where that is the case? Honestly, likely. To illustrate this, the whole reason I started looking into this feature originally is because I needed to ignore the value of the EKU extension. Yes that's not usually a good idea, but for that project that was the only thing that made sense.

So to sum up, I think this should be kept as is (with one important improvement I'll address in the next paragraph), but we should probably think of some ways to make it more obvious to consumers that this is indeed danger zone. Potentially putting the whole ExtensionPolicy class into hazmat.

Another thing this made me think about is the use case where someone wants to add additional constraints to the ones imposed by the current extension policies instead of relaxing them. Currently this basically requires reimplementing the original validators in python. So maybe we should expose the original rust validators as python functions, which consumers will be able to call in their own extension validator callbacks, if they want to extend the default behaviour.

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

No branches or pull requests

2 participants