der: add SetOf(Vec)::insert(_ordered); deprecate add (#1067)

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`.

Author: tarcieri

.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:

.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

cms/src/content_info.rs

@@ -61,7 +61,7 @@ impl TryFrom<Certificate> for ContentInfo {
 
     fn try_from(cert: Certificate) -> der::Result<Self> {
         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<PkiPath> for ContentInfo {
     fn try_from(pki_path: PkiPath) -> der::Result<Self> {
         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

der/src/arrayvec.rs

@@ -23,14 +23,11 @@ impl<T, const N: usize> ArrayVec<T, N> {
         }
     }
 
-    /// 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::<u8, 3>::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);
     }
 }

der/src/asn1/sequence_of.rs

@@ -30,7 +30,7 @@ impl<T, const N: usize> SequenceOf<T, N> {
 
     /// 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`].

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<T: DerOrd>(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.
 ///

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)
     };
 

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" }

x509-cert/src/builder.rs

@@ -501,7 +501,7 @@ where
     fn finalize(&mut self) -> der::Result<vec::Vec<u8>> {
         self.info
             .attributes
-            .add(self.extension_req.clone().try_into()?)?;
+            .insert(self.extension_req.clone().try_into()?)?;
 
         self.info.to_der()
     }

x509-cert/src/request.rs

@@ -117,7 +117,7 @@ impl TryFrom<ExtensionReq> for Attribute {
 
     fn try_from(extension_req: ExtensionReq) -> der::Result<Attribute> {
         let mut values: SetOfVec<AttributeValue> = Default::default();
-        values.add(Any::encode_from(&extension_req.0)?)?;
+        values.insert(Any::encode_from(&extension_req.0)?)?;
 
         Ok(Attribute {
             oid: ExtensionReq::OID,

x509-cert/tests/name.rs

@@ -122,18 +122,18 @@ 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,
         &hex!("311F300A060355040A0C03313233301106035504030C0A4A4F484E20534D495448")
     );
 
     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(