From 84338b02e193faa5e5075ac412b43f34cc2d4eb3 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 26 Jul 2023 23:25:32 +0200 Subject: [PATCH 1/2] Use traits to create some DER processing abstractions --- src/cert.rs | 57 +++---- src/crl.rs | 340 +++++++++++++++++++------------------ src/der.rs | 31 ++++ src/signed_data.rs | 22 +-- src/subject_name/verify.rs | 9 +- src/x509.rs | 28 +-- 6 files changed, 244 insertions(+), 243 deletions(-) diff --git a/src/cert.rs b/src/cert.rs index f36e509d..dae388e8 100644 --- a/src/cert.rs +++ b/src/cert.rs @@ -13,7 +13,7 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. use crate::der::Tag; -use crate::der::{self, CONSTRUCTED, CONTEXT_SPECIFIC}; +use crate::der::{self, DerIterator, FromDer, CONSTRUCTED, CONTEXT_SPECIFIC}; use crate::signed_data::SignedData; use crate::x509::{remember_extension, set_extension_once, DistributionPointName, Extension}; use crate::Error; @@ -150,11 +150,10 @@ impl<'a> Cert<'a> { /// Returns an iterator over the certificate's cRLDistributionPoints extension values, if any. #[allow(dead_code)] // TODO(@cpu): remove once used in CRL validation. - pub(crate) fn crl_distribution_points(&self) -> Option { - self.crl_distribution_points - .map(|crl_distribution_points| CrlDistributionPoints { - reader: untrusted::Reader::new(crl_distribution_points), - }) + pub(crate) fn crl_distribution_points( + &self, + ) -> Option, Error>>> { + self.crl_distribution_points.map(DerIterator::new) } } @@ -236,25 +235,6 @@ fn remember_cert_extension<'a>( }) } -/// Iterator over a certificate's certificate revocation list (CRL) distribution -/// points as described in RFC 5280 section 4.2.3.13[^1]. -/// -/// The CRL distribution point extensions describes how CRL information can be obtained for -/// a given certificate. -/// -/// [^1]: -pub(crate) struct CrlDistributionPoints<'a> { - reader: untrusted::Reader<'a>, -} - -impl<'a> Iterator for CrlDistributionPoints<'a> { - type Item = Result, Error>; - - fn next(&mut self) -> Option { - (!self.reader.at_end()).then(|| CrlDistributionPoint::from_der(&mut self.reader)) - } -} - /// A certificate revocation list (CRL) distribution point, describing a source of /// CRL information for a given certificate as described in RFC 5280 section 4.2.3.13[^1]. /// @@ -273,7 +253,17 @@ pub(crate) struct CrlDistributionPoint<'a> { } impl<'a> CrlDistributionPoint<'a> { - fn from_der(der: &mut untrusted::Reader<'a>) -> Result { + /// Return the distribution point names (if any). + #[allow(dead_code)] // TODO(@cpu): remove this once used in CRL validation. + pub(crate) fn names(&self) -> Result>, Error> { + self.distribution_point + .map(|input| DistributionPointName::from_der(&mut untrusted::Reader::new(input))) + .transpose() + } +} + +impl<'a> FromDer<'a> for CrlDistributionPoint<'a> { + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result { // RFC 5280 section §4.2.1.13: // A DistributionPoint consists of three fields, each of which is optional: // distributionPoint, reasons, and cRLIssuer. @@ -283,7 +273,7 @@ impl<'a> CrlDistributionPoint<'a> { crl_issuer: None, }; - der::nested(der, Tag::Sequence, Error::BadDer, |der| { + der::nested(reader, Tag::Sequence, Error::BadDer, |der| { const DISTRIBUTION_POINT_TAG: u8 = CONTEXT_SPECIFIC | CONSTRUCTED; const REASONS_TAG: u8 = CONTEXT_SPECIFIC | 1; const CRL_ISSUER_TAG: u8 = CONTEXT_SPECIFIC | CONSTRUCTED | 2; @@ -311,14 +301,6 @@ impl<'a> CrlDistributionPoint<'a> { } }) } - - /// Return the distribution point names (if any). - #[allow(dead_code)] // TODO(@cpu): remove this once used in CRL validation. - pub(crate) fn names(&self) -> Result>, Error> { - self.distribution_point - .map(DistributionPointName::from_der) - .transpose() - } } #[cfg(test)] @@ -328,7 +310,6 @@ mod tests { use crate::{ cert::{CrlDistributionPoint, DistributionPointName}, subject_name::GeneralName, - x509::GeneralNames, Error, RevocationReason, }; @@ -612,7 +593,9 @@ mod tests { .expect("missing second distribution point"), ); - fn get_names<'a>(point: &'a CrlDistributionPoint<'a>) -> GeneralNames<'a> { + fn get_names<'a>( + point: &'a CrlDistributionPoint<'a>, + ) -> impl Iterator, Error>> { match point .names() .expect("failed to parse distribution point names") diff --git a/src/crl.rs b/src/crl.rs index fc23ef96..390ea146 100644 --- a/src/crl.rs +++ b/src/crl.rs @@ -13,7 +13,7 @@ // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. use crate::cert::lenient_certificate_serial_number; -use crate::der::{self, Tag, CONSTRUCTED, CONTEXT_SPECIFIC}; +use crate::der::{self, DerIterator, FromDer, Tag, CONSTRUCTED, CONTEXT_SPECIFIC}; use crate::signed_data::{self, SignedData}; use crate::x509::{remember_extension, set_extension_once, DistributionPointName, Extension}; use crate::{Error, SignatureAlgorithm, Time}; @@ -133,119 +133,8 @@ impl<'a> BorrowedCertRevocationList<'a> { /// /// [^1]: pub fn from_der(crl_der: &'a [u8]) -> Result { - // Try to parse the CRL. - let reader = untrusted::Input::from(crl_der); - let (tbs_cert_list, signed_data) = reader.read_all(Error::BadDer, |crl_der| { - der::nested_limited( - crl_der, - Tag::Sequence, - Error::BadDer, - |signed_der| SignedData::from_der(signed_der, der::MAX_DER_SIZE), - der::MAX_DER_SIZE, - ) - })?; - - let crl = tbs_cert_list.read_all(Error::BadDer, |tbs_cert_list| { - // RFC 5280 §5.1.2.1: - // This optional field describes the version of the encoded CRL. When - // extensions are used, as required by this profile, this field MUST be - // present and MUST specify version 2 (the integer value is 1). - // RFC 5280 §5.2: - // Conforming CRL issuers are REQUIRED to include the authority key - // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) - // extensions in all CRLs issued. - // As a result of the above we parse this as a required section, not OPTIONAL. - // NOTE: Encoded value of version 2 is 1. - if der::small_nonnegative_integer(tbs_cert_list)? != 1 { - return Err(Error::UnsupportedCrlVersion); - } - - // RFC 5280 §5.1.2.2: - // This field MUST contain the same algorithm identifier as the - // signatureAlgorithm field in the sequence CertificateList - let signature = der::expect_tag_and_get_value(tbs_cert_list, Tag::Sequence)?; - if signature != signed_data.algorithm { - return Err(Error::SignatureAlgorithmMismatch); - } - - // RFC 5280 §5.1.2.3: - // The issuer field MUST contain a non-empty X.500 distinguished name (DN). - let issuer = der::expect_tag_and_get_value(tbs_cert_list, Tag::Sequence)?; - - // RFC 5280 §5.1.2.4: - // This field indicates the issue date of this CRL. thisUpdate may be - // encoded as UTCTime or GeneralizedTime. - // We do not presently enforce the correct choice of UTCTime or GeneralizedTime based on - // whether the date is post 2050. - der::time_choice(tbs_cert_list)?; - - // While OPTIONAL in the ASN.1 module, RFC 5280 §5.1.2.5 says: - // Conforming CRL issuers MUST include the nextUpdate field in all CRLs. - // We do not presently enforce the correct choice of UTCTime or GeneralizedTime based on - // whether the date is post 2050. - der::time_choice(tbs_cert_list)?; - - // RFC 5280 §5.1.2.6: - // When there are no revoked certificates, the revoked certificates list - // MUST be absent - // TODO(@cpu): Do we care to support empty CRLs if we don't support delta CRLs? - let revoked_certs = if tbs_cert_list.peek(Tag::Sequence.into()) { - der::expect_tag_and_get_value_limited( - tbs_cert_list, - Tag::Sequence, - der::MAX_DER_SIZE, - )? - } else { - untrusted::Input::from(&[]) - }; - - let mut crl = BorrowedCertRevocationList { - signed_data, - issuer, - revoked_certs, - issuing_distribution_point: None, - }; - - // RFC 5280 §5.1.2.7: - // This field may only appear if the version is 2 (Section 5.1.2.1). If - // present, this field is a sequence of one or more CRL extensions. - // RFC 5280 §5.2: - // Conforming CRL issuers are REQUIRED to include the authority key - // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) - // extensions in all CRLs issued. - // As a result of the above we parse this as a required section, not OPTIONAL. - der::nested( - tbs_cert_list, - Tag::ContextSpecificConstructed0, - Error::MalformedExtensions, - |tagged| { - der::nested_of_mut( - tagged, - Tag::Sequence, - Tag::Sequence, - Error::BadDer, - |extension| { - // RFC 5280 §5.2: - // If a CRL contains a critical extension - // that the application cannot process, then the application MUST NOT - // use that CRL to determine the status of certificates. However, - // applications may ignore unrecognized non-critical extensions. - crl.remember_extension(&Extension::parse(extension)?) - }, - ) - }, - )?; - - Ok(crl) - })?; - - // If an issuing distribution point extension is present, parse it up-front to validate - // that it only uses well-formed and supported features. - if let Some(der) = crl.issuing_distribution_point { - IssuingDistributionPoint::from_der(der)?; - } - - Ok(crl) + let input = untrusted::Input::from(crl_der); + input.read_all(Error::BadDer, ::from_der) } /// Convert the CRL to an [`OwnedCertRevocationList`]. This may error if any of the revoked @@ -357,27 +246,136 @@ impl CertRevocationList for BorrowedCertRevocationList<'_> { } } -impl<'a> IntoIterator for &'a BorrowedCertRevocationList<'a> { - type Item = Result, Error>; - type IntoIter = RevokedCerts<'a>; +impl<'a> FromDer<'a> for BorrowedCertRevocationList<'a> { + /// Try to parse the given bytes as a RFC 5280[^1] profile Certificate Revocation List (CRL). + /// + /// Webpki does not support: + /// * CRL versions other than version 2. + /// * CRLs missing the next update field. + /// * CRLs missing certificate revocation list extensions. + /// * Delta CRLs. + /// * CRLs larger than (2^32)-1 bytes in size. + /// + /// [^1]: + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result { + let (tbs_cert_list, signed_data) = der::nested_limited( + reader, + Tag::Sequence, + Error::BadDer, + |signed_der| SignedData::from_der(signed_der, der::MAX_DER_SIZE), + der::MAX_DER_SIZE, + )?; - fn into_iter(self) -> Self::IntoIter { - RevokedCerts { - reader: untrusted::Reader::new(self.revoked_certs), + let crl = tbs_cert_list.read_all(Error::BadDer, |tbs_cert_list| { + // RFC 5280 §5.1.2.1: + // This optional field describes the version of the encoded CRL. When + // extensions are used, as required by this profile, this field MUST be + // present and MUST specify version 2 (the integer value is 1). + // RFC 5280 §5.2: + // Conforming CRL issuers are REQUIRED to include the authority key + // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) + // extensions in all CRLs issued. + // As a result of the above we parse this as a required section, not OPTIONAL. + // NOTE: Encoded value of version 2 is 1. + if der::small_nonnegative_integer(tbs_cert_list)? != 1 { + return Err(Error::UnsupportedCrlVersion); + } + + // RFC 5280 §5.1.2.2: + // This field MUST contain the same algorithm identifier as the + // signatureAlgorithm field in the sequence CertificateList + let signature = der::expect_tag_and_get_value(tbs_cert_list, Tag::Sequence)?; + if signature != signed_data.algorithm { + return Err(Error::SignatureAlgorithmMismatch); + } + + // RFC 5280 §5.1.2.3: + // The issuer field MUST contain a non-empty X.500 distinguished name (DN). + let issuer = der::expect_tag_and_get_value(tbs_cert_list, Tag::Sequence)?; + + // RFC 5280 §5.1.2.4: + // This field indicates the issue date of this CRL. thisUpdate may be + // encoded as UTCTime or GeneralizedTime. + // We do not presently enforce the correct choice of UTCTime or GeneralizedTime based on + // whether the date is post 2050. + der::time_choice(tbs_cert_list)?; + + // While OPTIONAL in the ASN.1 module, RFC 5280 §5.1.2.5 says: + // Conforming CRL issuers MUST include the nextUpdate field in all CRLs. + // We do not presently enforce the correct choice of UTCTime or GeneralizedTime based on + // whether the date is post 2050. + der::time_choice(tbs_cert_list)?; + + // RFC 5280 §5.1.2.6: + // When there are no revoked certificates, the revoked certificates list + // MUST be absent + // TODO(@cpu): Do we care to support empty CRLs if we don't support delta CRLs? + let revoked_certs = if tbs_cert_list.peek(Tag::Sequence.into()) { + der::expect_tag_and_get_value_limited( + tbs_cert_list, + Tag::Sequence, + der::MAX_DER_SIZE, + )? + } else { + untrusted::Input::from(&[]) + }; + + let mut crl = BorrowedCertRevocationList { + signed_data, + issuer, + revoked_certs, + issuing_distribution_point: None, + }; + + // RFC 5280 §5.1.2.7: + // This field may only appear if the version is 2 (Section 5.1.2.1). If + // present, this field is a sequence of one or more CRL extensions. + // RFC 5280 §5.2: + // Conforming CRL issuers are REQUIRED to include the authority key + // identifier (Section 5.2.1) and the CRL number (Section 5.2.3) + // extensions in all CRLs issued. + // As a result of the above we parse this as a required section, not OPTIONAL. + der::nested( + tbs_cert_list, + Tag::ContextSpecificConstructed0, + Error::MalformedExtensions, + |tagged| { + der::nested_of_mut( + tagged, + Tag::Sequence, + Tag::Sequence, + Error::BadDer, + |extension| { + // RFC 5280 §5.2: + // If a CRL contains a critical extension + // that the application cannot process, then the application MUST NOT + // use that CRL to determine the status of certificates. However, + // applications may ignore unrecognized non-critical extensions. + crl.remember_extension(&Extension::parse(extension)?) + }, + ) + }, + )?; + + Ok(crl) + })?; + + // If an issuing distribution point extension is present, parse it up-front to validate + // that it only uses well-formed and supported features. + if let Some(der) = crl.issuing_distribution_point { + IssuingDistributionPoint::from_der(der)?; } - } -} -#[derive(Debug)] -pub struct RevokedCerts<'a> { - reader: untrusted::Reader<'a>, + Ok(crl) + } } -impl<'a> Iterator for RevokedCerts<'a> { +impl<'a> IntoIterator for &'a BorrowedCertRevocationList<'a> { type Item = Result, Error>; + type IntoIter = DerIterator<'a, BorrowedRevokedCert<'a>>; - fn next(&mut self) -> Option { - (!self.reader.at_end()).then(|| BorrowedRevokedCert::from_der(&mut self.reader)) + fn into_iter(self) -> Self::IntoIter { + DerIterator::new(self.revoked_certs) } } @@ -458,8 +456,40 @@ impl<'a> BorrowedRevokedCert<'a> { } } - fn from_der(der: &mut untrusted::Reader<'a>) -> Result { - der::nested(der, Tag::Sequence, Error::BadDer, |der| { + fn remember_extension(&mut self, extension: &Extension<'a>) -> Result<(), Error> { + remember_extension(extension, |id| { + match id { + // id-ce-cRLReasons 2.5.29.21 - RFC 5280 §5.3.1. + 21 => set_extension_once(&mut self.reason_code, || { + extension + .value + .read_all(Error::BadDer, RevocationReason::from_der) + }), + + // id-ce-invalidityDate 2.5.29.24 - RFC 5280 §5.3.2. + 24 => set_extension_once(&mut self.invalidity_date, || { + extension.value.read_all(Error::BadDer, der::time_choice) + }), + + // id-ce-certificateIssuer 2.5.29.29 - RFC 5280 §5.3.3. + // This CRL entry extension identifies the certificate issuer associated + // with an entry in an indirect CRL, that is, a CRL that has the + // indirectCRL indicator set in its issuing distribution point + // extension. + // We choose not to support indirect CRLs and so turn this into a more specific + // error rather than simply letting it fail as an unsupported critical extension. + 29 => Err(Error::UnsupportedIndirectCrl), + + // Unsupported extension + _ => extension.unsupported(), + } + }) + } +} + +impl<'a> FromDer<'a> for BorrowedRevokedCert<'a> { + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result { + der::nested(reader, Tag::Sequence, Error::BadDer, |der| { // RFC 5280 §4.1.2.2: // Certificate users MUST be able to handle serialNumber values up to 20 octets. // Conforming CAs MUST NOT use serialNumber values longer than 20 octets. @@ -517,34 +547,6 @@ impl<'a> BorrowedRevokedCert<'a> { Ok(revoked_cert) }) } - - fn remember_extension(&mut self, extension: &Extension<'a>) -> Result<(), Error> { - remember_extension(extension, |id| { - match id { - // id-ce-cRLReasons 2.5.29.21 - RFC 5280 §5.3.1. - 21 => set_extension_once(&mut self.reason_code, || { - RevocationReason::from_der(extension.value) - }), - - // id-ce-invalidityDate 2.5.29.24 - RFC 5280 §5.3.2. - 24 => set_extension_once(&mut self.invalidity_date, || { - extension.value.read_all(Error::BadDer, der::time_choice) - }), - - // id-ce-certificateIssuer 2.5.29.29 - RFC 5280 §5.3.3. - // This CRL entry extension identifies the certificate issuer associated - // with an entry in an indirect CRL, that is, a CRL that has the - // indirectCRL indicator set in its issuing distribution point - // extension. - // We choose not to support indirect CRLs and so turn this into a more specific - // error rather than simply letting it fail as an unsupported critical extension. - 29 => Err(Error::UnsupportedIndirectCrl), - - // Unsupported extension - _ => extension.unsupported(), - } - }) - } } /// Identifies the reason a certificate was revoked. @@ -571,16 +573,6 @@ pub enum RevocationReason { } impl RevocationReason { - // RFC 5280 §5.3.1. - fn from_der(value: untrusted::Input<'_>) -> Result { - value.read_all(Error::BadDer, |enumerated_reason| { - let value = der::expect_tag(enumerated_reason, Tag::Enum)?; - Self::try_from(value.value().read_all(Error::BadDer, |reason| { - reason.read_byte().map_err(|_| Error::BadDer) - })?) - }) - } - /// Return an iterator over all possible [RevocationReason] variants. pub fn iter() -> impl Iterator { use RevocationReason::*; @@ -600,6 +592,16 @@ impl RevocationReason { } } +impl<'a> FromDer<'a> for RevocationReason { + // RFC 5280 §5.3.1. + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result { + let value = der::expect_tag(reader, Tag::Enum)?; + Self::try_from(value.value().read_all(Error::BadDer, |reason| { + reason.read_byte().map_err(|_| Error::BadDer) + })?) + } +} + impl TryFrom for RevocationReason { type Error = Error; @@ -733,7 +735,7 @@ impl<'a> IssuingDistributionPoint<'a> { #[allow(dead_code)] // TODO(@cpu): remove this once used in CRL validation. pub(crate) fn names(&self) -> Result>, Error> { self.distribution_point - .map(DistributionPointName::from_der) + .map(|input| DistributionPointName::from_der(&mut untrusted::Reader::new(input))) .transpose() } } diff --git a/src/der.rs b/src/der.rs index 54667f2d..d7c0b37c 100644 --- a/src/der.rs +++ b/src/der.rs @@ -12,9 +12,40 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +use core::marker::PhantomData; + use crate::{calendar, time, Error}; pub(crate) use ring::io::der::{CONSTRUCTED, CONTEXT_SPECIFIC}; +#[derive(Debug)] +pub struct DerIterator<'a, T> { + reader: untrusted::Reader<'a>, + marker: PhantomData, +} + +impl<'a, T> DerIterator<'a, T> { + /// [`DerIterator`] will consume all of the bytes in `input` reading values of type `T`. + pub(crate) fn new(input: untrusted::Input<'a>) -> Self { + Self { + reader: untrusted::Reader::new(input), + marker: PhantomData, + } + } +} + +impl<'a, T: FromDer<'a>> Iterator for DerIterator<'a, T> { + type Item = Result; + + fn next(&mut self) -> Option { + (!self.reader.at_end()).then(|| T::from_der(&mut self.reader)) + } +} + +pub(crate) trait FromDer<'a>: Sized + 'a { + /// Parse a value of type `Self` from the given DER-encoded input. + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result; +} + // Copied (and extended) from ring's src/der.rs #[allow(clippy::upper_case_acronyms)] #[derive(Clone, Copy, Eq, PartialEq)] diff --git a/src/signed_data.rs b/src/signed_data.rs index 0b9856eb..6893ca55 100644 --- a/src/signed_data.rs +++ b/src/signed_data.rs @@ -12,9 +12,11 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -use crate::{der, Error}; use ring::signature; +use crate::der::{self, FromDer}; +use crate::error::Error; + /// X.509 certificates and related items that are signed are almost always /// encoded in the format "tbs||signatureAlgorithm||signature". This structure /// captures this pattern as an owned data type. @@ -206,7 +208,7 @@ pub(crate) fn verify_signature( msg: untrusted::Input, signature: untrusted::Input, ) -> Result<(), Error> { - let spki = SubjectPublicKeyInfo::from_der(spki_value)?; + let spki = spki_value.read_all(Error::BadDer, SubjectPublicKeyInfo::from_der)?; if !signature_alg .public_key_alg_id .matches_algorithm_id_value(spki.algorithm_id_value) @@ -226,19 +228,17 @@ struct SubjectPublicKeyInfo<'a> { key_value: untrusted::Input<'a>, } -impl<'a> SubjectPublicKeyInfo<'a> { +impl<'a> FromDer<'a> for SubjectPublicKeyInfo<'a> { // Parse the public key into an algorithm OID, an optional curve OID, and the // key value. The caller needs to check whether these match the // `PublicKeyAlgorithm` for the `SignatureAlgorithm` that is matched when // parsing the signature. - fn from_der(input: untrusted::Input<'a>) -> Result { - input.read_all(Error::BadDer, |input| { - let algorithm_id_value = der::expect_tag_and_get_value(input, der::Tag::Sequence)?; - let key_value = der::bit_string_with_no_unused_bits(input)?; - Ok(SubjectPublicKeyInfo { - algorithm_id_value, - key_value, - }) + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result { + let algorithm_id_value = der::expect_tag_and_get_value(reader, der::Tag::Sequence)?; + let key_value = der::bit_string_with_no_unused_bits(reader)?; + Ok(SubjectPublicKeyInfo { + algorithm_id_value, + key_value, }) } } diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 062e518a..20a21a9c 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -17,9 +17,10 @@ use super::{ ip_address::{self, IpAddrRef}, name::SubjectNameRef, }; +use crate::der::{self, FromDer}; use crate::{ cert::{Cert, EndEntityOrCa}, - der, Error, + Error, }; #[cfg(feature = "alloc")] use { @@ -404,8 +405,8 @@ pub(crate) enum GeneralName<'a> { Unsupported(u8), } -impl<'a> GeneralName<'a> { - pub(crate) fn from_der(input: &mut untrusted::Reader<'a>) -> Result { +impl<'a> FromDer<'a> for GeneralName<'a> { + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result { use ring::io::der::{CONSTRUCTED, CONTEXT_SPECIFIC}; use GeneralName::*; @@ -420,7 +421,7 @@ impl<'a> GeneralName<'a> { const IP_ADDRESS_TAG: u8 = CONTEXT_SPECIFIC | 7; const REGISTERED_ID_TAG: u8 = CONTEXT_SPECIFIC | 8; - let (tag, value) = der::read_tag_and_get_value(input)?; + let (tag, value) = der::read_tag_and_get_value(reader)?; Ok(match tag { DNS_NAME_TAG => DnsName(value), DIRECTORY_NAME_TAG => DirectoryName(value), diff --git a/src/x509.rs b/src/x509.rs index 2bc65cdb..863f0ae0 100644 --- a/src/x509.rs +++ b/src/x509.rs @@ -12,7 +12,7 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -use crate::der::{self, CONSTRUCTED, CONTEXT_SPECIFIC}; +use crate::der::{self, DerIterator, FromDer, CONSTRUCTED, CONTEXT_SPECIFIC}; use crate::subject_name::GeneralName; use crate::Error; @@ -85,22 +85,20 @@ pub(crate) enum DistributionPointName<'a> { /// The distribution point name is a relative distinguished name, relative to the CRL issuer. NameRelativeToCrlIssuer(untrusted::Input<'a>), /// The distribution point name is a sequence of [GeneralNames]. - FullName(GeneralNames<'a>), + FullName(DerIterator<'a, GeneralName<'a>>), } -impl<'a> DistributionPointName<'a> { - pub(crate) fn from_der(der: untrusted::Input<'a>) -> Result, Error> { +impl<'a> FromDer<'a> for DistributionPointName<'a> { + fn from_der(reader: &mut untrusted::Reader<'a>) -> Result, Error> { // RFC 5280 section §4.2.1.13: // When the distributionPoint field is present, it contains either a // SEQUENCE of general names or a single value, nameRelativeToCRLIssuer const FULL_NAME_TAG: u8 = CONTEXT_SPECIFIC | CONSTRUCTED; const NAME_RELATIVE_TO_CRL_ISSUER_TAG: u8 = CONTEXT_SPECIFIC | CONSTRUCTED | 1; - let (tag, value) = der::read_tag_and_get_value(&mut untrusted::Reader::new(der))?; + let (tag, value) = der::read_tag_and_get_value(reader)?; match tag { - FULL_NAME_TAG => Ok(DistributionPointName::FullName(GeneralNames { - reader: untrusted::Reader::new(value), - })), + FULL_NAME_TAG => Ok(DistributionPointName::FullName(DerIterator::new(value))), NAME_RELATIVE_TO_CRL_ISSUER_TAG => { Ok(DistributionPointName::NameRelativeToCrlIssuer(value)) } @@ -108,17 +106,3 @@ impl<'a> DistributionPointName<'a> { } } } - -/// An iterator over a series of X.509 [GeneralName] instances describing locations that can be used -/// to fetch a certificate revocation list for a certificate. -pub(crate) struct GeneralNames<'a> { - reader: untrusted::Reader<'a>, -} - -impl<'a> Iterator for GeneralNames<'a> { - type Item = Result, Error>; - - fn next(&mut self) -> Option { - (!self.reader.at_end()).then(|| GeneralName::from_der(&mut self.reader)) - } -} From 41c6d31fb1702b6c012ad42b451f28e8f14b1125 Mon Sep 17 00:00:00 2001 From: Dirkjan Ochtman Date: Wed, 26 Jul 2023 23:28:54 +0200 Subject: [PATCH 2/2] Improve consistency in import styles --- src/subject_name/verify.rs | 24 ++++++++++-------------- src/verify_cert.rs | 2 +- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/src/subject_name/verify.rs b/src/subject_name/verify.rs index 20a21a9c..0b463804 100644 --- a/src/subject_name/verify.rs +++ b/src/subject_name/verify.rs @@ -12,21 +12,17 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. -use super::{ - dns_name::{self, DnsNameRef}, - ip_address::{self, IpAddrRef}, - name::SubjectNameRef, -}; -use crate::der::{self, FromDer}; -use crate::{ - cert::{Cert, EndEntityOrCa}, - Error, -}; #[cfg(feature = "alloc")] -use { - alloc::vec::Vec, - dns_name::{GeneralDnsNameRef, WildcardDnsNameRef}, -}; +use alloc::vec::Vec; + +use super::dns_name::{self, DnsNameRef}; +#[cfg(feature = "alloc")] +use super::dns_name::{GeneralDnsNameRef, WildcardDnsNameRef}; +use super::ip_address::{self, IpAddrRef}; +use super::name::SubjectNameRef; +use crate::cert::{Cert, EndEntityOrCa}; +use crate::der::{self, FromDer}; +use crate::error::Error; pub(crate) fn verify_cert_dns_name( cert: &crate::EndEntityCert, diff --git a/src/verify_cert.rs b/src/verify_cert.rs index b4c0dff2..9c6956f6 100644 --- a/src/verify_cert.rs +++ b/src/verify_cert.rs @@ -12,8 +12,8 @@ // ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF // OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. +use crate::cert::{Cert, EndEntityOrCa}; use crate::{ - cert::{Cert, EndEntityOrCa}, der, signed_data, subject_name, time, CertRevocationList, Error, SignatureAlgorithm, TrustAnchor, };