feat(rust/rbac-registration): refactor cip509 extract key fns (#310)

* feat(rust/rbac-registration): refactor cip509 exctrat key fns

* fix: comments

* avoid extended public keys, refactor error type, add unit test

* remove outdoated doc comment

* Update rust/rbac-registration/src/cardano/cip509/utils/extract_key.rs

Co-authored-by: Alex Pozhylenkov <[email protected]>

* fix docs & split invalid length err

* fmt & debug

---------

Co-authored-by: Steven Johnson <[email protected]>
Co-authored-by: Alex Pozhylenkov <[email protected]>

Author: 3 people

rust/rbac-registration/src/cardano/cip509/utils/extract_key.rs

@@ -1,19 +1,86 @@
 //! Functions for extracting public key from X509 and C509 certificates with additional
 //! verification.
 
-use anyhow::{anyhow, Context};
+use std::borrow::Cow;
+
 use c509_certificate::c509::C509;
-use ed25519_dalek::{VerifyingKey, PUBLIC_KEY_LENGTH};
+use catalyst_types::problem_report::ProblemReport;
+use ed25519_dalek::{SignatureError, VerifyingKey, PUBLIC_KEY_LENGTH};
 use oid_registry::{Oid, OID_SIG_ED25519};
 use thiserror::Error;
-use x509_cert::Certificate as X509Certificate;
+use x509_cert::{spki, Certificate as X509Certificate};
+
+/// Common error type.
+#[derive(Debug, Error)]
+pub enum Error {
+    /// Unsupported signature algorithms.
+    #[error("Unsupported signature algorithm (oid: {oid})")]
+    UnsupportedSignatureAlgo {
+        /// An OID of unsupported signature algorithm.
+        oid: Oid<'static>,
+    },
+    /// Public key has invalid length.
+    #[error(
+        "Invalid public key length (found {bytes} bytes, but expected {PUBLIC_KEY_LENGTH} bytes)"
+    )]
+    InvalidPublicKeyLength {
+        /// Number of bytes found.
+        bytes: usize,
+    },
+    /// Public key is stored in a bit string, where number of unused bits is *not* equal
+    /// to zero.
+    #[error("Invalid public key is not octet aligned (found {bits} bits)")]
+    PublicKeyIsNotOctetAligned {
+        /// Number of bits found.
+        bits: usize,
+    },
+    /// Public key doesn't pass [`ed25519_dalek`] constraint check.
+    #[error("Invalid public key ({source})")]
+    PublicKeyIsNotEd25519 {
+        /// Underlying [`ed25519_dalek`] error.
+        #[from]
+        source: SignatureError,
+    },
+}
 
