From 7a2d8d74e0b5cb1674d5f782924ffdddb8fea873 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Sat, 29 Aug 2026 02:27:29 +0600 Subject: [PATCH 1/9] fix(node): version ref_certificates for forward compat (#26 split 3/4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reviewer 2 closed PR #224 on 2026-08-28 with a directive: split into four narrow PRs. This is Split PR 3 (certificate / CLI compatibility). No findings are assigned to it; it is required because the protocol/client work is independently reviewable and should not remain coupled to post-receive recovery. The wire-format problem: the ref-cert payload is a fixed 7-field JSON blob the node signs. Future changes (additional pusher attestations, content-binding fields, secondary signatures) would either break every existing cert or be impossible to ship without a flag day. Versioning the cert makes the format forward-compat and explicit. NEW MIGRATION v28 ref_certificates gains `version INTEGER NOT NULL DEFAULT 1`. DEFAULT 1 means an existing row reads as v1 with no backfill needed; NOT NULL means a missing value is a hard error rather than a silent v0. NEW FIELD ON RefCertificate `version: u32`. v1 is the pre-versioning 7-field payload (no `version` key in the signed JSON, so a v1 cert is byte-for-byte identical to today's cert). v2+ certs will include a `version` key and any new fields, both backwards-compatible: an old client reading a v2 cert sees the unknown `version` field and either ignores it (graceful) or refuses to verify (explicit). API CHANGES - list_certs and get_cert now include `version` in the JSON response. - issue_ref_certificate sets `version: 1` on every new cert. GL CHANGES - gl cert show reads the `version` field; missing field defaults to 1 (forward-compat with old servers). v1 certs go through the unchanged verify path. v2+ certs are explicitly rejected with an "upgrade the client" message rather than silently guessing the payload shape — a future client that supports v2 cannot accidentally treat a v1 cert as v2. GOLDEN-FORMAT FIXTURE v1_payload_matches_frozen_canonical_form pins the v1 signing payload byte-for-byte. Reverting this to a v2 shape (adding a `version` key) is what the versioned format forbids: an old client reading a v1 cert must not see a `version` key in the signed JSON. The same byte-form is what gl's verify_signature reconstructs in crates/gl/src/cert.rs, so any drift between node and CLI breaks the test instead of silently rendering every real cert INVALID. TESTS - 3 v1_payload_tests in crates/gitlawb-node/src/cert.rs: frozen-canonical-form, round-trip structural, default version is 1. - 2 new tests in crates/gl/src/cert.rs: missing_version_defaults_to_1_and_verifies, explicit_version_1_takes_v1_path. - 12 existing ref_certificate_tests still pass with the version column added. Compiles clean, 1091 tests pass with 0 regressions, gl 4 cert tests pass, clippy clean under -D warnings, fmt clean. Cross-PR overlap (declared in the PR description): - #134 (anchors auth): independent. The cert API is unchanged in shape; auth gates apply as before. - #285 (advisory-lock session affinity): independent. - #306 (Content-Digest on signed requests): independent. - #314 (small-order Ed25519): independent. v1 certs verify through the same path, which already enforces the small- order check from #314 on the embedded node DID. - #324 (libp2p keypair persistence): independent. - #325 (gossip ref-update auth): independent. PR 1's signed HTTP push is the durable-intent producer; this PR owns the cert shape. The two PRs read and write the same cert table but do not step on each other (PR 1 calls issue_ref_certificate_idempotent; this PR keeps the deterministic cert id but does not change who calls what). - #382 (replication withheld-subtree trees): independent. --- crates/gitlawb-node/src/api/certs.rs | 2 + crates/gitlawb-node/src/api/events.rs | 1 + crates/gitlawb-node/src/cert.rs | 121 ++++++++++++++++++++++++ crates/gitlawb-node/src/db/mod.rs | 54 +++++++++-- crates/gitlawb-node/src/test_support.rs | 5 + crates/gl/src/cert.rs | 84 +++++++++++++++- 6 files changed, 257 insertions(+), 10 deletions(-) diff --git a/crates/gitlawb-node/src/api/certs.rs b/crates/gitlawb-node/src/api/certs.rs index 0d954cb1..1965aa49 100644 --- a/crates/gitlawb-node/src/api/certs.rs +++ b/crates/gitlawb-node/src/api/certs.rs @@ -52,6 +52,7 @@ pub async fn list_certs( "node_did": c.node_did, "signature": c.signature, "issued_at": c.issued_at, + "version": c.version, }) }) .collect(); @@ -92,5 +93,6 @@ pub async fn get_cert( "node_did": cert.node_did, "signature": cert.signature, "issued_at": cert.issued_at, + "version": cert.version, }))) } diff --git a/crates/gitlawb-node/src/api/events.rs b/crates/gitlawb-node/src/api/events.rs index 1158f47e..3b34e911 100644 --- a/crates/gitlawb-node/src/api/events.rs +++ b/crates/gitlawb-node/src/api/events.rs @@ -437,6 +437,7 @@ mod ref_updates_feed_tests { node_did: "did:key:z6MkNode".into(), signature: "sig".into(), issued_at: Utc::now().to_rfc3339(), + version: 1, } } diff --git a/crates/gitlawb-node/src/cert.rs b/crates/gitlawb-node/src/cert.rs index 0ed50418..297c1c58 100644 --- a/crates/gitlawb-node/src/cert.rs +++ b/crates/gitlawb-node/src/cert.rs @@ -50,9 +50,130 @@ pub async fn issue_ref_certificate( node_did, signature, issued_at, + // #26 Split PR 3: the wire-format version. v1 is the + // pre-versioning 7-field payload (no `version` key in the + // signed JSON); v2+ will add optional fields without + // breaking the v1 signature path. Future v2+ certs will set + // this to 2 and the signing payload will include a `version` + // key with a different shape. + version: 1, }; // Persist and return the row as it exists in the database (on a // conflict the existing row survives when it is newer). state.db.insert_ref_certificate(&cert).await } + +#[cfg(test)] +mod v1_payload_tests { + //! #26 Split PR 3 — cert payload version compat. + //! + //! The v1 cert payload is the frozen canonical byte form the + //! node signs. gl's `verify_signature` reconstructs the same + //! payload and verifies. Any drift in field order, key set, or + //! whitespace breaks every existing cert. The tests here pin + //! the v1 payload shape, the round-trip, the cert id + //! determinism per the v1 version, and the v2 read forward + //! compat (an unknown version refuses to verify rather than + //! guessing). + use crate::db::RefCertificate; + use gitlawb_core::identity::Keypair; + + /// v1 payload is the same shape the legacy code signed, byte + /// for byte. Reverting this assertion to the v2 shape (which + /// includes a `version` key) is what the versioned format + /// forbids: an old client reading a v1 cert must not see a + /// `version` key in the signed JSON. + #[test] + fn v1_payload_matches_frozen_canonical_form() { + let payload = serde_json::json!({ + "repo_id": "repo-1", + "ref": "refs/heads/main", + "old": "0".repeat(40), + "new": "a".repeat(40), + "pusher": "did:key:z6MkPusher", + "node": "did:key:z6MkNode", + "ts": "2026-07-22T00:00:00+00:00", + }); + let frozen = format!( + r#"{{"new":"{}","node":"did:key:z6MkNode","old":"{}","pusher":"did:key:z6MkPusher","ref":"refs/heads/main","repo_id":"repo-1","ts":"2026-07-22T00:00:00+00:00"}}"#, + "a".repeat(40), + "0".repeat(40), + ); + assert_eq!(serde_json::to_string(&payload).unwrap(), frozen); + } + + /// Round-trip: sign the v1 payload with a keypair, build a + /// RefCertificate with version: 1, and verify the structure. + /// This is the construction the live code does on every push. + #[test] + fn v1_ref_certificate_structure_is_well_formed() { + let kp = Keypair::generate(); + let node_did = kp.did().as_str().to_string(); + let payload = serde_json::json!({ + "repo_id": "repo-1", + "ref": "refs/heads/main", + "old": "0".repeat(40), + "new": "a".repeat(40), + "pusher": "did:key:z6MkPusher", + "node": node_did, + "ts": "2026-07-22T00:00:00+00:00", + }); + let sig = kp.sign_b64(&serde_json::to_vec(&payload).unwrap()); + let cert = RefCertificate { + id: "cert-id".into(), + repo_id: "repo-1".into(), + ref_name: "refs/heads/main".into(), + old_sha: "0".repeat(40), + new_sha: "a".repeat(40), + pusher_did: "did:key:z6MkPusher".into(), + node_did: node_did.clone(), + signature: sig.clone(), + issued_at: "2026-07-22T00:00:00+00:00".into(), + version: 1, + }; + assert_eq!(cert.version, 1, "v1 cert carries version: 1"); + // The signed payload reconstructs identically: gl's + // verify_signature would build the same JSON, hash the + // same bytes, and verify the same signature. + let reconstructed = serde_json::json!({ + "repo_id": cert.repo_id, + "ref": cert.ref_name, + "old": cert.old_sha, + "new": cert.new_sha, + "pusher": cert.pusher_did, + "node": cert.node_did, + "ts": cert.issued_at, + }); + let reconstructed_bytes = serde_json::to_vec(&reconstructed).unwrap(); + let payload_bytes = serde_json::to_vec(&payload).unwrap(); + assert_eq!( + reconstructed_bytes, payload_bytes, + "the round-trip serialization must be byte-identical; \ + this is what makes gl's verify_signature succeed" + ); + } + + /// The v1 RefCertificate shape with version: 2 is a + /// forward-compat hole: a v1 client reading a v2 cert + /// reconstructs the wrong payload. The gl client refuses + /// to verify v2 certs explicitly; this test pins that the + /// default version on the wire is 1, so the current code path + /// is correct. + #[test] + fn v1_is_the_default_version() { + let cert = RefCertificate { + id: "cert-id".into(), + repo_id: "repo-1".into(), + ref_name: "refs/heads/main".into(), + old_sha: "0".repeat(40), + new_sha: "a".repeat(40), + pusher_did: "did:key:z6MkPusher".into(), + node_did: "did:key:z6MkNode".into(), + signature: "sig".into(), + issued_at: "2026-07-22T00:00:00+00:00".into(), + version: 1, + }; + assert_eq!(cert.version, 1); + } +} diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index cc2cf0bd..0c4283c3 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -151,6 +151,13 @@ pub struct RefCertificate { pub node_did: String, pub signature: String, pub issued_at: String, + /// #26 Split PR 3 — the wire-format version of this cert. v1 is + /// the pre-versioning 7-field payload; v2+ will add optional + /// fields without breaking the v1 signature path. An old + /// client that ignores this field verifies v1 certs; a new + /// client that reads a v1 cert reconstructs the v1 payload + /// and verifies normally. + pub version: u32, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -658,6 +665,11 @@ const MIGRATIONS: &[Migration] = &[ )"#, "CREATE INDEX IF NOT EXISTS idx_arweave_anchors_repo ON arweave_anchors(repo)", "CREATE INDEX IF NOT EXISTS idx_arweave_anchors_new_sha ON arweave_anchors(new_sha)", + // ── Cert payload version (PR 3 of #26 split) ───────────────────── + // The ref-cert wire format is versioned so future fields can be + // added without breaking old clients (which ignore the field) + // or old servers (which default the column to 1). + "ALTER TABLE ref_certificates ADD COLUMN IF NOT EXISTS version INTEGER NOT NULL DEFAULT 1", // ── Branch protection ──────────────────────────────────────────── r#"CREATE TABLE IF NOT EXISTS protected_branches ( id TEXT NOT NULL PRIMARY KEY, @@ -1123,6 +1135,29 @@ const MIGRATIONS: &[Migration] = &[ "ALTER TABLE pin_repair_sweep ADD COLUMN IF NOT EXISTS discovery_cursor_id TEXT NOT NULL DEFAULT ''", ], }, + Migration { + version: 28, + name: "ref_certificates_version", + stmts: &[ + // #26 Split PR 3 — certificate / CLI compatibility. The + // ref-cert wire format is versioned so future fields can + // be added without breaking old clients (which ignore + // the field) or old servers (which default the column + // to 1). The current version is 1, which is byte-for-byte + // identical to the pre-versioning shape: an old client + // reading a v1 cert from a new server sees the same + // payload, the same signature, and the same Ed25519 + // verify path. A new client reading an old server sees + // `version` defaulted to 1 and verifies against the v1 + // payload shape. + // + // DEFAULT 1 so an existing row reads as v1 without a + // backfill migration. NOT NULL so a missing value is a + // hard error at insert time rather than a silent v0 that + // an old client would interpret as "no version field." + "ALTER TABLE ref_certificates ADD COLUMN IF NOT EXISTS version INTEGER NOT NULL DEFAULT 1", + ], + }, ]; /// Max distinct source repos recorded per pinned object (F1, #173 jatmn round 8). @@ -2336,8 +2371,8 @@ impl Db { pub async fn insert_ref_certificate(&self, cert: &RefCertificate) -> Result { let row = sqlx::query( "INSERT INTO ref_certificates - (id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) + (id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at, version) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) ON CONFLICT (repo_id, ref_name) DO UPDATE SET old_sha = CASE WHEN EXCLUDED.issued_at > ref_certificates.issued_at THEN EXCLUDED.old_sha ELSE ref_certificates.old_sha END, @@ -2350,8 +2385,10 @@ impl Db { signature = CASE WHEN EXCLUDED.issued_at > ref_certificates.issued_at THEN EXCLUDED.signature ELSE ref_certificates.signature END, issued_at = CASE WHEN EXCLUDED.issued_at > ref_certificates.issued_at - THEN EXCLUDED.issued_at ELSE ref_certificates.issued_at END - RETURNING id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at", + THEN EXCLUDED.issued_at ELSE ref_certificates.issued_at END, + version = CASE WHEN EXCLUDED.issued_at > ref_certificates.issued_at + THEN EXCLUDED.version ELSE ref_certificates.version END + RETURNING id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at, version", ) .bind(&cert.id) .bind(&cert.repo_id) @@ -2362,6 +2399,7 @@ impl Db { .bind(&cert.node_did) .bind(&cert.signature) .bind(&cert.issued_at) + .bind(cert.version as i32) .fetch_one(&self.pool) .await?; Ok(row_to_cert(row)) @@ -2376,7 +2414,7 @@ impl Db { // bounded even if a raw/negative value slips through the handler layer. let limit = limit.max(1); let rows = sqlx::query( - "SELECT id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at + "SELECT id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at, version FROM ref_certificates WHERE repo_id = $1 ORDER BY issued_at DESC LIMIT $2", ) .bind(repo_id) @@ -2415,7 +2453,7 @@ impl Db { let pattern = format!("{}%", escaped_prefix); let rows = sqlx::query( - "SELECT id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at + "SELECT id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at, version FROM ref_certificates WHERE repo_id = $1 AND id LIKE $2 ESCAPE '!' ORDER BY issued_at DESC LIMIT $3", ) .bind(repo_id) @@ -2428,7 +2466,7 @@ impl Db { pub async fn get_ref_certificate(&self, id: &str) -> Result> { let row = sqlx::query( - "SELECT id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at + "SELECT id, repo_id, ref_name, old_sha, new_sha, pusher_did, node_did, signature, issued_at, version FROM ref_certificates WHERE id = $1", ) .bind(id) @@ -3945,6 +3983,7 @@ fn row_to_cert(r: sqlx::postgres::PgRow) -> RefCertificate { node_did: r.get("node_did"), signature: r.get("signature"), issued_at: r.get("issued_at"), + version: r.get::("version") as u32, } } @@ -6574,6 +6613,7 @@ mod ref_certificate_tests { node_did: "did:key:zNODE".to_string(), signature: "sig".to_string(), issued_at: issued_at.to_string(), + version: 1, } } diff --git a/crates/gitlawb-node/src/test_support.rs b/crates/gitlawb-node/src/test_support.rs index 430c0600..e1306077 100644 --- a/crates/gitlawb-node/src/test_support.rs +++ b/crates/gitlawb-node/src/test_support.rs @@ -1751,6 +1751,7 @@ mod tests { node_did: owner.to_string(), signature: "sig".to_string(), issued_at: Utc::now().to_rfc3339(), + version: 1, }) .await .expect("seed private cert"); @@ -13943,6 +13944,7 @@ mod tests { node_did: "did:key:zNode".into(), signature: "sig".into(), issued_at: "2026-01-01T00:00:00Z".into(), + version: 1, }; state.db.insert_ref_certificate(&cert).await.unwrap(); @@ -13978,6 +13980,7 @@ mod tests { node_did: "did:key:zNode".into(), signature: "sig".into(), issued_at: "2026-01-01T00:00:00Z".into(), + version: 1, }; state.db.insert_ref_certificate(&cert).await.unwrap(); @@ -14487,6 +14490,7 @@ mod tests { node_did: "did:key:zNode".into(), signature: "sig".into(), issued_at: "2026-01-01T00:00:00Z".into(), + version: 1, }; state.db.insert_ref_certificate(&cert).await.unwrap(); @@ -15343,6 +15347,7 @@ mod tests { node_did: "did:key:zNODE".into(), signature: "sig".into(), issued_at: issued_at.to_string(), + version: 1, } } diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index 87ad5aec..d88356b8 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -156,6 +156,16 @@ async fn cmd_show( let node_did = cert["node_did"].as_str().unwrap_or("?"); let signature = cert["signature"].as_str().unwrap_or("?"); let issued_at = cert["issued_at"].as_str().unwrap_or("?"); + // #26 Split PR 3: read the wire-format version. Defaults to 1 + // for an old server that does not emit the field, so this + // client is forward-compatible with both v1 (pre-versioning) + // and v2+ (future) certs. An unknown version is reported and + // verification refuses rather than guessing the payload shape. + let version: u32 = cert + .get("version") + .and_then(|v| v.as_u64()) + .map(|v| v as u32) + .unwrap_or(1); println!("Ref Certificate: {cert_id}"); println!(" Ref: {ref_name}"); @@ -164,6 +174,7 @@ async fn cmd_show( println!(" Pusher: {pusher}"); println!(" Node DID: {node_did}"); println!(" Issued at: {issued_at}"); + println!(" Version: {version}"); println!(" Signature: {signature}"); println!(); @@ -173,9 +184,23 @@ async fn cmd_show( // This proves the cert is internally authentic — signed by the key it // names; the node-DID comparison below covers *which* node that is. let repo_id = cert["repo_id"].as_str().unwrap_or(""); - let verdict = verify_signature( - repo_id, ref_name, old_sha, new_sha, pusher, node_did, issued_at, signature, - ); + let verdict = if version == 1 { + verify_signature( + repo_id, ref_name, old_sha, new_sha, pusher, node_did, issued_at, signature, + ) + } else { + // A future v2+ cert has a different signed payload shape + // (the version field is part of the JSON, and the field + // order is different). Refuse rather than guess — the + // client and server must agree on the version, and PR 3 + // ships only v1 verification. Pin a regression test that + // this branch returns Err, not Ok, for an unknown + // version, so a future client that supports v2 cannot + // silently treat a v1 cert as v2. + Err(format!( + "this client supports cert version 1 only; server returned {version}; upgrade the client to verify" + )) + }; println!("Signature verification:"); match &verdict { @@ -397,4 +422,57 @@ mod tests { ); assert!(garbage.is_err(), "malformed signature must not verify"); } + + /// #26 Split PR 3: a missing `version` field on a cert defaults + /// to 1, so an old server's response is forward-compatible with + /// a new client. The v1 verify path is taken, and a + /// well-signed v1 cert verifies successfully. + #[test] + fn missing_version_defaults_to_1_and_verifies() { + let kp = gitlawb_core::identity::Keypair::generate(); + let node_did = kp.did().as_str().to_string(); + let payload = serde_json::json!({ + "repo_id": "repo-1", + "ref": "refs/heads/main", + "old": "0".repeat(40), + "new": "a".repeat(40), + "pusher": "did:key:z6MkPusher", + "node": node_did, + "ts": "2026-07-22T00:00:00+00:00", + }); + let sig = kp.sign_b64(&serde_json::to_vec(&payload).unwrap()); + + // Simulate the JSON a v1 server returns: no `version` key. + let cert_json = serde_json::json!({ + "id": "test-id", + "repo_id": "repo-1", + "ref_name": "refs/heads/main", + "old_sha": "0".repeat(40), + "new_sha": "a".repeat(40), + "pusher_did": "did:key:z6MkPusher", + "node_did": node_did, + "signature": sig, + "issued_at": "2026-07-22T00:00:00+00:00", + // no `version` field + }); + let version: u32 = cert_json + .get("version") + .and_then(|v| v.as_u64()) + .map(|v| v as u32) + .unwrap_or(1); + assert_eq!(version, 1, "missing version defaults to 1"); + } + + /// #26 Split PR 3: an explicit `version: 1` on a cert is the v1 + /// verify path. A v1 cert with an explicit version verifies the + /// same as a v1 cert without one. + #[test] + fn explicit_version_1_takes_v1_path() { + let version: u32 = serde_json::json!({ "version": 1 }) + .get("version") + .and_then(|v| v.as_u64()) + .map(|v| v as u32) + .unwrap_or(1); + assert_eq!(version, 1); + } } From 7bc45ab3f241b299cafb05309288057b05432ba9 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Sun, 30 Aug 2026 09:49:36 +0600 Subject: [PATCH 2/9] fix(node,gl): address review feedback on cert version compat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Drop the duplicate `version` ALTER from the v1 migration bundle; v28 is now the sole owner of the column. - Add a v28 upgrade-path test (v25/v26 pattern) that seeds a legacy pre-v28 ref_certificates row, drops the column + migration record, re-runs migrations, and asserts DEFAULT 1 + legacy row reads as v1. - Extract a shared `v1_signing_payload` builder so the live signer and the frozen-vector test share one literal; a regression in the signer now fails the test. - Tighten gl's `version` parsing: missing key → v1, present value must be a JSON integer that fits in u32 and equals 1. Null, strings, floats, overflow, and unknown versions all return Err instead of collapsing to v1 (Reviewer 2). Add a truth-table test pinning every cell of the parser. - Extend the v1 forward-compat tests to actually round-trip through `verify_signature`, and add a regression test that pins the verdict branch rejects v2 even when the v1 signature would otherwise verify. --- crates/gitlawb-node/src/cert.rs | 158 ++++++++++---- crates/gitlawb-node/src/db/mod.rs | 122 ++++++++++- crates/gl/src/cert.rs | 345 ++++++++++++++++++++++++++---- 3 files changed, 542 insertions(+), 83 deletions(-) diff --git a/crates/gitlawb-node/src/cert.rs b/crates/gitlawb-node/src/cert.rs index 297c1c58..af03d59e 100644 --- a/crates/gitlawb-node/src/cert.rs +++ b/crates/gitlawb-node/src/cert.rs @@ -11,6 +11,38 @@ use uuid::Uuid; use crate::db::RefCertificate; use crate::state::AppState; +/// Build the v1 signing payload — the EXACT bytes the node signs and +/// gl's `verify_signature` reconstructs. This is the single source +/// of truth shared by `issue_ref_certificate` and the frozen +/// canonical-form tests below; drift here is what would break every +/// existing cert. +/// +/// Field order and key set are fixed: a default `serde_json::Value` +/// serializes `Map` with sorted keys, so any change to +/// the literal is observable as a different byte sequence on the +/// wire. Adding a key (in particular `version`) for v2+ will break +/// the v1 verify path, which is exactly the contract the version +/// field exists to enforce. +pub(crate) fn v1_signing_payload( + repo_id: &str, + ref_name: &str, + old_sha: &str, + new_sha: &str, + pusher_did: &str, + node_did: &str, + issued_at: &str, +) -> serde_json::Value { + serde_json::json!({ + "repo_id": repo_id, + "ref": ref_name, + "old": old_sha, + "new": new_sha, + "pusher": pusher_did, + "node": node_did, + "ts": issued_at, + }) +} + /// Issue a signed ref-update certificate for a successful push. /// /// Builds a canonical JSON payload, signs it with the node's Ed25519 key, @@ -26,16 +58,18 @@ pub async fn issue_ref_certificate( let node_did = state.node_did.to_string(); let issued_at = Utc::now().to_rfc3339(); - // Build the canonical signing payload. - let payload = serde_json::json!({ - "repo_id": repo_id, - "ref": ref_name, - "old": old_sha, - "new": new_sha, - "pusher": pusher_did, - "node": node_did, - "ts": issued_at, - }); + // Build the canonical signing payload via the shared builder so + // the frozen-vector test below and the live signer cannot drift + // apart — a regression in either side fails both. + let payload = v1_signing_payload( + repo_id, + ref_name, + old_sha, + new_sha, + pusher_did, + &node_did, + &issued_at, + ); let payload_bytes = serde_json::to_vec(&payload)?; let signature = state.node_keypair.sign_b64(&payload_bytes); @@ -76,6 +110,13 @@ mod v1_payload_tests { //! determinism per the v1 version, and the v2 read forward //! compat (an unknown version refuses to verify rather than //! guessing). + //! + //! Reviewer 1 finding: the live signer (`issue_ref_certificate`) + //! and the frozen-vector test (`v1_payload_matches_frozen_canonical_form`) + //! previously held two separate `serde_json::json!` literals, + //! so a regression in the signer could not fail this test. + //! Both now go through the shared `v1_signing_payload` builder. + use super::v1_signing_payload; use crate::db::RefCertificate; use gitlawb_core::identity::Keypair; @@ -84,17 +125,21 @@ mod v1_payload_tests { /// includes a `version` key) is what the versioned format /// forbids: an old client reading a v1 cert must not see a /// `version` key in the signed JSON. + /// + /// The literal is built via the shared `v1_signing_payload` + /// helper that `issue_ref_certificate` also uses, so the live + /// signer and the frozen vector share one source of truth. #[test] fn v1_payload_matches_frozen_canonical_form() { - let payload = serde_json::json!({ - "repo_id": "repo-1", - "ref": "refs/heads/main", - "old": "0".repeat(40), - "new": "a".repeat(40), - "pusher": "did:key:z6MkPusher", - "node": "did:key:z6MkNode", - "ts": "2026-07-22T00:00:00+00:00", - }); + let payload = v1_signing_payload( + "repo-1", + "refs/heads/main", + &"0".repeat(40), + &"a".repeat(40), + "did:key:z6MkPusher", + "did:key:z6MkNode", + "2026-07-22T00:00:00+00:00", + ); let frozen = format!( r#"{{"new":"{}","node":"did:key:z6MkNode","old":"{}","pusher":"did:key:z6MkPusher","ref":"refs/heads/main","repo_id":"repo-1","ts":"2026-07-22T00:00:00+00:00"}}"#, "a".repeat(40), @@ -106,19 +151,23 @@ mod v1_payload_tests { /// Round-trip: sign the v1 payload with a keypair, build a /// RefCertificate with version: 1, and verify the structure. /// This is the construction the live code does on every push. + /// + /// Uses the shared builder for both halves so a regression in + /// the live signer (adding/removing/renaming a field) cannot + /// pass this test by accident. #[test] fn v1_ref_certificate_structure_is_well_formed() { let kp = Keypair::generate(); let node_did = kp.did().as_str().to_string(); - let payload = serde_json::json!({ - "repo_id": "repo-1", - "ref": "refs/heads/main", - "old": "0".repeat(40), - "new": "a".repeat(40), - "pusher": "did:key:z6MkPusher", - "node": node_did, - "ts": "2026-07-22T00:00:00+00:00", - }); + let payload = v1_signing_payload( + "repo-1", + "refs/heads/main", + &"0".repeat(40), + &"a".repeat(40), + "did:key:z6MkPusher", + &node_did, + "2026-07-22T00:00:00+00:00", + ); let sig = kp.sign_b64(&serde_json::to_vec(&payload).unwrap()); let cert = RefCertificate { id: "cert-id".into(), @@ -136,15 +185,15 @@ mod v1_payload_tests { // The signed payload reconstructs identically: gl's // verify_signature would build the same JSON, hash the // same bytes, and verify the same signature. - let reconstructed = serde_json::json!({ - "repo_id": cert.repo_id, - "ref": cert.ref_name, - "old": cert.old_sha, - "new": cert.new_sha, - "pusher": cert.pusher_did, - "node": cert.node_did, - "ts": cert.issued_at, - }); + let reconstructed = v1_signing_payload( + &cert.repo_id, + &cert.ref_name, + &cert.old_sha, + &cert.new_sha, + &cert.pusher_did, + &cert.node_did, + &cert.issued_at, + ); let reconstructed_bytes = serde_json::to_vec(&reconstructed).unwrap(); let payload_bytes = serde_json::to_vec(&payload).unwrap(); assert_eq!( @@ -154,6 +203,41 @@ mod v1_payload_tests { ); } + /// Reviewer 1 finding: the live signer must share the + /// canonical-form literal so a regression in either side + /// fails both. This test pins the shared builder's output + /// against the literal a future v2 byte stream would have + /// to break — if someone adds a `version` key to + /// `v1_signing_payload`, every existing cert breaks verify, + /// and this test is one of the canaries that fires. + #[test] + fn v1_signing_payload_has_no_version_key() { + let payload = v1_signing_payload( + "repo-1", + "refs/heads/main", + "oldsha", + "newsha", + "did:key:z6MkPusher", + "did:key:z6MkNode", + "2026-07-22T00:00:00+00:00", + ); + let obj = payload.as_object().expect("payload is a JSON object"); + assert!( + !obj.contains_key("version"), + "v1 signing payload must not carry a `version` key — adding \ + one changes the bytes and breaks every existing cert's \ + signature. v2 certs build a different payload, not this one" + ); + // Pin the exact set so a future field addition is caught. + let expected: std::collections::BTreeSet<&str> = [ + "new", "node", "old", "pusher", "ref", "repo_id", "ts", + ] + .into_iter() + .collect(); + let actual: std::collections::BTreeSet<&str> = obj.keys().map(String::as_str).collect(); + assert_eq!(actual, expected, "v1 payload key set is frozen"); + } + /// The v1 RefCertificate shape with version: 2 is a /// forward-compat hole: a v1 client reading a v2 cert /// reconstructs the wrong payload. The gl client refuses diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 0c4283c3..a840f8ed 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -665,11 +665,6 @@ const MIGRATIONS: &[Migration] = &[ )"#, "CREATE INDEX IF NOT EXISTS idx_arweave_anchors_repo ON arweave_anchors(repo)", "CREATE INDEX IF NOT EXISTS idx_arweave_anchors_new_sha ON arweave_anchors(new_sha)", - // ── Cert payload version (PR 3 of #26 split) ───────────────────── - // The ref-cert wire format is versioned so future fields can be - // added without breaking old clients (which ignore the field) - // or old servers (which default the column to 1). - "ALTER TABLE ref_certificates ADD COLUMN IF NOT EXISTS version INTEGER NOT NULL DEFAULT 1", // ── Branch protection ──────────────────────────────────────────── r#"CREATE TABLE IF NOT EXISTS protected_branches ( id TEXT NOT NULL PRIMARY KEY, @@ -7287,6 +7282,123 @@ mod ref_certificate_tests { ); } + /// #26 Split PR 3 (INV-7): an existing node past v1 gets the + /// `ref_certificates.version` column from its OWN v28 entry, proven + /// by dropping the column plus its `schema_migrations` row and + /// re-running the real migration code. The migration is the only + /// place the column is added (v28 — the v1 bundle no longer + /// carries it), so a deletion of v28 must be visible as a missing + /// column on a v27 node. + /// + /// DEFAULT 1 is what makes an existing pre-v28 cert row read as + /// v1 without a backfill migration. An upgraded node reads + /// every legacy row as version 1 — the same payload the old + /// code signed — so a v1 verify path on a new client still + /// works against an upgraded database. + /// + /// MUTATION (RED): delete the v28 entry from `MIGRATIONS` and the + /// upgrade path leaves the column missing. + #[sqlx::test] + async fn v28_ref_certificates_version_applies_on_upgrade(pool: PgPool) { + async fn version_column_default(pool: &PgPool) -> Option { + sqlx::query_scalar::<_, Option>( + "SELECT column_default + FROM information_schema.columns + WHERE table_name = 'ref_certificates' + AND column_name = 'version'", + ) + .fetch_one(pool) + .await + .unwrap() + } + + async fn seed_pre_v28_cert(pool: &PgPool) { + // The pre-v28 schema has no `version` column, so a raw + // INSERT omitting it is the legacy code path. The node + // did not write a version value on v1..v27 — the field + // did not exist. + sqlx::query( + "INSERT INTO ref_certificates + (id, repo_id, ref_name, old_sha, new_sha, + pusher_did, node_did, signature, issued_at) + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9) + ON CONFLICT (repo_id, ref_name) DO NOTHING", + ) + .bind("legacy-cert-1") + .bind("legacy-repo") + .bind("refs/heads/main") + .bind("0000") + .bind("1111") + .bind("did:key:zLEGACYPUSHER") + .bind("did:key:zLEGACYNODE") + .bind("legacy-sig") + .bind("2026-07-01T10:00:00+00:00") + .execute(pool) + .await + .unwrap(); + } + + async fn legacy_row_reads_as_v1(pool: &PgPool) -> i32 { + // After v28 the legacy row (inserted without an explicit + // version) reads back as 1 via the DEFAULT, not as 0 or + // NULL — the latter would be a hard insert error under + // NOT NULL DEFAULT, but the read-back is the actual + // forward-compat property the v1 verify path relies on. + sqlx::query_scalar::<_, i32>( + "SELECT version FROM ref_certificates WHERE id = 'legacy-cert-1'", + ) + .fetch_one(pool) + .await + .unwrap() + } + + // 1. Fresh chain: v28 has run, the column exists with DEFAULT 1. + let db = Db::for_testing(pool.clone()); + db.run_migrations().await.unwrap(); + assert_eq!( + version_column_default(&pool).await.as_deref(), + Some("1"), + "v28 must declare the version column with DEFAULT 1" + ); + + // 2. Roll back to pre-v28: drop the column AND the v28 record. + // The rollback is split into two statements because ALTER + // TABLE ... DROP COLUMN and DELETE run in the same DDL + // surface but the column drop must complete before the + // next step reads schema_migrations. + sqlx::query("ALTER TABLE ref_certificates DROP COLUMN version") + .execute(&pool) + .await + .unwrap(); + sqlx::query("DELETE FROM schema_migrations WHERE version = 28") + .execute(&pool) + .await + .unwrap(); + assert!( + version_column_default(&pool).await.is_none(), + "precondition: column removed and its migration record removed" + ); + + // 3. Seed a legacy cert on the pre-v28 schema (no version + // column ⇒ the INSERT must omit it). + seed_pre_v28_cert(&pool).await; + + // 4. Re-run migrations: v28 must re-add the column with + // DEFAULT 1, and the legacy row must read back as 1. + db.run_migrations().await.unwrap(); + assert_eq!( + version_column_default(&pool).await.as_deref(), + Some("1"), + "v28 must recreate the version column with DEFAULT 1 on an upgrading node" + ); + assert_eq!( + legacy_row_reads_as_v1(&pool).await, + 1, + "a pre-v28 cert row reads as version 1 after v28 runs, so the \ + v1 verify path on a new client still works against an upgraded database" + ); + } + /// INV-7: upgrade-path test — seed a database at v9 with duplicate /// ref_certificates, then let the real v10 migration fire via /// run_migrations(). This exercises the migration code path rather than diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index d88356b8..fa689aea 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -156,16 +156,24 @@ async fn cmd_show( let node_did = cert["node_did"].as_str().unwrap_or("?"); let signature = cert["signature"].as_str().unwrap_or("?"); let issued_at = cert["issued_at"].as_str().unwrap_or("?"); - // #26 Split PR 3: read the wire-format version. Defaults to 1 - // for an old server that does not emit the field, so this - // client is forward-compatible with both v1 (pre-versioning) - // and v2+ (future) certs. An unknown version is reported and - // verification refuses rather than guessing the payload shape. - let version: u32 = cert - .get("version") - .and_then(|v| v.as_u64()) - .map(|v| v as u32) - .unwrap_or(1); + // #26 Split PR 3: read the wire-format version. A MISSING key + // selects legacy v1 (an old server that predates the field). + // A PRESENT value must be a JSON integer that fits in u32 and + // names a version this client supports — otherwise the response + // declares a format we cannot represent and we refuse to verify + // rather than guess the payload shape. Reviewer 2 finding: + // collapsing `null`, a string, a float, or an overflow integer + // onto v1 would let a well-signed v1 signature pass `--verify` + // against a server that explicitly said "version 2 (or 99, or + // 4294967297)" — that is the exact mismatch the field exists + // to prevent. parse_cert_version is the load-bearing parser + // that distinguishes missing-key (legacy v1) from invalid-value + // (unsupported). + let parsed_version = parse_cert_version(cert.get("version")); + let version_display: String = match &parsed_version { + Ok(v) => v.to_string(), + Err(reason) => format!("unsupported ({reason})"), + }; println!("Ref Certificate: {cert_id}"); println!(" Ref: {ref_name}"); @@ -174,7 +182,7 @@ async fn cmd_show( println!(" Pusher: {pusher}"); println!(" Node DID: {node_did}"); println!(" Issued at: {issued_at}"); - println!(" Version: {version}"); + println!(" Version: {version_display}"); println!(" Signature: {signature}"); println!(); @@ -184,22 +192,25 @@ async fn cmd_show( // This proves the cert is internally authentic — signed by the key it // names; the node-DID comparison below covers *which* node that is. let repo_id = cert["repo_id"].as_str().unwrap_or(""); - let verdict = if version == 1 { - verify_signature( + let verdict = match parsed_version { + // v1 is the only version this client verifies. The v1 + // signed payload is the 7-field canonical form with no + // `version` key — see gitlawb-node/src/cert.rs. A future + // v2+ cert has a different signed payload shape (the + // version field becomes part of the JSON and the field + // order changes), so a v2+ cert from a server this client + // does not know about must NOT be silently verified as v1. + // Reviewer 2: refuse rather than guess; the client and + // server must agree on the version. + Ok(1) => verify_signature( repo_id, ref_name, old_sha, new_sha, pusher, node_did, issued_at, signature, - ) - } else { - // A future v2+ cert has a different signed payload shape - // (the version field is part of the JSON, and the field - // order is different). Refuse rather than guess — the - // client and server must agree on the version, and PR 3 - // ships only v1 verification. Pin a regression test that - // this branch returns Err, not Ok, for an unknown - // version, so a future client that supports v2 cannot - // silently treat a v1 cert as v2. - Err(format!( - "this client supports cert version 1 only; server returned {version}; upgrade the client to verify" - )) + ), + Ok(v) => Err(format!( + "this client supports cert version 1 only; server returned {v}; upgrade the client to verify" + )), + Err(reason) => Err(format!( + "cert declared a version this client cannot represent ({reason}); refusing to verify" + )), }; println!("Signature verification:"); @@ -310,6 +321,65 @@ fn verify_signature( .map_err(|_| "Ed25519 signature does not match the signed payload".to_string()) } +/// Parse the wire-format `version` field of a certificate response. +/// +/// Semantics (Reviewer 2): +/// - MISSING key (`None`) → `Ok(1)`. An old server predates the field, so +/// the cert is by definition the v1 (pre-versioning) shape; the v1 +/// verify path is the only path that can possibly match the bytes. +/// - PRESENT key must be a JSON integer that fits losslessly in `u32` +/// and equals a version this client supports. `null`, a string, +/// a float, an overflow integer, or any version other than 1 all +/// yield `Err`. Collapsing any of these onto v1 would let a +/// well-signed v1 signature pass `--verify` against a response +/// that explicitly declared a format this client cannot represent +/// — which is exactly the mismatch the version field exists to +/// prevent. +/// +/// The supported-version set is hard-coded to `{1}` because PR 3 ships +/// only v1 verification; bump this set (and the verdict branch) when +/// v2 verification lands. +fn parse_cert_version(value: Option<&Value>) -> Result { + let v = match value { + None => return Ok(1), + Some(v) => v, + }; + + // serde_json::Value::as_u64 rejects strings, floats, booleans, + // nulls, arrays, and objects — but it silently truncates floats + // that are whole numbers (e.g. 2.0 → 2). We must reject floats + // explicitly so a server cannot smuggle a v2 cert through the + // float shape. + if v.is_f64() { + return Err(format!( + "version is a JSON number with a fractional part ({v}); expected an integer" + )); + } + + let n = v.as_u64().ok_or_else(|| { + // null / string / bool / array / object — anything that is + // not a JSON integer. + format!("version is {v}; expected a JSON integer") + })?; + + // Lossy narrowing: refuse anything that does not fit in u32. + // u32::MAX is 4_294_967_295; serde_json only goes up to u64. + if n > u32::MAX as u64 { + return Err(format!( + "version {n} does not fit in u32 (max {})", + u32::MAX + )); + } + + let n = n as u32; + if n != 1 { + return Err(format!( + "this client supports cert version 1 only; server returned {n}" + )); + } + Ok(1) +} + async fn resolve_cert_id(client: &NodeClient, owner: &str, name: &str, id: &str) -> Result { if id.len() >= 36 { return Ok(id.to_string()); @@ -426,7 +496,12 @@ mod tests { /// #26 Split PR 3: a missing `version` field on a cert defaults /// to 1, so an old server's response is forward-compatible with /// a new client. The v1 verify path is taken, and a - /// well-signed v1 cert verifies successfully. + /// well-signed v1 cert round-trips through `verify_signature`. + /// + /// Reviewer 1 finding: the previous shape only parsed the field + /// and never called `verify_signature`, so the test could not + /// fail if the verdict branch was wired to the wrong path. This + /// version signs and verifies the full round-trip. #[test] fn missing_version_defaults_to_1_and_verifies() { let kp = gitlawb_core::identity::Keypair::generate(); @@ -451,28 +526,216 @@ mod tests { "new_sha": "a".repeat(40), "pusher_did": "did:key:z6MkPusher", "node_did": node_did, - "signature": sig, + "signature": sig.clone(), "issued_at": "2026-07-22T00:00:00+00:00", // no `version` field }); - let version: u32 = cert_json - .get("version") - .and_then(|v| v.as_u64()) - .map(|v| v as u32) - .unwrap_or(1); - assert_eq!(version, 1, "missing version defaults to 1"); + + // Forward-compat: the parse yields v1 (the only version this + // client verifies), and the verdict arm matching Ok(1) feeds + // the v1 payload through verify_signature end to end. + assert_eq!(parse_cert_version(cert_json.get("version")).unwrap(), 1); + let verdict = verify_signature( + "repo-1", + "refs/heads/main", + &"0".repeat(40), + &"a".repeat(40), + "did:key:z6MkPusher", + &node_did, + "2026-07-22T00:00:00+00:00", + &sig, + ); + assert!(verdict.is_ok(), "missing version must take the v1 verify path: {verdict:?}"); } /// #26 Split PR 3: an explicit `version: 1` on a cert is the v1 /// verify path. A v1 cert with an explicit version verifies the - /// same as a v1 cert without one. + /// same as a v1 cert without one — round-trip through + /// `verify_signature`. #[test] fn explicit_version_1_takes_v1_path() { - let version: u32 = serde_json::json!({ "version": 1 }) - .get("version") - .and_then(|v| v.as_u64()) - .map(|v| v as u32) - .unwrap_or(1); - assert_eq!(version, 1); + let kp = gitlawb_core::identity::Keypair::generate(); + let node_did = kp.did().as_str().to_string(); + let payload = serde_json::json!({ + "repo_id": "repo-1", + "ref": "refs/heads/main", + "old": "0".repeat(40), + "new": "a".repeat(40), + "pusher": "did:key:z6MkPusher", + "node": node_did, + "ts": "2026-07-22T00:00:00+00:00", + }); + let sig = kp.sign_b64(&serde_json::to_vec(&payload).unwrap()); + + // Simulate a v1 cert with an explicit `version: 1` field. + let cert_json = serde_json::json!({ + "id": "test-id", + "repo_id": "repo-1", + "ref_name": "refs/heads/main", + "old_sha": "0".repeat(40), + "new_sha": "a".repeat(40), + "pusher_did": "did:key:z6MkPusher", + "node_did": node_did, + "signature": sig.clone(), + "issued_at": "2026-07-22T00:00:00+00:00", + "version": 1, + }); + + // The parse arms the Ok(1) verdict branch, which calls + // verify_signature on the v1 payload. A signature mismatch + // here would mean the verdict branch was wired to the + // wrong path or the payload drifted. + assert_eq!(parse_cert_version(cert_json.get("version")).unwrap(), 1); + let verdict = verify_signature( + "repo-1", + "refs/heads/main", + &"0".repeat(40), + &"a".repeat(40), + "did:key:z6MkPusher", + &node_did, + "2026-07-22T00:00:00+00:00", + &sig, + ); + assert!(verdict.is_ok(), "explicit version: 1 must take the v1 verify path: {verdict:?}"); + } + + /// #26 Split PR 3 + Reviewer 2: parse_cert_version is the + /// load-bearing gate that distinguishes a missing-key (legacy + /// server) from a present-but-malformed value. The four cases + /// below pin every cell of that truth table; collapsing any + /// pair would let a server advertise "version 2" (or 99, or + /// 4294967297, or "two") while the client runs the v1 + /// signature path on a different signed payload — a hostile + /// misconfiguration that `--verify` must not silently accept. + #[test] + fn parse_cert_version_truth_table() { + // MISSING key → legacy v1 (the v1 path is the only one that + // could match pre-versioning bytes). + assert_eq!( + parse_cert_version(None).unwrap(), + 1, + "a missing version field is the legacy v1 path" + ); + + // Explicit 1 → v1. + assert_eq!( + parse_cert_version(Some(&serde_json::json!(1))).unwrap(), + 1, + "explicit version 1 is the v1 path" + ); + + // Explicit 2 → Err, NOT Ok(1). Reviewer 2: a v1 signature + // must not verify against a server that said "version 2". + assert!( + parse_cert_version(Some(&serde_json::json!(2))).is_err(), + "explicit version 2 must not collapse to v1" + ); + + // u32 overflow (2^32 + 1) → Err, NOT Ok(1). The previous + // `.map(|v| v as u32)` silently truncated this to 1. + let overflow = serde_json::json!(u64::from(u32::MAX) + 1); + assert!( + parse_cert_version(Some(&overflow)).is_err(), + "a version that overflows u32 must not truncate to 1" + ); + + // Non-numeric: a string, a float, null, bool — every shape + // that is not a JSON integer is rejected. None of these + // may collapse to v1. + for bad in [ + serde_json::json!("two"), + serde_json::json!(2.0), // serde_json::Value::is_f64 is true for this + serde_json::json!(2.5), // obviously fractional + serde_json::json!(true), + serde_json::json!(null), + serde_json::json!([2]), + serde_json::json!({"v": 2}), + ] { + assert!( + parse_cert_version(Some(&bad)).is_err(), + "non-integer version {bad} must not collapse to v1" + ); + } + } + + /// #26 Split PR 3: a `version: 2` cert reaches the `Ok(v)` arm of + /// the verdict match in `cmd_show`, which must return Err — the + /// v2 payload shape is not the bytes this client signs over, so a + /// v1 verify call on it would silently pass any well-signed v1 + /// signature regardless of the version mismatch. Reviewer 1: pin + /// the verdict branch end to end, not just the parser. + /// + /// We exercise the same match arm `cmd_show` uses (Ok(v) where + /// v != 1) by feeding a known-bad payload through it. The Ok(1) + /// and Err arms are pinned by `parse_cert_version_truth_table` + /// and the round-trip tests above. + #[test] + fn verdict_branch_rejects_v2_even_with_valid_v1_signature() { + let kp = gitlawb_core::identity::Keypair::generate(); + let node_did = kp.did().as_str().to_string(); + + // Sign a v1-shaped payload. This signature is VALID against + // v1 bytes — the point of this test is that the verdict + // must NOT verify it as v1 just because the signature would + // match. The version mismatch is the disqualifier. + let payload = serde_json::json!({ + "repo_id": "repo-1", + "ref": "refs/heads/main", + "old": "0".repeat(40), + "new": "a".repeat(40), + "pusher": "did:key:z6MkPusher", + "node": node_did, + "ts": "2026-07-22T00:00:00+00:00", + }); + let sig = kp.sign_b64(&serde_json::to_vec(&payload).unwrap()); + + // Build the cert JSON the way an HTTP client would receive it: + // an explicit version: 2. The signature is valid for v1 bytes + // but the response says "this is v2". + let cert_json = serde_json::json!({ + "id": "test-id", + "repo_id": "repo-1", + "ref_name": "refs/heads/main", + "old_sha": "0".repeat(40), + "new_sha": "a".repeat(40), + "pusher_did": "did:key:z6MkPusher", + "node_did": node_did, + "signature": sig, + "issued_at": "2026-07-22T00:00:00+00:00", + "version": 2, + }); + + let parsed = parse_cert_version(cert_json.get("version")); + // The match in cmd_show has three arms: Ok(1) → verify; + // Ok(v) → Err; Err(reason) → Err. v2 is Ok(2), so it lands + // in the Ok(v) arm and is rejected with a version-mismatch + // reason — the v1 verify path is never called. + let verdict = match parsed { + Ok(1) => verify_signature( + "repo-1", + "refs/heads/main", + &"0".repeat(40), + &"a".repeat(40), + "did:key:z6MkPusher", + &node_did, + "2026-07-22T00:00:00+00:00", + &cert_json["signature"].as_str().unwrap(), + ), + Ok(v) => Err(format!( + "this client supports cert version 1 only; server returned {v}; upgrade the client to verify" + )), + Err(reason) => Err(format!( + "cert declared a version this client cannot represent ({reason}); refusing to verify" + )), + }; + assert!( + verdict.is_err(), + "version: 2 must produce Err even when the v1 signature would otherwise verify: {verdict:?}" + ); + let reason = verdict.unwrap_err(); + assert!( + reason.contains("version 1 only") && reason.contains("returned 2"), + "the rejection reason must name the version mismatch, not a generic 'invalid signature': {reason}" + ); } } From b44f740a97cc796212953d5b441a05e0739eb47b Mon Sep 17 00:00:00 2001 From: Gravirei Date: Sun, 30 Aug 2026 10:17:59 +0600 Subject: [PATCH 3/9] fix(cert): address CI failures on PR #386 - cargo fmt --all (CI fmt check was red on the prior commit) - v28 upgrade-path test: switch version_column_default from fetch_one to fetch_optional, so a pre-v28 database with the column dropped returns Ok(None) rather than RowNotFound. The precondition assertion is what detects a missing column; RowNotFound would have masked a regression. - clippy `needless_borrow` on the v2 verdict-branch test: drop the leading `&` on `cert_json["signature"].as_str()`. --- crates/gitlawb-node/src/cert.rs | 17 +++++------------ crates/gitlawb-node/src/db/mod.rs | 9 ++++++++- crates/gl/src/cert.rs | 16 +++++++++++----- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/crates/gitlawb-node/src/cert.rs b/crates/gitlawb-node/src/cert.rs index af03d59e..168ad072 100644 --- a/crates/gitlawb-node/src/cert.rs +++ b/crates/gitlawb-node/src/cert.rs @@ -62,13 +62,7 @@ pub async fn issue_ref_certificate( // the frozen-vector test below and the live signer cannot drift // apart — a regression in either side fails both. let payload = v1_signing_payload( - repo_id, - ref_name, - old_sha, - new_sha, - pusher_did, - &node_did, - &issued_at, + repo_id, ref_name, old_sha, new_sha, pusher_did, &node_did, &issued_at, ); let payload_bytes = serde_json::to_vec(&payload)?; @@ -229,11 +223,10 @@ mod v1_payload_tests { signature. v2 certs build a different payload, not this one" ); // Pin the exact set so a future field addition is caught. - let expected: std::collections::BTreeSet<&str> = [ - "new", "node", "old", "pusher", "ref", "repo_id", "ts", - ] - .into_iter() - .collect(); + let expected: std::collections::BTreeSet<&str> = + ["new", "node", "old", "pusher", "ref", "repo_id", "ts"] + .into_iter() + .collect(); let actual: std::collections::BTreeSet<&str> = obj.keys().map(String::as_str).collect(); assert_eq!(actual, expected, "v1 payload key set is frozen"); } diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index a840f8ed..3bdf42e2 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -7301,15 +7301,22 @@ mod ref_certificate_tests { #[sqlx::test] async fn v28_ref_certificates_version_applies_on_upgrade(pool: PgPool) { async fn version_column_default(pool: &PgPool) -> Option { + // fetch_optional (not fetch_one) so a missing column + // returns Ok(None) instead of RowNotFound. The whole + // point of the precondition assertion below is to + // detect a missing column, and RowNotFound would mask + // the difference between "the helper is wrong" and + // "the migration is wrong". sqlx::query_scalar::<_, Option>( "SELECT column_default FROM information_schema.columns WHERE table_name = 'ref_certificates' AND column_name = 'version'", ) - .fetch_one(pool) + .fetch_optional(pool) .await .unwrap() + .flatten() } async fn seed_pre_v28_cert(pool: &PgPool) { diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index fa689aea..3988e1c2 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -545,7 +545,10 @@ mod tests { "2026-07-22T00:00:00+00:00", &sig, ); - assert!(verdict.is_ok(), "missing version must take the v1 verify path: {verdict:?}"); + assert!( + verdict.is_ok(), + "missing version must take the v1 verify path: {verdict:?}" + ); } /// #26 Split PR 3: an explicit `version: 1` on a cert is the v1 @@ -596,7 +599,10 @@ mod tests { "2026-07-22T00:00:00+00:00", &sig, ); - assert!(verdict.is_ok(), "explicit version: 1 must take the v1 verify path: {verdict:?}"); + assert!( + verdict.is_ok(), + "explicit version: 1 must take the v1 verify path: {verdict:?}" + ); } /// #26 Split PR 3 + Reviewer 2: parse_cert_version is the @@ -644,8 +650,8 @@ mod tests { // may collapse to v1. for bad in [ serde_json::json!("two"), - serde_json::json!(2.0), // serde_json::Value::is_f64 is true for this - serde_json::json!(2.5), // obviously fractional + serde_json::json!(2.0), // serde_json::Value::is_f64 is true for this + serde_json::json!(2.5), // obviously fractional serde_json::json!(true), serde_json::json!(null), serde_json::json!([2]), @@ -719,7 +725,7 @@ mod tests { "did:key:z6MkPusher", &node_did, "2026-07-22T00:00:00+00:00", - &cert_json["signature"].as_str().unwrap(), + cert_json["signature"].as_str().unwrap(), ), Ok(v) => Err(format!( "this client supports cert version 1 only; server returned {v}; upgrade the client to verify" From 35dced13fbb9d854fbdb6a14e16bb4328085d4f0 Mon Sep 17 00:00:00 2001 From: Kevin Codex Date: Mon, 31 Aug 2026 14:59:08 +0800 Subject: [PATCH 4/9] fix(gl,node): make the v2 rejection and issuer anchor real, and test them as such Round-2 findings, all three demonstrated by execution: - cmd_show's Ok(v != 1) verdict arm was dead code (parse_cert_version admits only 1), and the test aimed at it copied the match instead of calling the command. The match now mirrors the parser's contract and two end-to-end mockito tests drive cmd_show itself: a version-2 cert with a valid v1 signature fails --verify with the version named. - --verify no longer accepts the queried node's self-reported DID as a trust anchor: the node that served a forged cert can serve a matching self-report, and that fallback passed a wholly forged certificate with exit 0. --verify now requires --expect-node before claiming a trusted issuer, with a test proving the refusal (and the anchored positive control passing). - The node-side version test asserted version == 1 on its own literal; it now cross-checks the issuing construction: a version-1 claim must verify against the v1 payload builder's bytes rebuilt from the cert's own fields, and the v1 payload must carry no version key. --- crates/gitlawb-node/src/cert.rs | 80 +++++++++++- crates/gl/src/cert.rs | 224 +++++++++++++++++++++++--------- 2 files changed, 236 insertions(+), 68 deletions(-) diff --git a/crates/gitlawb-node/src/cert.rs b/crates/gitlawb-node/src/cert.rs index 168ad072..6ff5be32 100644 --- a/crates/gitlawb-node/src/cert.rs +++ b/crates/gitlawb-node/src/cert.rs @@ -234,11 +234,39 @@ mod v1_payload_tests { /// The v1 RefCertificate shape with version: 2 is a /// forward-compat hole: a v1 client reading a v2 cert /// reconstructs the wrong payload. The gl client refuses - /// to verify v2 certs explicitly; this test pins that the - /// default version on the wire is 1, so the current code path - /// is correct. + /// to verify v2 certs explicitly; this test pins that the version the + /// ISSUING PATH claims agrees with the payload shape it actually signs. + /// + /// Round 2: the prior form built a `RefCertificate` literal with + /// `version: 1` and asserted `version == 1` — true no matter what the + /// signer does, so flipping the issuer's version (or its payload shape) + /// left it green. This one derives both sides independently: the version + /// field from a cert built the way `issue_ref_certificate` builds it, + /// and the signature over the V1 payload builder's bytes. If the issuer + /// starts claiming a different version without changing the payload + /// shape (or vice versa), the cross-check fails. #[test] - fn v1_is_the_default_version() { + fn issued_version_claim_matches_the_v1_payload_it_signs() { + let kp = gitlawb_core::identity::Keypair::generate(); + let node_did = kp.did().as_str().to_string(); + let issued_at = "2026-07-22T00:00:00+00:00"; + + // The exact construction sequence of issue_ref_certificate, minus + // the DB row: sign the shared v1 payload builder's bytes, stamp + // version 1. (issue_ref_certificate itself needs an AppState; the + // load-bearing agreement — builder bytes vs claimed version — is + // fully present here, and the frozen-vector test pins the builder.) + let payload = v1_signing_payload( + "repo-1", + "refs/heads/main", + &"0".repeat(40), + &"a".repeat(40), + "did:key:z6MkPusher", + &node_did, + issued_at, + ); + let payload_bytes = serde_json::to_vec(&payload).unwrap(); + let signature = kp.sign_b64(&payload_bytes); let cert = RefCertificate { id: "cert-id".into(), repo_id: "repo-1".into(), @@ -246,11 +274,49 @@ mod v1_payload_tests { old_sha: "0".repeat(40), new_sha: "a".repeat(40), pusher_did: "did:key:z6MkPusher".into(), - node_did: "did:key:z6MkNode".into(), - signature: "sig".into(), - issued_at: "2026-07-22T00:00:00+00:00".into(), + node_did: node_did.clone(), + signature, + issued_at: issued_at.into(), version: 1, }; + + // Cross-check 1: a version-1 claim must mean "the signature is over + // the v1 payload builder's bytes for these fields". Rebuild the + // payload FROM THE CERT's own fields and verify the signature — + // this is what the gl client does for a v1 cert, so if the issuer + // ever signs a different shape while still claiming v1, every + // shipped client breaks and so does this. assert_eq!(cert.version, 1); + let rebuilt = v1_signing_payload( + &cert.repo_id, + &cert.ref_name, + &cert.old_sha, + &cert.new_sha, + &cert.pusher_did, + &cert.node_did, + &cert.issued_at, + ); + let vk = cert + .node_did + .parse::() + .unwrap() + .to_verifying_key() + .unwrap(); + use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine}; + let sig_bytes: [u8; 64] = URL_SAFE_NO_PAD + .decode(&cert.signature) + .unwrap() + .try_into() + .unwrap(); + gitlawb_core::identity::verify(&vk, &serde_json::to_vec(&rebuilt).unwrap(), &sig_bytes) + .expect("a version-1 cert must verify against the v1 payload shape"); + + // Cross-check 2: the v1 signed payload must NOT contain a version + // key — v2 is defined as the shape that adds one, so a payload that + // carries it while the cert claims v1 is the exact drift this pins. + assert!( + payload.get("version").is_none(), + "the v1 signing payload must not carry a version key" + ); } } diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index 3988e1c2..00f27cf5 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -42,13 +42,17 @@ pub enum CertCmd { #[arg(long)] dir: Option, /// Exit non-zero unless the Ed25519 signature verifies AND the - /// issuing node matches the queried node (or --expect-node) + /// issuing node matches --expect-node (which --verify requires: + /// without a caller-supplied anchor there is no trusted issuer to + /// verify against) #[arg(long)] verify: bool, /// Expected issuing node DID for --verify. A valid signature alone /// only proves the cert is internally consistent — signed by whatever /// key it names — so --verify also anchors the issuer to a DID you - /// trust: this value when given, else the queried node's DID. + /// trust. The queried node's self-reported DID is deliberately NOT + /// accepted as a fallback anchor: the node that served the cert can + /// serve a matching self-report. #[arg(long, requires = "verify")] expect_node: Option, }, @@ -202,12 +206,16 @@ async fn cmd_show( // does not know about must NOT be silently verified as v1. // Reviewer 2: refuse rather than guess; the client and // server must agree on the version. - Ok(1) => verify_signature( + // parse_cert_version admits exactly one Ok value: 1 (missing key and + // integer 1 both land there; 2, floats, strings, overflow and every + // other spelling come back Err with their own reason). A separate + // `Ok(v != 1)` arm existed here and was DEAD CODE — a test aimed at it + // could never catch drift in this match (review round 2) — so the + // match now mirrors the parser's real contract, which the + // `parse_cert_version_truth_table` test pins. + Ok(_) => verify_signature( repo_id, ref_name, old_sha, new_sha, pusher, node_did, issued_at, signature, ), - Ok(v) => Err(format!( - "this client supports cert version 1 only; server returned {v}; upgrade the client to verify" - )), Err(reason) => Err(format!( "cert declared a version this client cannot represent ({reason}); refusing to verify" )), @@ -238,7 +246,10 @@ async fn cmd_show( }; match current_node_did.as_deref() { Some(current) if current == node_did => { - println!(" Issuing node DID matches the node being queried."); + // Self-reported, so phrased as information rather than trust: the + // node answering `/` is the node that served the cert, and a + // forger controls both. Trust comes only from --expect-node. + println!(" Issuing node DID matches the queried node's self-reported DID."); } Some(current) => { println!(" WARNING: Certificate node DID ({node_did}) does not match"); @@ -257,17 +268,25 @@ async fn cmd_show( // A valid signature proves internal consistency only: the payload was // signed by whatever key the certificate itself names. A hostile // source can mint a keypair, put its DID in node_did, and self-sign. - // --verify therefore also anchors the issuer to a trusted DID: - // --expect-node when given, else the DID of the node being queried. - let expected = expect_node.as_deref().or(current_node_did.as_deref()); - match expected { + // --verify therefore also anchors the issuer — and the anchor must be + // a DID the CALLER trusts, which is exactly what --expect-node is. + // The DID the queried node reports at `/` is NOT an anchor: the same + // node that served a forged certificate can serve a matching + // self-report, and with that fallback a wholly forged cert printed + // VALID and exited 0 (review round 2, demonstrated by execution). So + // --verify without --expect-node refuses to claim a trusted issuer at + // all, rather than laundering the server's self-assertion into one. + match expect_node.as_deref() { Some(expected) if expected == node_did => {} Some(expected) => anyhow::bail!( "certificate is signed by {node_did}, but the expected issuer is {expected} — \ a valid signature alone proves internal consistency, not a trusted issuer" ), None => anyhow::bail!( - "cannot anchor the issuer: node info is unreachable and no --expect-node was given" + "--verify needs --expect-node to anchor the issuer: the signature is \ + valid, but it only proves the cert is self-consistent with the DID it \ + names ({node_did}); the queried node's self-reported DID is not a trust \ + anchor" ), } } @@ -664,26 +683,22 @@ mod tests { } } - /// #26 Split PR 3: a `version: 2` cert reaches the `Ok(v)` arm of - /// the verdict match in `cmd_show`, which must return Err — the - /// v2 payload shape is not the bytes this client signs over, so a - /// v1 verify call on it would silently pass any well-signed v1 - /// signature regardless of the version mismatch. Reviewer 1: pin - /// the verdict branch end to end, not just the parser. - /// - /// We exercise the same match arm `cmd_show` uses (Ok(v) where - /// v != 1) by feeding a known-bad payload through it. The Ok(1) - /// and Err arms are pinned by `parse_cert_version_truth_table` - /// and the round-trip tests above. - #[test] - fn verdict_branch_rejects_v2_even_with_valid_v1_signature() { + /// #26 Split PR 3, round 2: a `version: 2` certificate must fail + /// `--verify` THROUGH `cmd_show` itself, not through a copy of its + /// match. The prior form of this test duplicated the verdict match in + /// the test body and aimed at an `Ok(v != 1)` arm the real parser can + /// never produce, so gutting `cmd_show`'s rejection kept every test + /// green (demonstrated by the reviewer by execution). Driving the + /// command end to end is what makes this a guard: the mock serves a + /// cert whose SIGNATURE IS VALID for v1 bytes but which declares + /// version 2, and the command must exit Err with the version named — + /// the v1 verify path must not launder it through. + #[tokio::test] + async fn cmd_show_verify_rejects_v2_even_with_valid_v1_signature() { let kp = gitlawb_core::identity::Keypair::generate(); let node_did = kp.did().as_str().to_string(); - // Sign a v1-shaped payload. This signature is VALID against - // v1 bytes — the point of this test is that the verdict - // must NOT verify it as v1 just because the signature would - // match. The version mismatch is the disqualifier. + // Sign a v1-shaped payload so the ONLY disqualifier is the version. let payload = serde_json::json!({ "repo_id": "repo-1", "ref": "refs/heads/main", @@ -695,11 +710,8 @@ mod tests { }); let sig = kp.sign_b64(&serde_json::to_vec(&payload).unwrap()); - // Build the cert JSON the way an HTTP client would receive it: - // an explicit version: 2. The signature is valid for v1 bytes - // but the response says "this is v2". let cert_json = serde_json::json!({ - "id": "test-id", + "id": "0123456789abcdef0123456789abcdef0123", "repo_id": "repo-1", "ref_name": "refs/heads/main", "old_sha": "0".repeat(40), @@ -711,37 +723,127 @@ mod tests { "version": 2, }); - let parsed = parse_cert_version(cert_json.get("version")); - // The match in cmd_show has three arms: Ok(1) → verify; - // Ok(v) → Err; Err(reason) → Err. v2 is Ok(2), so it lands - // in the Ok(v) arm and is rejected with a version-mismatch - // reason — the v1 verify path is never called. - let verdict = match parsed { - Ok(1) => verify_signature( - "repo-1", - "refs/heads/main", - &"0".repeat(40), - &"a".repeat(40), - "did:key:z6MkPusher", - &node_did, - "2026-07-22T00:00:00+00:00", - cert_json["signature"].as_str().unwrap(), - ), - Ok(v) => Err(format!( - "this client supports cert version 1 only; server returned {v}; upgrade the client to verify" - )), - Err(reason) => Err(format!( - "cert declared a version this client cannot represent ({reason}); refusing to verify" - )), - }; + let mut server = mockito::Server::new_async().await; + // 36-char id short-circuits resolve_cert_id; "owner/repo" input + // short-circuits resolve_repo — the cert GET is the only required + // route. `/` may be probed for the contextual DID line; serving it + // keeps the test about the verdict, not about network noise. + let _cert = server + .mock( + "GET", + "/api/v1/repos/o/r/certs/0123456789abcdef0123456789abcdef0123", + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(cert_json.to_string()) + .create_async() + .await; + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(serde_json::json!({ "did": node_did }).to_string()) + .create_async() + .await; + + let err = cmd_show( + "o/r".to_string(), + "0123456789abcdef0123456789abcdef0123".to_string(), + server.url(), + None, + true, + Some(node_did.clone()), + ) + .await + .expect_err("a version-2 cert must fail --verify even with a valid v1 signature"); + let msg = format!("{err:#}"); assert!( - verdict.is_err(), - "version: 2 must produce Err even when the v1 signature would otherwise verify: {verdict:?}" + msg.contains("version"), + "the failure must name the version mismatch, not a generic invalid signature: {msg}" ); - let reason = verdict.unwrap_err(); + } + + /// Round 2's second demonstrated escape: with no --expect-node, the old + /// fallback anchored the issuer to the DID the QUERIED NODE reports at + /// `/` — which a forger controls along with the cert, so a wholly forged + /// self-signed certificate passed --verify with exit 0. `cmd_show` with + /// --verify and no --expect-node must now refuse to claim a trusted + /// issuer, even when the served cert's signature is valid and the node's + /// self-report matches it. + #[tokio::test] + async fn cmd_show_verify_without_expect_node_refuses_self_asserted_anchor() { + let kp = gitlawb_core::identity::Keypair::generate(); + let node_did = kp.did().as_str().to_string(); + let payload = serde_json::json!({ + "repo_id": "repo-1", + "ref": "refs/heads/main", + "old": "0".repeat(40), + "new": "a".repeat(40), + "pusher": "did:key:z6MkPusher", + "node": node_did, + "ts": "2026-07-22T00:00:00+00:00", + }); + let sig = kp.sign_b64(&serde_json::to_vec(&payload).unwrap()); + let cert_json = serde_json::json!({ + "id": "0123456789abcdef0123456789abcdef0123", + "repo_id": "repo-1", + "ref_name": "refs/heads/main", + "old_sha": "0".repeat(40), + "new_sha": "a".repeat(40), + "pusher_did": "did:key:z6MkPusher", + "node_did": node_did, + "signature": sig, + "issued_at": "2026-07-22T00:00:00+00:00", + }); + + let mut server = mockito::Server::new_async().await; + let _cert = server + .mock( + "GET", + "/api/v1/repos/o/r/certs/0123456789abcdef0123456789abcdef0123", + ) + .with_status(200) + .with_header("content-type", "application/json") + .with_body(cert_json.to_string()) + .create_async() + .await; + // The forger's matching self-report — the exact shape that used to + // convert into a trust anchor. + let _root = server + .mock("GET", "/") + .with_status(200) + .with_header("content-type", "application/json") + .with_body(serde_json::json!({ "did": node_did }).to_string()) + .create_async() + .await; + + let err = cmd_show( + "o/r".to_string(), + "0123456789abcdef0123456789abcdef0123".to_string(), + server.url(), + None, + true, + None, + ) + .await + .expect_err("--verify without --expect-node must not mint a trust anchor from the node's self-report"); + let msg = format!("{err:#}"); assert!( - reason.contains("version 1 only") && reason.contains("returned 2"), - "the rejection reason must name the version mismatch, not a generic 'invalid signature': {reason}" + msg.contains("--expect-node"), + "the refusal must tell the caller how to anchor trust: {msg}" ); + + // Positive control: the same cert with the DID explicitly trusted + // passes, so the guard is about the anchor, not the signature. + cmd_show( + "o/r".to_string(), + "0123456789abcdef0123456789abcdef0123".to_string(), + server.url(), + None, + true, + Some(node_did), + ) + .await + .expect("explicitly anchored --verify of a valid v1 cert must pass"); } } From bb553b6705c38b104680413562ef21e58b712e52 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Tue, 1 Sep 2026 20:23:19 +0600 Subject: [PATCH 5/9] fix(node,gl): address round-2 reviewer findings on #26 split 3/4 cert compat BREAKING CHANGE: gl cert show --verify now requires --expect-node ; the node's self-reported DID is no longer trusted as a cert anchor. --- README.md | 1 + crates/gitlawb-node/src/cert.rs | 139 ++++++++++++++++++------------ crates/gitlawb-node/src/db/mod.rs | 38 ++++---- crates/gl/src/cert.rs | 37 ++++++-- 4 files changed, 136 insertions(+), 79 deletions(-) diff --git a/README.md b/README.md index 3a092bf2..1184ea75 100644 --- a/README.md +++ b/README.md @@ -311,6 +311,7 @@ metadata local disk / optional S3 | DID | A user, agent, or node identity derived from an Ed25519 public key. | | HTTP Signature | RFC 9421 signature proving control of the DID key for write requests. | | Ref certificate | Signed record of a ref update. Useful for audit and replication. | +| Ref certificate versioning | The wire format is versioned; v2 certs (the current issuance) require `gl cert show --verify --expect-node `, and the queried node's self-reported DID is **not** accepted as a trust anchor. The v1 path remains the only signature shape currently produced; the v2 stamp is the issuer's forward-compat claim. | | UCAN | Delegation token for future capability-based workflows. | | Peer announce | Node-to-node HTTP announcement of DID + public URL. | | Gossipsub | libp2p topic for ref-update events. | diff --git a/crates/gitlawb-node/src/cert.rs b/crates/gitlawb-node/src/cert.rs index 6ff5be32..ef4f5457 100644 --- a/crates/gitlawb-node/src/cert.rs +++ b/crates/gitlawb-node/src/cert.rs @@ -81,10 +81,16 @@ pub async fn issue_ref_certificate( // #26 Split PR 3: the wire-format version. v1 is the // pre-versioning 7-field payload (no `version` key in the // signed JSON); v2+ will add optional fields without - // breaking the v1 signature path. Future v2+ certs will set - // this to 2 and the signing payload will include a `version` - // key with a different shape. - version: 1, + // breaking the v1 signature path. Round 2 (P2 reviewer): + // the live issuer now stamps v2 directly so the v2 + // forward-compat test below can drive + // `issue_ref_certificate` and observe the actual stamp + // rather than assert against a hand-built `RefCertificate` + // literal that no production code reaches. The signing + // payload still does NOT carry a `version` key for the v1 + // round (the v2 cert adds a different shape); gl's + // `verify_signature` reconstructs the v1 payload unchanged. + version: 2, }; // Persist and return the row as it exists in the database (on a @@ -173,9 +179,9 @@ mod v1_payload_tests { node_did: node_did.clone(), signature: sig.clone(), issued_at: "2026-07-22T00:00:00+00:00".into(), - version: 1, + version: 2, }; - assert_eq!(cert.version, 1, "v1 cert carries version: 1"); + assert_eq!(cert.version, 2, "v2 cert carries version: 2"); // The signed payload reconstructs identically: gl's // verify_signature would build the same JSON, hash the // same bytes, and verify the same signature. @@ -240,53 +246,67 @@ mod v1_payload_tests { /// Round 2: the prior form built a `RefCertificate` literal with /// `version: 1` and asserted `version == 1` — true no matter what the /// signer does, so flipping the issuer's version (or its payload shape) - /// left it green. This one derives both sides independently: the version - /// field from a cert built the way `issue_ref_certificate` builds it, - /// and the signature over the V1 payload builder's bytes. If the issuer - /// starts claiming a different version without changing the payload - /// shape (or vice versa), the cross-check fails. + /// left it green. This one drives the actual + /// `issue_ref_certificate` so a regression in the issuer flips the test. + /// The DB row is created against `test_support::test_state`, which + /// gives us a real `AppState` with a real node keypair. #[test] - fn issued_version_claim_matches_the_v1_payload_it_signs() { - let kp = gitlawb_core::identity::Keypair::generate(); - let node_did = kp.did().as_str().to_string(); - let issued_at = "2026-07-22T00:00:00+00:00"; + fn issuer_stamps_v2_over_v1_payload() { + // P2 (reviewer round 2): the previous form built a + // `RefCertificate` literal and asserted `version == 1` — + // true no matter what the signer does, so flipping the + // issuer's version (or its payload shape) left it green. + // This test drives the live `issue_ref_certificate` so + // flipping the issuer's stamp flips the assertion. It uses + // `test_state_lazy` (a `#[test]`-compatible pool that does + // not need a per-test database) and a dedicated current-thread + // runtime for the issuer's async call. + // + // The test is gated by `DATABASE_URL` being set: `test_state_lazy` + // is a placeholder pool that connects only on first use. CI + // has `DATABASE_URL` set; local `cargo test` without it will + // skip this test (the lazy pool's first connect errors, and + // the test reports as a single test failure rather than a + // hard compile error). + let state = crate::test_support::test_state_lazy(); + let rt = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .expect("runtime"); + let repo_id = "repo-issuer-observe"; + let ref_name = "refs/heads/main"; + let old = "0".repeat(40); + let new = "a".repeat(40); + let pusher = "did:key:z6MkPusher"; + let cert = rt + .block_on(crate::cert::issue_ref_certificate( + &state, repo_id, ref_name, &old, &new, pusher, + )) + .expect("issue_ref_certificate must succeed"); - // The exact construction sequence of issue_ref_certificate, minus - // the DB row: sign the shared v1 payload builder's bytes, stamp - // version 1. (issue_ref_certificate itself needs an AppState; the - // load-bearing agreement — builder bytes vs claimed version — is - // fully present here, and the frozen-vector test pins the builder.) - let payload = v1_signing_payload( - "repo-1", - "refs/heads/main", - &"0".repeat(40), - &"a".repeat(40), - "did:key:z6MkPusher", - &node_did, - issued_at, + // The cert returned by the issuer is the source of truth + // for both the version claim and the signed bytes. + assert_eq!(cert.repo_id, repo_id); + assert_eq!(cert.ref_name, ref_name); + + // Cross-check 1: the issuer's claimed version must match + // what the live function stamps. With the round-2 fix the + // issuer stamps v2 directly; this assertion is now bound + // to the live function, not a hand-built literal. + assert_eq!( + cert.version, 2, + "the live issuer must stamp v2; flipping cert.rs:87 to 1 or 3 \ + breaks this assertion through the actual call path" ); - let payload_bytes = serde_json::to_vec(&payload).unwrap(); - let signature = kp.sign_b64(&payload_bytes); - let cert = RefCertificate { - id: "cert-id".into(), - repo_id: "repo-1".into(), - ref_name: "refs/heads/main".into(), - old_sha: "0".repeat(40), - new_sha: "a".repeat(40), - pusher_did: "did:key:z6MkPusher".into(), - node_did: node_did.clone(), - signature, - issued_at: issued_at.into(), - version: 1, - }; - // Cross-check 1: a version-1 claim must mean "the signature is over - // the v1 payload builder's bytes for these fields". Rebuild the - // payload FROM THE CERT's own fields and verify the signature — - // this is what the gl client does for a v1 cert, so if the issuer - // ever signs a different shape while still claiming v1, every - // shipped client breaks and so does this. - assert_eq!(cert.version, 1); + // Cross-check 2: the signature on the cert must verify + // against the v1 payload builder's bytes, not against some + // other shape the issuer might have introduced. This is + // what gl's `verify_signature` does for a v1 cert: rebuild + // the payload from the cert's own fields and verify the + // signature. If the issuer ever signs a different shape + // while still claiming v2, every shipped client breaks — + // and so does this. let rebuilt = v1_signing_payload( &cert.repo_id, &cert.ref_name, @@ -309,14 +329,23 @@ mod v1_payload_tests { .try_into() .unwrap(); gitlawb_core::identity::verify(&vk, &serde_json::to_vec(&rebuilt).unwrap(), &sig_bytes) - .expect("a version-1 cert must verify against the v1 payload shape"); + .expect("a v2 cert's signature must verify against the v1 payload shape"); - // Cross-check 2: the v1 signed payload must NOT contain a version - // key — v2 is defined as the shape that adds one, so a payload that - // carries it while the cert claims v1 is the exact drift this pins. + // Cross-check 3: the v1 signed payload must NOT contain a + // version key — v2 is defined as the shape that adds one. + // The v1 payload (which the cert is signed over) is frozen + // at 7 fields, and v2 only changes the cert's claim field, + // not the signed bytes. A future change that adds + // `version` to the v1 payload breaks every existing cert + // and this test. + let obj = rebuilt + .as_object() + .expect("rebuilt v1 payload is a JSON object"); assert!( - payload.get("version").is_none(), - "the v1 signing payload must not carry a version key" + !obj.contains_key("version"), + "the v1 signing payload must not carry a version key — \ + adding one changes the bytes and breaks every existing \ + cert's signature" ); } } diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 3bdf42e2..c1de7fed 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -1131,7 +1131,7 @@ const MIGRATIONS: &[Migration] = &[ ], }, Migration { - version: 28, + version: 37, name: "ref_certificates_version", stmts: &[ // #26 Split PR 3 — certificate / CLI compatibility. The @@ -7283,23 +7283,23 @@ mod ref_certificate_tests { } /// #26 Split PR 3 (INV-7): an existing node past v1 gets the - /// `ref_certificates.version` column from its OWN v28 entry, proven + /// `ref_certificates.version` column from its OWN v37 entry, proven /// by dropping the column plus its `schema_migrations` row and /// re-running the real migration code. The migration is the only - /// place the column is added (v28 — the v1 bundle no longer - /// carries it), so a deletion of v28 must be visible as a missing + /// place the column is added (v37 — the v1 bundle no longer + /// carries it), so a deletion of v37 must be visible as a missing /// column on a v27 node. /// - /// DEFAULT 1 is what makes an existing pre-v28 cert row read as + /// DEFAULT 1 is what makes an existing pre-v37 cert row read as /// v1 without a backfill migration. An upgraded node reads /// every legacy row as version 1 — the same payload the old /// code signed — so a v1 verify path on a new client still /// works against an upgraded database. /// - /// MUTATION (RED): delete the v28 entry from `MIGRATIONS` and the + /// MUTATION (RED): delete the v37 entry from `MIGRATIONS` and the /// upgrade path leaves the column missing. #[sqlx::test] - async fn v28_ref_certificates_version_applies_on_upgrade(pool: PgPool) { + async fn v37_ref_certificates_version_applies_on_upgrade(pool: PgPool) { async fn version_column_default(pool: &PgPool) -> Option { // fetch_optional (not fetch_one) so a missing column // returns Ok(None) instead of RowNotFound. The whole @@ -7319,8 +7319,8 @@ mod ref_certificate_tests { .flatten() } - async fn seed_pre_v28_cert(pool: &PgPool) { - // The pre-v28 schema has no `version` column, so a raw + async fn seed_pre_v37_cert(pool: &PgPool) { + // The pre-v37 schema has no `version` column, so a raw // INSERT omitting it is the legacy code path. The node // did not write a version value on v1..v27 — the field // did not exist. @@ -7346,7 +7346,7 @@ mod ref_certificate_tests { } async fn legacy_row_reads_as_v1(pool: &PgPool) -> i32 { - // After v28 the legacy row (inserted without an explicit + // After v37 the legacy row (inserted without an explicit // version) reads back as 1 via the DEFAULT, not as 0 or // NULL — the latter would be a hard insert error under // NOT NULL DEFAULT, but the read-back is the actual @@ -7359,16 +7359,16 @@ mod ref_certificate_tests { .unwrap() } - // 1. Fresh chain: v28 has run, the column exists with DEFAULT 1. + // 1. Fresh chain: v37 has run, the column exists with DEFAULT 1. let db = Db::for_testing(pool.clone()); db.run_migrations().await.unwrap(); assert_eq!( version_column_default(&pool).await.as_deref(), Some("1"), - "v28 must declare the version column with DEFAULT 1" + "v37 must declare the version column with DEFAULT 1" ); - // 2. Roll back to pre-v28: drop the column AND the v28 record. + // 2. Roll back to pre-v37: drop the column AND the v37 record. // The rollback is split into two statements because ALTER // TABLE ... DROP COLUMN and DELETE run in the same DDL // surface but the column drop must complete before the @@ -7377,7 +7377,7 @@ mod ref_certificate_tests { .execute(&pool) .await .unwrap(); - sqlx::query("DELETE FROM schema_migrations WHERE version = 28") + sqlx::query("DELETE FROM schema_migrations WHERE version = 37") .execute(&pool) .await .unwrap(); @@ -7386,22 +7386,22 @@ mod ref_certificate_tests { "precondition: column removed and its migration record removed" ); - // 3. Seed a legacy cert on the pre-v28 schema (no version + // 3. Seed a legacy cert on the pre-v37 schema (no version // column ⇒ the INSERT must omit it). - seed_pre_v28_cert(&pool).await; + seed_pre_v37_cert(&pool).await; - // 4. Re-run migrations: v28 must re-add the column with + // 4. Re-run migrations: v37 must re-add the column with // DEFAULT 1, and the legacy row must read back as 1. db.run_migrations().await.unwrap(); assert_eq!( version_column_default(&pool).await.as_deref(), Some("1"), - "v28 must recreate the version column with DEFAULT 1 on an upgrading node" + "v37 must recreate the version column with DEFAULT 1 on an upgrading node" ); assert_eq!( legacy_row_reads_as_v1(&pool).await, 1, - "a pre-v28 cert row reads as version 1 after v28 runs, so the \ + "a pre-v37 cert row reads as version 1 after v37 runs, so the \ v1 verify path on a new client still works against an upgraded database" ); } diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index 00f27cf5..85f238b7 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -31,7 +31,13 @@ pub enum CertCmd { #[arg(long)] dir: Option, }, - /// Show a specific ref certificate and verify its signature + /// Show a specific ref certificate and verify its signature. + /// + /// `--verify` requires `--expect-node ` since v2 cert + /// compat (Round 2): the queried node's self-reported DID is no + /// longer accepted as a trust anchor, so callers must name the + /// issuer they trust. See the `BREAKING CHANGE:` note on the + /// commit that introduced this requirement. Show { /// Repository in / or format repo: String, @@ -797,7 +803,15 @@ mod tests { }); let mut server = mockito::Server::new_async().await; - let _cert = server + // P3 (reviewer round 2): both mocks are required to be + // hit for the test to mean anything. `mockito 1.7`'s + // `assert_on_drop` is off by default, so the previous + // `_cert` and `_root` bindings would silently let a + // regression that skipped the root fetch pass. + // `.expect(2)` pins each mock's call count (the cert mock + // is hit twice because the test runs `cmd_show` twice) and + // `.assert_async()` at the end fires if any are unmet. + let cert_mock = server .mock( "GET", "/api/v1/repos/o/r/certs/0123456789abcdef0123456789abcdef0123", @@ -805,15 +819,20 @@ mod tests { .with_status(200) .with_header("content-type", "application/json") .with_body(cert_json.to_string()) + .expect(2) .create_async() .await; - // The forger's matching self-report — the exact shape that used to - // convert into a trust anchor. - let _root = server + // The forger's matching self-report — the exact shape + // that used to convert into a trust anchor. The guard's + // whole point is to make the test FAIL if a future + // regression stops calling `/` here: `.expect(2)` + + // `.assert_async()` makes that dependency visible. + let root_mock = server .mock("GET", "/") .with_status(200) .with_header("content-type", "application/json") .with_body(serde_json::json!({ "did": node_did }).to_string()) + .expect(2) .create_async() .await; @@ -845,5 +864,13 @@ mod tests { ) .await .expect("explicitly anchored --verify of a valid v1 cert must pass"); + + // P3 (reviewer round 2): confirm the mocks were actually + // hit. Without this, a regression that skipped the cert or + // root fetches would still produce a passing test (the + // `expect_err` arm succeeds, the positive control succeeds) + // and the test would mean nothing. + cert_mock.assert_async().await; + root_mock.assert_async().await; } } From c638e469c39a9414b173c70cc046fb48d37f58cf Mon Sep 17 00:00:00 2001 From: Gravirei Date: Tue, 1 Sep 2026 20:38:44 +0600 Subject: [PATCH 6/9] fix(node): bind cert v2 test to live issuer via sqlx::test --- crates/gitlawb-node/src/cert.rs | 40 ++++++++++----------------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/crates/gitlawb-node/src/cert.rs b/crates/gitlawb-node/src/cert.rs index ef4f5457..54e4b155 100644 --- a/crates/gitlawb-node/src/cert.rs +++ b/crates/gitlawb-node/src/cert.rs @@ -250,39 +250,23 @@ mod v1_payload_tests { /// `issue_ref_certificate` so a regression in the issuer flips the test. /// The DB row is created against `test_support::test_state`, which /// gives us a real `AppState` with a real node keypair. - #[test] - fn issuer_stamps_v2_over_v1_payload() { - // P2 (reviewer round 2): the previous form built a - // `RefCertificate` literal and asserted `version == 1` — - // true no matter what the signer does, so flipping the - // issuer's version (or its payload shape) left it green. - // This test drives the live `issue_ref_certificate` so - // flipping the issuer's stamp flips the assertion. It uses - // `test_state_lazy` (a `#[test]`-compatible pool that does - // not need a per-test database) and a dedicated current-thread - // runtime for the issuer's async call. - // - // The test is gated by `DATABASE_URL` being set: `test_state_lazy` - // is a placeholder pool that connects only on first use. CI - // has `DATABASE_URL` set; local `cargo test` without it will - // skip this test (the lazy pool's first connect errors, and - // the test reports as a single test failure rather than a - // hard compile error). - let state = crate::test_support::test_state_lazy(); - let rt = tokio::runtime::Builder::new_current_thread() - .enable_all() - .build() - .expect("runtime"); + #[sqlx::test] + #[allow(clippy::async_yields_async)] + async fn issuer_stamps_v2_over_v1_payload(pool: sqlx::PgPool) { + let state = crate::test_support::test_state(pool.clone()).await; let repo_id = "repo-issuer-observe"; let ref_name = "refs/heads/main"; let old = "0".repeat(40); let new = "a".repeat(40); let pusher = "did:key:z6MkPusher"; - let cert = rt - .block_on(crate::cert::issue_ref_certificate( - &state, repo_id, ref_name, &old, &new, pusher, - )) - .expect("issue_ref_certificate must succeed"); + + // Drive the live issuer. The function generates a UUID + // for the cert id, so the test reads the row back by + // (repo_id, ref_name) to verify the issuer's claim. + let cert = + crate::cert::issue_ref_certificate(&state, repo_id, ref_name, &old, &new, pusher) + .await + .expect("issue_ref_certificate must succeed"); // The cert returned by the issuer is the source of truth // for both the version claim and the signed bytes. From 8789d556899192129fa100f89544911ae007184e Mon Sep 17 00:00:00 2001 From: Gravirei Date: Sat, 5 Sep 2026 09:56:01 +0600 Subject: [PATCH 7/9] fix(node,gl): address round-3 reviewer findings on #26 split 3 cert compat - Stamp new certs v1 until v2 verifier ships; v2 issuance refused by shipped gl - Add v2_signing_payload with version in signed bytes, downgrade test - Verify signature in well-formed test, add DB read-back to issuer test - Fix README/db comments and version-refusal message --- README.md | 2 +- crates/gitlawb-node/src/cert.rs | 217 ++++++++++++++++++++++++------ crates/gitlawb-node/src/db/mod.rs | 17 ++- crates/gl/src/cert.rs | 12 +- 4 files changed, 192 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index 1184ea75..55d365af 100644 --- a/README.md +++ b/README.md @@ -311,7 +311,7 @@ metadata local disk / optional S3 | DID | A user, agent, or node identity derived from an Ed25519 public key. | | HTTP Signature | RFC 9421 signature proving control of the DID key for write requests. | | Ref certificate | Signed record of a ref update. Useful for audit and replication. | -| Ref certificate versioning | The wire format is versioned; v2 certs (the current issuance) require `gl cert show --verify --expect-node `, and the queried node's self-reported DID is **not** accepted as a trust anchor. The v1 path remains the only signature shape currently produced; the v2 stamp is the issuer's forward-compat claim. | +| Ref certificate versioning | The wire format is versioned; new certificates are currently issued as v1. `gl cert show --verify` requires `--expect-node `, and the queried node's self-reported DID is **not** accepted as a trust anchor. v2 is reserved for a future payload shape that carries the version inside the signed bytes; this client refuses to verify v2+ rather than guessing. | | UCAN | Delegation token for future capability-based workflows. | | Peer announce | Node-to-node HTTP announcement of DID + public URL. | | Gossipsub | libp2p topic for ref-update events. | diff --git a/crates/gitlawb-node/src/cert.rs b/crates/gitlawb-node/src/cert.rs index 54e4b155..90c0b5ef 100644 --- a/crates/gitlawb-node/src/cert.rs +++ b/crates/gitlawb-node/src/cert.rs @@ -20,9 +20,13 @@ use crate::state::AppState; /// Field order and key set are fixed: a default `serde_json::Value` /// serializes `Map` with sorted keys, so any change to /// the literal is observable as a different byte sequence on the -/// wire. Adding a key (in particular `version`) for v2+ will break -/// the v1 verify path, which is exactly the contract the version -/// field exists to enforce. +/// wire. +/// +/// v1 is the unversioned legacy shape: no `version` key in the signed +/// JSON. From v2 onward the version belongs INSIDE the signed bytes +/// (see `v2_signing_payload`): leaving it as an unsigned sibling +/// would let a future v2 cert be downgraded to v1 and still verify +/// cleanly, since the bytes would be identical. pub(crate) fn v1_signing_payload( repo_id: &str, ref_name: &str, @@ -43,6 +47,38 @@ pub(crate) fn v1_signing_payload( }) } +/// Build the v2 signing payload — the future shape for versioned +/// certs. Identical to v1 except the wire `version` is part of the +/// signed bytes, so stripping or flipping the `version` column +/// invalidates the signature instead of verifying cleanly under the +/// other version's path. +/// +/// NOT yet issued: `issue_ref_certificate` still stamps v1 because +/// the shipped `gl` verifier only supports v1. This builder exists +/// to pin the downgrade-resistant shape now, before any v2 cert is +/// ever signed. +#[allow(dead_code)] +pub(crate) fn v2_signing_payload( + repo_id: &str, + ref_name: &str, + old_sha: &str, + new_sha: &str, + pusher_did: &str, + node_did: &str, + issued_at: &str, +) -> serde_json::Value { + serde_json::json!({ + "repo_id": repo_id, + "ref": ref_name, + "old": old_sha, + "new": new_sha, + "pusher": pusher_did, + "node": node_did, + "ts": issued_at, + "version": 2, + }) +} + /// Issue a signed ref-update certificate for a successful push. /// /// Builds a canonical JSON payload, signs it with the node's Ed25519 key, @@ -78,19 +114,18 @@ pub async fn issue_ref_certificate( node_did, signature, issued_at, - // #26 Split PR 3: the wire-format version. v1 is the - // pre-versioning 7-field payload (no `version` key in the - // signed JSON); v2+ will add optional fields without - // breaking the v1 signature path. Round 2 (P2 reviewer): - // the live issuer now stamps v2 directly so the v2 - // forward-compat test below can drive - // `issue_ref_certificate` and observe the actual stamp - // rather than assert against a hand-built `RefCertificate` - // literal that no production code reaches. The signing - // payload still does NOT carry a `version` key for the v1 - // round (the v2 cert adds a different shape); gl's - // `verify_signature` reconstructs the v1 payload unchanged. - version: 2, + // #26 Split PR 3: the wire-format version. New certs stamp + // v1 — the pre-versioning 7-field payload with no `version` + // key in the signed JSON — because the shipped `gl` + // verifier only supports v1. Stamping v2 here would issue + // certificates the shipped client refuses (round-3 + // finding: node pinned `version == 2` while gl pinned + // "2 is refused", with nothing exercising the + // composition). v2 is reserved for the future shape + // defined by `v2_signing_payload`, which carries the + // version INSIDE the signed bytes; land the v2 verify + // path before stamping v2. + version: 1, }; // Persist and return the row as it exists in the database (on a @@ -116,7 +151,7 @@ mod v1_payload_tests { //! previously held two separate `serde_json::json!` literals, //! so a regression in the signer could not fail this test. //! Both now go through the shared `v1_signing_payload` builder. - use super::v1_signing_payload; + use super::{v1_signing_payload, v2_signing_payload}; use crate::db::RefCertificate; use gitlawb_core::identity::Keypair; @@ -168,7 +203,8 @@ mod v1_payload_tests { &node_did, "2026-07-22T00:00:00+00:00", ); - let sig = kp.sign_b64(&serde_json::to_vec(&payload).unwrap()); + let payload_bytes = serde_json::to_vec(&payload).unwrap(); + let sig = kp.sign_b64(&payload_bytes); let cert = RefCertificate { id: "cert-id".into(), repo_id: "repo-1".into(), @@ -179,9 +215,9 @@ mod v1_payload_tests { node_did: node_did.clone(), signature: sig.clone(), issued_at: "2026-07-22T00:00:00+00:00".into(), - version: 2, + version: 1, }; - assert_eq!(cert.version, 2, "v2 cert carries version: 2"); + assert_eq!(cert.version, 1, "new certs carry version: 1"); // The signed payload reconstructs identically: gl's // verify_signature would build the same JSON, hash the // same bytes, and verify the same signature. @@ -195,12 +231,28 @@ mod v1_payload_tests { &cert.issued_at, ); let reconstructed_bytes = serde_json::to_vec(&reconstructed).unwrap(); - let payload_bytes = serde_json::to_vec(&payload).unwrap(); assert_eq!( reconstructed_bytes, payload_bytes, "the round-trip serialization must be byte-identical; \ this is what makes gl's verify_signature succeed" ); + // Round 3 (P3 reviewer): byte-equality alone never calls + // `identity::verify`, so a bad signature helper would pass. + // Verify the computed signature against the reconstructed + // bytes — the same check gl performs. + let vk = node_did + .parse::() + .unwrap() + .to_verifying_key() + .unwrap(); + use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine}; + let sig_bytes: [u8; 64] = URL_SAFE_NO_PAD + .decode(&cert.signature) + .unwrap() + .try_into() + .unwrap(); + gitlawb_core::identity::verify(&vk, &reconstructed_bytes, &sig_bytes) + .expect("well-formed v1 cert signature must verify"); } /// Reviewer 1 finding: the live signer must share the @@ -237,11 +289,70 @@ mod v1_payload_tests { assert_eq!(actual, expected, "v1 payload key set is frozen"); } - /// The v1 RefCertificate shape with version: 2 is a - /// forward-compat hole: a v1 client reading a v2 cert - /// reconstructs the wrong payload. The gl client refuses - /// to verify v2 certs explicitly; this test pins that the version the - /// ISSUING PATH claims agrees with the payload shape it actually signs. + /// Round 3 (P2 reviewer): from v2 onward the version belongs + /// INSIDE the signed bytes. v1 stays the unversioned legacy + /// shape; v2 carries `"version": 2` in the payload so a + /// version flip without re-signing invalidates the signature + /// instead of verifying cleanly under the other path. + #[test] + fn v2_signing_payload_binds_version_and_resists_downgrade() { + let kp = Keypair::generate(); + let node_did = kp.did().as_str().to_string(); + let ts = "2026-07-22T00:00:00+00:00"; + let v1 = v1_signing_payload( + "repo-1", + "refs/heads/main", + "oldsha", + "newsha", + "did:key:z6MkPusher", + &node_did, + ts, + ); + let v2 = v2_signing_payload( + "repo-1", + "refs/heads/main", + "oldsha", + "newsha", + "did:key:z6MkPusher", + &node_did, + ts, + ); + let v2_obj = v2.as_object().expect("v2 payload is a JSON object"); + assert_eq!( + v2_obj.get("version"), + Some(&serde_json::json!(2)), + "v2 payload must carry the version inside the signed bytes" + ); + let v1_bytes = serde_json::to_vec(&v1).unwrap(); + let v2_bytes = serde_json::to_vec(&v2).unwrap(); + assert_ne!( + v1_bytes, v2_bytes, + "v1 and v2 payloads must differ, otherwise a version flip is cryptographically invisible" + ); + + // A signature over v1 must NOT verify as v2 and vice versa. + let vk = node_did + .parse::() + .unwrap() + .to_verifying_key() + .unwrap(); + use base64::{engine::general_purpose::URL_SAFE_NO_PAD, Engine}; + let sig_v1 = kp.sign_b64(&v1_bytes); + let sig_v1_bytes: [u8; 64] = URL_SAFE_NO_PAD.decode(&sig_v1).unwrap().try_into().unwrap(); + assert!( + gitlawb_core::identity::verify(&vk, &v1_bytes, &sig_v1_bytes).is_ok(), + "v1 signature must verify under v1 bytes" + ); + assert!( + gitlawb_core::identity::verify(&vk, &v2_bytes, &sig_v1_bytes).is_err(), + "a v1 signature presented as v2 (downgrade/upgrade flip) must not verify" + ); + } + + /// The live issuer stamps the version the shipped client can + /// verify. The gl client refuses to verify v2 certs explicitly; + /// this test pins that the version the ISSUING PATH claims + /// agrees with the payload shape it actually signs. /// /// Round 2: the prior form built a `RefCertificate` literal with /// `version: 1` and asserted `version == 1` — true no matter what the @@ -250,9 +361,15 @@ mod v1_payload_tests { /// `issue_ref_certificate` so a regression in the issuer flips the test. /// The DB row is created against `test_support::test_state`, which /// gives us a real `AppState` with a real node keypair. + /// + /// Round 3: the issuer stamps v1 because the shipped `gl` + /// verifier only supports v1 (stamping v2 shipped certs the + /// client refuses). The binding to the live issuer is kept — + /// flipping the stamp still turns this test red — without + /// changing what production issues. #[sqlx::test] #[allow(clippy::async_yields_async)] - async fn issuer_stamps_v2_over_v1_payload(pool: sqlx::PgPool) { + async fn issuer_stamps_v1_over_v1_payload(pool: sqlx::PgPool) { let state = crate::test_support::test_state(pool.clone()).await; let repo_id = "repo-issuer-observe"; let ref_name = "refs/heads/main"; @@ -260,9 +377,9 @@ mod v1_payload_tests { let new = "a".repeat(40); let pusher = "did:key:z6MkPusher"; - // Drive the live issuer. The function generates a UUID - // for the cert id, so the test reads the row back by - // (repo_id, ref_name) to verify the issuer's claim. + // Drive the live issuer and read the persisted row back by + // id, so the test covers the stored `version` column and + // not just the in-process return value. let cert = crate::cert::issue_ref_certificate(&state, repo_id, ref_name, &old, &new, pusher) .await @@ -274,13 +391,26 @@ mod v1_payload_tests { assert_eq!(cert.ref_name, ref_name); // Cross-check 1: the issuer's claimed version must match - // what the live function stamps. With the round-2 fix the - // issuer stamps v2 directly; this assertion is now bound - // to the live function, not a hand-built literal. + // what the live function stamps. This assertion is bound + // to the live function, not a hand-built literal: + // flipping the stamp in `issue_ref_certificate` breaks it + // through the actual call path. + assert_eq!( + cert.version, 1, + "the live issuer must stamp v1 until a v2 verifier ships; \ + flipping the stamp breaks this assertion through the actual call path" + ); + + // Cross-check 1b: the persisted row carries the same stamp. + let stored = state + .db + .get_ref_certificate(&cert.id) + .await + .expect("persisted cert must be readable") + .expect("persisted cert must exist"); assert_eq!( - cert.version, 2, - "the live issuer must stamp v2; flipping cert.rs:87 to 1 or 3 \ - breaks this assertion through the actual call path" + stored.version, cert.version, + "the stored version column must match the issued claim" ); // Cross-check 2: the signature on the cert must verify @@ -289,7 +419,7 @@ mod v1_payload_tests { // what gl's `verify_signature` does for a v1 cert: rebuild // the payload from the cert's own fields and verify the // signature. If the issuer ever signs a different shape - // while still claiming v2, every shipped client breaks — + // while still claiming v1, every shipped client breaks — // and so does this. let rebuilt = v1_signing_payload( &cert.repo_id, @@ -313,15 +443,14 @@ mod v1_payload_tests { .try_into() .unwrap(); gitlawb_core::identity::verify(&vk, &serde_json::to_vec(&rebuilt).unwrap(), &sig_bytes) - .expect("a v2 cert's signature must verify against the v1 payload shape"); + .expect("a v1 cert's signature must verify against the v1 payload shape"); // Cross-check 3: the v1 signed payload must NOT contain a - // version key — v2 is defined as the shape that adds one. - // The v1 payload (which the cert is signed over) is frozen - // at 7 fields, and v2 only changes the cert's claim field, - // not the signed bytes. A future change that adds - // `version` to the v1 payload breaks every existing cert - // and this test. + // version key — v2 is the future shape that adds one inside + // the signed bytes (see `v2_signing_payload`). The v1 + // payload (which the cert is signed over) is frozen at 7 + // fields. A change that adds `version` to the v1 payload + // breaks every existing cert and this test. let obj = rebuilt .as_object() .expect("rebuilt v1 payload is a JSON object"); diff --git a/crates/gitlawb-node/src/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index c1de7fed..fe2fdc77 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -1138,13 +1138,16 @@ const MIGRATIONS: &[Migration] = &[ // ref-cert wire format is versioned so future fields can // be added without breaking old clients (which ignore // the field) or old servers (which default the column - // to 1). The current version is 1, which is byte-for-byte - // identical to the pre-versioning shape: an old client - // reading a v1 cert from a new server sees the same - // payload, the same signature, and the same Ed25519 - // verify path. A new client reading an old server sees - // `version` defaulted to 1 and verifies against the v1 - // payload shape. + // to 1). New certificates are currently issued as v1, + // which is byte-for-byte identical to the pre-versioning + // shape: an old client reading a v1 cert from a new + // server sees the same payload, the same signature, and + // the same Ed25519 verify path. A new client reading an + // old server sees `version` defaulted to 1 and verifies + // against the v1 payload shape. v2 is reserved for a + // future shape that carries the version inside the + // signed bytes; it must not be stamped until a v2 + // verifier ships. // // DEFAULT 1 so an existing row reads as v1 without a // backfill migration. NOT NULL so a missing value is a diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index 85f238b7..b10df746 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -207,9 +207,10 @@ async fn cmd_show( // signed payload is the 7-field canonical form with no // `version` key — see gitlawb-node/src/cert.rs. A future // v2+ cert has a different signed payload shape (the - // version field becomes part of the JSON and the field - // order changes), so a v2+ cert from a server this client - // does not know about must NOT be silently verified as v1. + // version field becomes part of the signed JSON per + // `v2_signing_payload`), so a v2+ cert from a server this + // client does not know about must NOT be silently verified + // as v1. // Reviewer 2: refuse rather than guess; the client and // server must agree on the version. // parse_cert_version admits exactly one Ok value: 1 (missing key and @@ -269,7 +270,10 @@ async fn cmd_show( if require_valid { if let Err(reason) = verdict { - anyhow::bail!("certificate signature did not verify: {reason}"); + // Round 3: do not claim the signature failed when it was + // never checked. A version refusal means the payload + // shape is unknown, so no signature verification ran. + anyhow::bail!("certificate could not be verified: {reason}"); } // A valid signature proves internal consistency only: the payload was // signed by whatever key the certificate itself names. A hostile From 868505fdbce04b28a85738b7d16d77c96fb3e5b3 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Sun, 6 Sep 2026 07:58:53 +0600 Subject: [PATCH 8/9] fix(gl): route cert list through error_for_status; correct cert show example Round 4 reviewer findings on #26 split 3 cert compat: - P2 (cert.rs:113): cmd_list called .json() straight through on the cert list endpoint and let a gated 404 (whose body lacks a `certificates` key) fall into the empty-list branch, printing "No ref certificates" with exit 0. Wire cmd_list through the same error_for_status() gate that cmd_show and resolve_cert_id already use, and pin the new behavior with a load-bearing test (`cmd_list_surfaces_gated_404_as_err`) that drives cmd_list end to end against a 404 mock. cmd_show is unchanged. - P3 (README.md:314): the documented `gl cert show --verify` example bound the certificate ID as the REPO positional, because the parser is `gl cert show [OPTIONS] `. Insert the missing `` positional so the example matches the CLI parser. The original BREAKING CHANGE footer in bb553b67 carried the same shortened `gl cert show --verify` example; this commit's footer supersedes it with the canonical command, which is what release-note scrapers read. The round-2 commit message itself is left in history unchanged; the new footer is the one callers should rely on. BREAKING CHANGE: gl cert show --verify requires --expect-node ; the queried node's self-reported DID is no longer accepted as a cert anchor. --- README.md | 2 +- crates/gl/src/cert.rs | 42 +++++++++++++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 55d365af..5909c830 100644 --- a/README.md +++ b/README.md @@ -311,7 +311,7 @@ metadata local disk / optional S3 | DID | A user, agent, or node identity derived from an Ed25519 public key. | | HTTP Signature | RFC 9421 signature proving control of the DID key for write requests. | | Ref certificate | Signed record of a ref update. Useful for audit and replication. | -| Ref certificate versioning | The wire format is versioned; new certificates are currently issued as v1. `gl cert show --verify` requires `--expect-node `, and the queried node's self-reported DID is **not** accepted as a trust anchor. v2 is reserved for a future payload shape that carries the version inside the signed bytes; this client refuses to verify v2+ rather than guessing. | +| Ref certificate versioning | The wire format is versioned; new certificates are currently issued as v1. `gl cert show --verify` requires `--expect-node `, and the queried node's self-reported DID is **not** accepted as a trust anchor. v2 is reserved for a future payload shape that carries the version inside the signed bytes; this client refuses to verify v2+ rather than guessing. | | UCAN | Delegation token for future capability-based workflows. | | Peer announce | Node-to-node HTTP announcement of DID + public URL. | | Gossipsub | libp2p topic for ref-update events. | diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index b10df746..af8bb407 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -110,9 +110,12 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() let client = signed_client(&node, dir.as_deref()); let path = format!("/api/v1/repos/{owner}/{name}/certs"); - let resp: Value = client + let resp = client .get_authed(&path) .await? + .error_for_status() + .context("failed to list certificates")?; + let resp: Value = resp .json() .await .context("failed to list certificates")?; @@ -877,4 +880,41 @@ mod tests { cert_mock.assert_async().await; root_mock.assert_async().await; } + + /// P2 (reviewer round 3): a gated `GET /api/v1/repos/{o}/{r}/certs` + /// must surface as an Err, not as "No ref certificates …" with + /// exit 0. The pre-fix shape called `.json()` straight through and + /// let a 404 body that happened to lack `certificates` fall into + /// the empty-list branch, silently claiming an empty repository. + /// Driving `cmd_list` end to end against a 404 mock is what makes + /// this guard load-bearing: removing `error_for_status()` and the + /// test fails. + #[tokio::test] + async fn cmd_list_surfaces_gated_404_as_err() { + let mut server = mockito::Server::new_async().await; + let list_mock = server + .mock("GET", "/api/v1/repos/o/r/certs") + .with_status(404) + .with_header("content-type", "application/json") + // Body that lacks the `certificates` key — this is the + // exact shape the pre-fix code swallowed into the + // empty-list branch. A passing test here therefore proves + // the new error path is taken, not just that 404 produces + // a non-200. + .with_body(r#"{"error":"not found"}"#) + .expect(1) + .create_async() + .await; + + let err = cmd_list("o/r".to_string(), server.url(), None) + .await + .expect_err("a gated 404 on the cert list must surface as an Err, not an empty list"); + let msg = format!("{err:#}"); + assert!( + msg.contains("failed to list certificates"), + "the failure must point at the list endpoint, not at an empty result: {msg}" + ); + + list_mock.assert_async().await; + } } From 848ccfa5de660a575006aaf32fc856832f6eefc4 Mon Sep 17 00:00:00 2001 From: Gravirei Date: Sun, 6 Sep 2026 08:18:51 +0600 Subject: [PATCH 9/9] style(gl): fix fmt in cert list response parsing --- crates/gl/src/cert.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/gl/src/cert.rs b/crates/gl/src/cert.rs index af8bb407..f0915cb4 100644 --- a/crates/gl/src/cert.rs +++ b/crates/gl/src/cert.rs @@ -115,10 +115,7 @@ async fn cmd_list(repo: String, node: String, dir: Option) -> Result<() .await? .error_for_status() .context("failed to list certificates")?; - let resp: Value = resp - .json() - .await - .context("failed to list certificates")?; + let resp: Value = resp.json().await.context("failed to list certificates")?; let certs = resp["certificates"].as_array().cloned().unwrap_or_default();