chore: apply suggestions from code review (#13)

Co-authored-by: Falko Galperin <[email protected]>

src/common/test_helper.rs

@@ -30,8 +30,6 @@ use {
     alloc::string::{String, ToString},
     alloc::vec::Vec,
 };
-//use crate::token::MultipleEncryptCipher;
-//use crate::{CoseEncryptCipher, CoseMacCipher, CoseSignCipher};
 
 /// Helper function for tests which ensures that [`value`] serializes to the hexadecimal bytestring
 /// [expected_hex] and deserializes back to [`value`].

src/error/mod.rs

@@ -266,12 +266,17 @@ where
     /// headers or the key itself).
     NoAlgorithmDeterminable,
     /// The provided key does not support the given operation.
+    /// The first field specifies the operations provided by the key, the second one
+    /// specifies the set of operations that would fulfill the requirements of the 
+    /// operation (only one of which must be provided by the key).
     KeyOperationNotPermitted(BTreeSet<KeyOperation>, BTreeSet<KeyOperation>),
     /// Key in given curve must be in different format.
     KeyTypeCurveMismatch(KeyType, EllipticCurve),
     /// Provided algorithm requires a different key type.
     KeyTypeAlgorithmMismatch(KeyType, Algorithm),
     /// Algorithm provided in key does not match algorithm selected for operation.
+    /// The first field specifies the algorithm provided by the key, the second one 
+    /// specifies the algorithm provided in the headers.
     KeyAlgorithmMismatch(Algorithm, Algorithm),
     /// At least one header was provided both in the protected and the unprotected bucket
     /// simultaneously.
@@ -299,7 +304,7 @@ where
     /// corresponding error.
     NoMatchingKeyFound(Vec<(CoseKey, CoseCipherError<T>)>),
     /// For structures that are validated using a CEK encoded in [CoseRecipient] structures
-    /// ([CoseEncrypt](coset::CoseEncrypt), [CoseMac](coset::CoseMac): Unable to find a suitable
+    /// ([CoseEncrypt](coset::CoseEncrypt), [CoseMac](coset::CoseMac)): Unable to find a suitable
     /// recipient to decrypt.
     ///
     /// The first element provides the errors that occurred while decrypting each recipient, the

src/token/cose/aad.rs

@@ -5,7 +5,7 @@ use coset::{EncryptionContext, Header};
 #[cfg(not(feature = "std"))]
 use alloc::vec::Vec;
 
-/// A trait for types that can determine the corresponding Additional Authenticated Data to be
+/// A trait for types that can determine the corresponding additional authenticated data to be
 /// provided for a given COSE structure.
 pub trait AadProvider: Sized {
     /// Look up the additional authenticated data for the given COSE structure.
@@ -25,7 +25,7 @@ pub trait AadProvider: Sized {
         unprotected: Option<&Header>,
     ) -> Option<&[u8]>;
 
-    /// Lookup up the additional authenticated data for nested COSE structures nested inside the
+    /// Look up the additional authenticated data for nested COSE structures nested inside the
     /// one whose decryption was actually requested.
     ///
     /// For the provided implementations, this will usually return an empty slice.
@@ -187,7 +187,7 @@ impl<T: AadProvider, U: AadProvider> AadProvider for (T, U) {
     }
 }
 
-/// Swap lookup_aad and lookup_nested_aad of an existing [`AadProvider`] .
+/// Swap lookup_aad and lookup_nested_aad of an existing [`AadProvider`].
 pub struct InvertedAadProvider<T: AadProvider>(pub T);
 
 impl<T: AadProvider> AadProvider for InvertedAadProvider<T> {

src/token/cose/crypto_impl/openssl.rs

@@ -215,7 +215,7 @@ fn get_ecdsa_group_params(
             Ok((48, EcGroup::from_curve_name(Nid::SECP384R1).unwrap()))
         }
         EllipticCurve::Assigned(iana::EllipticCurve::P_521) => {
-            // ECDSA using P-384 curve, coordinates are padded to 528 bits (521 bits rounded up
+            // ECDSA using P-521 curve, coordinates are padded to 528 bits (521 bits rounded up
             // to the nearest full bytes).
             Ok((66, EcGroup::from_curve_name(Nid::SECP521R1).unwrap()))
         }

src/token/cose/encrypted/encrypt/mod.rs

@@ -27,7 +27,7 @@ use crate::token::cose::recipient::{
 #[cfg(all(test, feature = "std"))]
 mod tests;
 
-/// Extensions to the [`CoseEncryptBuilder`]  type that enable usage of cryptographic backends.
+/// Extensions to the [`CoseEncryptBuilder`] type that enable usage of cryptographic backends.
 pub trait CoseEncryptBuilderExt: Sized {
     /// Attempts to encrypt the provided payload using a cryptographic backend.
     ///
@@ -46,19 +46,19 @@ pub trait CoseEncryptBuilderExt: Sized {
     ///                    override headers previously set using
     ///                    [`CoseEncryptBuilder::unprotected`](CoseEncryptBuilder).
     /// - `payload`      - payload which should be added to the resulting
-    ///                    [`CoseMac0`](coset::CoseMac0) instance and for which the MAC should be
-    ///                    calculated. Will override a payload previously set using
+    ///                    [`CoseEncrypt`](coset::CoseEncrypt) instance in encrypted form. 
+    ///                    Will override a payload previously set using
     ///                    [`CoseEncryptBuilder::payload`](CoseEncryptBuilder).
     /// - `external_aad` - provider of additional authenticated data that should be included in the
     ///                    MAC calculation.
     ///
     /// # Errors
     ///
     /// If the COSE structure, selected [`CoseKey`](coset::CoseKey) or AAD (or any combination of
-    /// those) are malformed or otherwise unsuitable for MAC calculation, this function will return
+    /// those) are malformed or otherwise unsuitable for encryption, this function will return
     /// the most fitting [`CoseCipherError`] for the specific type of error.
     ///
-    /// If Additional Authenticated Data is provided even though the chosen algorithm is not an AEAD
+    /// If additional authenticated data is provided even though the chosen algorithm is not an AEAD
     /// algorithm, a [`CoseCipherError::AadUnsupported`] will be returned.
     ///
     /// If the COSE object is not malformed, but an error in the cryptographic backend occurs, a
@@ -67,7 +67,7 @@ pub trait CoseEncryptBuilderExt: Sized {
     /// occur.
     ///
     /// If the COSE object is not malformed, but the key provider does not provide a key, a
-    /// [`CoseCipherError::NoMatchingKeyFound`] error will be returned.
+    /// [`CoseCipherError::NoMatchingKeyFound`] will be returned.
     ///
     /// # Examples
     ///
@@ -76,7 +76,6 @@ pub trait CoseEncryptBuilderExt: Sized {
         self,
         backend: &mut B,
         key_provider: &CKP,
-
         protected: Option<Header>,
         unprotected: Option<Header>,
         payload: &[u8],
@@ -244,8 +243,7 @@ pub trait CoseEncryptExt {
     /// the key provider must provide the key used directly for MAC calculation.
     /// If your key provider can/should be able to provide the key for a contained
     /// [`CoseRecipient`](coset::CoseRecipient), not for the [`CoseEncrypt`] instance itself, use
-    /// [`CoseEncrypt::try_decrypt_with_recipients`]
-    /// instead.
+    /// [`CoseEncrypt::try_decrypt_with_recipients`] instead.
     ///
     /// # Parameters
     ///
@@ -258,10 +256,10 @@ pub trait CoseEncryptExt {
     /// # Errors
     ///
     /// If the COSE structure, selected [`CoseKey`](coset::CoseKey) or AAD (or any combination of
-    /// those) are malformed or otherwise unsuitable for MAC calculation, this function will return
+    /// those) are malformed or otherwise unsuitable for encryption, this function will return
     /// the most fitting [`CoseCipherError`] for the specific type of error.
     ///
-    /// If Additional Authenticated Data is provided even though the chosen algorithm is not an AEAD
+    /// If additional authenticated data is provided even though the chosen algorithm is not an AEAD
     /// algorithm, a [`CoseCipherError::AadUnsupported`] will be returned.
     ///
     /// If the COSE object is not malformed, but an error in the cryptographic backend occurs, a
@@ -274,8 +272,8 @@ pub trait CoseEncryptExt {
     ///
     /// The error will then contain a list of attempted keys and the corresponding error that led to
     /// the verification error for that key.
-    /// For an invalid MAC for an otherwise valid and suitable object+key pairing, this would
-    /// usually be a [`CoseCipherError::VerificationFailure`].
+    /// For an invalid encrypted payload or AAD for an otherwise valid and suitable object+key
+    ///  pairing, this would usually be a [`CoseCipherError::VerificationFailure`].
     ///
     /// # Examples
     ///
@@ -370,7 +368,7 @@ pub trait CoseEncryptExt {
     /// or otherwise unsuitable for decryption, this function will return the most fitting
     /// [`CoseCipherError`] for the specific type of error.
     ///
-    /// If Additional Authenticated Data is provided even though the chosen algorithm is not an AEAD
+    /// If additional authenticated data is provided even though the chosen algorithm is not an AEAD
     /// algorithm, a [`CoseCipherError::AadUnsupported`] will be returned.
     ///
     /// If the COSE object is not malformed, but an error in the cryptographic backend occurs, a
@@ -387,8 +385,8 @@ pub trait CoseEncryptExt {
     ///
     /// The error will then contain a list of attempted keys and the corresponding error that led to
     /// the verification error for that key.
-    /// For an invalid MAC for an otherwise valid and suitable object+key pairing, this would
-    /// usually be a [`CoseCipherError::VerificationFailure`].
+    /// For an invalid encrypted payload or AAD for an otherwise valid and suitable object+key
+    ///  pairing, this would usually be a [`CoseCipherError::VerificationFailure`].
     ///
     /// # Examples
     ///

src/token/cose/encrypted/encrypt/tests.rs

@@ -156,7 +156,7 @@ impl<B: CryptoBackend + EncryptCryptoBackend + KeyDistributionCryptoBackend> Cos
             let plaintext = verify_result.expect("unable to verify token");
 
             assert_eq!(case.input.plaintext.as_bytes(), plaintext.as_slice());
-            // IV is apprarently taken from rng_stream field, not header field, but still implicitly
+            // IV is apparently taken from rng_stream field, not header field, but still implicitly
             // added to header. ugh...
             let mut unprotected = test_case.unprotected.clone().unwrap_or_default();
             let mut protected = test_case.protected.clone().unwrap_or_default();

src/token/cose/encrypted/encrypt0/mod.rs

@@ -47,7 +47,7 @@ pub trait CoseEncrypt0BuilderExt: Sized {
     /// or otherwise unsuitable for encryption, this function will return the most fitting
     /// [`CoseCipherError`] for the specific type of error.
     ///
-    /// If Additional Authenticated Data is provided even though the chosen algorithm is not an AEAD
+    /// If additional authenticated data is provided even though the chosen algorithm is not an AEAD
     /// algorithm, a [`CoseCipherError::AadUnsupported`] will be returned.
     ///
     /// If the COSE object is not malformed, but an error in the cryptographic backend occurs, a
@@ -56,7 +56,7 @@ pub trait CoseEncrypt0BuilderExt: Sized {
     /// occur.
     ///
     /// If the COSE object is not malformed, but the key provider does not provide a key, a
-    /// [`CoseCipherError::NoMatchingKeyFound`] error will be returned.
+    /// [`CoseCipherError::NoMatchingKeyFound`] will be returned.
     ///
     /// # Examples
     ///
@@ -173,7 +173,7 @@ pub trait CoseEncrypt0Ext {
     /// or otherwise unsuitable for decryption, this function will return the most fitting
     /// [`CoseCipherError`] for the specific type of error.
     ///
-    /// If Additional Authenticated Data is provided even though the chosen algorithm is not an AEAD
+    /// If additional authenticated data is provided even though the chosen algorithm is not an AEAD
     /// algorithm, a [`CoseCipherError::AadUnsupported`] will be returned.
     ///
     /// If the COSE object is not malformed, but an error in the cryptographic backend occurs, a
@@ -182,7 +182,7 @@ pub trait CoseEncrypt0Ext {
     /// occur.
     ///
     /// If the COSE object is not malformed, but decryption fails for all key candidates provided
-    /// by the key provider a [`CoseCipherError::NoMatchingKeyFound`] error will be returned.
+    /// by the key provider a [`CoseCipherError::NoMatchingKeyFound`] will be returned.
     ///
     /// The error will then contain a list of attempted keys and the corresponding error that led to
     /// the verification error for that key.

src/token/cose/encrypted/encrypt0/tests.rs

@@ -38,7 +38,7 @@ impl<B: CryptoBackend + EncryptCryptoBackend> CoseStructTestHelper<B> for CoseEn
             .first()
             .expect("test case has no recipient");
 
-        // Need to generate an IV. Have to do this quite ugly, because we have implemented our IV
+        // Need to generate an IV. Have to do this quite uglily, because we have implemented our IV
         // generation on the header builder only.
         let alg = if let coset::Algorithm::Assigned(alg) = encrypt0_cfg
             .protected
@@ -113,7 +113,7 @@ impl<B: CryptoBackend + EncryptCryptoBackend> CoseStructTestHelper<B> for CoseEn
             let plaintext = verify_result.expect("unable to verify token");
 
             assert_eq!(case.input.plaintext.as_bytes(), plaintext.as_slice());
-            // IV is apprarently taken from rng_stream field, not header field, but still implicitly added to header.
+            // IV is apparently taken from rng_stream field, not header field, but still implicitly added to header.
             // ugh...
             let mut unprotected = test_case.unprotected.clone().unwrap_or_default();
             let mut protected = test_case.protected.clone().unwrap_or_default();

src/token/cose/encrypted/mod.rs

@@ -37,7 +37,7 @@ pub trait EncryptCryptoBackend: CryptoBackend {
     /// # Arguments
     ///
     /// * `alg` - The AES-GCM variant to use.
-    ///           If unsupported by the backend, a [`CoseCipherError::UnsupportedAlgorithm`] error
+    ///           If unsupported by the backend, a [`CoseCipherError::UnsupportedAlgorithm`]
     ///           should be returned.
     ///           If the given algorithm is an IANA-assigned value that is unknown, the
     ///           implementation should return [`CoseCipherError::UnsupportedAlgorithm`] (in case
@@ -111,7 +111,7 @@ pub trait EncryptCryptoBackend: CryptoBackend {
     /// # Returns
     ///
     /// It is expected that the return value is either the computed plaintext if decryption and
-    /// authentication are successful, or a [`CoseCipherError::VerificationFailure`] one of these
+    /// authentication are successful, or a [`CoseCipherError::VerificationFailure`] if one of these
     /// steps fails even though the input is well-formed.
     ///
     /// # Errors
@@ -215,6 +215,7 @@ pub(crate) fn try_decrypt<B: EncryptCryptoBackend, CKP: KeyProvider>(
                         )))?;
 
                     // Authentication tag is 16 bytes long and should be included in the ciphertext.
+                    // Empty payloads are allowed, therefore we check for ciphertext.len() < 16, not <= 16.
                     if ciphertext.len() < 16 {
                         return Err(CoseCipherError::VerificationFailure);
                     }

src/token/cose/header_util.rs

@@ -24,7 +24,7 @@ use crate::token::cose::{CryptoBackend, EncryptCryptoBackend};
 /// A header parameter that can be used in a COSE header.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum HeaderParam {
-    /// Generic header parmeter applicable to all algorithms.
+    /// Generic header parameter applicable to all algorithms.
     Generic(iana::HeaderParameter),
     /// Header parameter that is specific for a set of algorithms.
     Algorithm(iana::HeaderAlgorithmParameter),

src/token/cose/key.rs

@@ -29,7 +29,7 @@ fn find_param_by_label<'a>(label: &Label, param_vec: &'a [(Label, Value)]) -> Op
 /// An IANA-defined key parameter for a [`CoseKey`](coset::CoseKey).
 #[derive(Debug, Clone, Copy, PartialEq)]
 pub enum KeyParam {
-    /// Key parameters that are not specific for a certain key type.
+    /// Key parameters that are not specific to a certain key type.
     Common(iana::KeyParameter),
     /// Key parameters specific to EC2 elliptic curve keys.
     Ec2(iana::Ec2KeyParameter),
@@ -130,7 +130,7 @@ pub type EllipticCurve = RegisteredLabelWithPrivate<iana::EllipticCurve>;
 ///
 /// In the CBOR representation of this key, the sign and Y coordinate are both represented as
 /// different types of the same map field (`y`), see
-/// <https://datatracker.ietf.org/doc/html/rfc9053#section-7.1.1>.
+/// [section 7.1.1 of RFC 9053](https://datatracker.ietf.org/doc/html/rfc9053#section-7.1.1).
 #[derive(Clone, Debug, PartialEq)]
 pub struct CoseEc2Key<'a, OE: Display> {
     /// Key that is referenced by this view.
@@ -168,7 +168,7 @@ impl<'a, OE: Display> TryFrom<&'a CoseKey> for CoseEc2Key<'a, OE> {
         ]))?;
         // Curve must be of correct type
         let crv = EllipticCurve::from_cbor_value(crv.clone()).map_err(|_e| {
-            // TODO e as error source (as soon as we use core::error::Error).
+            // TODO (#14) e as error source (as soon as we use core::error::Error).
             CoseCipherError::InvalidKeyParam(iana::Ec2KeyParameter::Crv.into(), crv.clone())
         })?;
 
@@ -291,7 +291,7 @@ impl<'a, OE: Display> TryFrom<&'a CoseKey> for CoseOkpKey<'a, OE> {
 
         // Curve must be of correct type
         let crv = EllipticCurve::from_cbor_value(crv.clone()).map_err(|_e| {
-            // TODO e as error source (as soon as we use core::error::Error).
+            // TODO (#14): e as error source (as soon as we use core::error::Error).
             CoseCipherError::InvalidKeyParam(iana::OkpKeyParameter::Crv.into(), crv.clone())
         })?;
 
@@ -403,7 +403,7 @@ pub trait KeyProvider: Sized {
     /// matching the one provided, or all [`CoseKey`](coset::CoseKey)s available if key_id is None.
     fn lookup_key(&self, key_id: Option<&[u8]>) -> impl Iterator<Item = impl Borrow<CoseKey>>;
 
-    /// Create a [`KeyProvider`] filtering this key providers output for keys with key IDs
+    /// Create a [`KeyProvider`] filtering this key provider's output for keys with key IDs
     /// matching the COSE structure's header.
     fn match_key_ids(self) -> KeyProviderFilterMatchingKeyId<Self> {
         KeyProviderFilterMatchingKeyId(self)
@@ -457,7 +457,7 @@ impl KeyProvider for CoseKey {
     }
 }
 
-/// [`KeyProvider`] that filters another [`KeyProvider`] s output to only output keys with
+/// [`KeyProvider`] that filters another [`KeyProvider`]s output to only output keys with
 /// key IDs matching the ones provided in the COSE structure's headers.
 pub struct KeyProviderFilterMatchingKeyId<T: KeyProvider>(T);
 
@@ -601,11 +601,13 @@ pub(crate) fn ensure_valid_ecdsa_key<BE: Display>(
         if ec2_key.x.is_none() {
             return Err(CoseCipherError::MissingKeyParam(vec![
                 iana::Ec2KeyParameter::X.into(),
+                iana::Ec2KeyParameter::D.into(),
             ]));
         }
         if ec2_key.y.is_none() {
             return Err(CoseCipherError::MissingKeyParam(vec![
                 iana::Ec2KeyParameter::Y.into(),
+                iana::Ec2KeyParameter::D.into(),
             ]));
         }
     }