-
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 custom verification groundwork #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. |
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
@@ -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)
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 not really sold on the need for a totally separate builder, but maybe we can split the difference and have the constructor be PolicyBuilder.custom()
, that way it's not a totally separate type, but still makes clear you're doing something a bit different.
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.
Importantly: If they're the same class, there's a lot of duplication that goes away, including in both the implementation and the tests. Then we make the focus really on the new APIs.
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.
If we are not adding a separate class, I don't think an implementation with custom
would be very clean either to be honest. I think I will just put all of the new functionality into PolicyBuilder
, which, I agree, will make the implementation a lot more pleasant. I'll do that as the last commit in this review round so it's easily revertable in case we reach a different conclusion in the end.
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. |
Hey @alex, just wanted to make sure this is still in the review queue (sorry for ping, completely understand if you don't have the time right now) |
No worries, don't hesitate to ping. Will review now (and then I'll immediately disappear for the weekend :-)) |
docs/x509/verification.rst
Outdated
@@ -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.
I'm not really sold on the need for a totally separate builder, but maybe we can split the difference and have the constructor be PolicyBuilder.custom()
, that way it's not a totally separate type, but still makes clear you're doing something a bit different.
docs/x509/verification.rst
Outdated
@@ -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.
Importantly: If they're the same class, there's a lot of duplication that goes away, including in both the implementation and the tests. Then we make the focus really on the new APIs.
de53b3e
to
1ebb1e7
Compare
Rebased on main. |
1ebb1e7
to
3138e8e
Compare
@alex I have merged the CustomPolicyBuilder functionality into PolicyBuilder last week. I think the scope of this PR is finally getting to a point where we can merge it soon. |
Sorry I've been very slow to review this, it's on my TODO list for today.
…On Mon, Oct 7, 2024 at 7:29 AM Ivan Desiatov ***@***.***> wrote:
@alex <https://github.com/alex> I have merged the CustomPolicyBuilder
functionality into PolicyBuilder last week. I think the scope of this PR is
finally getting to a point where we can merge it soon.
—
Reply to this email directly, view it on GitHub
<#11559 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBHLTDV7GC27RVVC7YLZ2JWCRAVCNFSM6AAAAABNZ3TLJCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJWGY3DIMRWGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
No worries whatsoever! |
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.
This mostly looks good, but we require 100% coverage, so I'm not sure the final changes to subjects
are reachable until we add a bit more functionality.
Hi @alex, regarding the coverage issue, I knew that the |
We have 100% combined coverage between all of our CI jobs, but no individual job has 100% by itself. That 100% is a requirement. |
Ok, I have now found the combined coverage report artifact in the CI job, didn't see that before. I don't really see a way to make the With the removal of this change, this PR has only minor changes, but I guess that just means we can merge it and go on to the next one that will have actual changes. |
FWIW, you should be able to include the type declaration change, just not
the final parsing bit.
…On Tue, Oct 8, 2024 at 7:14 AM Ivan Desiatov ***@***.***> wrote:
Ok, I have now found the combined coverage report artifact in the CI job,
didn't see that before. I don't really see a way to make the subjects
field optional without changes that are outside this PR's scope, so I'll
just remove that. I see a couple other coverage issues I'll have to fix as
well.
With the removal of this change, this PR has only minor changes, but I
guess that just means we can merge it and go on to the next one that will
have actual changes.
—
Reply to this email directly, view it on GitHub
<#11559 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBCQB45URAHWSJYCZD3Z2O47ZAVCNFSM6AAAAABNZ3TLJCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJZGU2TOMJYGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
All that is necessary for evil to succeed is for good people to do nothing.
|
Good point, I did as you suggested, so let's see if I got everything. I also removed the extension policy fields from |
a81d862
to
06f15ee
Compare
06f15ee
to
a5b9a42
Compare
CustomPolicyBuilder
foundation.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.
Thanks for sticking with this!
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.