fix(core): reject zero threshold, canonicalize did:key, document replay (#509)

crates/gitlawb-core/src/cert.rs

@@ -5,6 +5,28 @@
 //!
 //! The schema is frozen at v1. All fields are mandatory for forward compatibility.
 //! Nodes that receive a certificate with an unknown version MUST reject it.
+//!
+//! ## Replay
+//!
+//! A certificate proves *who* asserted a ref update and *what* they asserted.
+//! It does not prove the assertion is fresh. [`RefUpdateBody::seq`] and
+//! [`RefUpdateBody::nonce`] are inside the signed bytes (see
+//! [`RefUpdateBody::to_signing_bytes`]), so a verifier can trust the values,
+//! but deciding whether a nonce is being reused or a `seq` is going backwards
+//! needs state.
+//!
+//! This crate is deliberately stateless — it is embedded in both nodes and
+//! clients — so replay is **not** enforced here and cannot be. Ownership of
+//! the check sits with the verifier that accepts and persists certificates,
+//! which must:
+//!
+//! - reject a `nonce` it has already accepted for the same `(repo, ref_name)`;
+//! - reject a `seq` that does not advance past the last accepted `seq` for
+//!   that `(repo, ref_name)`.
+//!
+//! Until such a verifier exists, replay of ref-update certificates is not
+//! enforced anywhere in this workspace. This contract exists so the first one
+//! inherits the requirement instead of re-deriving it.
 
 use std::collections::HashSet;
 
@@ -41,11 +63,19 @@ pub struct RefUpdateBody {
     pub from: String,
     /// The new commit hash (SHA-256 hex).
     pub to: String,
-    /// Monotonically increasing sequence number. Prevents replay.
+    /// Monotonically increasing sequence number, scoped to `(repo, ref_name)`.
+    ///
+    /// Signed, but not enforced by this crate: core holds no state. The
+    /// verifier that persists certificates must reject a `seq` that does not
+    /// advance for the same repo and ref. See the module docs on replay.
     pub seq: u64,
     /// RFC 3339 timestamp of this update.
     pub timestamp: DateTime<Utc>,
-    /// Random nonce for deduplication.
+    /// Random nonce, unique per certificate.
+    ///
+    /// Signed, but not enforced by this crate: core holds no state. The
+    /// verifier that persists certificates must reject a nonce it has already
+    /// accepted for the same repo and ref. See the module docs on replay.
     pub nonce: String,
 }
 
@@ -140,7 +170,16 @@ impl RefUpdateCert {
     ///
     /// Counts distinct signer DIDs, not signature entries: a repeated
     /// signature from the same maintainer counts once.
+    ///
+    /// `threshold` must be at least 1. Zero is rejected instead of being
+    /// treated as "no signatures required": a caller that derives the
+    /// threshold from configuration and gets 0 would otherwise accept any
+    /// certificate, whether its signatures verify or not.
     pub fn satisfies_threshold(&self, maintainers: &[Did], threshold: usize) -> Result<bool> {
+        if threshold == 0 {
+            return Err(Error::RefCert("threshold must be at least 1".to_string()));
+        }
+
         let valid = self.verify_all()?;
         let distinct_signers: HashSet<&Did> =
             valid.iter().filter(|d| maintainers.contains(d)).collect();
@@ -378,7 +417,7 @@ mod tests {
     }
 
     #[test]
-    fn threshold_zero_is_always_satisfied() {
+    fn threshold_zero_is_rejected() {
         let kp = Keypair::generate();
         let cert = RefUpdateCert::new(
             kp.did(),
@@ -389,6 +428,120 @@ mod tests {
             &kp,
         )
         .unwrap();
-        assert!(cert.satisfies_threshold(&[], 0).unwrap());
+
+        // Zero must never read as "no signatures required", even with an empty
+        // maintainer set.
+        let err = cert.satisfies_threshold(&[], 0).unwrap_err();
+        assert!(
+            err.to_string().contains("threshold must be at least 1"),
+            "zero must be refused as an invalid threshold: {err}"
+        );
+    }
+
+    /// The threshold is checked before any signature work, so a zero is
+    /// reported as a threshold error even when the certificate's signatures
+    /// are also invalid. Otherwise a broken certificate would let a caller
+    /// mistake the failure for a legitimate verification result.
+    #[test]
+    fn threshold_zero_is_rejected_before_signature_verification() {
+        let kp = Keypair::generate();
+        let other = Keypair::generate();
+        let mut cert = RefUpdateCert::new(
+            kp.did(),
+            "refs/heads/main".to_string(),
+            dummy_hash('0'),
+            dummy_hash('a'),
+            1,
+            &kp,
+        )
+        .unwrap();
+        cert.signatures[0].sig = other.sign_b64(b"unrelated bytes");
+
+        let err = cert.satisfies_threshold(&[kp.did()], 0).unwrap_err();
+        assert!(
+            err.to_string().contains("threshold must be at least 1"),
+            "the invalid threshold must win over the invalid signature: {err}"
+        );
+    }
+
+    /// `seq` and `nonce` are the verifier's replay inputs, so they have to be
+    /// inside the signed body; tapping either one must invalidate the
+    /// certificate rather than leave it verifying with different values.
+    #[test]
+    fn replay_fields_are_covered_by_the_signature() {
+        let kp = Keypair::generate();
+        let cert = RefUpdateCert::new(
+            kp.did(),
+            "refs/heads/main".to_string(),
+            dummy_hash('0'),
+            dummy_hash('a'),
+            7,
+            &kp,
+        )
+        .unwrap();
+        cert.verify_all().expect("baseline must verify");
+
+        let mut bumped = cert.clone();
+        bumped.body.seq = 8;
+        assert!(
+            matches!(bumped.verify_all(), Err(Error::SignatureInvalid)),
+            "changing seq must invalidate the signature"
+        );
+
+        let mut renamed = cert.clone();
+        renamed.body.nonce = "different-nonce".to_string();
+        assert!(
+            matches!(renamed.verify_all(), Err(Error::SignatureInvalid)),
+            "changing nonce must invalidate the signature"
+        );
+    }
+
+    /// A signature minted by a real key must not verify when the signer is
+    /// spelled in another multibase alphabet. Otherwise the same identity
+    /// would satisfy a maintainer allowlist under several DID strings.
+    ///
+    /// Base64url is deliberate: its 47-character method-id fits under the
+    /// pre-existing length bound, so this test reaches the canonical-prefix
+    /// rule and fails on the pre-fix code instead of being refused for length.
+    #[test]
+    fn verify_all_rejects_a_non_canonical_did_spelling() {
+        let kp = Keypair::generate();
+        let mut cert = RefUpdateCert::new(
+            kp.did(),
+            "refs/heads/main".to_string(),
+            dummy_hash('0'),
+            dummy_hash('a'),
+            1,
+            &kp,
+        )
+        .unwrap();
+
+        let mut prefixed = vec![0xed, 0x01];
+        prefixed.extend_from_slice(&kp.verifying_key().to_bytes());
+        cert.signatures[0].signer = format!(
+            "did:key:{}",
+            multibase::encode(multibase::Base::Base64Url, &prefixed)
+        )
+        .parse()
+        .expect("method is still `key`");
+
+        let err = cert
+            .verify_all()
+            .expect_err("a non-canonical did:key spelling must not resolve");
+        assert!(
+            err.to_string().contains("base58btc"),
+            "rejection must come from the canonical-prefix rule, not the length guard: {err}"
+        );
+        assert!(
+            !matches!(
+                cert.satisfies_threshold(&[cert.signatures[0].signer.clone()], 1),
+                Ok(true)
+            ),
+            "the alternate spelling must not satisfy a threshold"
+        );
+
+        // The canonical spelling of the same key is unaffected.
+        cert.signatures[0].signer = kp.did();
+        assert!(cert.satisfies_threshold(&[kp.did()], 1).unwrap());
     }
 }

crates/gitlawb-core/src/did.rs

@@ -20,6 +20,19 @@ use crate::{Error, Result};
 /// Value: 0xed01 encoded as a varint.
 const ED25519_MULTICODEC: &[u8] = &[0xed, 0x01];
 
+/// The only multibase prefix a `did:key` method-specific id may carry.
+///
+/// The DID:key spec encodes Ed25519 keys as base58btc, which is multibase
+/// `z`. `multibase::decode` happily accepts every other alphabet, and each
+/// spelling of the same key bytes would produce a distinct DID string while
+/// gates, allowlists, and database rows compare DIDs as strings — one
+/// identity would read as several. Rejecting the alternatives here keeps the
+/// mapping from `did:key` to key single-valued for every caller of
+/// [`Did::to_verifying_key`] (HTTP signatures, UCAN, ref certs, peer
+/// announces). `gitlawb-attest` enforces the same rule for attestation
+/// signers.
+const DID_KEY_MULTIBASE_PREFIX: char = 'z';
+
 /// A Decentralized Identifier.
 #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
 #[serde(transparent)]
@@ -71,7 +84,13 @@ impl Did {
 
     /// Resolve the Ed25519 verifying key from a `did:key`.
     ///
-    /// Returns `Err` if the DID is not a `did:key` or the key bytes are invalid.
+    /// Only the canonical base58btc (`z`) spelling is accepted. Other
+    /// multibase encodings decode to the same key but spell a different DID
+    /// string, so accepting them would let one identity appear under several
+    /// identifiers to string-comparing gates and allowlists.
+    ///
+    /// Returns `Err` if the DID is not a `did:key`, uses a non-canonical
+    /// multibase prefix, or the key bytes are invalid.
     pub fn to_verifying_key(&self) -> Result<VerifyingKey> {
         if !self.is_did_key() {
             return Err(Error::InvalidDid(format!(
@@ -93,9 +112,27 @@ impl Did {
             ));
         }
 
-        let (_, bytes) =
+        // Cheap prefix gate before the decode, then the base check on what
+        // multibase actually parsed. The first keeps a clearly non-canonical id
+        // off the decoder. The second is belt-and-braces — multibase maps a
+        // leading `z` only to `Base58Btc` — but it states the requirement
+        // directly instead of relying on that mapping, and `gitlawb-attest`
+        // makes the same pair of checks.
+        if !self.method_id().starts_with(DID_KEY_MULTIBASE_PREFIX) {
+            return Err(Error::InvalidDid(
+                "did:key must use base58btc (z-prefix)".to_string(),
+            ));
+        }
+
+        let (base, bytes) =
             multibase::decode(self.method_id()).map_err(|e| Error::InvalidDid(e.to_string()))?;
 
+        if base != Base::Base58Btc {
+            return Err(Error::InvalidDid(
+                "did:key must use base58btc (z-prefix)".to_string(),
+            ));
+        }
+
         if !bytes.starts_with(ED25519_MULTICODEC) {
             return Err(Error::InvalidDid(
                 "not an ed25519 multicodec key".to_string(),
@@ -369,4 +406,122 @@ mod tests {
         );
         did.to_verifying_key().expect("a real did:key must resolve");
     }
+
+    /// `multibase::decode` accepts every alphabet, so the same Ed25519 key can
+    /// be spelled as several `did:key` strings. Only base58btc is canonical;
+    /// the rest must be refused so one identity cannot appear as several to
+    /// gates and allowlists that compare DID strings.
+    #[test]
+    fn to_verifying_key_rejects_non_base58btc_multibase() {
+        use ed25519_dalek::SigningKey;
+        let signing = SigningKey::from_bytes(&[9u8; 32]);
+        let mut prefixed = ED25519_MULTICODEC.to_vec();
+        prefixed.extend_from_slice(&signing.verifying_key().to_bytes());
+
+        let canonical = Did::from_verifying_key(&signing.verifying_key());
+        assert_eq!(
+            canonical.to_verifying_key().unwrap().to_bytes(),
+            signing.verifying_key().to_bytes(),
+            "base58btc is the accepted spelling"
+        );
+
+        // These spellings fit inside the method-id bound, so they reach the
+        // canonical-prefix check and must be refused there.
+        for base in [Base::Base64Url, Base::Base32Lower] {
+            let alternative = format!("did:key:{}", multibase::encode(base, &prefixed));
+            let did: Did = alternative.parse().expect("method is still `key`");
+            assert_ne!(
+                did.as_str(),
+                canonical.as_str(),
+                "the same key must not compare equal under another spelling"
+            );
+
+            let err = did
+                .to_verifying_key()
+                .expect_err("non-canonical multibase must be refused");
+            assert!(
+                matches!(err, Error::InvalidDid(_)),
+                "must be an InvalidDid, got {err:?}"
+            );
+            assert!(
+                err.to_string().contains("base58btc"),
+                "the error must name the canonical encoding: {err}"
+            );
+        }
+
+        // A base16 spelling of the same key is longer than the method-id
+        // bound, so it is refused by the length guard before the prefix check.
+        // Either way it must not resolve.
+        for base in [Base::Base16Lower, Base::Base16Upper] {
+            let alternative = format!("did:key:{}", multibase::encode(base, &prefixed));
+            let did: Did = alternative.parse().expect("method is still `key`");
+            assert!(
+                matches!(did.to_verifying_key(), Err(Error::InvalidDid(_))),
+                "a base16 spelling of a real key must not resolve"
+            );
+        }
+    }
+
+    /// base58flickr (`Z`) is the non-bitcoin sibling alphabet. Its spelling of
+    /// a real key fits under the length bound, so it must be refused for the
+    /// prefix, not merely for its size.
+    #[test]
+    fn to_verifying_key_rejects_a_base58flickr_spelling() {
+        use ed25519_dalek::SigningKey;
+        let signing = SigningKey::from_bytes(&[11u8; 32]);
+        let mut prefixed = ED25519_MULTICODEC.to_vec();
+        prefixed.extend_from_slice(&signing.verifying_key().to_bytes());
+
+        let did: Did = format!(
+            "did:key:{}",
+            multibase::encode(Base::Base58Flickr, &prefixed)
+        )
+        .parse()
+        .expect("method is still `key`");
+
+        let err = did
+            .to_verifying_key()
+            .expect_err("base58flickr must be refused");
+        assert!(
+            err.to_string().contains("base58btc"),
+            "the error must name the canonical encoding: {err}"
+        );
+    }
+
+    /// An empty method-specific id must be refused by the prefix rule without
+    /// panicking on a missing first character.
+    #[test]
+    fn to_verifying_key_rejects_an_empty_method_id() {
+        let did: Did = "did:key:".parse().expect("method is still `key`");
+        let err = did
+            .to_verifying_key()
+            .expect_err("an empty method id must be refused");
+        assert!(
+            err.to_string().contains("base58btc"),
+            "the error must name the canonical encoding: {err}"
+        );
+    }
+
+    /// A canonical base58btc spelling is not enough on its own: the bytes must
+    /// also carry the ed25519-pub multicodec. This pins the check that stops a
+    /// key of another type from resolving as an Ed25519 key.
+    #[test]
+    fn to_verifying_key_rejects_base58btc_with_a_wrong_multicodec() {
+        use ed25519_dalek::SigningKey;
+        let signing = SigningKey::from_bytes(&[12u8; 32]);
+        let mut buf = vec![0x12, 0x00]; // sha2-256, not ed25519-pub
+        buf.extend_from_slice(&signing.verifying_key().to_bytes());
+
+        let did: Did = format!("did:key:{}", multibase::encode(Base::Base58Btc, &buf))
+            .parse()
+            .expect("method is still `key`");
+
+        let err = did
+            .to_verifying_key()
+            .expect_err("a non-ed25519 multicodec must be refused");
+        assert!(
+            err.to_string().contains("ed25519"),
+            "the error must name the expected multicodec: {err}"
+        );
+    }
 }