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

certificate: add a get_extension helper #8892

Merged
merged 29 commits into from
May 10, 2023
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
351af50
certificate: add a `get_extension` helper
woodruffw May 8, 2023
302eb18
certificate: OID by ref
woodruffw May 8, 2023
3cd1a7d
certificate: syntax
woodruffw May 8, 2023
c37e66f
x509, src: `check_duplicate_extensions`
woodruffw May 8, 2023
326634c
src: simplify
woodruffw May 8, 2023
a900f6b
src: everyone loves newtypes
woodruffw May 8, 2023
4c59f62
rust: refactor-o-rama
woodruffw May 8, 2023
b393f1b
src: look upon my works
woodruffw May 8, 2023
07e7cee
src: continue blasting the code
woodruffw May 8, 2023
897161e
src/rust: actually commit my changes
woodruffw May 8, 2023
22b41c5
src: clippage
woodruffw May 8, 2023
0ce6afb
relocate
woodruffw May 8, 2023
b2a9e07
src: dedupe
woodruffw May 8, 2023
2811cc8
src: cleanup
woodruffw May 8, 2023
e293f9c
clippage
woodruffw May 8, 2023
0893194
src: dedupe
woodruffw May 8, 2023
2dbb3eb
common: cleanup
woodruffw May 8, 2023
aa924dd
src: unused impls
woodruffw May 9, 2023
5189ca7
more deletion
woodruffw May 9, 2023
e9d8e9f
clippage
woodruffw May 9, 2023
8ec095e
extensions: add a `get_extension` test
woodruffw May 9, 2023
e8ef3b7
extensions: unused derives
woodruffw May 9, 2023
ea5da30
tests/x509: dup ext check for tbs_precertificate_bytes
woodruffw May 9, 2023
b9c14ac
Merge remote-tracking branch 'upstream/main' into tob-x509-get-extension
woodruffw May 9, 2023
f6f0367
certificate: remove `extensions()`
woodruffw May 9, 2023
fb585cf
extensions: docs
woodruffw May 9, 2023
cdd65d4
extensions: newtype
woodruffw May 9, 2023
212b75f
rust: better error types, dedupe
woodruffw May 10, 2023
e2bb769
Revert "rust: better error types, dedupe"
woodruffw May 10, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/rust/cryptography-x509/src/certificate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use crate::common;
use crate::extensions;
use crate::extensions::Extensions;
use crate::name;

#[derive(asn1::Asn1Read, asn1::Asn1Write, Hash, PartialEq, Clone)]
Expand Down Expand Up @@ -31,7 +32,13 @@ pub struct TbsCertificate<'a> {
#[implicit(2)]
pub subject_unique_id: Option<asn1::BitString<'a>>,
#[explicit(3)]
pub extensions: Option<extensions::Extensions<'a>>,
pub raw_extensions: Option<extensions::RawExtensions<'a>>,
}

impl<'a> TbsCertificate<'a> {
pub fn extensions(&'a self) -> Result<Option<Extensions<'a>>, asn1::ObjectIdentifier> {
Extensions::from_raw_extensions(self.raw_extensions.as_ref())
}
}

#[derive(asn1::Asn1Read, asn1::Asn1Write, Hash, PartialEq, Clone)]
Expand Down
10 changes: 7 additions & 3 deletions src/rust/cryptography-x509/src/crl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@
// 2.0, and the BSD License. See the LICENSE file in the root of this repository
// for complete details.

use crate::{common, extensions, name};
use crate::{
common,
extensions::{self},
name,
};

pub type ReasonFlags<'a> =
Option<common::Asn1ReadableOrWritable<'a, asn1::BitString<'a>, asn1::OwnedBitString>>;
Expand Down Expand Up @@ -31,14 +35,14 @@ pub struct TBSCertList<'a> {
pub next_update: Option<common::Time>,
pub revoked_certificates: RevokedCertificates<'a>,
#[explicit(0)]
pub crl_extensions: Option<extensions::Extensions<'a>>,
pub raw_crl_extensions: Option<extensions::RawExtensions<'a>>,
}

