search

pyca / cryptography

cryptography is a package designed to expose cryptographic primitives and recipes to Python developers.
https://cryptography.io
Other
6.68k stars 1.54k forks source link

X509 custom verification groundwork #11559

Closed deivse closed 1 month ago

deivse commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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

deivse commented 2 months ago

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

ulrikstrid commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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

deivse commented 1 month ago

Rebased on main.

deivse commented 1 month ago

@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 commented 1 month ago

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 https://github.com/pyca/cryptography/pull/11559#issuecomment-2396664266, 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 commented 1 month ago

No worries whatsoever!

deivse commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/pyca/cryptography/pull/11559#issuecomment-2399557180, 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 commented 1 month ago

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.