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

Start converting src/x509/verify.rs to new pyo3 APIs #10736

Merged
merged 5 commits into from
Apr 5, 2024

Conversation

facutuesca
Copy link
Contributor

@facutuesca facutuesca commented Apr 5, 2024

Part of #10676

This fixes all except one of the migration warnings for src/x509/verify.rs. The remaining warning should be fixed by the changes on verify.rs on this PR: link

cc @alex @reaperhulk

src/rust/Cargo.toml Outdated Show resolved Hide resolved
alex
alex previously approved these changes Apr 5, 2024
@alex alex enabled auto-merge (squash) April 5, 2024 12:21
@facutuesca facutuesca disabled auto-merge April 5, 2024 12:21
@facutuesca
Copy link
Contributor Author

Ah hold off the review, I pushed too early, still need to fix a couple of errors

@facutuesca
Copy link
Contributor Author

facutuesca commented Apr 5, 2024

@alex Done, please check the last commit: I had to remove a call to Bound::to_str because it's not supported in Python < 3.10.
The change is:

// before
        let value = subject
            .getattr(pyo3::intern!(py, "value"))?
            .downcast::<pyo3::types::PyString>()?
            .clone();
        Ok(SubjectOwner::DNSName(value.to_str()?.to_owned()))

// after
        let value = subject
            .getattr(pyo3::intern!(py, "value"))?
            .extract::<String>()?;
        Ok(SubjectOwner::DNSName(value))

So I changed the downcast + clone to a extract::<String>() call. Do you think that makes sense?

Ok(SubjectOwner::IPAddress(value.into()))
.downcast::<pyo3::types::PyBytes>()?
.clone();
Ok(SubjectOwner::IPAddress(value.clone().unbind()))
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this clone is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, fixed!

Ok(SubjectOwner::DNSName(value.to_str()?.to_owned()))
} else if subject.is_instance(types::IP_ADDRESS.get(py)?)? {
.extract::<String>()?;
Ok(SubjectOwner::DNSName(value))
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment reminding us that when our minimum python is 3.10, we can switch to borrowing the str.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@alex alex enabled auto-merge (squash) April 5, 2024 13:26
@facutuesca
Copy link
Contributor Author

Looks like a GHA error on one of the runners:

INTERNALERROR>   File "/home/runner/work/cryptography/cryptography/.nox/tests-nocoverage/lib/pypy3.10/site-packages/xdist/dsession.py", line 137, in loop_once
INTERNALERROR>     raise RuntimeError("Unexpectedly no active workers available")
INTERNALERROR> RuntimeError: Unexpectedly no active workers available

@alex alex merged commit afe3951 into pyca:main Apr 5, 2024
58 checks passed
@facutuesca facutuesca deleted the x509-verify-pyo3 branch April 5, 2024 13:40
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.

2 participants