X509 custom verification groundwork #11559

deivse · 2024-09-07T09:22:33Z

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 separate verify.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 and PolicyBuilder, 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.

deivse · 2024-09-07T09:45:00Z

Marking this as ready for review.

I'm quite certain that the CI / linux (3.12, rust,tests, beta) check failing has nothing to do with my changes as I haven't touched any crypto primitives in this PR and the error is an OpenSSL error during RSA private key generation. (Now passing.)

The rust coverage check is also failing, but from what I can tell coverage on main was below 100% before this PR, and I only slightly reduced it for verify.rs due to reasons mentioned in PR description.

alex · 2024-09-11T04:07:38Z

(Sorry I've been slow to get to this, it's at the top of the queue for tomorrow.)

deivse · 2024-09-11T08:16:33Z

(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.

src/rust/src/x509/verify.rs

alex

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)

alex · 2024-09-11T12:44:05Z

docs/x509/verification.rst

+    .. 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


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.

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.

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

alex · 2024-09-12T00:21:49Z

docs/x509/verification.rst

@@ -294,3 +330,82 @@ the root of trust:
            for server verification.

        :returns: An instance of :class:`ClientVerifier`
+
+.. class:: CustomPolicyBuilder


Do we need a new CustomPolicyBuilder? Can they just be methods on the existing PolicyBuilder?

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)

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.

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.

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.

ulrikstrid · 2024-09-19T08:32:59Z

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!

deivse · 2024-09-19T08:34:49Z

Will there be a separate PR for setting EKU in the policy?

Yes! This is just to reduce the scope of this PR as requested by alex and reaperhulk.

deivse · 2024-09-26T07:20:26Z

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)

alex · 2024-09-28T02:06:42Z

No worries, don't hesitate to ping. Will review now (and then I'll immediately disappear for the weekend :-))

alex · 2024-09-28T02:08:15Z

docs/x509/verification.rst

@@ -294,3 +330,82 @@ the root of trust:
            for server verification.

        :returns: An instance of :class:`ClientVerifier`
+
+.. class:: CustomPolicyBuilder


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

src/rust/src/x509/verify.rs

alex · 2024-09-28T02:12:54Z

docs/x509/verification.rst

@@ -294,3 +330,82 @@ the root of trust:
            for server verification.

        :returns: An instance of :class:`ClientVerifier`
+
+.. class:: CustomPolicyBuilder


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.

deivse · 2024-09-29T14:17:33Z

Rebased on main.

…bjects`.

deivse · 2024-10-07T11:29:21Z

@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.

alex · 2024-10-07T11:31:34Z

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.

deivse · 2024-10-07T11:32:56Z

No worries whatsoever!

alex

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.

src/rust/src/x509/verify.rs

deivse · 2024-10-08T08:51:29Z

Hi @alex, regarding the coverage issue, I knew that the subjects is None path would not be reachable, but decided to keep it in this PR anyway, because when I ran nox -e tests locally on main the coverage was below 100% anyway. However I can move that change to one of the next PRs, the one with the actual changes to cryptography-x509-verification, just tell me what you prefer. Also, is coverage on main actually below 100% or are my local results somehow different?

alex · 2024-10-08T10:55:28Z

We have 100% combined coverage between all of our CI jobs, but no individual job has 100% by itself.

That 100% is a requirement.

deivse · 2024-10-08T11:13:42Z

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.

alex · 2024-10-08T11:16:23Z

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.

deivse · 2024-10-08T11:27:00Z

Good point, I did as you suggested, so let's see if I got everything. I also removed the extension policy fields from PolicyBuilders, since coverage was failing for #[derive(Clone)] since the fields were always None, so clone wasn't actually called.
I'll add that back in in the PR where I add the extension policy setters.

alex

Thanks for sticking with this!

deivse marked this pull request as ready for review September 7, 2024 09:45

deivse force-pushed the x509_verification_custom_builder branch 2 times, most recently from 11874a5 to 0852630 Compare September 11, 2024 08:32

deivse commented Sep 11, 2024

View reviewed changes

src/rust/src/x509/verify.rs Outdated Show resolved Hide resolved

alex reviewed Sep 12, 2024

View reviewed changes

alex reviewed Sep 28, 2024

View reviewed changes

deivse mentioned this pull request Sep 28, 2024

Reduce code duplication in PolicyBuilder already set checks. #11666

Merged

deivse force-pushed the x509_verification_custom_builder branch 2 times, most recently from de53b3e to 1ebb1e7 Compare September 29, 2024 14:17

deivse added 7 commits September 29, 2024 16:18

Add CustomPolicyBuilder foundation.

37f4cec

Add EKU getters to ClientVerifier and ServerVerifier.

f40b863

Document the implemented part of custom verification.

42c00a7

Remove subject field from VerifiedClient, rename sans back to `su…