-/// Error type for unsupported signature algorithms.
-#[derive(Error, Debug)]
-#[error("Unsupported signature algorithm: {oid}")]
-pub struct SignatureAlgoError {
-    /// An OID of unsupported signature algorithm.
-    oid: String,
+impl Error {
+    /// Shortcut function to report `Self` into a [`ProblemReport`].
+    pub fn report_problem(&self, context: &str, report: &ProblemReport) {
+        match self {
+            Error::UnsupportedSignatureAlgo { oid } => {
+                report.invalid_value(
+                    "subject_public_key_algorithm",
+                    &oid.to_id_string(),
+                    "Currently the only supported signature algorithm is ED25519",
+                    context,
+                );
+            },
+            Error::InvalidPublicKeyLength { bytes } => {
+                report.invalid_value(
+                    "subject_public_key",
+                    &format!("{bytes} bytes"),
+                    &format!("Must be {PUBLIC_KEY_LENGTH} bytes long"),
+                    context,
+                );
+            },
+            Error::PublicKeyIsNotOctetAligned { bits } => {
+                report.invalid_value(
+                    "subject_public_key",
+                    &format!("{bits} bits"),
+                    "Bit string must be octet aligned having no unused bits",
+                    context,
+                );
+            },
+            Error::PublicKeyIsNotEd25519 { source } => {
+                report.invalid_value(
+                    "subject_public_key",
+                    &source.to_string(),
+                    "Must be an Ed25519 public key",
+                    context,
+                );
+            },
+        }
+    }
 }
 
 /// Returns `VerifyingKey` from the given X509 certificate.
@@ -22,24 +89,21 @@ pub struct SignatureAlgoError {
 ///
 /// Returns an error if the signature algorithm is not supported and
 /// the public key cannot be extracted.
-pub fn x509_key(cert: &X509Certificate) -> anyhow::Result<VerifyingKey> {
-    let oid: Oid = cert
-        .tbs_certificate
-        .subject_public_key_info
-        .algorithm
-        .oid
-        .to_string()
-        .parse()
-        // `Context` cannot be used here because `OidParseError` doesn't implement `std::Error`.
-        .map_err(|e| anyhow!("Invalid signature algorithm OID: {e:?}"))?;
-    check_signature_algorithm(&oid)?;
-    let extended_public_key = cert
+///
+/// Returns an error if public key has unexpected value.
+pub fn x509_key(cert: &X509Certificate) -> Result<VerifyingKey, Error> {
+    let oid = &cert.tbs_certificate.subject_public_key_info.algorithm.oid;
+    check_signature_algorithm(&spki_oid_as_asn1_rs_oid(oid))?;
+    let public_key = &cert
         .tbs_certificate
         .subject_public_key_info
-        .subject_public_key
+        .subject_public_key;
+    let public_key_bytes = public_key
         .as_bytes()
-        .context("Invalid subject_public_key value (has unused bits)")?;
-    verifying_key(extended_public_key).context("Unable to get verifying key from X509 certificate")
+        .ok_or(Error::PublicKeyIsNotOctetAligned {
+            bits: public_key.bit_len(),
+        })?;
+    verifying_key(public_key_bytes)
 }
 
 /// Returns `VerifyingKey` from the given C509 certificate.
@@ -48,47 +112,65 @@ pub fn x509_key(cert: &X509Certificate) -> anyhow::Result<VerifyingKey> {
 ///
 /// Returns an error if the signature algorithm is not supported and
 /// the public key cannot be extracted.
-pub fn c509_key(cert: &C509) -> anyhow::Result<VerifyingKey> {
+///
+/// Returns an error if public key has unexpected value.
+pub fn c509_key(cert: &C509) -> Result<VerifyingKey, Error> {
     let oid = cert
         .tbs_cert()
         .subject_public_key_algorithm()
         .algo_identifier()
         .oid();
     check_signature_algorithm(oid)?;
-    verifying_key(cert.tbs_cert().subject_public_key())
-        .context("Unable to get verifying key from C509 certificate")
+    let public_key = cert.tbs_cert().subject_public_key();
+    verifying_key(public_key)
 }
 
-/// Checks that the signature algorithm is supported.
-fn check_signature_algorithm(oid: &Oid) -> Result<(), SignatureAlgoError> {
+/// Checks that the signature algorithm with the given [`spki::ObjectIdentifier`] is
+/// supported.
+fn check_signature_algorithm(oid: &Oid) -> Result<(), Error> {
     // Currently the only supported signature algorithm is ED25519.
-    if *oid != OID_SIG_ED25519 {
-        return Err(SignatureAlgoError {
-            oid: oid.to_id_string(),
-        });
+    if *oid == OID_SIG_ED25519 {
+        Ok(())
+    } else {
+        Err(Error::UnsupportedSignatureAlgo {
+            oid: oid.to_owned(),
+        })
     }
-    Ok(())
 }
 
-// TODO: The very similar logic exists in the `rbac-registration` crate. It should be
-// moved somewhere and reused. See https://github.com/input-output-hk/catalyst-voices/issues/1952
-/// Creates `VerifyingKey` from the given extended public key.
-fn verifying_key(extended_public_key: &[u8]) -> anyhow::Result<VerifyingKey> {
-    /// An extender public key length in bytes.
-    const EXTENDED_PUBLIC_KEY_LENGTH: usize = 64;
+/// Converts [`spki::ObjectIdentifier`] ref to an [`Oid`].
+fn spki_oid_as_asn1_rs_oid(oid: &'_ spki::ObjectIdentifier) -> Oid<'_> {
+    // Note that this conversion always succeeds as both crates omit header.
+    Oid::new(Cow::Borrowed(oid.as_bytes()))
+}
+
+/// Creates [`VerifyingKey`] from the first 32 bytes in a slice.
+/// Since only prefix bytes are used, both extended and common public keys are supported
+/// here.
+fn verifying_key(public_key: &[u8]) -> Result<VerifyingKey, Error> {
+    public_key
+        // TODO: replace with checked `[u8; 32]` conversion once we only support common ed25119.
+        .first_chunk()
+        // Public key is too short.
+        .ok_or(Error::InvalidPublicKeyLength {
+            bytes: public_key.len(),
+        })
+        .and_then(|bytes| VerifyingKey::from_bytes(bytes).map_err(Error::from))
+}
+
+#[cfg(test)]
+mod tests {
+    use oid_registry::{asn1_rs, OID_SIG_ED25519};
+    use x509_cert::spki;
+
+    #[test]
+    fn spki_oid_as_asn1_rs_oid() {
+        let spki_oid = spki::ObjectIdentifier::new_unwrap("1.3.101.112");
+        let asn1_rs_oid = asn1_rs::oid!(1.3.101 .112);
+        assert_eq!(spki_oid.to_string(), asn1_rs_oid.to_id_string());
 
-    if extended_public_key.len() != EXTENDED_PUBLIC_KEY_LENGTH {
-        return Err(anyhow!(
-            "Unexpected extended public key length in certificate: {}, expected {EXTENDED_PUBLIC_KEY_LENGTH}",
-            extended_public_key.len()
-        ));
+        let converted_spki_oid = super::spki_oid_as_asn1_rs_oid(&spki_oid);
+        assert_eq!(converted_spki_oid.to_string(), asn1_rs_oid.to_id_string());
+        assert_eq!(converted_spki_oid, OID_SIG_ED25519);
     }
-    // This should never fail because of the check above.
-    let public_key = extended_public_key
-        .get(0..PUBLIC_KEY_LENGTH)
-        .context("Unable to get public key part")?;
-    let bytes: &[u8; PUBLIC_KEY_LENGTH] = public_key
-        .try_into()
-        .context("Invalid public key length in X509 certificate")?;
-    VerifyingKey::from_bytes(bytes).context("Invalid public key in X509 certificate")
 }

rust/rbac-registration/src/cardano/cip509/validation.rs

@@ -13,7 +13,7 @@ use catalyst_types::{
     hashes::{Blake2b128Hash, Blake2b256Hash},
     problem_report::ProblemReport,
 };
-use ed25519_dalek::{Signature, VerifyingKey, PUBLIC_KEY_LENGTH};
+use ed25519_dalek::{Signature, VerifyingKey};
 use pallas::{
     codec::{
         minicbor::{Encode, Encoder},
@@ -464,70 +464,16 @@ fn validate_role_0(
 
 /// Extracts `VerifyingKey` from the given `X509` certificate.
 fn x509_cert_key(cert: &X509, context: &str, report: &ProblemReport) -> Option<VerifyingKey> {
-    let Some(extended_public_key) = cert
-        .tbs_certificate
-        .subject_public_key_info
-        .subject_public_key
-        .as_bytes()
-    else {
-        report.invalid_value(
-            "subject_public_key",
-            "is not octet aligned",
-            "Must not have unused bits",
-            context,
-        );
-        return None;
-    };
-    verifying_key(extended_public_key, context, report)
+    x509_key(cert)
+        .inspect_err(|err| err.report_problem(context, report))
+        .ok()
 }
 
 /// Extracts `VerifyingKey` from the given `C509` certificate.
 fn c509_cert_key(cert: &C509, context: &str, report: &ProblemReport) -> Option<VerifyingKey> {
-    verifying_key(cert.tbs_cert().subject_public_key(), context, report)
-}
-
-/// Creates `VerifyingKey` from the given extended public key.
-fn verifying_key(
-    extended_public_key: &[u8], context: &str, report: &ProblemReport,
-) -> Option<VerifyingKey> {
-    /// An extender public key length in bytes.
-    const EXTENDED_PUBLIC_KEY_LENGTH: usize = 64;
-
-    if extended_public_key.len() != EXTENDED_PUBLIC_KEY_LENGTH {
-        report.other(
-            &format!("Unexpected extended public key length in certificate: {}, expected {EXTENDED_PUBLIC_KEY_LENGTH}",
-                extended_public_key.len()),
-            context,
-        );
-        return None;
-    }
-
-    // This should never fail because of the check above.
-    let Some(public_key) = extended_public_key.get(0..PUBLIC_KEY_LENGTH) else {
-        report.other("Unable to get public key part", context);
-        return None;
-    };
-
-    let bytes: &[u8; PUBLIC_KEY_LENGTH] = match public_key.try_into() {
-        Ok(v) => v,
-        Err(e) => {
-            report.other(
-                &format!("Invalid public key length in X509 certificate: {e:?}"),
-                context,
-            );
-            return None;
-        },
-    };
-    match VerifyingKey::from_bytes(bytes) {
-        Ok(k) => Some(k),
-        Err(e) => {
-            report.other(
-                &format!("Invalid public key in C509 certificate: {e:?}"),
-                context,
-            );
-            None
-        },
-    }
+    c509_key(cert)
+        .inspect_err(|err| err.report_problem(context, report))
+        .ok()
 }
 
 #[cfg(test)]