From 2f826d569305ef491f6d686e85ac9fe49e2e5fc8 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Wed, 28 Oct 2026 14:38:10 +0600 Subject: [PATCH] fix(core): reject zero threshold, canonicalize did:key, document replay (#509) --- crates/gitlawb-core/src/cert.rs | 161 +++++++++++++++++++++++++++++++- crates/gitlawb-core/src/did.rs | 159 ++++++++++++++++++++++++++++++- 2 files changed, 314 insertions(+), 6 deletions(-) diff --git a/crates/gitlawb-core/src/cert.rs b/crates/gitlawb-core/src/cert.rs index 5f30ce9c..74f907c0 100644 --- a/crates/gitlawb-core/src/cert.rs +++ b/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, - /// 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 { + 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()); } } diff --git a/crates/gitlawb-core/src/did.rs b/crates/gitlawb-core/src/did.rs index e3775845..427e58b4 100644 --- a/crates/gitlawb-core/src/did.rs +++ b/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 { 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}" + ); + } }