fix(rust/signed-doc): Added missing validation of the kid field (#292)

* add catalyst id validation for signed doc signature

* add tests

* fix spelling

* fix clippy

Author: Mr-Leshiy

rust/catalyst-types/src/id_uri/mod.rs

@@ -1,4 +1,5 @@
 //! Catalyst ID URI.
+//! <https://input-output-hk.github.io/catalyst-libs/architecture/08_concepts/rbac_id_uri/catalyst-id-uri/>
 
 // cspell: words userinfo rngs Fftx csprng
 
@@ -25,6 +26,7 @@ use key_rotation::KeyRotation;
 use role_index::RoleIndex;
 
 /// Catalyst ID
+/// <https://input-output-hk.github.io/catalyst-libs/architecture/08_concepts/rbac_id_uri/catalyst-id-uri/>
 ///
 /// Identity of Catalyst Registration.
 /// Optionally also identifies a specific Signed Document Key
@@ -66,7 +68,7 @@ impl IdUri {
     /// * Wednesday, January 1, 2025 12:00:00 AM
     const MIN_NONCE: i64 = 1_735_689_600;
     /// URI scheme for Catalyst
-    const SCHEME: &Scheme = Scheme::new_or_panic("id.catalyst");
+    pub const SCHEME: &Scheme = Scheme::new_or_panic("id.catalyst");
 
     /// Get the cosmetic username from the URI.
     #[must_use]
@@ -144,6 +146,12 @@ impl IdUri {
         self.id
     }
 
+    /// Was `IdUri` formatted as an uri when it was parsed.
+    #[must_use]
+    pub fn is_uri(&self) -> bool {
+        !self.id
+    }
+
     /// Add or change the username in a Catalyst ID URI.
     #[must_use]
     pub fn with_username(self, name: &str) -> Self {

rust/signed_doc/bins/mk_signed_doc.rs

@@ -83,7 +83,7 @@ impl Cli {
 
                 let new_signed_doc = signed_doc
                     .into_builder()
-                    .add_signature(|message| sk.sign::<()>(&message).to_bytes().to_vec(), kid)?
+                    .add_signature(|message| sk.sign::<()>(&message).to_bytes().to_vec(), &kid)?
                     .build();
                 save_signed_doc(new_signed_doc, &doc)?;
             },

rust/signed_doc/src/builder.rs

@@ -2,8 +2,8 @@
 use catalyst_types::{id_uri::IdUri, problem_report::ProblemReport};
 
 use crate::{
-    CatalystSignedDocument, Content, InnerCatalystSignedDocument, Metadata, Signatures,
-    PROBLEM_REPORT_CTX,
+    signature::Signature, CatalystSignedDocument, Content, InnerCatalystSignedDocument, Metadata,
+    Signatures, PROBLEM_REPORT_CTX,
 };
 
 /// Catalyst Signed Document Builder.
@@ -55,7 +55,7 @@ impl Builder {
     /// content, due to malformed data, or when the signed document cannot be
     /// converted into `coset::CoseSign`.
     pub fn add_signature(
-        mut self, sign_fn: impl FnOnce(Vec<u8>) -> Vec<u8>, kid: IdUri,
+        mut self, sign_fn: impl FnOnce(Vec<u8>) -> Vec<u8>, kid: &IdUri,
     ) -> anyhow::Result<Self> {
         let cose_sign = self
             .0
@@ -69,7 +69,9 @@ impl Builder {
             .build();
         let data_to_sign = cose_sign.tbs_data(&[], &signature);
         signature.signature = sign_fn(data_to_sign);
-        self.0.signatures.push(kid, signature);
+        if let Some(sign) = Signature::from_cose_sig(signature, &self.0.report) {
+            self.0.signatures.push(sign);
+        }
 
         Ok(self)
     }

rust/signed_doc/src/lib.rs

@@ -223,7 +223,7 @@ impl Decode<'_, ()> for CatalystSignedDocument {
 
         let report = ProblemReport::new(PROBLEM_REPORT_CTX);
         let metadata = Metadata::from_protected_header(&cose_sign.protected, &report);
-        let signatures = Signatures::from_cose_sig(&cose_sign.signatures, &report);
+        let signatures = Signatures::from_cose_sig_list(&cose_sign.signatures, &report);
 
         let content = if let Some(payload) = cose_sign.payload {
             Content::from_encoded(payload, metadata.content_encoding(), &report)

rust/signed_doc/src/signature/mod.rs

@@ -13,6 +13,35 @@ pub struct Signature {
     signature: CoseSignature,
 }
 
+impl Signature {
+    /// Convert COSE Signature to `Signature`.
+    pub(crate) fn from_cose_sig(signature: CoseSignature, report: &ProblemReport) -> Option<Self> {
+        match IdUri::try_from(signature.protected.header.key_id.as_ref()) {
+            Ok(kid) if kid.is_uri() => Some(Self { kid, signature }),
+            Ok(kid) => {
+                report.invalid_value(
+                    "COSE signature protected header key ID",
+                    &kid.to_string(),
+                    &format!(
+                        "COSE signature protected header key ID must be a Catalyst Id URI, missing URI schema {}", IdUri::SCHEME
+                    ),
+                    "Converting COSE signature header key ID to IdUri",
+                );
+                None
+            },
+            Err(e) => {
+                report.conversion_error(
+                    "COSE signature protected header key ID",
+                    &format!("{:?}", &signature.protected.header.key_id),
+                    &format!("{e:?}"),
+                    "Converting COSE signature header key ID to IdUri",
+                );
+                None
+            },
+        }
+    }
+}
+
 /// List of Signatures.
 #[derive(Debug, Clone, Default)]
 pub struct Signatures(Vec<Signature>);
@@ -42,9 +71,9 @@ impl Signatures {
         self.0.iter().map(|sig| sig.signature.clone())
     }
 
-    /// Add a new signature
-    pub(crate) fn push(&mut self, kid: IdUri, signature: CoseSignature) {
-        self.0.push(Signature { kid, signature });
+    /// Add a `Signature` object into the list
+    pub(crate) fn push(&mut self, sign: Signature) {
+        self.0.push(sign);
     }
 
     /// Number of signatures.
@@ -60,27 +89,19 @@ impl Signatures {
     }
 
     /// Convert list of COSE Signature to `Signatures`.
-    pub(crate) fn from_cose_sig(cose_sigs: &[CoseSignature], error_report: &ProblemReport) -> Self {
-        let mut signatures = Vec::new();
-
-        cose_sigs
+    pub(crate) fn from_cose_sig_list(cose_sigs: &[CoseSignature], report: &ProblemReport) -> Self {
+        let res = cose_sigs
             .iter()
             .cloned()
             .enumerate()
-            .for_each(|(idx, signature)| {
-                match IdUri::try_from(signature.protected.header.key_id.as_ref()) {
-                    Ok(kid) => signatures.push(Signature { kid, signature }),
-                    Err(e) => {
-                        error_report.conversion_error(
-                            &format!("COSE signature protected header key ID at id {idx}"),
-                            &format!("{:?}", &signature.protected.header.key_id),
-                            &format!("{e:?}"),
-                            "Converting COSE signature header key ID to IdUri",
-                        );
-                    },
+            .filter_map(|(idx, signature)| {
+                let sign = Signature::from_cose_sig(signature, report);
+                if sign.is_none() {
+                    report.other(&format!("COSE signature protected header key ID at id {idx}"), "Converting COSE signatures list to Catalyst Signed Documents signatures list",);
                 }
-            });
+                sign
+            }).collect();
 
-        Self(signatures)
+        Self(res)
     }
 }

rust/signed_doc/src/validator/rules/signature_kid.rs

@@ -68,7 +68,7 @@ mod tests {
                 "content-type": ContentType::Json.to_string(),
             }))
             .unwrap()
-            .add_signature(|m| sk.sign(&m).to_vec(), kid)
+            .add_signature(|m| sk.sign(&m).to_vec(), &kid)
             .unwrap()
             .build();
 

rust/signed_doc/tests/common/mod.rs

@@ -85,7 +85,7 @@ pub fn create_dummy_signed_doc(
     let signed_doc = Builder::new()
         .with_decoded_content(content)
         .with_json_metadata(with_metadata.unwrap_or(metadata))?
-        .add_signature(|m| sk.sign(&m).to_vec(), kid.clone())?
+        .add_signature(|m| sk.sign(&m).to_vec(), &kid)?
         .build();
 
     Ok((signed_doc, pk, kid))

rust/signed_doc/tests/decoding.rs

@@ -1,18 +1,25 @@
 //! Integration test for COSE decoding part.
 
 use catalyst_signed_doc::*;
+use catalyst_types::id_uri::role_index::RoleIndex;
+use common::create_dummy_key_pair;
+use ed25519_dalek::ed25519::signature::Signer;
 
 mod common;
 
 #[test]
 fn catalyst_signed_doc_cbor_roundtrip_test() {
     let (uuid_v7, uuid_v4, metadata_fields) = common::test_metadata();
+    let (sk, _, kid) = create_dummy_key_pair(RoleIndex::ROLE_0).unwrap();
+
     let content = serde_json::to_vec(&serde_json::Value::Null).unwrap();
 
     let doc = Builder::new()
         .with_json_metadata(metadata_fields.clone())
         .unwrap()
         .with_decoded_content(content.clone())
+        .add_signature(|m| sk.sign(&m).to_vec(), &kid)
+        .unwrap()
         .build();
 
     assert!(!doc.problem_report().is_problematic());
@@ -27,3 +34,23 @@ fn catalyst_signed_doc_cbor_roundtrip_test() {
     assert_eq!(decoded.doc_content().decoded_bytes().unwrap(), &content);
     assert_eq!(decoded.doc_meta(), &extra_fields);
 }
+
+#[test]
+fn catalyst_signed_doc_cbor_roundtrip_kid_as_id_test() {
+    let (_, _, metadata_fields) = common::test_metadata();
+    let (sk, _, kid) = create_dummy_key_pair(RoleIndex::ROLE_0).unwrap();
+    // transform Catalyst ID URI form to the ID form
+    let kid = kid.as_id();
+
+    let content = serde_json::to_vec(&serde_json::Value::Null).unwrap();
+
+    let doc = Builder::new()
+        .with_json_metadata(metadata_fields.clone())
+        .unwrap()
+        .with_decoded_content(content.clone())
+        .add_signature(|m| sk.sign(&m).to_vec(), &kid)
+        .unwrap()
+        .build();
+
+    assert!(doc.problem_report().is_problematic());
+}

rust/signed_doc/tests/signature.rs

@@ -37,11 +37,11 @@ async fn multiple_signatures_validation_test() {
         .with_decoded_content(serde_json::to_vec(&serde_json::Value::Null).unwrap())
         .with_json_metadata(common::test_metadata().2)
         .unwrap()
-        .add_signature(|m| sk1.sign(&m).to_vec(), kid1.clone())
+        .add_signature(|m| sk1.sign(&m).to_vec(), &kid1)
         .unwrap()
-        .add_signature(|m| sk2.sign(&m).to_vec(), kid2.clone())
+        .add_signature(|m| sk2.sign(&m).to_vec(), &kid2)
         .unwrap()
-        .add_signature(|m| sk3.sign(&m).to_vec(), kid3.clone())
+        .add_signature(|m| sk3.sign(&m).to_vec(), &kid3)
         .unwrap()
         .build();