eb4050b

…bjects`.

Remove EKU-related setters, getters and documentation from this PR.

36adbe9

Use double backticks in reStructuredText.

624be1d

Remove CustomPolicyBuilder in favor of extending PolicyBuilder.

3138e8e

deivse force-pushed the x509_verification_custom_builder branch from 1ebb1e7 to 3138e8e Compare September 29, 2024 14:19

deivse requested a review from alex September 29, 2024 14:24

alex reviewed Oct 8, 2024

View reviewed changes

src/rust/src/x509/verify.rs Outdated Show resolved Hide resolved

src/rust/src/x509/verify.rs Outdated Show resolved Hide resolved

Code style improvements.

866dffa

deivse force-pushed the x509_verification_custom_builder branch from a81d862 to 06f15ee Compare October 8, 2024 11:27

Resolve coverage issues.

a5b9a42

deivse force-pushed the x509_verification_custom_builder branch from 06f15ee to a5b9a42 Compare October 8, 2024 11:28

deivse requested a review from alex October 8, 2024 11:34

deivse changed the title ~~X509 CustomPolicyBuilder foundation.~~ X509 custom verification groundwork Oct 8, 2024

alex approved these changes Oct 9, 2024

View reviewed changes

alex merged commit 1767ad0 into pyca:main Oct 9, 2024
59 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

X509 custom verification groundwork #11559

X509 custom verification groundwork #11559

deivse commented Sep 7, 2024

deivse commented Sep 7, 2024 •

edited

Loading

alex commented Sep 11, 2024

deivse commented Sep 11, 2024

alex left a comment

alex Sep 11, 2024

deivse Sep 12, 2024

deivse Sep 19, 2024

alex Sep 12, 2024

deivse Sep 12, 2024

alex Sep 28, 2024

alex Sep 28, 2024

deivse Sep 28, 2024 •

edited

Loading

ulrikstrid commented Sep 19, 2024

deivse commented Sep 19, 2024 •

edited

Loading

deivse commented Sep 26, 2024

alex commented Sep 28, 2024

alex Sep 28, 2024

alex Sep 28, 2024

deivse commented Sep 29, 2024

deivse commented Oct 7, 2024 •

edited

Loading

alex commented Oct 7, 2024 via email

deivse commented Oct 7, 2024

alex left a comment

deivse commented Oct 8, 2024

alex commented Oct 8, 2024

deivse commented Oct 8, 2024 •

edited

Loading

alex commented Oct 8, 2024 via email

deivse commented Oct 8, 2024 •

edited

Loading

alex left a comment

X509 custom verification groundwork #11559

X509 custom verification groundwork #11559

Conversation

deivse commented Sep 7, 2024

deivse commented Sep 7, 2024 • edited Loading

alex commented Sep 11, 2024

deivse commented Sep 11, 2024

alex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivse Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

ulrikstrid commented Sep 19, 2024

deivse commented Sep 19, 2024 • edited Loading

deivse commented Sep 26, 2024

alex commented Sep 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deivse commented Sep 29, 2024

deivse commented Oct 7, 2024 • edited Loading

alex commented Oct 7, 2024 via email

deivse commented Oct 7, 2024

alex left a comment

Choose a reason for hiding this comment

deivse commented Oct 8, 2024

alex commented Oct 8, 2024

deivse commented Oct 8, 2024 • edited Loading

alex commented Oct 8, 2024 via email

deivse commented Oct 8, 2024 • edited Loading

alex left a comment

Choose a reason for hiding this comment

deivse commented Sep 7, 2024 •

edited

Loading

deivse Sep 28, 2024 •

edited

Loading

deivse commented Sep 19, 2024 •

edited

Loading

deivse commented Oct 7, 2024 •

edited

Loading

deivse commented Oct 8, 2024 •

edited

Loading

deivse commented Oct 8, 2024 •

edited

Loading