support negative serials in certificate parsing #6626
Conversation
but raise a warning every time we see it. also proactively raise on initial parse of the certificate, not just when accessing the serial_number attribute
src/rust/src/asn1.rs
Outdated
| v: asn1::BigUint<'_>, | ||
| v: &'_ [u8], | ||
| ) -> pyo3::PyResult<&'p pyo3::PyAny> { | ||
| let signed = big_byte_slice_is_negative(py, v)?; |
There was a problem hiding this comment.
This seems like a weird way to solve this, why not just take signed as a parameter that teh caller specifies based on context?
src/rust/src/asn1.rs
Outdated
| bytes: &'_ [u8], | ||
| ) -> pyo3::PyResult<bool> { | ||
| if bytes[0] & 0x80 != 0 { | ||
| let cryptography_warning = py.import("cryptography.utils")?.getattr("DeprecatedIn36")?; |
There was a problem hiding this comment.
This is not the right place for this warning. This function has an ostensibly generic purpose and instead it's got behavior that's totally specific to serials.
src/rust/src/x509/certificate.rs
Outdated
| cert_version(py, raw.borrow_value().tbs_cert.version)?; | ||
| // determine if the serial is negative and raise a warning if it is. We want to drop support | ||
| // for this sort of invalid encoding eventually. | ||
| big_byte_slice_is_negative(py, raw.borrow_value().tbs_cert.serial.as_bytes())?; |
There was a problem hiding this comment.
See! This functions behavior in no way matches it's name.
src/rust/src/asn1.rs
Outdated
| v: asn1::BigUint<'_>, | ||
| v: &'_ [u8], | ||
| ) -> pyo3::PyResult<&'p pyo3::PyAny> { | ||
| warn_if_negative(py, v)?; |
There was a problem hiding this comment.
I still object to this being part of the behavior of `big_byte_slice_to_py_int.
Functions behavior should be predictable and well factored -- generic functions like type conversion shouldn't have context specific warnings like this. Just put warn_if_negative calls in the serial accessors. And please also name it warn_if_negative_serial based on the warning message.
Co-authored-by: Alex Gaynor <[email protected]>
src/rust/src/x509/ocsp_req.rs
Outdated
| Ok(big_byte_slice_to_py_int( | ||
| py, | ||
| self.cert_id()?.serial_number.as_bytes(), | ||
| )?) |
There was a problem hiding this comment.
This is uncovered because of silliness. assign self.cert_id()?.serial_number.as_bytes() to a local var
but raise a warning every time we see it. also proactively raise on
initial parse of the certificate, not just when accessing the
serial_number attribute