Migrate PKCS7 backend to Rust #10228
Conversation
facutuesca
force-pushed
the
pkcs7-rust
branch
from
3dba583
to
af8aa03
Compare
src/rust/src/pkcs7.rs
Outdated
| let nid = pkcs7.type_().map_or(openssl::nid::Nid::UNDEF, |x| x.nid()); | ||
| assert_ne!(nid, openssl::nid::Nid::UNDEF); | ||
| if nid != openssl::nid::Nid::PKCS7_SIGNED { |
There was a problem hiding this comment.
What's the reason for this assertion? Can we not just do nid = pkcs7.type_().map(|t| t.nid()); if nid != Some(PKCS7_SIGNED) {}?
There was a problem hiding this comment.
I replicated the behavior from the Python implementation: src. Should I change it?
There was a problem hiding this comment.
Yes, I think my formulation here is better --
it's important to note that the openssl_assert() would produce a python execption while this code would produce a rust panic, so they're not quite teh same
src/rust/src/pkcs7.rs
Outdated
| )), | ||
| Some(c) => c | ||
| .iter() | ||
| .map(|c| load_pem_x509_certificate(py, c.to_pem()?.as_slice(), None)) |
There was a problem hiding this comment.
Round-trip via DER, it'll be faster.
facutuesca
force-pushed
the
pkcs7-rust
branch
from
af8aa03
to
d0d4407
Compare
|
Leaving as a draft for now since |
|
I believe that's because BoringSSL is missing PKCS#7 functionality -- we should just disable this there. |
|
facutuesca
force-pushed
the
pkcs7-rust
branch
3 times, most recently
from
05c5b5b
to
9bee3fc
Compare
facutuesca
force-pushed
the
pkcs7-rust
branch
from
9bee3fc
to
6efad7f
Compare
| exceptions::UnsupportedAlgorithm::new_err(( | ||
| "PKCS#7 is not supported by this backend.", | ||
| exceptions::Reasons::BACKEND_MISSING_INTERFACE, | ||
| )), |
There was a problem hiding this comment.
Use UNSUPPORTED_SERIALIZATION here I think
src/rust/src/pkcs7.rs
Outdated
| fn load_pkcs7_certificates( | ||
| py: pyo3::Python<'_>, | ||
| pkcs7: Pkcs7, | ||
| ) -> CryptographyResult<Vec<x509::certificate::Certificate>> { |
There was a problem hiding this comment.
Small nit: Instead of returning a Vec, I'd suggest the &pyo3::types::PyList. This avoids a copy+malloc round trip.
See verify.rs l239 for an example of this
facutuesca
force-pushed
the
pkcs7-rust
branch
2 times, most recently
from
b94a208
to
71ec332
Compare
src/rust/src/pkcs7.rs
Outdated
| result.append( | ||
| load_der_x509_certificate( | ||
| py, | ||
| pyo3::types::PyBytes::new(py, c.to_der()?.as_slice()).into_py(py), | ||
| None, | ||
| )? | ||
| .into_py(py), | ||
| )?; |
There was a problem hiding this comment.
You're getting coverage issues here. Best resolution is to rewrite as:
let cert_der = pyo3::types::PyBytes::new(py, c.to_der()?.as_slice()).into_py(py);
let cert = load_der_x509_certificate(py, cert_der, None)?;
result.append(cert.into_py(py))?;
Head branch was pushed to by a user without write access
facutuesca
force-pushed
the
pkcs7-rust
branch
from
71ec332
to
6ded9d7
Compare
facutuesca
force-pushed
the
pkcs7-rust
branch
from
6ded9d7
to
e278436
Compare
Part of #8770. Now that
rust-opensslhas the necessary bindings, this PR replaces the Python implementation with calls to the Rust OpenSSL bindings.