Skip to content
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

PyPolicyDefinition and related Verifier API changes #12390

Merged
merged 12 commits into from
Feb 5, 2025

Conversation

deivse
Copy link
Contributor

@deivse deivse commented Feb 3, 2025

No description provided.

@deivse
Copy link
Contributor Author

deivse commented Feb 3, 2025

This is just the rust changes, I haven't adjusted the tests for the new API where things are accessible via .policy instead of directly, nor have I updated the .pyi files. I'll get to that tomorrow.

@deivse
Copy link
Contributor Author

deivse commented Feb 3, 2025

Buut, this is a good time to discuss if we do want these things to be accessible only via the policy field on the verifiers, or if we want to keep the old API alongside that for backwards-compatibility/convenience. Not sure about that to be honest.

@reaperhulk
Copy link
Member

If they're just aliases then my opinion here is that we keep, but raise warnings and remove in 46. We have explicitly stated this isn't a stable API yet and is not subject to our backwards compatibility policy, but we do have consumers so we should give them a window for migration. @alex do you have an opinion?

@alex
Copy link
Member

alex commented Feb 3, 2025

What @reaperhulk said sounds fine.

@deivse
Copy link
Contributor Author

deivse commented Feb 3, 2025

Any pointers on what to look at for implementing deprecation warnings for python getters in rust? I think I have seen something along these lines in the codebase but I have no idea where anymore. (Will update the PR tomorrow - it's already evening here in Prague)

@reaperhulk
Copy link
Member

let warning_cls = types::DEPRECATED_IN_42.get(py)?;
let message = cstr_from_literal!("Properties that return a naïve datetime object have been deprecated. Please switch to next_update_utc.");
pyo3::PyErr::warn(py, &warning_cls, message, 1)?;
should point you the right direction

@deivse
Copy link
Contributor Author

deivse commented Feb 3, 2025

Exactly what I was looking for, thank you! Will report back tomorrow.

@deivse
Copy link
Contributor Author

deivse commented Feb 4, 2025

I decided to rename PyPolicyDefinition to PyPolicy. The original name was to better reflect that it holds a PolicyDefinition, not a Policy, but I think I prefer to maintain the convention of PyXXX name directly corresponding to XXX in python.

PS: Will add the deprecated getters back in in a bit.

@deivse deivse marked this pull request as ready for review February 4, 2025 12:46
src/cryptography/hazmat/bindings/_rust/x509.pyi Outdated Show resolved Hide resolved
src/rust/src/x509/verify.rs Outdated Show resolved Hide resolved
Comment on lines 466 to 468
SubjectOwner::None => Err(pyo3::exceptions::PyRuntimeError::new_err(
"subject must be set for ServerVerifiers",
)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this should be unreachable from the Python API, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If yes, this can just be a panic.

Copy link
Contributor Author

@deivse deivse Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I came up with a cleaner and better solution in general - instead of adding a None value to SubjectOwner, I just made OwnedPolicyDefinition use Option<SubjectOwner> as the owner. This way the build_subject function doesn't need to handle None (.expect is used in the build_server_verifier method), and even the additional test shouldn't be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take a look and tell me if u prefer it that way. If not I can revert back to old behaviour with a panic instead of returning pyerror.

src/rust/src/x509/verify/py_policy.rs Outdated Show resolved Hide resolved
@deivse deivse force-pushed the custom_x509_verif_py_policy branch from 601db4e to a039d3d Compare February 5, 2025 14:01
@alex alex merged commit 4f9ca1c into pyca:main Feb 5, 2025
63 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants