-
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
X509 CustomPolicyBuilder
foundation.
#11559
base: main
Are you sure you want to change the base?
Conversation
Marking this as ready for review.
The rust coverage check is also failing, but from what I can tell coverage on |
(Sorry I've been slow to get to this, it's at the top of the queue for tomorrow.) |
No worries! I will make a couple more changes today after looking through the PR with fresh eyes. |
f169597
to
11874a5
Compare
11874a5
to
0852630
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Start with some thoughts on the API in the docs, once we work through these I can shift to reviewing the implementation. (If there's points you'd like a set of eyes on before then, please feel free to just ping me)
docs/x509/verification.rst
Outdated
.. 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 |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
docs/x509/verification.rst
Outdated
@@ -156,6 +168,18 @@ the root of trust: | |||
:type: :class:`Store` | |||
|
|||
The verifier's trust store. | |||
|
|||
.. attribute:: eku |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. 😄
There was a problem hiding this comment.
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.
|
||
:returns: A new instance of :class:`PolicyBuilder` | ||
|
||
.. method:: max_chain_depth(new_max_chain_depth) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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.
@@ -294,3 +330,82 @@ the root of trust: | |||
for server verification. | |||
|
|||
:returns: An instance of :class:`ClientVerifier` | |||
|
|||
.. class:: CustomPolicyBuilder |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
Will there be a separate PR for setting EKU in the policy? This is currently a blocker for us using cryptography for chain verification. We have a workaround using PyOpenSSL for these parts but would be awesome if we could use cryptography all the way. Thank you for you work! |
Yes! This is just to reduce the scope of this PR as requested by alex and reaperhulk. |
This PR adds the
CustomPolicyBuilder
class and part of it's API as discussed in #11165.So far this is mostly boilerplate code. The next PR, that will touch the actual
cryptography-x509-verification
crate more, will add the actual custom extension policy and user-provided extension validator callbacks support. For the next PR I also plan to separateverify.rs
into multiple files since it's getting kinda long, but I decided to hold back on doing that for now in favour of having a working diff.I have extended the tests to run with both
CustomPolicyBuilder
andPolicyBuilder
, but there is still a slight reduction in coverage due to some things that need the rest of the implementation to be tested.I have also updated the docs. Some undocumented features are referenced that are not yet contained in this PR. I plan to complete the docs in the next PR.
PS: This PR is definitely on the bigger side by cryptography standards. I felt that it might be fine since the code in this one isn't very cognitively taxing and a lot of it is documentation/.pyi boilerplate as well. But feel free to suggest how to break this up further if you decide it's needed.