#[derive(asn1::Asn1Read, asn1::Asn1Write, PartialEq, Hash, Clone)]
pub struct RevokedCertificate<'a> {
pub user_certificate: asn1::BigUint<'a>,
pub revocation_date: common::Time,
pub crl_entry_extensions: Option<extensions::Extensions<'a>>,
pub raw_crl_entry_extensions: Option<extensions::RawExtensions<'a>>,
}

#[derive(asn1::Asn1Read, asn1::Asn1Write)]
Expand Down
2 changes: 1 addition & 1 deletion src/rust/cryptography-x509/src/csr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub struct CertificationRequestInfo<'a> {
impl CertificationRequestInfo<'_> {
pub fn get_extension_attribute(
&self,
) -> Result<Option<extensions::Extensions<'_>>, asn1::ParseError> {
) -> Result<Option<extensions::RawExtensions<'_>>, asn1::ParseError> {
for attribute in self.attributes.unwrap_read().clone() {
if attribute.type_id == oid::EXTENSION_REQUEST
|| attribute.type_id == oid::MS_EXTENSION_REQUEST
Expand Down
79 changes: 78 additions & 1 deletion src/rust/cryptography-x509/src/extensions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,58 @@
// 2.0, and the BSD License. See the LICENSE file in the root of this repository
// for complete details.

use std::collections::HashSet;

use crate::common;
use crate::crl;
use crate::name;

pub type Extensions<'a> = common::Asn1ReadableOrWritable<
pub type RawExtensions<'a> = common::Asn1ReadableOrWritable<
'a,
asn1::SequenceOf<'a, Extension<'a>>,
asn1::SequenceOfWriter<'a, Extension<'a>, Vec<Extension<'a>>>,
>;

pub struct Extensions<'a> {
inner: RawExtensions<'a>,
}

impl<'a> Extensions<'a> {
pub fn from_raw_extensions(
raw: Option<&RawExtensions<'a>>,
) -> Result<Option<Self>, asn1::ObjectIdentifier> {
Comment on lines +29 to +30
Copy link
Contributor Author

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 smelly Result<Option<...>> type).

Copy link
Member

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 make Extensions into an iterator, and just handle the Option<> internally. But let's do that in a seperate PR, this one is already complicated enough.

match raw {
Some(raw_exts) => {
let mut seen_oids = HashSet::new();

for ext in raw_exts.unwrap_read().clone() {
if !seen_oids.insert(ext.extn_id.clone()) {
return Err(ext.extn_id);
}
}

Ok(Some(Self {
inner: raw_exts.clone(),
}))
}
None => Ok(None),
}
}

/// Retrieves the extension identified by the given OID,
/// or None if the extension is not present (or no extensions are present).
pub fn get_extension(&self, oid: &asn1::ObjectIdentifier) -> Option<Extension> {
let mut extensions = self.inner.unwrap_read().clone();

extensions.find(|ext| &ext.extn_id == oid)
}

/// Returns a reference to the underlying extensions.
pub fn as_raw(&self) -> &RawExtensions<'_> {
&self.inner
}
}

#[derive(asn1::Asn1Read, asn1::Asn1Write, PartialEq, Eq, Hash, Clone)]
pub struct Extension<'a> {
pub extn_id: asn1::ObjectIdentifier,
Expand Down Expand Up @@ -174,3 +216,38 @@ pub struct BasicConstraints {
pub ca: bool,
pub path_length: Option<u64>,
}

