From bdebff12390ab312507dca4174a6c0d0c48956a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Huang=20ChiaCheng=20=28=E9=BB=83=E5=AE=B6=E6=94=BF=29?= <1557604+suzuke@users.noreply.github.com> Date: Wed, 8 Jul 2026 12:03:17 +0800 Subject: [PATCH 1/2] feat(core): consolidate signing/verify primitives + scheme envelope (embedder P1a) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1a moves the security contract into agentic-git-core so signer and verifier share ONE source (no drift). Isolated to the agentic-git repo — the agend-terminal daemon wiring + daemon signer swap is P1b. - Move `ensure_key` (lock-free first-writer-wins `hard_link` provisioning) + its `open_new_0600` helper verbatim from the run CLI into core as the single key primitive; core gains a `getrandom` dep. The run CLI now calls `core::ensure_key` / `core::sign_binding` (its local copies removed). - Add `sign_binding` = ensure_key + sign — closes the reviewer4 hole where the low-level `sign` panicked on a fresh home. - `verify` becomes a typed `Result<(), VerifyError{MissingKey | MalformedTag | MacMismatch | UnsupportedScheme{..}}>` (was `bool`). It parses a self-describing tag ENVELOPE (`ag-hmac-sha256:v::`) BEFORE the MAC check, so an HMAC scheme skew is diagnosable — the shim maps `UnsupportedScheme` to a LOUD deny (`verify_sidecar`) instead of a silent "unbound". - Add `BINDING_FORMAT_VERSION` / `HMAC_ALGO_VERSION` consts and `envelope_tag` (the "can produce" capability). ORDERING INVARIANT (fleet-safety — the emphasized landmine): the DEFAULT emit stays the BARE hex tag (implicit scheme v1), byte-identical to the pre-P1a signer, so an unswapped shim and every on-disk sidecar keep verifying. The envelope is a capability core can parse + produce, NOT the default output (that flips in P2). RED-first proven: making `sign_binding` emit the envelope fails the byte-identical test against an independent python-HMAC oracle. reviewer4 acceptance conditions (RED-first): #2 a malformed `ag-hmac-sha256:` prefixed tag NEVER falls back to bare-hex legacy (extra colon / non-`v` algo / non-hex / unknown key-format -> MalformedTag/UnsupportedScheme); #3 a newer scheme or any tamper NEVER authenticates; #4 bare-hex legacy accepted (the migration window). (#1 lockfile-single-source is P1b — agend-terminal.) Regression: the run CLI's concurrent first-writer-wins is preserved — the existing session_mode race tests (driving the real `run` binary) stay green through the core reroute, plus a direct core-level N-thread race test. cargo test (workspace) 0-failed; clippy --workspace --all-targets clean; cargo package -p agentic-git-core green. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Huang ChiaCheng (黃家政) <1557604+suzuke@users.noreply.github.com> --- Cargo.lock | 1 + crates/agentic-git-core/Cargo.toml | 1 + crates/agentic-git-core/src/integrity_core.rs | 254 +++++++++++++++--- .../src/integrity_core/p1a_contract.rs | 206 ++++++++++++++ crates/agentic-git/src/cli.rs | 92 +------ crates/agentic-git/src/cli/tests.rs | 22 +- crates/agentic-git/src/main.rs | 29 +- crates/agentic-git/tests/session_mode.rs | 8 +- 8 files changed, 472 insertions(+), 141 deletions(-) create mode 100644 crates/agentic-git-core/src/integrity_core/p1a_contract.rs diff --git a/Cargo.lock b/Cargo.lock index a98fc36..bd6e25a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -19,6 +19,7 @@ dependencies = [ name = "agentic-git-core" version = "0.1.0" dependencies = [ + "getrandom", "hex", "hmac", "sha2", diff --git a/crates/agentic-git-core/Cargo.toml b/crates/agentic-git-core/Cargo.toml index dc519c9..2de143d 100644 --- a/crates/agentic-git-core/Cargo.toml +++ b/crates/agentic-git-core/Cargo.toml @@ -8,6 +8,7 @@ license.workspace = true repository.workspace = true [dependencies] +getrandom = "0.4" hex = "0.4" hmac = "0.13" sha2 = "0.11" diff --git a/crates/agentic-git-core/src/integrity_core.rs b/crates/agentic-git-core/src/integrity_core.rs index b00ad28..7e105f9 100644 --- a/crates/agentic-git-core/src/integrity_core.rs +++ b/crates/agentic-git-core/src/integrity_core.rs @@ -1,15 +1,24 @@ -//! #1651: shared HMAC-SHA256 integrity primitives — the VERIFY + key-read half, -//! used by BOTH the main daemon (`config_integrity`, which adds the sign side) -//! and the standalone `agentic-git` shim (which only verifies). The shim cannot -//! link the lib and `config_integrity` lives in the main-binary tree, so this -//! file is shared by source: `config_integrity` declares it as a module and the -//! shim pulls THE SAME file via `#[path = "../integrity_core.rs"] mod -//! integrity_core;`. One source ⟹ no signer/verifier algorithm drift (a drift -//! in a security check fails silently: too-loose → fail-open, too-strict → -//! false-deny). +//! #1651 / embedder P1a: shared HMAC-SHA256 integrity primitives — the SINGLE +//! source for key provisioning, signing, and verifying binding sidecars. Used by +//! the standalone `agentic-git` shim (verifies) AND any embedding system (a fleet +//! daemon, an orchestrator, tests) that SIGNS sidecars. One source ⟹ no +//! signer/verifier drift (a drift in a security check fails silently: too-loose → +//! fail-open, too-strict → false-deny). //! -//! Threat model is documented in `config_integrity` (the signer): same-uid -//! injection-containment defense-in-depth, NOT a security boundary. +//! P1a consolidates the contract: `ensure_key` (lock-free first-writer-wins key +//! provisioning, moved verbatim from the run CLI), `sign_binding` (provision + +//! sign — closes the reviewer4 hole where the low-level `sign` panicked without a +//! key), and a TYPED `verify` that parses a self-describing tag ENVELOPE before the +//! MAC check so an HMAC scheme skew is diagnosable, not a silent "unbound". +//! +//! **Ordering invariant (fleet-safety):** the DEFAULT emit stays the BARE hex tag +//! (implicit scheme v1) — byte-identical to the pre-P1a signer — so an unswapped +//! verifier and every on-disk sidecar keep verifying. The envelope is a CAPABILITY +//! (`envelope_tag` produces it, `verify` accepts it), NOT the default output; that +//! switch happens in a later phase. +//! +//! Threat model: same-uid injection-containment defense-in-depth, NOT a security +//! boundary (a same-uid agent could read the key and re-sign — #1653 ceiling). // #1934 (hmac 0.13): `new_from_slice` moved behind the explicit `KeyInit` // trait import (no longer implied by `Mac`). Construction + tag semantics are @@ -23,6 +32,24 @@ type HmacSha256 = Hmac; pub(crate) const KEY_LEN: usize = 32; pub(crate) const KEY_FILE: &str = ".config-integrity-key"; +/// The binding-FORMAT version — the schema of the binding JSON FIELDS, carried +/// INSIDE the HMAC-covered content (an injected agent cannot forge it without the +/// key). Bumped when the binding's fields change. Independent of `HMAC_ALGO_VERSION`. +pub const BINDING_FORMAT_VERSION: u32 = 1; + +/// The HMAC-algorithm / tag-format version — carried in the tag ENVELOPE, OUTSIDE +/// HMAC protection, so `verify` can read it BEFORE the MAC check. Bumped ONLY when +/// key-derivation / tag format / hash algo changes. Independent of the binding format. +pub const HMAC_ALGO_VERSION: u32 = 1; + +/// The scheme identifier that prefixes an enveloped tag (`:v::`). +/// A tag WITHOUT this prefix is a bare-hex legacy tag (implicit scheme v1). +pub const SCHEME_ID: &str = "ag-hmac-sha256"; + +/// The key-format component of the envelope: `raw` = the 32-byte raw key file read +/// by [`read_key`]. An enveloped tag naming any other key-format is unsupported. +pub const HMAC_KEY_FORMAT: &str = "raw"; + pub(crate) fn key_path(home: &Path) -> PathBuf { home.join(KEY_FILE) } @@ -33,31 +60,174 @@ pub(crate) fn read_key(home: &Path) -> Option<[u8; KEY_LEN]> { bytes.try_into().ok() } -/// Constant-time verify of `content` against the hex `tag`. Returns `false` on -/// any error (no key yet, malformed tag, mismatch) — callers treat `false` as -/// "not authentic" and fail closed. -pub fn verify(home: &Path, content: &[u8], tag: &str) -> bool { - let Some(key) = read_key(home) else { - return false; - }; - let Ok(mut mac) = HmacSha256::new_from_slice(&key) else { - return false; - }; +/// Open `path` write-only, `create_new` (fail if it exists), mode 0600 on unix. +/// A key/tmp file must be born with its restrictive mode AT OPEN TIME, not gain it +/// via a later chmod — `fs::write` + `set_permissions` leaves a umask-dependent +/// window where another local uid can read the HMAC key and forge binding sidecars. +/// `create_new` additionally refuses a pre-planted path. (Moved from the run CLI in +/// P1a so it travels with `ensure_key`.) +fn open_new_0600(path: &Path) -> std::io::Result { + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + } + opts.open(path) +} + +/// Atomic, lock-free first-writer-wins key provisioning (moved verbatim from the +/// run CLI in P1a — the concurrency-robust `hard_link` variant, now the single core +/// key primitive). Write to a unique temp file, fsync, then `hard_link` it into +/// place — `AlreadyExists` means we LOST the race, not that we failed; we discard +/// our tmp and defer to the survivor. The key path only ever appears fully written +/// (never a partial/truncated file), matching [`read_key`]'s exactly-32-bytes +/// contract. +/// +/// Idempotent: an existing exactly-[`KEY_LEN`] key is reused. A wrong-size key is a +/// hard `Err` — refuse to overwrite (fail-closed; a guarded session without a +/// signable binding must not silently degrade). +/// +/// We MUST NOT fall back to `rename` on `hard_link` failure: `hard_link` fails +/// `AlreadyExists` (first-writer-wins, never clobbers a live key) whereas `rename` +/// overwrites (last-writer-wins) — the clobber this exists to prevent. The correct +/// fallback for an exotic no-hard_link FS is `create_new` (O_EXCL) directly on the +/// key path (preserves first-writer-wins); not implemented as the default. +pub fn ensure_key(home: &Path) -> Result<(), String> { + let key_path = key_path(home); + if let Ok(meta) = std::fs::metadata(&key_path) { + if meta.len() as usize == KEY_LEN { + return Ok(()); // already provisioned — reuse. + } + return Err(format!( + "integrity key at {} exists but is not exactly {KEY_LEN} bytes (corrupt) — refusing \ + to overwrite; a guarded session without a signable binding must not silently \ + degrade. Remove it manually only if you are certain it is safe to regenerate.", + key_path.display() + )); + } + + let mut rand_suffix = [0u8; 8]; + getrandom::fill(&mut rand_suffix).map_err(|e| format!("getrandom: {e}"))?; + let tmp_path = home.join(format!( + "key.tmp.{}.{}", + std::process::id(), + hex::encode(rand_suffix) + )); + + let mut key = [0u8; KEY_LEN]; + getrandom::fill(&mut key).map_err(|e| format!("getrandom: {e}"))?; + { + use std::io::Write; + let mut f = open_new_0600(&tmp_path).map_err(|e| format!("create temp key: {e}"))?; + f.write_all(&key) + .map_err(|e| format!("write temp key: {e}"))?; + // fsync before the hard_link "publish" — the link must never observe + // a not-yet-durable write. + let _ = f.sync_all(); + } + + match std::fs::hard_link(&tmp_path, &key_path) { + Ok(()) => { + let _ = std::fs::remove_file(&tmp_path); + Ok(()) + } + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + // Someone else won the race; their key stands. + let _ = std::fs::remove_file(&tmp_path); + Ok(()) + } + Err(e) => { + let _ = std::fs::remove_file(&tmp_path); + Err(format!("hard_link key provisioning: {e}")) + } + } +} + +/// Why `verify` failed — a TYPED result (replaces the pre-P1a `bool`) so a caller +/// can distinguish a benign "not authentic / unbound" from an HMAC SCHEME SKEW that +/// is diagnosable. All variants are fail-closed (deny); only the DIAGNOSIS differs. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum VerifyError { + /// No integrity key at `home` yet. + MissingKey, + /// The tag is structurally invalid (bad envelope fields / extra colon / a + /// non-`v` algo / non-hex MAC, or a foreign non-hex prefix). NEVER a + /// silent bare-hex-legacy retry — that would be a fail-open scheme bypass. + MalformedTag, + /// The MAC did not match the content under this key (tampered / wrong key / + /// downgrade whose bytes don't authenticate). + MacMismatch, + /// The envelope names an algo/key-format this runtime does not implement (e.g. + /// a NEWER scheme) — reported BEFORE any MAC computation. The shim maps this to + /// a LOUD deny: signer and verifier were built from different core versions. + UnsupportedScheme { + tag_scheme: String, + runtime_scheme: String, + }, +} + +/// The scheme string THIS runtime implements, for `UnsupportedScheme` diagnostics. +fn runtime_scheme() -> String { + format!("{SCHEME_ID}:v{HMAC_ALGO_VERSION}:{HMAC_KEY_FORMAT}") +} + +/// Resolve a tag to its hex MAC, enforcing the scheme envelope. A tag carrying the +/// `:` prefix is parsed strictly (fail-closed, NEVER a legacy fallback); +/// a tag with no prefix and no colon is a bare-hex legacy tag (implicit scheme v1); +/// anything else (a colon but not our prefix) is malformed. +fn tag_to_hex(tag: &str) -> Result<&str, VerifyError> { + let tag = tag.trim(); + if let Some(body) = tag + .strip_prefix(SCHEME_ID) + .and_then(|r| r.strip_prefix(':')) + { + // Enveloped: v::. Malformed → fail-closed, never legacy. + let parts: Vec<&str> = body.split(':').collect(); + if parts.len() != 3 { + return Err(VerifyError::MalformedTag); // missing field / extra colon + } + let Some(algo) = parts[0].strip_prefix('v').and_then(|n| n.parse::().ok()) else { + return Err(VerifyError::MalformedTag); // non-`v` algo + }; + if algo != HMAC_ALGO_VERSION || parts[1] != HMAC_KEY_FORMAT { + return Err(VerifyError::UnsupportedScheme { + tag_scheme: format!("{SCHEME_ID}:v{algo}:{}", parts[1]), + runtime_scheme: runtime_scheme(), + }); + } + Ok(parts[2]) + } else if tag.contains(':') { + // A colon but not our scheme prefix = a malformed / foreign prefix. Must + // NOT fall back to bare-hex legacy (that would be a fail-open bypass). + Err(VerifyError::MalformedTag) + } else { + Ok(tag) // bare-hex legacy (implicit scheme v1) + } +} + +/// Constant-time verify of `content` against `tag` (a bare-hex legacy tag OR a +/// scheme envelope). Returns `Ok(())` iff authentic; every failure mode is a typed +/// [`VerifyError`] and fail-closed. The scheme envelope is parsed BEFORE the MAC +/// check so an `UnsupportedScheme` skew fires even when the MAC could never match. +pub fn verify(home: &Path, content: &[u8], tag: &str) -> Result<(), VerifyError> { + let key = read_key(home).ok_or(VerifyError::MissingKey)?; + let hex = tag_to_hex(tag)?; + let mut mac = HmacSha256::new_from_slice(&key).map_err(|_| VerifyError::MacMismatch)?; mac.update(content); - let Ok(tag_bytes) = hex::decode(tag.trim()) else { - return false; - }; - mac.verify_slice(&tag_bytes).is_ok() + let tag_bytes = hex::decode(hex).map_err(|_| VerifyError::MalformedTag)?; + mac.verify_slice(&tag_bytes) + .map_err(|_| VerifyError::MacMismatch) } -/// Reference signer: HMAC over the EXISTING home key (deliberately no key -/// generation side-effect — provisioning the key is the embedder's job). -/// The exact counterpart of the verifier above: an embedding system (daemon, -/// orchestrator, tests) signs binding sidecars with this, so signer and -/// verifier can never drift. +/// Reference signer: HMAC over the EXISTING home key, returned as a BARE hex tag. +/// Deliberately no key-generation side-effect — provisioning is [`sign_binding`]'s +/// (or the embedder's) job. The exact counterpart of [`verify`]'s legacy path. /// /// # Panics -/// Panics if the home key does not exist — callers provision it first. +/// Panics if the home key does not exist — callers provision it first (use +/// [`sign_binding`], which provisions then signs). pub fn sign(home: &Path, content: &[u8]) -> String { let key = read_key(home).expect("integrity key must exist before signing"); let mut mac = HmacSha256::new_from_slice(&key).expect("HMAC accepts any key length"); @@ -65,6 +235,25 @@ pub fn sign(home: &Path, content: &[u8]) -> String { hex::encode(mac.finalize().into_bytes()) } +/// Provision the key if missing, then sign — the safe public signer for an embedder +/// (daemon / orchestrator / run CLI). Closes the reviewer4 hole where the low-level +/// [`sign`] panicked on a fresh home. The output is the BARE hex tag (implicit +/// scheme v1), byte-identical to [`sign`] and to the pre-P1a signer, so an unswapped +/// verifier and existing on-disk sidecars keep verifying (the P1a ordering invariant). +pub fn sign_binding(home: &Path, content: &[u8]) -> Result { + ensure_key(home)?; + Ok(sign(home, content)) +} + +/// Wrap a bare hex MAC in the self-describing scheme envelope +/// (`:v::`). This is the +/// CAPABILITY the migration will switch the default emit to in a later phase; +/// `sign_binding` does NOT emit it yet (byte-identical bare hex is the default). +/// [`verify`] already accepts the enveloped form. +pub fn envelope_tag(hex: &str) -> String { + format!("{SCHEME_ID}:v{HMAC_ALGO_VERSION}:{HMAC_KEY_FORMAT}:{hex}") +} + /// #1934 cross-version pin: the HMAC-SHA256 output must be byte-identical /// across the RustCrypto stack upgrade (hmac 0.12→0.13, sha2 0.10→0.11, /// digest →0.11). The expected tag was generated BEFORE the upgrade and @@ -94,3 +283,6 @@ mod cross_version_pin_1934 { ); } } + +#[cfg(test)] +mod p1a_contract; diff --git a/crates/agentic-git-core/src/integrity_core/p1a_contract.rs b/crates/agentic-git-core/src/integrity_core/p1a_contract.rs new file mode 100644 index 0000000..40685d9 --- /dev/null +++ b/crates/agentic-git-core/src/integrity_core/p1a_contract.rs @@ -0,0 +1,206 @@ +//! Embedder P1a contract tests for `integrity_core`: the byte-identical ordering +//! invariant, the scheme envelope + typed `verify` (reviewer4 acceptance +//! conditions #2/#3/#4), `sign_binding` provisioning (reviewer4 hole), and the +//! first-writer-wins key race regression. + +use super::*; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +static SEQ: AtomicU64 = AtomicU64::new(0); + +fn tmp_home(tag: &str) -> PathBuf { + let n = SEQ.fetch_add(1, Ordering::Relaxed); + let p = std::env::temp_dir().join(format!("agcore-p1a-{tag}-{}-{n}", std::process::id())); + let _ = std::fs::remove_dir_all(&p); + std::fs::create_dir_all(&p).unwrap(); + p +} + +/// Write a fixed key so signing is deterministic. +fn write_key(home: &Path, key: &[u8]) { + std::fs::write(key_path(home), key).unwrap(); +} + +// ── The ORDERING INVARIANT (fleet-safety): default emit is BARE hex, byte- ── +// ── identical to the pre-P1a signer; an unswapped verifier keeps verifying. ── + +#[test] +fn sign_binding_emits_byte_identical_bare_hex() { + // Independent oracle (python hmac.new([7]*32, CONTENT, sha256).hexdigest()), + // mirroring the #1934 golden-fixture methodology. If `sign_binding` ever emits + // the envelope by default (the ordering landmine), the equality + no-colon + // assertions go RED. + let home = tmp_home("byte-id"); + write_key(&home, &[7u8; 32]); + const CONTENT: &[u8] = + b"agend-p1a byte-identical fixture: sign_binding emits bare hex (implicit scheme v1)"; + const GOLDEN: &str = "38a831c3949dfecc48223256eab15f9c59022454a822e95546450d3b7c8c20c3"; + + let sig = sign_binding(&home, CONTENT).expect("sign_binding"); + assert_eq!(sig, GOLDEN, "sign_binding must be byte-identical bare hex"); + assert!(!sig.contains(':'), "default emit must be BARE hex, not the envelope"); + assert_eq!(sig.len(), 64, "HMAC-SHA256 hex is 64 chars"); + // the low-level signer agrees, and the legacy verify path accepts it. + assert_eq!(sign(&home, CONTENT), sig, "sign_binding == low-level sign"); + assert_eq!(verify(&home, CONTENT, &sig), Ok(()), "legacy bare-hex verify accepts"); + + let _ = std::fs::remove_dir_all(&home); +} + +#[test] +fn sign_binding_provisions_then_signs_no_panic() { + // reviewer4 hole: the low-level `sign` panics on a fresh home; `sign_binding` + // must provision the key FIRST, then sign — no panic, and the output verifies. + let home = tmp_home("provision"); + assert!(read_key(&home).is_none(), "fresh home has no key"); + let sig = sign_binding(&home, b"body").expect("sign_binding must provision + sign"); + assert_eq!(read_key(&home).map(|k| k.len()), Some(32), "key now provisioned"); + assert_eq!(verify(&home, b"body", &sig), Ok(())); + let _ = std::fs::remove_dir_all(&home); +} + +// ── Envelope capability + backward-compat (reviewer4 cond #4: bare hex accepted) ── + +#[test] +fn verify_accepts_bare_hex_legacy_and_v1_envelope_roundtrip() { + let home = tmp_home("roundtrip"); + write_key(&home, &[9u8; 32]); + let hex = sign(&home, b"payload"); + // cond #4: a no-prefix bare-hex tag verifies (the legacy window). + assert_eq!(verify(&home, b"payload", &hex), Ok(())); + // the envelope capability round-trips: `envelope_tag` output verifies too. + let env = envelope_tag(&hex); + assert_eq!(env, format!("ag-hmac-sha256:v1:raw:{hex}")); + assert_eq!(verify(&home, b"payload", &env), Ok(()), "v1 envelope must verify"); + let _ = std::fs::remove_dir_all(&home); +} + +// ── reviewer4 cond #2: a malformed PREFIXED tag must NEVER fall back to legacy ── + +#[test] +fn verify_malformed_prefixed_tag_never_falls_to_legacy() { + let home = tmp_home("malformed"); + write_key(&home, &[3u8; 32]); + let valid = sign(&home, b"data"); // a hex that WOULD verify as bare-hex legacy + + // The load-bearing case: wrapping the valid hex in a MALFORMED envelope must + // NOT silently retry it as bare-hex legacy (that would be a fail-open bypass). + let cases = [ + format!("ag-hmac-sha256:v1:raw:{valid}:extra"), // extra colon / field + format!("ag-hmac-sha256:x1:raw:{valid}"), // non-`v` algo + "ag-hmac-sha256:v1:raw:zznothex".to_string(), // non-hex MAC + format!("ag-hmac-sha256:{valid}"), // missing algo/key-format fields + "garbage:prefix:xyz".to_string(), // foreign prefix (has colon) + ]; + for c in cases { + let r = verify(&home, b"data", &c); + assert!(r.is_err(), "malformed prefixed tag must fail closed: {c:?} -> {r:?}"); + assert_ne!(r, Ok(()), "must NEVER authenticate: {c:?}"); + } + // unknown key-format is a recognized-but-unsupported scheme. + assert!(matches!( + verify(&home, b"data", &format!("ag-hmac-sha256:v1:weird:{valid}")), + Err(VerifyError::UnsupportedScheme { .. }) + )); + let _ = std::fs::remove_dir_all(&home); +} + +// ── reviewer4 cond #3: downgrade / newer-scheme / tamper must NEVER authenticate ── + +#[test] +fn verify_unsupported_newer_scheme_and_tamper_never_ok() { + let home = tmp_home("downgrade"); + write_key(&home, &[5u8; 32]); + let hex = sign(&home, b"msg"); + + // a NEWER algo the runtime doesn't implement → UnsupportedScheme BEFORE any MAC + // check (fires even though the MAC bytes are otherwise valid), never Ok. + let newer = format!("ag-hmac-sha256:v2:raw:{hex}"); + assert!(matches!( + verify(&home, b"msg", &newer), + Err(VerifyError::UnsupportedScheme { tag_scheme, runtime_scheme }) + if tag_scheme == "ag-hmac-sha256:v2:raw" && runtime_scheme == "ag-hmac-sha256:v1:raw" + )); + + // tamper: a bit-flipped MAC (bare AND enveloped) → MacMismatch, never Ok. + let mut bytes = hex::decode(&hex).unwrap(); + bytes[0] ^= 0x01; + let flipped = hex::encode(&bytes); + assert_eq!(verify(&home, b"msg", &flipped), Err(VerifyError::MacMismatch)); + assert_eq!( + verify(&home, b"msg", &envelope_tag(&flipped)), + Err(VerifyError::MacMismatch), + "a wrong MAC wrapped in a valid envelope still fails the MAC check" + ); + // right MAC, wrong content → MacMismatch. + assert_eq!(verify(&home, b"other", &hex), Err(VerifyError::MacMismatch)); + let _ = std::fs::remove_dir_all(&home); +} + +#[test] +fn verify_missing_key_is_typed() { + let home = tmp_home("nokey"); + assert_eq!(verify(&home, b"x", "deadbeef"), Err(VerifyError::MissingKey)); + let _ = std::fs::remove_dir_all(&home); +} + +// ── Regression: `ensure_key` first-writer-wins under concurrency (direct core) ── + +#[test] +fn ensure_key_concurrent_threads_one_key_survives() { + let home = tmp_home("race"); + let n = 16; + let results: Vec<_> = std::thread::scope(|s| { + (0..n) + .map(|_| { + let h = home.clone(); + s.spawn(move || ensure_key(&h)) + }) + .collect::>() + .into_iter() + .map(|j| j.join().unwrap()) + .collect() + }); + assert!(results.iter().all(|r| r.is_ok()), "every racer succeeds: {results:?}"); + // exactly ONE 32-byte key survives, and it reads back cleanly. + let key = read_key(&home).expect("a key must exist after the race"); + assert_eq!(key.len(), 32); + assert_eq!( + std::fs::metadata(key_path(&home)).unwrap().len(), + 32, + "one 32-byte key survives — no partial/torn file" + ); + // idempotent: a follow-up call reuses it, bytes unchanged. + ensure_key(&home).unwrap(); + assert_eq!(read_key(&home), Some(key)); + let _ = std::fs::remove_dir_all(&home); +} + +#[test] +fn ensure_key_refuses_corrupt_size_fail_closed() { + let home = tmp_home("corrupt"); + write_key(&home, &[1u8; 16]); // wrong size + assert!( + ensure_key(&home).is_err(), + "a wrong-size key must be a hard Err (refuse to overwrite), not a silent regen" + ); + let _ = std::fs::remove_dir_all(&home); +} + +/// Δ2 regression (moved from the run CLI with `open_new_0600` in P1a): the temp +/// key file must be 0600 FROM BIRTH — reverting to `fs::write` + post-hoc chmod +/// turns this RED (the freshly created file would carry umask-default perms at +/// open time). And `create_new` must refuse a pre-planted path. +#[cfg(unix)] +#[test] +fn key_tmp_file_is_0600_at_creation() { + use std::os::unix::fs::PermissionsExt; + let home = tmp_home("0600"); + let p = home.join("k.tmp"); + let f = open_new_0600(&p).expect("create"); + let mode = f.metadata().unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "temp key must be born 0600, got {mode:o}"); + assert!(open_new_0600(&p).is_err(), "create_new must refuse existing path"); + let _ = std::fs::remove_dir_all(&home); +} diff --git a/crates/agentic-git/src/cli.rs b/crates/agentic-git/src/cli.rs index e940b18..6857b99 100644 --- a/crates/agentic-git/src/cli.rs +++ b/crates/agentic-git/src/cli.rs @@ -15,15 +15,6 @@ use std::process::Command; use agentic_git_core::integrity_core; -/// Disk-contract filename for the shared HMAC key (mirrors -/// `agentic_git_core::integrity_core::KEY_FILE`, which is `pub(crate)` to -/// that crate and therefore not reachable from here — this is the *public* -/// disk contract the INVARIANT clause documents, not a private implementation -/// detail we're reaching into). `sign`/`verify` themselves are the only -/// public API session mode calls. -const KEY_FILE: &str = ".config-integrity-key"; -const KEY_LEN: usize = 32; - pub fn cli_main() -> ! { let args: Vec = std::env::args().skip(1).collect(); match args.first().map(String::as_str) { @@ -474,64 +465,7 @@ fn provision_home(home: &Path) -> Result<(), String> { let p = home.join(d); std::fs::create_dir_all(&p).map_err(|e| format!("create {}: {e}", p.display()))?; } - ensure_key(home) -} - -/// Δ2: atomic, lock-free first-writer-wins key provisioning. Write to a -/// unique temp file, fsync, then `hard_link` it into place — `AlreadyExists` -/// means we lost the race, not that we failed; we discard our tmp and defer -/// to the survivor. The key path only ever appears fully written (never a -/// partial/truncated file), matching `integrity_core::read_key`'s -/// exactly-32-bytes contract. -fn ensure_key(home: &Path) -> Result<(), String> { - let key_path = home.join(KEY_FILE); - if let Ok(meta) = std::fs::metadata(&key_path) { - if meta.len() as usize == KEY_LEN { - return Ok(()); // already provisioned — reuse. - } - return Err(format!( - "integrity key at {} exists but is not exactly {KEY_LEN} bytes (corrupt) — refusing \ - to overwrite; a guarded session without a signable binding must not silently \ - degrade. Remove it manually only if you are certain it is safe to regenerate.", - key_path.display() - )); - } - - let mut rand_suffix = [0u8; 8]; - getrandom::fill(&mut rand_suffix).map_err(|e| format!("getrandom: {e}"))?; - let tmp_path = home.join(format!( - "key.tmp.{}.{}", - std::process::id(), - hex_lower(&rand_suffix) - )); - - let mut key = [0u8; KEY_LEN]; - getrandom::fill(&mut key).map_err(|e| format!("getrandom: {e}"))?; - { - use std::io::Write; - let mut f = open_new_0600(&tmp_path).map_err(|e| format!("create temp key: {e}"))?; - f.write_all(&key) - .map_err(|e| format!("write temp key: {e}"))?; - // fsync before the hard_link "publish" — the link must never observe - // a not-yet-durable write. - let _ = f.sync_all(); - } - - match std::fs::hard_link(&tmp_path, &key_path) { - Ok(()) => { - let _ = std::fs::remove_file(&tmp_path); - Ok(()) - } - Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { - // Someone else won the race; their key stands. - let _ = std::fs::remove_file(&tmp_path); - Ok(()) - } - Err(e) => { - let _ = std::fs::remove_file(&tmp_path); - Err(format!("hard_link key provisioning: {e}")) - } - } + integrity_core::ensure_key(home) } // ── Preconditions (step 1) ───────────────────────────────────────────── @@ -1033,7 +967,13 @@ fn run_cmd(raw_args: &[String]) -> ! { eprintln!("agentic-git: run: write binding.json: {e}"); std::process::exit(1); } - let sig = integrity_core::sign(&home, content.as_bytes()); + let sig = match integrity_core::sign_binding(&home, content.as_bytes()) { + Ok(s) => s, + Err(e) => { + eprintln!("agentic-git: run: sign binding: {e}"); + std::process::exit(1); + } + }; if let Err(e) = std::fs::write(runtime_dir.join("binding.json.sig"), sig) { eprintln!("agentic-git: run: write binding.json.sig: {e}"); std::process::exit(1); @@ -1085,19 +1025,3 @@ fn run_cmd(raw_args: &[String]) -> ! { #[cfg(test)] mod tests; - -/// Impl-review finding (Δ2 literalism): secret-bearing files must carry -/// their restrictive mode AT OPEN TIME, not gain it via a later chmod — -/// `fs::write` + `set_permissions` leaves a umask-dependent window where -/// another local uid can read the HMAC key and forge binding sidecars. -/// `create_new` additionally refuses a pre-planted path. -fn open_new_0600(path: &Path) -> std::io::Result { - let mut opts = std::fs::OpenOptions::new(); - opts.write(true).create_new(true); - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt; - opts.mode(0o600); - } - opts.open(path) -} diff --git a/crates/agentic-git/src/cli/tests.rs b/crates/agentic-git/src/cli/tests.rs index 80bb994..c61ba91 100644 --- a/crates/agentic-git/src/cli/tests.rs +++ b/crates/agentic-git/src/cli/tests.rs @@ -207,23 +207,5 @@ fn check_reuse_branch_mismatch_is_hard_error() { std::fs::remove_dir_all(&home).ok(); } -/// Impl-review regression (Δ2): the temp key file must be 0600 FROM BIRTH — -/// reverting `open_new_0600` to `fs::write` + post-hoc chmod turns this RED -/// (the freshly created file would carry umask-default perms at open time). -#[cfg(unix)] -#[test] -fn key_tmp_file_is_0600_at_creation_review4() { - use std::os::unix::fs::PermissionsExt; - let dir = std::env::temp_dir().join(format!("agit-0600-{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - std::fs::create_dir_all(&dir).unwrap(); - let p = dir.join("k.tmp"); - let f = super::open_new_0600(&p).expect("create"); - // Stat while the handle is still open and BEFORE any write/chmod could - // have happened: mode must already be 0600. - let mode = f.metadata().unwrap().permissions().mode() & 0o777; - assert_eq!(mode, 0o600, "temp key must be born 0600, got {mode:o}"); - // And create_new must refuse a pre-planted path. - assert!(super::open_new_0600(&p).is_err(), "create_new must refuse existing path"); - let _ = std::fs::remove_dir_all(&dir); -} +// (Δ2 `open_new_0600` 0600-at-birth test moved to agentic-git-core with the helper +// in P1a — see integrity_core::p1a_contract::key_tmp_file_is_0600_at_creation.) diff --git a/crates/agentic-git/src/main.rs b/crates/agentic-git/src/main.rs index fe1b757..3030c48 100644 --- a/crates/agentic-git/src/main.rs +++ b/crates/agentic-git/src/main.rs @@ -505,6 +505,31 @@ struct Binding { worktree: Option, } +/// Fail-closed HMAC verify with LOUD diagnosis on scheme skew (embedder P1a). +/// Preserves today's "anything but authentic → not authentic" posture (every +/// non-`Ok` fails closed, identical to the former `!verify(..)` bool), but an +/// `UnsupportedScheme` — the sidecar was signed with an HMAC scheme this shim was +/// NOT built to verify (signer/verifier from different `agentic-git-core` versions) +/// — is surfaced LOUD to stderr instead of a silent "unbound", so the drift is +/// diagnosable rather than a mysterious fleet-wide unbind. +fn verify_sidecar(home: &str, content: &[u8], tag: &str) -> bool { + match integrity_core::verify(Path::new(home), content, tag) { + Ok(()) => true, + Err(integrity_core::VerifyError::UnsupportedScheme { + tag_scheme, + runtime_scheme, + }) => { + eprintln!( + "agentic-git: HMAC scheme skew — binding signed with scheme {tag_scheme}, this \ + shim implements {runtime_scheme}; signer and verifier were built from different \ + agentic-git-core versions — rebuild-together / rebind." + ); + false + } + Err(_) => false, + } +} + fn read_binding(home: &str, agent: &str) -> Binding { let dir = PathBuf::from(home).join("runtime").join(agent); let path = dir.join("binding.json"); @@ -521,7 +546,7 @@ fn read_binding(home: &str, agent: &str) -> Binding { // blind-write, NOT a security boundary: a same-uid agent could read the key // and re-sign (accepted); true sealing needs OS-isolation (parked #1653). let tag = std::fs::read_to_string(dir.join("binding.json.sig")).unwrap_or_default(); - if !integrity_core::verify(Path::new(home), content.as_bytes(), &tag) { + if !verify_sidecar(home, content.as_bytes(), &tag) { return Binding::default(); } let v: serde_json::Value = match serde_json::from_str(&content) { @@ -2175,7 +2200,7 @@ fn load_protected_refs(home: &str) -> Vec { }; let tag = std::fs::read_to_string(PathBuf::from(home).join("policy.toml.sig")).unwrap_or_default(); - if !integrity_core::verify(Path::new(home), content.as_bytes(), &tag) { + if !verify_sidecar(home, content.as_bytes(), &tag) { return refs; // tampered / unsigned → fail-closed (override ignored) } refs.extend(parse_protected_refs(&content)); diff --git a/crates/agentic-git/tests/session_mode.rs b/crates/agentic-git/tests/session_mode.rs index 95325b4..8d8770f 100644 --- a/crates/agentic-git/tests/session_mode.rs +++ b/crates/agentic-git/tests/session_mode.rs @@ -212,7 +212,7 @@ fn test3_binding_sidecar_verifies_and_tamper_is_unbound_deny() { let content = std::fs::read_to_string(dir.join("binding.json")).expect("read binding"); let sig = std::fs::read_to_string(dir.join("binding.json.sig")).expect("read sidecar"); assert!( - agentic_git_core::integrity_core::verify(&home, content.as_bytes(), &sig), + agentic_git_core::integrity_core::verify(&home, content.as_bytes(), &sig).is_ok(), "freshly written binding must verify" ); @@ -221,7 +221,7 @@ fn test3_binding_sidecar_verifies_and_tamper_is_unbound_deny() { assert_ne!(tampered, content); std::fs::write(dir.join("binding.json"), &tampered).unwrap(); assert!( - !agentic_git_core::integrity_core::verify(&home, tampered.as_bytes(), &sig), + agentic_git_core::integrity_core::verify(&home, tampered.as_bytes(), &sig).is_err(), "tampered binding must fail verify" ); @@ -497,7 +497,7 @@ fn delta2_concurrent_first_runs_race_key_both_bindings_verify() { let content = std::fs::read_to_string(dir.join("binding.json")).unwrap(); let sig = std::fs::read_to_string(dir.join("binding.json.sig")).unwrap(); assert!( - agentic_git_core::integrity_core::verify(&home, content.as_bytes(), &sig), + agentic_git_core::integrity_core::verify(&home, content.as_bytes(), &sig).is_ok(), "{agent}'s binding must verify against the one surviving key" ); } @@ -740,7 +740,7 @@ fn concurrent_worktree_provision_stress_all_succeed() { let content = std::fs::read_to_string(dir.join("binding.json")).unwrap(); let sig = std::fs::read_to_string(dir.join("binding.json.sig")).unwrap(); assert!( - agentic_git_core::integrity_core::verify(&home, content.as_bytes(), &sig), + agentic_git_core::integrity_core::verify(&home, content.as_bytes(), &sig).is_ok(), "{agent}'s binding must verify against the surviving key" ); } From ad3600795f7564bde24534be1f9ddb92e59f830f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Huang=20ChiaCheng=20=28=E9=BB=83=E5=AE=B6=E6=94=BF=29?= <1557604+suzuke@users.noreply.github.com> Date: Wed, 8 Jul 2026 12:16:12 +0800 Subject: [PATCH 2/2] fix(release): bump agentic-git-core to 0.2.0 + package interdependent crates together (reviewer4 D1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1a added new public API (`VerifyError`, `ensure_key`, `sign_binding`) + a breaking `verify` bool->Result change to agentic-git-core WITHOUT bumping its version, so the version/package contract was incoherent: `cargo package -p agentic-git` resolves `agentic-git-core = "0.1.0"` from crates.io (the OLD published API) and fails to compile the new call sites — the PR's ubuntu CI failure. A released binary depending on published core 0.1.0 could not build with the new code. - Bump the workspace (core) version 0.1.0 -> 0.2.0 — a 0.x minor bump for the new + breaking API, exactly what the workspace comment prescribes ("core bumps when its API changes"). The shim keeps its own independent 0.2.2. - Update the binary's dep requirement to `agentic-git-core { path, version = "0.2.0" }`. - CI: package the two interdependent crates in ONE `cargo package -p agentic-git-core -p agentic-git --allow-dirty` so the binary's verify build resolves core from the just-packaged sibling (build-from-workspace), NOT the older published registry version. No crates.io publish happens (the release-time publish stays manual / operator-gated). dev3 VERIFIED the crypto surface and reviewer4 confirmed no crypto fail-open — this is purely the package/version contract fix; no core logic changed. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Huang ChiaCheng (黃家政) <1557604+suzuke@users.noreply.github.com> --- .github/workflows/ci.yml | 11 +++++++---- Cargo.lock | 2 +- Cargo.toml | 4 +++- crates/agentic-git/Cargo.toml | 2 +- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ccb78ab..f96c64a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,10 +23,13 @@ jobs: if: matrix.os == 'ubuntu-latest' # Publish gate: `cargo package` with the verify build catches crates that # can't stand alone (e.g. an `include_str!` reaching outside the crate) — - # the class of bug that silently half-published a release once. - - run: cargo package -p agentic-git-core --allow-dirty - if: matrix.os == 'ubuntu-latest' - - run: cargo package -p agentic-git --allow-dirty + # the class of bug that silently half-published a release once. Package BOTH + # interdependent crates in ONE invocation so the binary's verify build + # resolves `agentic-git-core` from the just-packaged sibling + # (build-from-workspace), NOT the older published registry version — required + # once the binary uses core API not yet released (embedder migration P1a). + # No crates.io publish happens here; the release-time publish stays manual. + - run: cargo package -p agentic-git-core -p agentic-git --allow-dirty if: matrix.os == 'ubuntu-latest' # Windows has cfg(windows) code paths (copy-instead-of-symlink shim wiring, diff --git a/Cargo.lock b/Cargo.lock index bd6e25a..8142324 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -17,7 +17,7 @@ dependencies = [ [[package]] name = "agentic-git-core" -version = "0.1.0" +version = "0.2.0" dependencies = [ "getrandom", "hex", diff --git a/Cargo.toml b/Cargo.toml index c190dd0..6c211cf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,9 @@ members = ["crates/agentic-git-core", "crates/agentic-git"] # agentic-git-core is the stable contract crate — it stays on its own version # and only bumps when its API changes. The shim (crates/agentic-git) sets its # own `version` explicitly, so the two can move independently. -version = "0.1.0" +# 0.2.0 (embedder P1a): new public API (VerifyError, ensure_key, sign_binding, +# typed verify) + a breaking `verify` bool→Result change = a 0.x minor bump. +version = "0.2.0" edition = "2021" rust-version = "1.88" license = "Apache-2.0" diff --git a/crates/agentic-git/Cargo.toml b/crates/agentic-git/Cargo.toml index 83d9ae1..b08faa7 100644 --- a/crates/agentic-git/Cargo.toml +++ b/crates/agentic-git/Cargo.toml @@ -8,7 +8,7 @@ license.workspace = true repository.workspace = true [dependencies] -agentic-git-core = { path = "../agentic-git-core", version = "0.1.0" } +agentic-git-core = { path = "../agentic-git-core", version = "0.2.0" } chrono = "0.4" serde_json = "1" which = "8"