-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 Verification: base Python support for extension policies #12432
Conversation
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.
Code looks good, so I'm going to merge as is.
However, I'm not wild about the name rust_policy
, since it's all Rust code! What do you think about inner_policy
?
Yeah that makes sense. I'll rename it in the next PR. |
Actually I'll make a separate PR. It's a very small change but the other PR is still quite large so I'll try to avoid cluttering it as much as possible. |
@@ -201,6 +201,9 @@ class PolicyBuilder: | |||
def time(self, new_time: datetime.datetime) -> PolicyBuilder: ... | |||
def store(self, new_store: Store) -> PolicyBuilder: ... | |||
def max_chain_depth(self, new_max_chain_depth: int) -> PolicyBuilder: ... | |||
def extension_policies( | |||
self, new_ca_policy: ExtensionPolicy, new_ee_policy: ExtensionPolicy |
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.
Might I suggest making new_ca_policy
and new_ee_policy
keyword-only arguments?
- It will force calling code to be clearer to readers, which I think is good.
- It will avoid possible confusion between the two parameters, which have the same type.
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.
Opened #12476 to address this.
That sounds reasonable to me, do you want to send a PR?
…On Sat, Feb 15, 2025, 4:48 AM Ran Benita ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/cryptography/hazmat/bindings/_rust/x509.pyi
<#12432 (comment)>:
> @@ -201,6 +201,9 @@ class PolicyBuilder:
def time(self, new_time: datetime.datetime) -> PolicyBuilder: ...
def store(self, new_store: Store) -> PolicyBuilder: ...
def max_chain_depth(self, new_max_chain_depth: int) -> PolicyBuilder: ...
+ def extension_policies(
+ self, new_ca_policy: ExtensionPolicy, new_ee_policy: ExtensionPolicy
Might I suggest making new_ca_policy and new_ee_policy keyword-only
arguments?
- It will force calling code to be clearer to readers, which I think
is good.
- It will avoid possible confusion between the two parameters, which
have the same type.
—
Reply to this email directly, view it on GitHub
<#12432 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAAGBHWERBDMVOIOIFIR232P4LP5AVCNFSM6AAAAABW4U2YT6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDMMJZGM3TONRXHE>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
|
If not, I can make a PR. Should be quick. |
@deivse please do, thanks! BTW, I am testing this and it works well so far! |
Two more comments from testing: When I run mypy which has venv/lib/python3.13/site-packages/cryptography/hazmat/bindings/_rust/x509.pyi:229: error: Type parameter lists are only supported in Python 3.12 and greater [syntax] that's on this code: cryptography/src/cryptography/hazmat/bindings/_rust/x509.pyi Lines 229 to 243 in ca6db8f
with mypy 1.11.2, but I also tried with mypy 1.15.0 and it still complains. So it's probably best to avoid the new Python 3.12 syntax? The py_webauthn tests use a x509 v1 certificate, which the verification doesn't like even with
But, I'm not sure if it's a realistic certificate or just a test thing. I will try to ask the py_webauthn author. Here is the cert for reference Details
|
Hi @bluetech, glad you're finding this useful! Some observations regarding the first point:
However, this behaviour only occurs with type parameter lists. The I had some doubts about this initially, but me and Alex eventually agreed that this should work since it's only in a .pyi file, and it does seem to work, but only with some syntax?.. 😄 I'm wondering if this behaviour is intended or if we should open an issue with |
Regarding the x509 v1 certificate, that is intended behaviour. |
That’s correct, we don’t support v1 and have no current plans to do so. |
No description provided.