X509 CustomPolicyBuilder foundation.
#11559
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. |
deivse
force-pushed
the
x509_verification_custom_builder
branch
from
f169597
to
11874a5
Compare
deivse
force-pushed
the
x509_verification_custom_builder
branch
from
11874a5
to
0852630
Compare
| .. 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.
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.
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.
| @@ -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.
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.
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.
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. 😄
|
|
||
| :returns: A new instance of :class:`PolicyBuilder` | ||
|
|
||
| .. method:: max_chain_depth(new_max_chain_depth) |
There was a problem hiding this comment.
Same for this, I think this could be in its own PR.
There was a problem hiding this comment.
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.
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.
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.
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.
Do we need a new CustomPolicyBuilder? Can they just be methods on the existing PolicyBuilder?
There was a problem hiding this comment.
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)
This PR adds the
CustomPolicyBuilderclass 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-verificationcrate more, will add the actual custom extension policy and user-provided extension validator callbacks support. For the next PR I also plan to separateverify.rsinto 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
CustomPolicyBuilderandPolicyBuilder, 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.