-
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
certificate: add a get_extension
helper
#8892
Changes from all commits
351af50
302eb18
3cd1a7d
c37e66f
326634c
a900f6b
4c59f62
b393f1b
07e7cee
897161e
22b41c5
0ce6afb
b2a9e07
2811cc8
e293f9c
0893194
2dbb3eb
aa924dd
5189ca7
e9d8e9f
8ec095e
e8ef3b7
ea5da30
b9c14ac
f6f0367
fb585cf
cdd65d4
212b75f
e2bb769
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,13 +9,13 @@ use crate::error::{CryptographyError, CryptographyResult}; | |
use crate::x509::{extensions, sct, sign}; | ||
use crate::{exceptions, x509}; | ||
use cryptography_x509::common::Asn1ReadableOrWritable; | ||
use cryptography_x509::extensions::Extension; | ||
use cryptography_x509::extensions::{ | ||
AuthorityKeyIdentifier, BasicConstraints, DisplayText, DistributionPoint, | ||
DistributionPointName, MSCertificateTemplate, NameConstraints, PolicyConstraints, | ||
PolicyInformation, PolicyQualifierInfo, Qualifier, SequenceOfAccessDescriptions, | ||
PolicyInformation, PolicyQualifierInfo, Qualifier, RawExtensions, SequenceOfAccessDescriptions, | ||
SequenceOfSubtrees, UserNotice, | ||
}; | ||
use cryptography_x509::extensions::{Extension, Extensions}; | ||
use cryptography_x509::{common, name, oid}; | ||
use once_cell::sync::Lazy; | ||
use pyo3::{IntoPy, ToPyObject}; | ||
|
@@ -193,9 +193,9 @@ impl Certificate { | |
let val = self.raw.borrow_value(); | ||
let mut tbs_precert = val.tbs_cert.clone(); | ||
// Remove the SCT list extension | ||
match tbs_precert.extensions { | ||
Some(extensions) => { | ||
let readable_extensions = extensions.unwrap_read().clone(); | ||
match val.tbs_cert.extensions() { | ||
Ok(Some(extensions)) => { | ||
let readable_extensions = extensions.as_raw().unwrap_read().clone(); | ||
let ext_count = readable_extensions.len(); | ||
let filtered_extensions: Vec<Extension<'_>> = readable_extensions | ||
.filter(|x| x.extn_id != oid::PRECERT_SIGNED_CERTIFICATE_TIMESTAMPS_OID) | ||
|
@@ -207,18 +207,26 @@ impl Certificate { | |
), | ||
)); | ||
} | ||
let filtered_extensions: Extensions<'_> = Asn1ReadableOrWritable::new_write( | ||
let filtered_extensions: RawExtensions<'_> = Asn1ReadableOrWritable::new_write( | ||
asn1::SequenceOfWriter::new(filtered_extensions), | ||
); | ||
tbs_precert.extensions = Some(filtered_extensions); | ||
tbs_precert.raw_extensions = Some(filtered_extensions); | ||
let result = asn1::write_single(&tbs_precert)?; | ||
Ok(pyo3::types::PyBytes::new(py, &result)) | ||
} | ||
None => Err(CryptographyError::from( | ||
Ok(None) => Err(CryptographyError::from( | ||
pyo3::exceptions::PyValueError::new_err( | ||
"Could not find any extensions in TBS certificate", | ||
), | ||
)), | ||
Err(oid) => { | ||
let oid_obj = oid_to_py_oid(py, &oid)?; | ||
Err(exceptions::DuplicateExtension::new_err(( | ||
format!("Duplicate {} extension found", oid), | ||
oid_obj.into_py(py), | ||
)) | ||
.into()) | ||
} | ||
Comment on lines
+222
to
+229
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To avoid needing to repeat this stanza anywhere we touch extensions, can we make There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, good idea -- I'll do that now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 212b75f is my attempt at this -- it saves us the repetition of this error clause in exchange for a new error enum + |
||
} | ||
} | ||
|
||
|
@@ -360,7 +368,7 @@ impl Certificate { | |
x509::parse_and_cache_extensions( | ||
py, | ||
&mut self.cached_extensions, | ||
&self.raw.borrow_value().tbs_cert.extensions, | ||
&self.raw.borrow_value().tbs_cert.raw_extensions, | ||
|oid, ext_data| match *oid { | ||
oid::PRECERT_POISON_OID => { | ||
asn1::parse_single::<()>(ext_data)?; | ||
|
@@ -1035,7 +1043,7 @@ fn create_x509_certificate( | |
spki: asn1::parse_single(spki_bytes)?, | ||
issuer_unique_id: None, | ||
subject_unique_id: None, | ||
extensions: x509::common::encode_extensions( | ||
raw_extensions: x509::common::encode_extensions( | ||
py, | ||
builder.getattr(pyo3::intern!(py, "_extensions"))?, | ||
extensions::encode_extension, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,11 +6,10 @@ use crate::asn1::{oid_to_py_oid, py_oid_to_oid}; | |
use crate::error::{CryptographyError, CryptographyResult}; | ||
use crate::{exceptions, x509}; | ||
use cryptography_x509::common::{Asn1ReadableOrWritable, AttributeTypeValue, RawTlv}; | ||
use cryptography_x509::extensions::{AccessDescription, Extension, Extensions}; | ||
use cryptography_x509::extensions::{AccessDescription, Extension, Extensions, RawExtensions}; | ||
use cryptography_x509::name::{GeneralName, Name, OtherName, UnvalidatedIA5String}; | ||
use pyo3::types::IntoPyDict; | ||
use pyo3::{IntoPy, ToPyObject}; | ||
use std::collections::HashSet; | ||
|
||
/// Parse all sections in a PEM file and return the first matching section. | ||
/// If no matching sections are found, return an error. | ||
|
@@ -391,27 +390,30 @@ pub(crate) fn parse_and_cache_extensions< | |
>( | ||
py: pyo3::Python<'p>, | ||
cached_extensions: &mut Option<pyo3::PyObject>, | ||
raw_exts: &Option<Extensions<'_>>, | ||
raw_extensions: &Option<RawExtensions<'_>>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we actually want this method to take an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That SGTM, although if I understand correctly doing that will require us to make the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, let's leave this change for a seperate PR. |
||
parse_ext: F, | ||
) -> pyo3::PyResult<pyo3::PyObject> { | ||
if let Some(cached) = cached_extensions { | ||
return Ok(cached.clone_ref(py)); | ||
} | ||
|
||
let extensions = match Extensions::from_raw_extensions(raw_extensions.as_ref()) { | ||
Ok(extensions) => extensions, | ||
Err(oid) => { | ||
let oid_obj = oid_to_py_oid(py, &oid)?; | ||
return Err(exceptions::DuplicateExtension::new_err(( | ||
format!("Duplicate {} extension found", oid), | ||
oid_obj.into_py(py), | ||
))); | ||
} | ||
}; | ||
|
||
let x509_module = py.import(pyo3::intern!(py, "cryptography.x509"))?; | ||
let exts = pyo3::types::PyList::empty(py); | ||
let mut seen_oids = HashSet::new(); | ||
if let Some(raw_exts) = raw_exts { | ||
for raw_ext in raw_exts.unwrap_read().clone() { | ||
if let Some(extensions) = extensions { | ||
for raw_ext in extensions.as_raw().unwrap_read().clone() { | ||
let oid_obj = oid_to_py_oid(py, &raw_ext.extn_id)?; | ||
|
||
if seen_oids.contains(&raw_ext.extn_id) { | ||
return Err(exceptions::DuplicateExtension::new_err(( | ||
format!("Duplicate {} extension found", raw_ext.extn_id), | ||
oid_obj.into_py(py), | ||
))); | ||
} | ||
|
||
let extn_value = match parse_ext(&raw_ext.extn_id, raw_ext.extn_value)? { | ||
Some(e) => e, | ||
None => x509_module.call_method1( | ||
|
@@ -424,7 +426,6 @@ pub(crate) fn parse_and_cache_extensions< | |
(oid_obj, raw_ext.critical, extn_value), | ||
)?; | ||
exts.append(ext_obj)?; | ||
seen_oids.insert(raw_ext.extn_id); | ||
} | ||
} | ||
let extensions = x509_module | ||
|
@@ -445,7 +446,7 @@ pub(crate) fn encode_extensions< | |
py: pyo3::Python<'p>, | ||
py_exts: &'p pyo3::PyAny, | ||
encode_ext: F, | ||
) -> pyo3::PyResult<Option<Extensions<'p>>> { | ||
) -> pyo3::PyResult<Option<RawExtensions<'p>>> { | ||
let unrecognized_extension_type: &pyo3::types::PyType = py | ||
.import(pyo3::intern!(py, "cryptography.x509"))? | ||
.getattr(pyo3::intern!(py, "UnrecognizedExtension"))? | ||
|
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.
I'm not super happy with this signature -- we end up pushing an
Option
through as a no-op to avoid having to destructure the optional extensions in multiple other places, but that results in more invalid states than are strictly needed here (and the smellyResult<Option<...>>
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.
I think a reasonable follow up is, instead of
as_raw()
, we makeExtensions
into an iterator, and just handle theOption<>
internally. But let's do that in a seperate PR, this one is already complicated enough.