From 696ffee9c108ba7ec915405868123a8a3ffde6f0 Mon Sep 17 00:00:00 2001 From: Tony Arcieri Date: Mon, 15 May 2023 11:58:08 -0600 Subject: [PATCH] der: add `SetOf(Vec)::insert(_ordered)`; deprecate `add` Renames the insertion methods on `SetOf` and `SetOfVec` from `add` to `insert_ordered`, deprecating the previous `add` method. Also adds an `insert` method which pushes onto the underlying `(Array)Vec` then calls `der_sort`. This should be relatively fast since the underlying algorithm is insertion sort and operating on data which is mostly in-order. The name `insert` is more consistent with `BTreeSet`/`HashSet`. --- .github/workflows/pkcs7.yml | 5 --- .github/workflows/x509-cert.yml | 11 ++--- cms/src/content_info.rs | 4 +- der/src/arrayvec.rs | 17 ++++---- der/src/asn1/sequence_of.rs | 2 +- der/src/asn1/set_of.rs | 73 ++++++++++++++++++++++++--------- pkcs7/tests/content_tests.rs | 4 +- x509-cert/fuzz/Cargo.toml | 8 +++- x509-cert/src/builder.rs | 2 +- x509-cert/src/request.rs | 2 +- x509-cert/tests/name.rs | 8 ++-- 11 files changed, 84 insertions(+), 52 deletions(-) diff --git a/.github/workflows/pkcs7.yml b/.github/workflows/pkcs7.yml index ab6f73711..dd5805ada 100644 --- a/.github/workflows/pkcs7.yml +++ b/.github/workflows/pkcs7.yml @@ -40,11 +40,6 @@ jobs: - uses: RustCrypto/actions/cargo-hack-install@master - run: cargo hack build --target ${{ matrix.target }} --feature-powerset - minimal-versions: - uses: RustCrypto/actions/.github/workflows/minimal-versions.yml@master - with: - working-directory: ${{ github.workflow }} - test: runs-on: ubuntu-latest strategy: diff --git a/.github/workflows/x509-cert.yml b/.github/workflows/x509-cert.yml index 0c52777c7..41a5d8eb0 100644 --- a/.github/workflows/x509-cert.yml +++ b/.github/workflows/x509-cert.yml @@ -40,11 +40,12 @@ jobs: - uses: RustCrypto/actions/cargo-hack-install@master - run: cargo hack build --target ${{ matrix.target }} --feature-powerset --exclude-features arbitrary,builder,default,std - minimal-versions: - uses: RustCrypto/actions/.github/workflows/minimal-versions.yml@master - with: - working-directory: ${{ github.workflow }} - install-zlint: true +# TODO(tarcieri): re-enable after next `der` release +# minimal-versions: +# uses: RustCrypto/actions/.github/workflows/minimal-versions.yml@master +# with: +# working-directory: ${{ github.workflow }} +# install-zlint: true test: runs-on: ubuntu-latest diff --git a/cms/src/content_info.rs b/cms/src/content_info.rs index 1d6963649..fca6482db 100644 --- a/cms/src/content_info.rs +++ b/cms/src/content_info.rs @@ -61,7 +61,7 @@ impl TryFrom for ContentInfo { fn try_from(cert: Certificate) -> der::Result { let mut certs = CertificateSet(Default::default()); - certs.0.add(CertificateChoices::Certificate(cert))?; + certs.0.insert(CertificateChoices::Certificate(cert))?; // include empty CRLs field instead of omitting it to match OpenSSL's behavior let sd = SignedData { @@ -93,7 +93,7 @@ impl TryFrom for ContentInfo { fn try_from(pki_path: PkiPath) -> der::Result { let mut certs = CertificateSet(Default::default()); for cert in pki_path { - certs.0.add(CertificateChoices::Certificate(cert))?; + certs.0.insert(CertificateChoices::Certificate(cert))?; } // include empty CRLs field instead of omitting it to match OpenSSL's behavior diff --git a/der/src/arrayvec.rs b/der/src/arrayvec.rs index 21f134196..6ce608d97 100644 --- a/der/src/arrayvec.rs +++ b/der/src/arrayvec.rs @@ -23,14 +23,11 @@ impl ArrayVec { } } - /// Add an element to this [`ArrayVec`]. - /// - /// Items MUST be added in lexicographical order according to the `Ord` - /// impl on `T`. - pub fn add(&mut self, element: T) -> Result<()> { + /// Push an item into this [`ArrayVec`]. + pub fn push(&mut self, item: T) -> Result<()> { match self.length.checked_add(1) { Some(n) if n <= N => { - self.elements[self.length] = Some(element); + self.elements[self.length] = Some(item); self.length = n; Ok(()) } @@ -138,11 +135,11 @@ mod tests { #[test] fn add() { let mut vec = ArrayVec::::new(); - vec.add(1).unwrap(); - vec.add(2).unwrap(); - vec.add(3).unwrap(); + vec.push(1).unwrap(); + vec.push(2).unwrap(); + vec.push(3).unwrap(); - assert_eq!(vec.add(4).err().unwrap(), ErrorKind::Overlength.into()); + assert_eq!(vec.push(4).err().unwrap(), ErrorKind::Overlength.into()); assert_eq!(vec.len(), 3); } } diff --git a/der/src/asn1/sequence_of.rs b/der/src/asn1/sequence_of.rs index 42c2a9ca8..befb0298f 100644 --- a/der/src/asn1/sequence_of.rs +++ b/der/src/asn1/sequence_of.rs @@ -30,7 +30,7 @@ impl SequenceOf { /// Add an element to this [`SequenceOf`]. pub fn add(&mut self, element: T) -> Result<()> { - self.inner.add(element) + self.inner.push(element) } /// Get an element of this [`SequenceOf`]. diff --git a/der/src/asn1/set_of.rs b/der/src/asn1/set_of.rs index fcfaeb8c1..ff0131242 100644 --- a/der/src/asn1/set_of.rs +++ b/der/src/asn1/set_of.rs @@ -44,21 +44,32 @@ where } } - /// Add an element to this [`SetOf`]. + /// Add an item to this [`SetOf`]. /// /// Items MUST be added in lexicographical order according to the /// [`DerOrd`] impl on `T`. + #[deprecated(since = "0.7.6", note = "use `insert` or `insert_ordered` instead")] pub fn add(&mut self, new_elem: T) -> Result<()> { + self.insert_ordered(new_elem) + } + + /// Insert an item into this [`SetOf`]. + pub fn insert(&mut self, item: T) -> Result<()> { + self.inner.push(item)?; + der_sort(self.inner.as_mut()) + } + + /// Insert an item into this [`SetOf`]. + /// + /// Items MUST be added in lexicographical order according to the + /// [`DerOrd`] impl on `T`. + pub fn insert_ordered(&mut self, item: T) -> Result<()> { // Ensure set elements are lexicographically ordered - if let Some(last_elem) = self.inner.last() { - match new_elem.der_cmp(last_elem)? { - Ordering::Less => return Err(ErrorKind::SetOrdering.into()), - Ordering::Equal => return Err(ErrorKind::SetDuplicate.into()), - Ordering::Greater => (), - } + if let Some(last) = self.inner.last() { + check_der_ordering(last, &item)?; } - self.inner.add(new_elem) + self.inner.push(item) } /// Get the nth element from this [`SetOf`]. @@ -102,7 +113,7 @@ where let mut result = Self::new(); while !reader.is_finished() { - result.inner.add(T::decode(reader)?)?; + result.inner.push(T::decode(reader)?)?; } der_sort(result.inner.as_mut())?; @@ -149,7 +160,7 @@ where let mut result = SetOf::new(); for elem in arr { - result.add(elem)?; + result.insert_ordered(elem)?; } Ok(result) @@ -232,16 +243,9 @@ where /// /// Items MUST be added in lexicographical order according to the /// [`DerOrd`] impl on `T`. - pub fn add(&mut self, new_elem: T) -> Result<()> { - // Ensure set elements are lexicographically ordered - if let Some(last_elem) = self.inner.last() { - if new_elem.der_cmp(last_elem)? != Ordering::Greater { - return Err(ErrorKind::SetOrdering.into()); - } - } - - self.inner.push(new_elem); - Ok(()) + #[deprecated(since = "0.7.6", note = "use `insert` or `insert_ordered` instead")] + pub fn add(&mut self, item: T) -> Result<()> { + self.insert_ordered(item) } /// Extend a [`SetOfVec`] using an iterator. @@ -256,6 +260,26 @@ where der_sort(&mut self.inner) } + /// Insert an item into this [`SetOfVec`]. Must be unique. + pub fn insert(&mut self, item: T) -> Result<()> { + self.inner.push(item); + der_sort(&mut self.inner) + } + + /// Insert an item into this [`SetOfVec`]. Must be unique. + /// + /// Items MUST be added in lexicographical order according to the + /// [`DerOrd`] impl on `T`. + pub fn insert_ordered(&mut self, item: T) -> Result<()> { + // Ensure set elements are lexicographically ordered + if let Some(last) = self.inner.last() { + check_der_ordering(last, &item)?; + } + + self.inner.push(item); + Ok(()) + } + /// Borrow the elements of this [`SetOfVec`] as a slice. pub fn as_slice(&self) -> &[T] { self.inner.as_slice() @@ -409,6 +433,15 @@ where } } +/// Ensure set elements are lexicographically ordered using [`DerOrd`]. +fn check_der_ordering(a: &T, b: &T) -> Result<()> { + match a.der_cmp(b)? { + Ordering::Less => Ok(()), + Ordering::Equal => Err(ErrorKind::SetDuplicate.into()), + Ordering::Greater => Err(ErrorKind::SetOrdering.into()), + } +} + /// Sort a mut slice according to its [`DerOrd`], returning any errors which /// might occur during the comparison. /// diff --git a/pkcs7/tests/content_tests.rs b/pkcs7/tests/content_tests.rs index 2e1483d9c..8c99f18a6 100644 --- a/pkcs7/tests/content_tests.rs +++ b/pkcs7/tests/content_tests.rs @@ -207,7 +207,7 @@ fn create_pkcs7_signed_data() { parameters: None, }; let mut digest_algorithms = DigestAlgorithmIdentifiers::new(); - digest_algorithms.add(digest_algorithm).unwrap(); + digest_algorithms.insert(digest_algorithm).unwrap(); digest_algorithms }; @@ -223,7 +223,7 @@ fn create_pkcs7_signed_data() { let cert: x509_cert::Certificate = x509_cert::Certificate::from_pem(cert_pem).unwrap(); let cert_choice = CertificateChoices::Certificate(cert); let mut certs = CertificateSet::new(); - certs.add(cert_choice).unwrap(); + certs.insert(cert_choice).unwrap(); Some(certs) }; diff --git a/x509-cert/fuzz/Cargo.toml b/x509-cert/fuzz/Cargo.toml index ca9ea6601..2547850dd 100644 --- a/x509-cert/fuzz/Cargo.toml +++ b/x509-cert/fuzz/Cargo.toml @@ -10,8 +10,14 @@ cargo-fuzz = true [dependencies] libfuzzer-sys = "0.4" -x509-cert = { path = ".." } +x509-cert = "*" # Prevents this crate from interfering with the workspace [workspace] members = ["."] + +[patch.crates-io] +der = { path = "../../der" } +der_derive = { path = "../../der/derive" } +pem-rfc7468 = { path = "../../pem-rfc7468" } +spki = { path = "../../spki" } diff --git a/x509-cert/src/builder.rs b/x509-cert/src/builder.rs index bc3a9c526..09d45de78 100644 --- a/x509-cert/src/builder.rs +++ b/x509-cert/src/builder.rs @@ -501,7 +501,7 @@ where fn finalize(&mut self) -> der::Result> { self.info .attributes - .add(self.extension_req.clone().try_into()?)?; + .insert(self.extension_req.clone().try_into()?)?; self.info.to_der() } diff --git a/x509-cert/src/request.rs b/x509-cert/src/request.rs index 6878de728..506a995f2 100644 --- a/x509-cert/src/request.rs +++ b/x509-cert/src/request.rs @@ -117,7 +117,7 @@ impl TryFrom for Attribute { fn try_from(extension_req: ExtensionReq) -> der::Result { let mut values: SetOfVec = Default::default(); - values.add(Any::encode_from(&extension_req.0)?)?; + values.insert(Any::encode_from(&extension_req.0)?)?; Ok(Attribute { oid: ExtensionReq::OID, diff --git a/x509-cert/tests/name.rs b/x509-cert/tests/name.rs index 1d080627f..7201e4958 100644 --- a/x509-cert/tests/name.rs +++ b/x509-cert/tests/name.rs @@ -122,8 +122,8 @@ fn decode_rdn() { assert_eq!(utf8a.to_string(), "JOHN SMITH"); let mut from_scratch = RelativeDistinguishedName::default(); - assert!(from_scratch.0.add(atav1a.clone()).is_ok()); - assert!(from_scratch.0.add(atav2a.clone()).is_ok()); + assert!(from_scratch.0.insert(atav1a.clone()).is_ok()); + assert!(from_scratch.0.insert(atav2a.clone()).is_ok()); let reencoded = from_scratch.to_der().unwrap(); assert_eq!( reencoded, @@ -131,9 +131,9 @@ fn decode_rdn() { ); let mut from_scratch2 = RelativeDistinguishedName::default(); - assert!(from_scratch2.0.add(atav2a.clone()).is_ok()); + assert!(from_scratch2.0.insert_ordered(atav2a.clone()).is_ok()); // fails when caller adds items not in DER lexicographical order - assert!(from_scratch2.0.add(atav1a.clone()).is_err()); + assert!(from_scratch2.0.insert_ordered(atav1a.clone()).is_err()); // allow out-of-order RDNs (see: RustCrypto/formats#625) assert!(RelativeDistinguishedName::from_der(