#[cfg(test)]
mod tests {
use asn1::SequenceOfWriter;

use crate::oid::{AUTHORITY_KEY_IDENTIFIER_OID, BASIC_CONSTRAINTS_OID};

use super::{BasicConstraints, Extension, Extensions};

#[test]
fn test_get_extension() {
let extension_value = BasicConstraints {
ca: true,
path_length: Some(3),
};
let extension = Extension {
extn_id: BASIC_CONSTRAINTS_OID,
critical: true,
extn_value: &asn1::write_single(&extension_value).unwrap(),
};
let extensions = SequenceOfWriter::new(vec![extension]);

let der = asn1::write_single(&extensions).unwrap();

let extensions: Extensions =
Extensions::from_raw_extensions(Some(&asn1::parse_single(&der).unwrap()))
.unwrap()
.unwrap();

assert!(&extensions.get_extension(&BASIC_CONSTRAINTS_OID).is_some());
assert!(&extensions
.get_extension(&AUTHORITY_KEY_IDENTIFIER_OID)
.is_none());
}
}
10 changes: 7 additions & 3 deletions src/rust/cryptography-x509/src/ocsp_req.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@
// 2.0, and the BSD License. See the LICENSE file in the root of this repository
// for complete details.

use crate::{common, extensions, name};
use crate::{
common,
extensions::{self},
name,
};

#[derive(asn1::Asn1Read, asn1::Asn1Write)]
pub struct TBSRequest<'a> {
Expand All @@ -17,14 +21,14 @@ pub struct TBSRequest<'a> {
asn1::SequenceOfWriter<'a, Request<'a>>,
>,
#[explicit(2)]
pub request_extensions: Option<extensions::Extensions<'a>>,
pub raw_request_extensions: Option<extensions::RawExtensions<'a>>,
}

#[derive(asn1::Asn1Read, asn1::Asn1Write)]
pub struct Request<'a> {
pub req_cert: CertID<'a>,
#[explicit(0)]
pub single_request_extensions: Option<extensions::Extensions<'a>>,
pub single_request_extensions: Option<extensions::RawExtensions<'a>>,
}

#[derive(asn1::Asn1Read, asn1::Asn1Write)]
Expand Down
10 changes: 7 additions & 3 deletions src/rust/cryptography-x509/src/ocsp_resp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@
// 2.0, and the BSD License. See the LICENSE file in the root of this repository
// for complete details.

use crate::{certificate, common, crl, extensions, name, ocsp_req};
use crate::{
certificate, common, crl,
extensions::{self},
name, ocsp_req,
};

#[derive(asn1::Asn1Read, asn1::Asn1Write)]
pub struct OCSPResponse<'a> {
Expand Down Expand Up @@ -47,7 +51,7 @@ pub struct ResponseData<'a> {
asn1::SequenceOfWriter<'a, SingleResponse<'a>, Vec<SingleResponse<'a>>>,
>,
#[explicit(1)]
pub response_extensions: Option<extensions::Extensions<'a>>,
pub raw_response_extensions: Option<extensions::RawExtensions<'a>>,
}

#[derive(asn1::Asn1Read, asn1::Asn1Write)]
Expand All @@ -66,7 +70,7 @@ pub struct SingleResponse<'a> {
#[explicit(0)]
pub next_update: Option<asn1::GeneralizedTime>,
#[explicit(1)]
pub single_extensions: Option<extensions::Extensions<'a>>,
pub raw_single_extensions: Option<extensions::RawExtensions<'a>>,
}

#[derive(asn1::Asn1Read, asn1::Asn1Write)]
Expand Down
28 changes: 18 additions & 10 deletions src/rust/src/x509/certificate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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)
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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 extensions return a more-specific error type and then we can do a From impl on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good idea -- I'll do that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 + From implementation.

}
}

Expand Down Expand Up @@ -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)?;
Expand Down Expand Up @@ -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,
Expand Down
31 changes: 16 additions & 15 deletions src/rust/src/x509/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<'_>>,
Copy link
Member

Choose a reason for hiding this comment

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

I think we actually want this method to take an Option<Extensions> and then the callers can obtain the extensions, via an extensions() method on the various cryptography-x509 types. That way those types have the right methods that can be used from the validator crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Iterator change mentioned in #8892 (comment): without that change, we'd need to perform the fallible conversion from RawExtensions -> Extensions before parse_and_cache_extensions, defeating the point of having it inside of that function. Or did I miss something?

Copy link
Member

Choose a reason for hiding this comment

The 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(
Expand All @@ -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
Expand All @@ -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"))?
Expand Down
Loading