signatures: Use KeyId and SigningKeyAlgorithm to parse key IDs

crates/ruma-common/Cargo.toml

@@ -33,6 +33,9 @@ unstable-msc3932 = ["unstable-msc3931"]
 unstable-msc4210 = []
 unstable-unspecified = []
 
+# Allow IDs to exceed 255 bytes.
+compat-arbitrary-length-ids = ["ruma-identifiers-validation/compat-arbitrary-length-ids"]
+
 # Don't validate `ServerSigningKeyVersion`.
 compat-server-signing-key-version = ["ruma-identifiers-validation/compat-server-signing-key-version"]
 

crates/ruma-common/src/identifiers.rs

@@ -26,8 +26,8 @@ pub use self::{
     device_id::{DeviceId, OwnedDeviceId},
     event_id::{EventId, OwnedEventId},
     key_id::{
-        CrossSigningKeyId, CrossSigningOrDeviceSigningKeyId, DeviceKeyId, DeviceSigningKeyId,
-        KeyAlgorithm, KeyId, OneTimeKeyId, OwnedCrossSigningKeyId,
+        AnyKeyName, CrossSigningKeyId, CrossSigningOrDeviceSigningKeyId, DeviceKeyId,
+        DeviceSigningKeyId, KeyAlgorithm, KeyId, OneTimeKeyId, OwnedCrossSigningKeyId,
         OwnedCrossSigningOrDeviceSigningKeyId, OwnedDeviceKeyId, OwnedDeviceSigningKeyId,
         OwnedKeyId, OwnedOneTimeKeyId, OwnedServerSigningKeyId, OwnedSigningKeyId,
         ServerSigningKeyId, SigningKeyId,

crates/ruma-common/src/identifiers/crypto_algorithms.rs

@@ -24,7 +24,7 @@ pub enum DeviceKeyAlgorithm {
 
 /// The signing key algorithms defined in the Matrix spec.
 #[doc = include_str!(concat!(env!("CARGO_MANIFEST_DIR"), "/src/doc/string_enum.md"))]
-#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, StringEnum)]
+#[derive(Clone, PartialEq, Eq, PartialOrd, Ord, Hash, StringEnum)]
 #[non_exhaustive]
 #[ruma_enum(rename_all = "snake_case")]
 pub enum SigningKeyAlgorithm {

crates/ruma-common/src/identifiers/key_id.rs

@@ -204,6 +204,19 @@ impl KeyAlgorithm for DeviceKeyAlgorithm {}
 
 impl KeyAlgorithm for OneTimeKeyAlgorithm {}
 
+/// An opaque identifier type to use with [`KeyId`].
+///
+/// This type has no semantic value and no validation is done. It is meant to be able to use the
+/// [`KeyId`] API without validating the key name.
+#[derive(PartialEq, Eq, PartialOrd, Ord, Hash, IdZst)]
+pub struct AnyKeyName(str);
+
+impl KeyName for AnyKeyName {
+    fn validate(_s: &str) -> Result<(), ruma_common::IdParseError> {
+        Ok(())
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use assert_matches2::assert_matches;

crates/ruma-identifiers-validation/Cargo.toml

@@ -12,6 +12,9 @@ rust-version = { workspace = true }
 all-features = true
 
 [features]
+# Allow IDs to exceed 255 bytes.
+compat-arbitrary-length-ids = []
+
 # Don't validate the version in `server_signing_key_version::validate`.
 compat-server-signing-key-version = []
 

crates/ruma-identifiers-validation/src/lib.rs

@@ -23,6 +23,7 @@ pub const MAX_BYTES: usize = 255;
 
 /// Checks if an identifier is valid.
 fn validate_id(id: &str, sigil: u8) -> Result<(), Error> {
+    #[cfg(not(feature = "compat-arbitrary-length-ids"))]
     if id.len() > MAX_BYTES {
         return Err(Error::MaximumLengthExceeded);
     }

crates/ruma-signatures/CHANGELOG.md

@@ -1,5 +1,14 @@
 # [unreleased]
 
+Breaking changes:
+
+- `Algorithm` is replaced by `SigningKeyAlgorithm` from `ruma-common`.
+  - `Signature::new()` returns an `IdParseError`.
+  - `Error::UnsupportedAlgorithm` is removed since it is now unused.
+- The `compat-signature-id` cargo feature was removed. No validation is done on
+  the key name of a key ID, to stop assuming that this crate is only used to
+  check server signatures.
+
 Bug fixes:
 
 - Do not check the signature of the server of the sender of `m.room.member`

crates/ruma-signatures/Cargo.toml

@@ -14,8 +14,6 @@ edition = "2021"
 all-features = true
 
 [features]
-# Allow extra characters in signature IDs not allowed in the specification.
-compat-signature-id = []
 ring-compat = ["dep:subslice"]
 unstable-exhaustive-types = []
 

crates/ruma-signatures/src/error.rs

@@ -34,10 +34,6 @@ pub enum Error {
     #[error("malformed signature ID: expected version to contain only characters in the character set `[a-zA-Z0-9_]`, found `{0}`")]
     InvalidVersion(String),
 
-    /// The signature uses an unsupported algorithm.
-    #[error("signature uses an unsupported algorithm: {0}")]
-    UnsupportedAlgorithm(String),
-
     /// PDU was too large
     #[error("PDU is larger than maximum of 65535 bytes")]
     PduSize,

crates/ruma-signatures/src/functions.rs

@@ -10,15 +10,14 @@ use base64::{alphabet, Engine};
 use ruma_common::{
     canonical_json::{redact, JsonType},
     serde::{base64::Standard, Base64},
-    CanonicalJsonObject, CanonicalJsonValue, OwnedEventId, OwnedServerName,
-    OwnedServerSigningKeyId, RoomVersionId, UserId,
+    AnyKeyName, CanonicalJsonObject, CanonicalJsonValue, OwnedEventId, OwnedServerName,
+    OwnedServerSigningKeyId, RoomVersionId, SigningKeyAlgorithm, SigningKeyId, UserId,
 };
 use serde_json::to_string as to_json_string;
 use sha2::{digest::Digest, Sha256};
 
 use crate::{
     keys::{KeyPair, PublicKeyMap},
-    split_id,
     verification::{Ed25519Verifier, Verified, Verifier},
     Error, JsonError, ParseError, VerificationError,
 };
@@ -580,9 +579,13 @@ pub fn verify_event(
 
         let mut checked = false;
         for (key_id, signature) in signature_set {
-            // Since only ed25519 is supported right now, we don't actually need to check what the
-            // algorithm is. If it split successfully, it's ed25519.
-            if split_id(key_id).is_err() {
+            // If we cannot parse the key ID, ignore.
+            let Ok(parsed_key_id) = <&SigningKeyId<AnyKeyName>>::try_from(key_id.as_str()) else {
+                continue;
+            };
+
+            // If the signature uses an unknown algorithm, ignore.
+            if parsed_key_id.algorithm() != SigningKeyAlgorithm::Ed25519 {
                 continue;
             }
 

crates/ruma-signatures/src/keys.rs

@@ -9,9 +9,9 @@ use ed25519_dalek::{pkcs8::ALGORITHM_OID, SecretKey, Signer, SigningKey, PUBLIC_
 use pkcs8::{
     der::zeroize::Zeroizing, DecodePrivateKey, EncodePrivateKey, ObjectIdentifier, PrivateKeyInfo,
 };
-use ruma_common::serde::Base64;
+use ruma_common::{serde::Base64, SigningKeyAlgorithm, SigningKeyId};
 
-use crate::{signatures::Signature, Algorithm, Error, ParseError};
+use crate::{signatures::Signature, Error, ParseError};
 
 #[cfg(feature = "ring-compat")]
 mod compat;
@@ -156,9 +156,11 @@ impl Ed25519KeyPair {
 impl KeyPair for Ed25519KeyPair {
     fn sign(&self, message: &[u8]) -> Signature {
         Signature {
-            algorithm: Algorithm::Ed25519,
+            key_id: SigningKeyId::from_parts(
+                SigningKeyAlgorithm::Ed25519,
+                self.version.as_str().into(),
+            ),
             signature: self.signing_key.sign(message).to_bytes().to_vec(),
-            version: self.version.clone(),
         }
     }
 }

crates/ruma-signatures/src/lib.rs

@@ -44,7 +44,7 @@
 
 #![warn(missing_docs)]
 
-use ruma_common::serde::{AsRefStr, DisplayAsRefStr};
+pub use ruma_common::{IdParseError, SigningKeyAlgorithm};
 
 pub use self::{
     error::{Error, JsonError, ParseError, VerificationError},
@@ -63,48 +63,6 @@ mod keys;
 mod signatures;
 mod verification;
 
-/// The algorithm used for signing data.
-#[derive(Clone, Debug, Eq, Hash, PartialEq, AsRefStr, DisplayAsRefStr)]
-#[cfg_attr(not(feature = "unstable-exhaustive-types"), non_exhaustive)]
-#[ruma_enum(rename_all = "snake_case")]
-pub enum Algorithm {
-    /// The Ed25519 digital signature algorithm.
-    Ed25519,
-}
-
-/// Extract the algorithm and version from a key identifier.
-fn split_id(id: &str) -> Result<(Algorithm, String), Error> {
-    /// The length of a valid signature ID.
-    const SIGNATURE_ID_LENGTH: usize = 2;
-
-    let signature_id: Vec<&str> = id.split(':').collect();
-
-    let signature_id_length = signature_id.len();
-
-    if signature_id_length != SIGNATURE_ID_LENGTH {
-        return Err(Error::InvalidLength(signature_id_length));
-    }
-
-    let version = signature_id[1];
-
-    #[cfg(feature = "compat-signature-id")]
-    const EXTRA_ALLOWED: [u8; 3] = [b'_', b'+', b'/'];
-    #[cfg(not(feature = "compat-signature-id"))]
-    const EXTRA_ALLOWED: [u8; 1] = [b'_'];
-    if !version.bytes().all(|ch| ch.is_ascii_alphanumeric() || EXTRA_ALLOWED.contains(&ch)) {
-        return Err(Error::InvalidVersion(version.into()));
-    }
-
-    let algorithm_input = signature_id[0];
-
-    let algorithm = match algorithm_input {
-        "ed25519" => Algorithm::Ed25519,
-        algorithm => return Err(Error::UnsupportedAlgorithm(algorithm.into())),
-    };
-
-    Ok((algorithm, signature_id[1].to_owned()))
-}
-
 #[cfg(test)]
 mod tests {
     use std::collections::BTreeMap;

crates/ruma-signatures/src/signatures.rs

@@ -1,33 +1,24 @@
 //! Digital signatures and collections of signatures.
 
-use ruma_common::serde::{base64::Standard, Base64};
-
-use crate::{split_id, Algorithm, Error};
+use ruma_common::{
+    serde::{base64::Standard, Base64},
+    AnyKeyName, IdParseError, OwnedSigningKeyId, SigningKeyAlgorithm, SigningKeyId,
+};
 
 /// A digital signature.
 #[derive(Clone, Debug, Eq, Hash, PartialEq)]
 pub struct Signature {
-    /// The cryptographic algorithm that generated this signature.
-    pub(crate) algorithm: Algorithm,
+    /// The ID of the key used to generate this signature.
+    pub(crate) key_id: OwnedSigningKeyId<AnyKeyName>,
 
     /// The signature data.
     pub(crate) signature: Vec<u8>,
-
-    /// The "version" of the key identifier for the public key used to generate this signature.
-    pub(crate) version: String,
 }
 
 impl Signature {
     /// Creates a signature from raw bytes.
     ///
-    /// While a signature can be created directly using struct literal syntax, this constructor can
-    /// be used to automatically determine the algorithm and version from a key identifier in the
-    /// form *algorithm:version*, e.g. "ed25519:1".
-    ///
-    /// This constructor will ensure that the version does not contain characters that violate the
-    /// guidelines in the specification. Because it may be necessary to represent signatures with
-    /// versions that don't adhere to these guidelines, it's possible to simply use the struct
-    /// literal syntax to construct a `Signature` with an arbitrary key.
+    /// This constructor will ensure that the key ID has the correct `algorithm:key_name` format.
     ///
     /// # Parameters
     ///
@@ -38,18 +29,16 @@ impl Signature {
     ///
     /// Returns an error if:
     ///
-    /// * The key ID specifies an unknown algorithm.
     /// * The key ID is malformed.
-    /// * The key ID contains a version with invalid characters.
-    pub fn new(id: &str, bytes: &[u8]) -> Result<Self, Error> {
-        let (algorithm, version) = split_id(id)?;
+    pub fn new(id: &str, bytes: &[u8]) -> Result<Self, IdParseError> {
+        let key_id = SigningKeyId::<AnyKeyName>::parse(id)?;
 
-        Ok(Self { algorithm, signature: bytes.to_vec(), version })
+        Ok(Self { key_id: key_id.into(), signature: bytes.to_vec() })
     }
 
     /// The algorithm used to generate the signature.
-    pub fn algorithm(&self) -> &Algorithm {
-        &self.algorithm
+    pub fn algorithm(&self) -> SigningKeyAlgorithm {
+        self.key_id.algorithm()
     }
 
     /// The raw bytes of the signature.
@@ -67,7 +56,7 @@ impl Signature {
     /// The key identifier, a string containing the signature algorithm and the key "version"
     /// separated by a colon, e.g. "ed25519:1".
     pub fn id(&self) -> String {
-        format!("{}:{}", self.algorithm, self.version)
+        self.key_id.to_string()
     }
 
     /// The "version" of the key used for this signature.
@@ -75,31 +64,32 @@ impl Signature {
     /// Versions are used as an identifier to distinguish signatures generated from different keys
     /// but using the same algorithm on the same homeserver.
     pub fn version(&self) -> &str {
-        &self.version
+        self.key_id.key_name().as_ref()
     }
 }
 
 #[cfg(test)]
 mod tests {
+    use ruma_common::SigningKeyAlgorithm;
+
     use super::Signature;
 
     #[test]
     fn valid_key_id() {
-        Signature::new("ed25519:abcdef", &[]).unwrap();
+        let signature = Signature::new("ed25519:abcdef", &[]).unwrap();
+        assert_eq!(signature.algorithm(), SigningKeyAlgorithm::Ed25519);
+        assert_eq!(signature.version(), "abcdef");
     }
 
     #[test]
-    fn invalid_valid_key_id_length() {
-        Signature::new("ed25519:abcdef:123456", &[]).unwrap_err();
+    fn unknown_key_id_algorithm() {
+        let signature = Signature::new("foobar:abcdef", &[]).unwrap();
+        assert_eq!(signature.algorithm().as_str(), "foobar");
+        assert_eq!(signature.version(), "abcdef");
     }
 
     #[test]
-    fn invalid_key_id_version() {
-        Signature::new("ed25519:abc!def", &[]).unwrap_err();
-    }
-
-    #[test]
-    fn invalid_key_id_algorithm() {
-        Signature::new("foobar:abcdef", &[]).unwrap_err();
+    fn invalid_key_id_format() {
+        Signature::new("ed25519", &[]).unwrap_err();
     }
 }

crates/ruma/Cargo.toml

@@ -123,10 +123,12 @@ compat = [
     "compat-optional",
     "compat-unset-avatar",
     "compat-get-3pids",
-    "compat-signature-id",
     "compat-tag-info",
 ]
 
+# Allow IDs to exceed 255 bytes.
+compat-arbitrary-length-ids = ["ruma-common/compat-arbitrary-length-ids"]
+
 # Don't validate `ServerSigningKeyVersion`.
 compat-server-signing-key-version = ["ruma-common/compat-server-signing-key-version"]
 
@@ -162,9 +164,6 @@ compat-get-3pids = ["ruma-client-api?/compat-get-3pids"]
 # since that's what Synapse sends.
 compat-upload-signatures = ["ruma-client-api?/compat-upload-signatures"]
 
-# Allow extra characters in signature IDs not allowed in the specification.
-compat-signature-id = ["ruma-signatures?/compat-signature-id"]
-
 # Allow TagInfo to contain a stringified floating-point value for the `order` field.
 compat-tag-info = ["ruma-events?/compat-tag-info"]