diff --git a/crates/did-git-sign/README.md b/crates/did-git-sign/README.md index 209fa9a..d22ac3b 100644 --- a/crates/did-git-sign/README.md +++ b/crates/did-git-sign/README.md @@ -15,14 +15,23 @@ git calls `did-git-sign` with the commit data on stdin. The tool: 4. Produces an SSH signature (PROTOCOL.sshsig format) and writes it to stdout 5. Zeroizes the key material from memory -Your DID verification method ID (e.g. `did:webvh:abc:example.com#key-0`) is used -as the git `user.email`, linking every commit to your decentralized identity. +Your DID verification method ID (e.g. `did:webvh:abc:example.com#key-0`) is +recorded in a `Signed-by-DID:` git trailer, linking every commit to your +decentralized identity. -That field is load-bearing, not decorative. An sshsig blob carries a raw Ed25519 -key and **no identity**, so the committer header is the only place a commit +That trailer is load-bearing, not decorative. An sshsig blob carries a raw +Ed25519 key and **no identity**, so the trailer is the only place a commit states which DID signed it — [`verify-trust`][verify-trust] reads it, resolves -that DID, and requires it to publish the signing key. A commit whose committer -is an ordinary address fails CI as `noSignerDid` however valid its signature. +that DID, and requires it to publish the signing key. A commit carrying no DID +claim fails CI as `noSignerDid` however valid its signature. + +The trailer sits inside the commit message, which is part of the payload the +signature covers, so it is as tamper-evident as the committer header was. It +lives there rather than in `user.email` so that `user.email` can stay an +ordinary address, which is what GitHub and GitLab match commits against when +attributing them to an account. A `commit-msg` hook installed by `init` writes +it; older commits that carry the DID in `user.email` still verify, through a +fallback in `verify-trust`. Signing therefore refuses when the committer names a DID other than the key's; see [Selecting which community persona signs](#selecting-which-community-persona-signs). @@ -66,13 +75,16 @@ did-git-sign init --global --vta-did did:webvh:scid:your-vta.example.com Saves config to `~/.config/did-git-sign/` and sets global git config. -This also sets `user.email` to your DID key id for **every repository on the -machine** — that is the identity your commits claim, and it must match the key -that signs them. Right for one community; wrong for two, and quietly so, since -commits in the other community would claim this DID. `init` prints the -per-remote alternative when you use `--global`; see +This also sets `did-git-sign.key` and `core.hooksPath` for **every repository +on the machine** — that pair decides the identity your commits claim, and it +must match the key that signs them. Right for one community; wrong for two, and +quietly so, since commits in the other community would claim this DID. `init` +prints the per-remote alternative when you use `--global`; see [Selecting which community persona signs](#selecting-which-community-persona-signs). +`init` refuses to take `core.hooksPath` if something else already owns it +(husky, lefthook, pre-commit), rather than silently stopping those hooks. + ### Non-interactive Name the persona and key to skip the picker (and `--yes` to skip the @@ -113,9 +125,20 @@ The `init` command performs the following: - `gpg.ssh.defaultKeyFile = ` - `commit.gpgsign = true` - `user.signingKey = ` - - `user.email = ` — the commit's identity claim; see below + - `did-git-sign.key = ` — selects the signing persona *and* is + the claim the `commit-msg` hook writes into the trailer; see below + - `core.hooksPath = ` — see below - `user.name = ` (if provided) + + `user.email` is left alone: it stays an ordinary address so forges can + attribute your commits to your account. 5. **Creates an `allowed_signers` file** for signature verification and sets `gpg.ssh.allowedSignersFile` +6. **Installs a `commit-msg` hook** that appends the `Signed-by-DID:` trailer. + Because `core.hooksPath` is a single slot, the hook directory it installs + also carries a delegating stub for every other standard hook, each of which + execs the repository's own `.git/hooks/` — so hooks you already have, + and hooks you add later, keep running. `uninstall` removes the directory and + unsets `core.hooksPath`. ## Usage @@ -169,25 +192,27 @@ in the keyring (i.e. you ran `init` for that persona); otherwise signing fails with a clear message rather than silently signing as a different persona. ```bash -# One commit as a specific persona (move user.email with it — see below): -DID_GIT_SIGN_KEY=did:webvh:abc:example.com#key-1 \ - git -c user.email=did:webvh:abc:example.com#key-1 commit -m "…" +# One commit as a specific persona: +DID_GIT_SIGN_KEY=did:webvh:abc:example.com#key-1 git commit -m "…" # Pin a persona for this repository: -git config did-git-sign.key did:webvh:abc:example.com#key-1 -git config user.email did:webvh:abc:example.com#key-1 +git config did-git-sign.key did:webvh:abc:example.com#key-1 ``` -**The persona and the committer must agree.** The key selection above chooses -what signs; `user.email` chooses what the commit *claims*. Naming different -DIDs produces a commit that cannot verify — the claimed DID does not publish -the key that signed — so signing refuses outright, naming both halves, rather -than writing a commit that fails in CI as `unknownKey`. +**One setting, so the persona and the claim cannot drift.** The `commit-msg` +hook reads the same selector the signer does, in the same order — +`DID_GIT_SIGN_KEY`, then `did-git-sign.key` — so whatever picks the key also +writes the claim. This is why the second `user.email` line each example used to +carry is gone: there is nothing left to keep in step by hand. + +Signing still refuses a commit whose claim and key disagree, naming both +halves, rather than writing one that fails in CI as `unknownKey`. That now only +happens if you write a `Signed-by-DID:` trailer yourself, or commit with the +hook bypassed (`--no-verify`) in a repo whose `user.email` is a different DID. For contributors in more than one community, do not manage this per repository by hand: a `git config --local` you forget does not error, it signs as the -wrong community. Use git's conditional includes, one file per community, with -both settings together so they cannot drift: +wrong community. Use git's conditional includes, one file per community: ```ini # ~/.gitconfig @@ -197,8 +222,6 @@ both settings together so they cannot drift: ```ini # ~/.config/git/community-openvtc -[user] - email = did:webvh:abc:example.com#key-0 [did-git-sign] key = did:webvh:abc:example.com#key-0 ``` diff --git a/crates/did-git-sign/src/init.rs b/crates/did-git-sign/src/init.rs index deac38b..352b2b3 100644 --- a/crates/did-git-sign/src/init.rs +++ b/crates/did-git-sign/src/init.rs @@ -104,6 +104,25 @@ pub fn install(args: InstallArgs<'_>) -> Result { let config_dir = config_path.parent().unwrap_or(Path::new(".")); setup_allowed_signers(config_dir, &entry, args.global)?; + // Install the hook dispatcher that injects the Signed-by-DID trailer while + // preserving any repository hooks shadowed by core.hooksPath. + // + // Non-fatal, because the rest of the install is still worth keeping — but + // loudly so. Without the hook, commits carry no DID claim, and `sign` + // refuses them rather than writing something CI would reject as + // `noSignerDid`. Saying only "no trailer" would understate that: signing + // does not degrade here, it stops. + if let Err(e) = install_hook_dispatcher(args.global) { + eprintln!( + "warning: could not install the git hook that writes the Signed-by-DID trailer:\n \ + {e}\n \ + Until this is resolved, `git commit` will refuse to sign in this repository: \ + a commit with no DID claim cannot be verified. Resolve the conflict above and \ + re-run `did-git-sign init`, or set user.email to '{}' as the legacy claim.", + cfg.did_key_id + ); + } + // If we just shadowed a global user.signingKey with a local one, tell // the caller so they can surface it. Best-effort — failures here are // non-fatal. @@ -235,11 +254,21 @@ pub fn uninstall(global: bool, did_key_id: &str) -> Result { "gpg.ssh.defaultKeyFile", "gpg.ssh.allowedSignersFile", "commit.gpgsign", + "did-git-sign.key", ] { if git_config_unset(scope, key) { summary.git_config_keys_unset.push(key.to_string()); } } + match unset_did_git_sign_hooks_path(scope, global) { + Ok(true) => summary + .git_config_keys_unset + .push("core.hooksPath".to_string()), + Ok(false) => {} + Err(e) => summary + .warnings + .push(format!("could not inspect core.hooksPath: {e}")), + } Ok(summary) } @@ -275,6 +304,40 @@ fn git_config_unset(scope: &str, key: &str) -> bool { .unwrap_or(false) } +fn unset_did_git_sign_hooks_path(scope: &str, global: bool) -> Result { + let Some(expected) = expected_hooks_dir(global)? else { + return Ok(false); + }; + let Some(configured) = git_config_get(scope, "core.hooksPath")? else { + return Ok(false); + }; + if Path::new(configured.trim()) == expected { + return Ok(git_config_unset(scope, "core.hooksPath")); + } + Ok(false) +} + +fn expected_hooks_dir(global: bool) -> Result> { + if global { + return Ok(Some( + dirs::config_dir() + .context("cannot determine config directory")? + .join("did-git-sign") + .join("hooks"), + )); + } + + let output = Command::new("git") + .args(["rev-parse", "--absolute-git-dir"]) + .output() + .context("failed to find .git directory")?; + if !output.status.success() { + return Ok(None); + } + let git_dir = PathBuf::from(String::from_utf8_lossy(&output.stdout).trim()); + Ok(Some(git_dir.join("did-git-sign-hooks"))) +} + /// Initialize git configuration for DID-based SSH signing. pub fn setup_git(config_path: &Path, cfg: &SigningConfig, global: bool) -> Result<()> { let scope = if global { "--global" } else { "--local" }; @@ -305,19 +368,14 @@ pub fn setup_git(config_path: &Path, cfg: &SigningConfig, global: bool) -> Resul // Enable commit signing by default git_config(scope, "commit.gpgsign", "true")?; - // The committer identity IS the DID claim. An sshsig blob carries a raw - // Ed25519 key and no identity, so `user.email` is the only place a commit - // states which DID signed it — `verify-trust` reads it from the committer - // header (inside the payload the signature covers), resolves that DID, and - // requires it to publish the signing key. Left unset, every commit fails - // the CI check as `noSignerDid` however valid its signature. + // The committer identity IS the DID claim. With the Signed-by-DID trailer + // flow, the DID is injected as a trailer by the commit-msg hook rather + // than set as user.email. This lets user.email stay a normal email for + // git-host attribution (GitLab/GitHub account linking). // - // This was removed once, on the grounds that git's own SSH verification - // uses the allowed_signers principal rather than user.email. That reasoning - // does not hold either way round: `allowed_signers_entry` writes the - // principal as `did_key_id`, and git matches principals against the - // committer email — so leaving it unset breaks the local check too. - git_config(scope, "user.email", &cfg.did_key_id)?; + // For backwards compatibility, also store the DID in did-git-sign.key + // git config so the hook can read it. + git_config(scope, "did-git-sign.key", &cfg.did_key_id)?; // Optionally set user.name if let Some(name) = &cfg.user_name { @@ -411,6 +469,30 @@ fn git_config(scope: &str, key: &str, value: &str) -> Result<()> { Ok(()) } +/// Read one git config value. A missing key is not an error. +fn git_config_get(scope: &str, key: &str) -> Result> { + let output = Command::new("git") + .arg("config") + .arg(scope) + .arg("--get") + .arg(key) + .output() + .context("failed to run git config")?; + + if output.status.success() { + return Ok(Some( + String::from_utf8_lossy(&output.stdout) + .trim_end_matches(['\r', '\n']) + .to_string(), + )); + } + if output.status.code() == Some(1) { + return Ok(None); + } + let stderr = String::from_utf8_lossy(&output.stderr); + anyhow::bail!("git config {scope} --get {key} failed: {stderr}"); +} + /// Format an Ed25519 public key as an SSH public key string (e.g., `ssh-ed25519 AAAA...`). pub fn ssh_public_key_string(public_key_bytes: &[u8; 32]) -> String { format!("ssh-ed25519 {}", base64_encode_pubkey(public_key_bytes)) @@ -429,6 +511,187 @@ fn base64_encode_pubkey(public_key_bytes: &[u8; 32]) -> String { base64::engine::general_purpose::STANDARD.encode(&blob) } +/// The commit-msg hook script. Reads the DID the same way the signer selects +/// its key — `DID_GIT_SIGN_KEY`, then `did-git-sign.key` git config — and adds +/// a `Signed-by-DID:` trailer if one is not already present. This is how +/// `verify-trust` discovers the signer DID without requiring `user.email` to +/// be a DID. +/// +/// Placement is delegated to `git interpret-trailers` rather than done by +/// hand: it finds the final trailer block, inserts the blank line that +/// separates a trailer block from the body, and leaves an existing trailer +/// alone. Appending with `sed` instead was wrong twice over. `sed -i ''` is +/// BSD-only syntax — GNU sed reads the empty argument as a filename, fails, +/// and leaves the file unchanged, so the strip loop that re-tests the same +/// condition spins forever and `git commit` hangs on Linux. And on a +/// one-line message (`git commit -m fix`) the trailer landed glued to the +/// subject line, where git's own parser reads no trailers at all. +/// +/// `Signed-off-by:` is added only when `did-git-sign.signoff` is true. A DCO +/// sign-off is an assertion the committer makes about their right to submit +/// the code, not one a signing tool may make on their behalf, so it is +/// opt-in. +const COMMIT_MSG_HOOK: &str = r#"#!/bin/sh +# Installed by did-git-sign — chains the repo commit-msg hook, then adds the Signed-by-DID trailer. +git_dir=$(git rev-parse --absolute-git-dir 2>/dev/null) || exit 0 +repo_hook="$git_dir/hooks/commit-msg" +if [ -x "$repo_hook" ] && [ "$repo_hook" != "$0" ]; then + "$repo_hook" "$@" || exit $? +fi + +msg_file="$1" +[ -n "$msg_file" ] || exit 0 + +# Same precedence the signer uses (R-G-1): env var, then per-repo git config. +# They must agree — the hook writes the claim and the signer checks it against +# the key it actually uses, so reading a different selector here would make +# `DID_GIT_SIGN_KEY=… git commit` refuse to sign its own commit. +DID=$(printf '%s' "${DID_GIT_SIGN_KEY:-}" | tr -d '\r\n') +[ -z "$DID" ] && DID=$(git config did-git-sign.key 2>/dev/null) +[ -z "$DID" ] && exit 0 +case "$DID" in + did:*) ;; + *) exit 0 ;; +esac +case "$DID" in + *[[:space:]]*) + echo "did-git-sign: the selected DID contains whitespace; refusing to write a trailer" >&2 + exit 1 + ;; +esac + +# Opt-in: a DCO sign-off is the committer's assertion to make, not ours. +if [ "$(git config --bool did-git-sign.signoff 2>/dev/null)" = "true" ]; then + NAME=$(git config user.name 2>/dev/null | tr -d '\r\n') + EMAIL=$(git config user.email 2>/dev/null | tr -d '\r\n') + git interpret-trailers --in-place --if-exists doNothing \ + --trailer "Signed-off-by: $NAME <$EMAIL>" "$msg_file" || exit 1 +fi + +git interpret-trailers --in-place --if-exists doNothing \ + --trailer "Signed-by-DID: $DID" "$msg_file" || exit 1 +"#; + +const STANDARD_GIT_HOOKS: &[&str] = &[ + "applypatch-msg", + "commit-msg", + "fsmonitor-watchman", + "post-applypatch", + "post-checkout", + "post-commit", + "post-index-change", + "post-merge", + "post-receive", + "post-rewrite", + "post-update", + "pre-applypatch", + "pre-auto-gc", + "pre-commit", + "pre-merge-commit", + "pre-push", + "pre-rebase", + "pre-receive", + "prepare-commit-msg", + "proc-receive", + "push-to-checkout", + "reference-transaction", + "sendemail-validate", + "update", +]; + +fn delegating_hook(hook_name: &str) -> String { + format!( + r#"#!/bin/sh +# Installed by did-git-sign — delegates to the repository's {hook_name} hook. +git_dir=$(git rev-parse --absolute-git-dir 2>/dev/null) || exit 0 +repo_hook="$git_dir/hooks/{hook_name}" +[ -x "$repo_hook" ] || exit 0 +[ "$repo_hook" != "$0" ] || exit 0 +exec "$repo_hook" "$@" +"# + ) +} + +/// Install the hook dispatcher that injects the `Signed-by-DID:` trailer while +/// delegating every other standard Git hook back to the repository's default +/// `.git/hooks` directory. +/// +/// For a **local** install, writes to `.git/did-git-sign-hooks/` in the current +/// repo and sets repo-local `core.hooksPath`. For a **global** install, writes +/// to `~/.config/did-git-sign/hooks/` and sets global `core.hooksPath` (git +/// 2.9+). The original `.git/hooks` directory remains the source for repository +/// hooks, including hooks added after did-git-sign is installed. +fn install_hook_dispatcher(global: bool) -> Result<()> { + let hooks_dir = expected_hooks_dir(global)? + .context("not inside a git repository — cannot install hook dispatcher")?; + let scope = if global { "--global" } else { "--local" }; + + // `core.hooksPath` is a single slot, and husky, lefthook and pre-commit + // all claim it. Taking it from one of them is silent breakage: the + // delegating hooks below fall back to `$git_dir/hooks`, never to whatever + // was configured here before, so every hook that tool installed simply + // stops running. Refuse in both scopes — the local case is the common one. + if let Some(existing) = git_config_get(scope, "core.hooksPath")? + && Path::new(existing.trim()) != hooks_dir + { + anyhow::bail!( + "{scope} core.hooksPath is already set to '{existing}'; refusing to overwrite it. \ + Unset it, or add the Signed-by-DID trailer logic to that directory's commit-msg \ + hook manually." + ); + } + + let hooks_dir_str = hooks_dir + .to_str() + .context("hooks directory path is not valid UTF-8")? + .to_string(); + + // Populate the directory *before* pointing git at it. `write_executable_hook` + // refuses to clobber a hook it did not write, so this loop can fail partway; + // if `core.hooksPath` already named this directory by then, the hooks that + // were never written would silently stop running instead of the install + // failing cleanly with the old configuration still intact. + std::fs::create_dir_all(&hooks_dir)?; + for hook_name in STANDARD_GIT_HOOKS { + let hook_path = hooks_dir.join(hook_name); + let content = if *hook_name == "commit-msg" { + COMMIT_MSG_HOOK.to_string() + } else { + delegating_hook(hook_name) + }; + + write_executable_hook(&hook_path, &content)?; + } + + git_config(scope, "core.hooksPath", &hooks_dir_str)?; + + Ok(()) +} + +fn write_executable_hook(hook_path: &Path, content: &str) -> Result<()> { + if hook_path.exists() { + let existing = std::fs::read_to_string(hook_path).unwrap_or_default(); + if existing.contains("Installed by did-git-sign") { + std::fs::write(hook_path, content)?; + } else { + anyhow::bail!( + "hook already exists at {}; merge manually", + hook_path.display() + ); + } + } else { + std::fs::write(hook_path, content)?; + } + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + std::fs::set_permissions(hook_path, std::fs::Permissions::from_mode(0o755))?; + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; @@ -565,6 +828,262 @@ mod tests { } } + #[test] + fn delegating_hook_targets_default_repo_hook_dir() { + let hook = delegating_hook("pre-push"); + assert!(hook.contains("git rev-parse --absolute-git-dir")); + assert!(hook.contains("$git_dir/hooks/pre-push")); + assert!(hook.contains("exec \"$repo_hook\" \"$@\"")); + } + + /// Write `COMMIT_MSG_HOOK` into a throwaway repo and return its path. + /// + /// The hook is a shell script, so string assertions about it prove very + /// little — both bugs this suite now guards (a `sed -i ''` loop that spun + /// forever under GNU sed, and a trailer appended straight onto a one-line + /// subject) passed every `contains` check that existed. These tests run it. + #[cfg(unix)] + fn repo_with_commit_msg_hook(dir: &Path, did: &str) -> PathBuf { + for args in [ + vec!["init", "-q"], + vec!["config", "user.name", "T Ester"], + vec!["config", "user.email", "t@example.com"], + vec!["config", "commit.gpgsign", "false"], + vec!["config", "did-git-sign.key", did], + ] { + let out = Command::new("git") + .args(["-C", dir.to_str().unwrap()]) + .args(&args) + .output() + .unwrap(); + assert!(out.status.success(), "git {args:?} failed"); + } + + let hook = dir.join(".git").join("hooks").join("commit-msg"); + std::fs::create_dir_all(hook.parent().unwrap()).unwrap(); + write_executable_hook(&hook, COMMIT_MSG_HOOK).unwrap(); + hook + } + + /// Ask git — not our own parser — which trailers it can see in HEAD. + #[cfg(unix)] + fn git_trailer(dir: &Path, key: &str) -> String { + let out = Command::new("git") + .args([ + "-C", + dir.to_str().unwrap(), + "log", + "-1", + &format!("--format=%(trailers:key={key},valueonly)"), + ]) + .output() + .unwrap(); + String::from_utf8_lossy(&out.stdout).trim().to_string() + } + + /// A one-line message is the common case (`git commit -m "fix thing"`), + /// and it is the case a naive append gets wrong: with no blank line + /// between subject and trailer, git's own trailer parser reads *nothing*, + /// so the DID would be invisible to `git log`, to forges, and to every + /// tool that asks git rather than re-implementing the format. + #[test] + #[cfg(unix)] + fn commit_msg_hook_trailer_is_readable_by_git_on_a_one_line_message() { + let dir = tempfile::tempdir().unwrap(); + let did = "did:webvh:QmAbc:example.com#key-0"; + repo_with_commit_msg_hook(dir.path(), did); + + std::fs::write(dir.path().join("f.txt"), "hi").unwrap(); + for args in [vec!["add", "f.txt"], vec!["commit", "-q", "-m", "subject"]] { + let out = Command::new("git") + .args(["-C", dir.path().to_str().unwrap()]) + .args(&args) + .output() + .unwrap(); + assert!( + out.status.success(), + "git {args:?} failed: {}", + String::from_utf8_lossy(&out.stderr) + ); + } + + assert_eq!( + git_trailer(dir.path(), "Signed-by-DID"), + did, + "git itself must parse the trailer the hook wrote" + ); + } + + /// A message ending in blank lines drove the old strip loop. `sed -i ''` + /// is BSD syntax; GNU sed reads the empty argument as a filename, fails, + /// and leaves the file untouched, so the loop re-tested the same condition + /// forever and `git commit` hung. Termination is the assertion — and it is + /// bounded, because a regression here hangs rather than fails, and a CI job + /// that runs to its timeout says much less than one that fails. + /// + /// Note this only reproduces where sed is GNU sed: on a BSD userland (macOS) + /// the old code worked and this test passes either way. CI runs Linux. + #[test] + #[cfg(unix)] + fn commit_msg_hook_terminates_on_a_message_with_trailing_blank_lines() { + let dir = tempfile::tempdir().unwrap(); + let did = "did:webvh:QmAbc:example.com#key-0"; + let hook = repo_with_commit_msg_hook(dir.path(), did); + + let msg = dir.path().join("MSG"); + std::fs::write(&msg, "a message\n\n\n\n").unwrap(); + + let mut child = Command::new(&hook) + .arg(&msg) + .current_dir(dir.path()) + .spawn() + .unwrap(); + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + let status = loop { + match child.try_wait().unwrap() { + Some(status) => break status, + None if std::time::Instant::now() >= deadline => { + let _ = child.kill(); + panic!("commit-msg hook did not terminate — the trailing-blank-line loop spun"); + } + None => std::thread::sleep(std::time::Duration::from_millis(20)), + } + }; + + assert!(status.success(), "hook failed: {status}"); + assert!( + std::fs::read_to_string(&msg).unwrap().contains(did), + "hook must still write the trailer" + ); + } + + /// Running twice must not stack duplicate trailers — amends and rebases + /// re-run the hook over a message that already carries one. + #[test] + #[cfg(unix)] + fn commit_msg_hook_is_idempotent() { + let dir = tempfile::tempdir().unwrap(); + let did = "did:webvh:QmAbc:example.com#key-0"; + let hook = repo_with_commit_msg_hook(dir.path(), did); + + let msg = dir.path().join("MSG"); + std::fs::write(&msg, "subject\n").unwrap(); + for _ in 0..2 { + assert!( + Command::new(&hook) + .arg(&msg) + .current_dir(dir.path()) + .status() + .unwrap() + .success() + ); + } + + let body = std::fs::read_to_string(&msg).unwrap(); + assert_eq!( + body.matches("Signed-by-DID:").count(), + 1, + "trailer must not be duplicated: {body}" + ); + } + + /// A DCO sign-off asserts something about the committer's right to submit + /// the code. The signing tool must not assert it for them, so the trailer + /// is opt-in via `did-git-sign.signoff`. + #[test] + #[cfg(unix)] + fn commit_msg_hook_adds_signoff_only_when_opted_in() { + let dir = tempfile::tempdir().unwrap(); + let did = "did:webvh:QmAbc:example.com#key-0"; + let hook = repo_with_commit_msg_hook(dir.path(), did); + let msg = dir.path().join("MSG"); + + std::fs::write(&msg, "subject\n").unwrap(); + Command::new(&hook) + .arg(&msg) + .current_dir(dir.path()) + .status() + .unwrap(); + assert!( + !std::fs::read_to_string(&msg) + .unwrap() + .contains("Signed-off-by:"), + "sign-off must not be added by default" + ); + + assert!( + Command::new("git") + .args([ + "-C", + dir.path().to_str().unwrap(), + "config", + "did-git-sign.signoff", + "true", + ]) + .status() + .unwrap() + .success() + ); + std::fs::write(&msg, "subject\n").unwrap(); + Command::new(&hook) + .arg(&msg) + .current_dir(dir.path()) + .status() + .unwrap(); + assert!( + std::fs::read_to_string(&msg) + .unwrap() + .contains("Signed-off-by:"), + "sign-off must be added once opted in" + ); + } + + /// `core.hooksPath` is a single slot that husky, lefthook and pre-commit + /// also claim. The dispatcher's delegating hooks fall back to + /// `$git_dir/hooks`, never to a previously configured path, so taking the + /// slot would silently stop every hook that tool installed. + #[test] + #[serial_test::serial] + fn install_hook_dispatcher_refuses_to_take_a_local_hooks_path_it_does_not_own() { + let dir = tempfile::tempdir().unwrap(); + Command::new("git") + .args(["init", "-q"]) + .current_dir(dir.path()) + .output() + .unwrap(); + Command::new("git") + .args(["-C", dir.path().to_str().unwrap()]) + .args(["config", "core.hooksPath", ".husky"]) + .output() + .unwrap(); + + let err = { + let _cwd = CwdGuard::change_to(dir.path()); + install_hook_dispatcher(false).unwrap_err().to_string() + }; + assert!(err.contains(".husky"), "names the path it refused: {err}"); + + // And it must have left that configuration alone. + let out = Command::new("git") + .args(["-C", dir.path().to_str().unwrap()]) + .args(["config", "--local", "core.hooksPath"]) + .output() + .unwrap(); + assert_eq!(String::from_utf8_lossy(&out.stdout).trim(), ".husky"); + } + + #[test] + fn commit_msg_hook_chains_before_adding_signed_by_did() { + let chain_pos = COMMIT_MSG_HOOK.find("repo_hook=").unwrap(); + let did_pos = COMMIT_MSG_HOOK + .find("DID=$(git config did-git-sign.key") + .unwrap(); + assert!(chain_pos < did_pos); + assert!(COMMIT_MSG_HOOK.contains("$git_dir/hooks/commit-msg")); + assert!(COMMIT_MSG_HOOK.contains("Signed-by-DID: $DID")); + } + #[test] fn test_different_keys_produce_different_ssh_strings() { let key_a = [0x00; 32]; @@ -595,20 +1114,14 @@ mod tests { } } - /// `setup_git` must write `user.email` as the signing DID's key id. - /// - /// This inverts an earlier regression guard that asserted the opposite. That - /// guard's reasoning — git's SSH verification matches the allowed_signers - /// principal, not user.email — does not survive either direction: - /// [`allowed_signers_entry`] writes the principal as `did_key_id` and git - /// matches principals *against the committer email*, so an unset value - /// breaks local verification too. And `verify-trust` has no other channel - /// for the identity at all: an sshsig carries a key, never a DID, so a - /// commit whose committer is not a DID fails CI as `noSignerDid` no matter - /// how valid its signature. + /// `setup_git` must write the signing DID into `did-git-sign.key` git + /// config so the commit-msg hook can read it and inject the + /// `Signed-by-DID:` trailer. Previously the DID was written to + /// `user.email`, but that broke git-host attribution (GitLab/GitHub + /// account linking). #[test] #[serial_test::serial] - fn setup_git_writes_the_signing_did_as_user_email() { + fn setup_git_writes_did_to_git_config_key() { let dir = tempfile::tempdir().unwrap(); std::process::Command::new("git") .args(["init"]) @@ -616,9 +1129,6 @@ mod tests { .output() .unwrap(); - // Move into the temp repo so that `git config --local` targets it. - // The inner block ensures CwdGuard is dropped (and CWD restored) before - // the assertions run, keeping the verify step independent of CWD. let original_cwd = std::env::current_dir().unwrap(); { let _cwd = CwdGuard::change_to(dir.path()); @@ -628,39 +1138,54 @@ mod tests { user_name: None, }; setup_git(&config_path, &cfg, false).unwrap(); - // _cwd drops here: original directory is restored } - // Pin the invariant explicitly so a future edit that moves the - // verify command inside the guard's scope (or drops the guard) is - // caught loudly rather than silently regressing the CWD-independence - // promise the inner block makes. assert_eq!( std::env::current_dir().unwrap(), original_cwd, "CwdGuard must restore the original directory on drop" ); - // Verify with an explicit -C so the check is not sensitive to the current CWD. + // did-git-sign.key must carry the signing DID for the commit-msg hook. let out = std::process::Command::new("git") .args([ "-C", dir.path().to_str().unwrap(), "config", "--local", - "user.email", + "did-git-sign.key", ]) .output() .unwrap(); assert!( out.status.success(), - "user.email must be set by setup_git: without it every commit fails \ - verify-trust as noSignerDid" + "did-git-sign.key must be set by setup_git" ); assert_eq!( String::from_utf8_lossy(&out.stdout).trim(), "did:webvh:test#key-0", - "user.email must be the signing DID's verification-method id" ); + + // user.email must NOT be overwritten to a DID. + let email_out = std::process::Command::new("git") + .args([ + "-C", + dir.path().to_str().unwrap(), + "config", + "--local", + "user.email", + ]) + .output() + .unwrap(); + + if email_out.status.success() { + let email = String::from_utf8_lossy(&email_out.stdout) + .trim() + .to_string(); + assert!( + !email.starts_with("did:"), + "user.email must not be set to a DID (got {email})" + ); + } } } diff --git a/crates/did-git-sign/src/main.rs b/crates/did-git-sign/src/main.rs index 6b4b554..a2d7c7b 100644 --- a/crates/did-git-sign/src/main.rs +++ b/crates/did-git-sign/src/main.rs @@ -193,6 +193,9 @@ enum Commands { /// back to the DID claiming it. See `init --resolve-agent-names`. #[arg(long)] resolve_agent_names: bool, + /// Path to a did.jsonl file to verify the signing key against. + #[arg(long)] + did_jsonl: Option, }, /// Remove this host's did-git-sign install: deletes the JSON config, @@ -295,7 +298,8 @@ async fn main() -> Result<()> { Some(Commands::Verify) => cmd_verify().await, Some(Commands::Health { resolve_agent_names, - }) => cmd_health(resolve_agent_names).await, + did_jsonl, + }) => cmd_health(resolve_agent_names, did_jsonl.as_deref()).await, Some(Commands::Uninstall { global, local, @@ -727,7 +731,7 @@ async fn cmd_verify() -> Result<()> { Ok(()) } -async fn cmd_health(resolve_agent_names: bool) -> Result<()> { +async fn cmd_health(resolve_agent_names: bool, did_jsonl: Option<&std::path::Path>) -> Result<()> { let (config_path, cfg) = load_config()?; println!("did-git-sign health check"); @@ -823,6 +827,41 @@ async fn cmd_health(resolve_agent_names: bool) -> Result<()> { " {}", init::allowed_signers_entry(&cfg, verifying_key.as_bytes()) ); + + // Verify the local did.jsonl publishes this key (if provided). + if let Some(path) = did_jsonl { + println!(); + print!("DID doc check: "); + match check_key_in_did_log(path, verifying_key.as_bytes()) { + Ok(true) => { + println!("OK (key found in {})", path.display()); + let local_mb = multibase::encode( + multibase::Base::Base58Btc, + [ + vgi_core::ED25519_MULTICODEC_PREFIX.as_slice(), + verifying_key.as_bytes(), + ] + .concat(), + ); + println!(" publicKeyMultibase: {local_mb}"); + } + Ok(false) => { + println!("MISMATCH"); + println!(" {} does not contain the signing key.", path.display()); + let local_mb = multibase::encode( + multibase::Base::Base58Btc, + [ + vgi_core::ED25519_MULTICODEC_PREFIX.as_slice(), + verifying_key.as_bytes(), + ] + .concat(), + ); + println!(" Local key (multibase): {local_mb}"); + println!(" Re-export or re-run onboarding."); + } + Err(e) => println!("FAILED ({e})"), + } + } } Err(e) => { println!("FAILED"); @@ -839,6 +878,29 @@ async fn cmd_health(resolve_agent_names: bool) -> Result<()> { Ok(()) } +/// Check whether a local did.jsonl file publishes the given Ed25519 key. +fn check_key_in_did_log(path: &std::path::Path, local_key: &[u8; 32]) -> Result { + use vgi_core::ed25519_keys_from_doc; + + let content = + std::fs::read_to_string(path).with_context(|| format!("cannot read {}", path.display()))?; + + // did.jsonl is JSONL — the DID document state is in the first line's "state" field. + let first_line = content.lines().next().context("empty did.jsonl")?; + let entry: serde_json::Value = + serde_json::from_str(first_line).context("invalid JSON in did.jsonl")?; + let state = entry + .get("state") + .context("no 'state' field in did.jsonl entry")?; + + let published_keys = ed25519_keys_from_doc(state); + if published_keys.is_empty() { + anyhow::bail!("DID document has no Ed25519 keys"); + } + + Ok(published_keys.iter().any(|k| k == local_key)) +} + fn cmd_uninstall( global_flag: bool, local_flag: bool, diff --git a/crates/did-git-sign/src/sign.rs b/crates/did-git-sign/src/sign.rs index a8f764a..f709cea 100644 --- a/crates/did-git-sign/src/sign.rs +++ b/crates/did-git-sign/src/sign.rs @@ -2,7 +2,7 @@ use anyhow::{Context, Result}; use ed25519_dalek::SigningKey; use std::io::Read; use std::path::Path; -use vgi_core::{committer_did, committer_identity, create_ssh_signature}; +use vgi_core::create_ssh_signature; use crate::config::{self, SigningConfig}; use crate::policy; @@ -91,39 +91,63 @@ fn bare_did(did_key_id: &str) -> &str { .unwrap_or(did_key_id) } -/// Refuse to sign a commit whose committer identity disagrees with the key +/// Refuse to sign a commit whose signer identity disagrees with the key /// being used (R-G-3). /// -/// Two settings choose an identity — `user.email`, which becomes the commit's -/// claim, and the persona selection that picks the key — and when they name -/// different DIDs the commit is born unverifiable. `verify-trust` would reject -/// it as `unknownKey`: the claimed DID does not publish the key that signed. -/// That is a correct verdict pointing at the wrong thing, arriving in CI, on a +/// Two settings choose an identity — the DID the commit claims, and the +/// persona selection that picks the key — and when they name different DIDs +/// the commit is born unverifiable. `verify-trust` would reject it as +/// `unknownKey`: the claimed DID does not publish the key that signed. That +/// is a correct verdict pointing at the wrong thing, arriving in CI, on a /// commit already written. Catching it here turns a confusing remote failure /// into a local one that names both halves. /// +/// The claim is read the same way `verify-trust` reads it — [`signer_did`]: +/// the `Signed-by-DID:` trailer first, then the committer email for legacy +/// commits. At sign time the trailer may already be present (the commit-msg +/// hook ran) or absent (no hook, legacy flow), so only a *conflicting* claim +/// is refused. +/// /// Only commit-shaped payloads are checked. A payload with no `committer` /// header (a tag, or a non-git namespace) carries no claim to disagree with. /// The comparison is on **bare DIDs**, matching what the verifier actually -/// requires — signing with `#key-1` while the committer says `#key-0` verifies +/// requires — signing with `#key-1` while the claim says `#key-0` verifies /// fine, since the check is that the DID publishes the key, not which one. fn check_committer_matches_key(data: &[u8], did_key_id: &str, source: KeySource) -> Result<()> { - let Some(identity) = committer_identity(data) else { - return Ok(()); - }; + use vgi_core::{committer_identity, conflicting_signer_dids, signer_did}; + let signing_did = bare_did(did_key_id); - match committer_did(data) { + + // Two explicit claims that disagree with each other. No key satisfies + // both, so there is no point asking which one matches ours — + // `verify-trust` fails the commit closed whichever key signs it. + if let Some((trailer, committer)) = conflicting_signer_dids(data) { + anyhow::bail!( + "did-git-sign: Signed-by-DID trailer claims '{trailer}' but committer claims \ + '{committer}'. Remove one claim or make them match before signing." + ); + } + + match signer_did(data) { Some(claimed) if claimed == signing_did => Ok(()), Some(claimed) => anyhow::bail!( - "did-git-sign: this commit would claim '{claimed}' but is being signed with a key \ - held by '{signing_did}' (selected via {source}). The commit would fail \ - verification as unknownKey. Set user.email to a '{signing_did}' key id, or select \ - the persona matching the committer." + "did-git-sign: this commit claims signer '{claimed}' but would be signed with a key \ + held by '{signing_did}' (selected via {source}), so it would fail verification as \ + unknownKey. Point the claim at the signing key — \ + `git config did-git-sign.key '{did_key_id}'` for the Signed-by-DID trailer, or \ + `git config user.email` for a legacy DID committer — or select the persona \ + matching the claim." ), + // Tags and non-git namespaces carry no committer header, so there is + // no commit identity claim for this guard to compare. + None if committer_identity(data).is_none() => Ok(()), + // A committer exists but neither it nor a trailer names a DID: the + // commit-msg hook did not run. None => anyhow::bail!( - "did-git-sign: this commit's committer is <{identity}>, which is not a DID, so the \ - commit would state no signer identity and fail verification as noSignerDid. Set \ - user.email to '{did_key_id}' (git config user.email '{did_key_id}')." + "did-git-sign: no Signed-by-DID trailer and user.email is not a DID, so the commit \ + would state no signer identity and fail verification as noSignerDid. Run \ + 'did-git-sign init' to install the commit-msg hook, or set user.email to \ + '{did_key_id}'." ), } } @@ -295,21 +319,81 @@ mod tests { } #[test] - fn a_non_did_committer_is_refused_before_signing() { - // Signing would succeed and the commit would fail CI as noSignerDid. + fn a_non_did_committer_without_trailer_is_refused() { + // No trailer and no DID in committer email = missing commit-msg hook. let commit = commit_committed_by("alice@example.com"); let error = check_committer_matches_key(&commit, &format!("{SIGNER}#key-0"), KeySource::ConfigFile) .unwrap_err() .to_string(); + assert!( + error.contains("noSignerDid"), + "must refuse when no DID claim exists: {error}" + ); + } + + #[test] + fn trailer_matching_key_is_accepted() { + let commit = format!( + "tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\ + author A 1700000000 +0000\n\ + committer A 1700000000 +0000\n\ + \n\ + message\n\ + \n\ + Signed-by-DID: {SIGNER}#key-0\n" + ); + assert!( + check_committer_matches_key( + commit.as_bytes(), + &format!("{SIGNER}#key-0"), + KeySource::ConfigFile + ) + .is_ok() + ); + } - assert!(error.contains("noSignerDid"), "names the verdict: {error}"); + #[test] + fn trailer_conflicting_with_key_is_refused() { + let commit = "tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\ + author A 1700000000 +0000\n\ + committer A 1700000000 +0000\n\ + \n\ + message\n\ + \n\ + Signed-by-DID: did:webvh:QmOther:other.example#key-0\n"; assert!( - error.contains("alice@example.com"), - "names the offending identity: {error}" + check_committer_matches_key( + commit.as_bytes(), + &format!("{SIGNER}#key-0"), + KeySource::ConfigFile + ) + .is_err() ); } + #[test] + fn conflicting_trailer_and_committer_dids_are_refused() { + let commit = format!( + "tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\ + author A 1700000000 +0000\n\ + committer A 1700000000 +0000\n\ + \n\ + message\n\ + \n\ + Signed-by-DID: {SIGNER}#key-0\n" + ); + let error = check_committer_matches_key( + commit.as_bytes(), + &format!("{SIGNER}#key-0"), + KeySource::ConfigFile, + ) + .unwrap_err() + .to_string(); + assert!(error.contains("Signed-by-DID"), "names trailer: {error}"); + assert!(error.contains("committer"), "names committer: {error}"); + } + #[test] fn a_payload_with_no_committer_is_not_a_claim_to_check() { // Tags and non-git namespaces carry no committer header, so there is diff --git a/crates/verify-trust/src/lib.rs b/crates/verify-trust/src/lib.rs index c9ead54..64b675f 100644 --- a/crates/verify-trust/src/lib.rs +++ b/crates/verify-trust/src/lib.rs @@ -62,8 +62,8 @@ use trql_client::{ TrqlError, TrqpQuery, }; use vgi_core::{ - GIT_SSHSIG_NAMESPACE, committer_did, committer_identity, ed25519_keys_from_doc, - normalize_sshsig_armor, split_signed_commit, + GIT_SSHSIG_NAMESPACE, committer_identity, conflicting_signer_dids, ed25519_keys_from_doc, + normalize_sshsig_armor, signer_did, split_signed_commit, }; use vta_sdk::display_name::{DisplayName, NameBook, NameSource}; @@ -148,6 +148,9 @@ pub enum CommitStatus { /// Signed, but the `committer` header names no DID, so the commit asserts /// no identity to resolve or authorize. NoSignerDid { committer: String }, + /// The commit carries both a `Signed-by-DID:` trailer and a DID committer + /// identity, and they name different DIDs. + ConflictingSignerDids { trailer: String, committer: String }, /// The claimed DID could not be resolved, so its published keys are /// unknown. Fails closed: an unresolvable signer is not a trusted one. UnresolvedSigner { did: String, error: String }, @@ -300,7 +303,8 @@ pub fn read_range(repo_dir: &Path, range: &str) -> Result> { pub fn claimed_signer_dids(commits: &[RangeCommit], max_signers: usize) -> Result> { let dids: BTreeSet = commits .iter() - .filter_map(|commit| committer_did(&commit.raw)) + .filter(|commit| conflicting_signer_dids(&commit.raw).is_none()) + .filter_map(|commit| signer_did(&commit.raw)) .collect(); if dids.len() > max_signers { bail!( @@ -373,6 +377,7 @@ pub enum SignatureCheck { Unsigned, Malformed(String), NoSignerDid { committer: String }, + ConflictingSignerDids { trailer: String, committer: String }, UnresolvedSigner { did: String, error: String }, UnknownKey { did: String, fingerprint: String }, BadSignature { signer_did: String }, @@ -425,7 +430,10 @@ pub fn check_commit_signature( // The identity is read from the payload — the bytes the signature covers — // so a claim that survives verification is one the signer committed to. - let Some(claimed) = committer_did(&payload) else { + if let Some((trailer, committer)) = conflicting_signer_dids(&payload) { + return SignatureCheck::ConflictingSignerDids { trailer, committer }; + } + let Some(claimed) = signer_did(&payload) else { return SignatureCheck::NoSignerDid { committer: committer_identity(&payload).unwrap_or_else(|| "(absent)".to_string()), }; @@ -687,6 +695,9 @@ fn status_of(signature: SignatureCheck, decisions: &RegistryDecisions) -> Commit SignatureCheck::Unsigned => CommitStatus::Unsigned, SignatureCheck::Malformed(detail) => CommitStatus::Malformed(detail), SignatureCheck::NoSignerDid { committer } => CommitStatus::NoSignerDid { committer }, + SignatureCheck::ConflictingSignerDids { trailer, committer } => { + CommitStatus::ConflictingSignerDids { trailer, committer } + } SignatureCheck::UnresolvedSigner { did, error } => { CommitStatus::UnresolvedSigner { did, error } } @@ -817,6 +828,11 @@ fn print_report(args: &VerifyTrustArgs, report: &TrustReport) -> Result<()> { CommitStatus::NoSignerDid { committer } => { println!("NO-SIGNER {short} committer <{committer}> is not a DID"); } + CommitStatus::ConflictingSignerDids { trailer, committer } => { + println!( + "CONFLICT {short} Signed-by-DID {trailer} disagrees with committer DID {committer}" + ); + } CommitStatus::Malformed(detail) => { println!("MALFORMED {short} {detail}"); } @@ -1041,6 +1057,29 @@ mod tests { ); } + #[test] + fn conflicting_trailer_and_committer_dids_fail_closed() { + let payload = format!( + "tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\ + author A U Thor 1700000000 +0000\n\ + committer A U Thor 1700000000 +0000\n\ + \n\ + a message\n\ + \n\ + Signed-by-DID: {SIGNER}#key-0\n" + ); + let (key, public) = test_key(); + let commit = sign_commit(&payload, &key); + + assert_eq!( + check_commit_signature(commit.as_bytes(), &signers_publishing(public), None), + SignatureCheck::ConflictingSignerDids { + trailer: SIGNER.to_string(), + committer: "did:webvh:QmCommitter:example.com".to_string(), + } + ); + } + #[test] fn a_claimed_did_that_did_not_resolve_fails_closed() { let payload = unsigned_commit(); diff --git a/crates/vgi-core/src/commit.rs b/crates/vgi-core/src/commit.rs index b57e5e2..8e88e6c 100644 --- a/crates/vgi-core/src/commit.rs +++ b/crates/vgi-core/src/commit.rs @@ -117,6 +117,70 @@ pub fn committer_did(commit: &[u8]) -> Option { Some(did.to_string()) } +/// The signer DID a commit claims, checking the `Signed-by-DID:` trailer +/// first, then falling back to the committer email for legacy commits. +/// +/// The trailer is the canonical location for new commits (it lets +/// `user.email` be a normal email for git-host attribution). Old commits +/// that carried the DID in the committer email still verify via the +/// fallback. +#[must_use] +pub fn signer_did(commit: &[u8]) -> Option { + trailer_did(commit).or_else(|| committer_did(commit)) +} + +/// Return both explicit identity claims when the final `Signed-by-DID:` +/// trailer and legacy DID committer identity disagree. +#[must_use] +pub fn conflicting_signer_dids(commit: &[u8]) -> Option<(String, String)> { + let trailer = trailer_did(commit)?; + let committer = committer_did(commit)?; + (trailer != committer).then_some((trailer, committer)) +} + +/// Extract a bare DID from a `Signed-by-DID:` trailer in the commit body's +/// final trailer block. +fn trailer_did(commit: &[u8]) -> Option { + let text = std::str::from_utf8(commit).ok()?; + let (_, body) = text.split_once("\n\n")?; + + let mut lines: Vec<&str> = body.lines().collect(); + while lines.last().is_some_and(|line| line.trim().is_empty()) { + lines.pop(); + } + + let mut trailer_start = lines.len(); + while trailer_start > 0 && is_trailer_line(lines[trailer_start - 1]) { + trailer_start -= 1; + } + if trailer_start == lines.len() { + return None; + } + + for line in lines[trailer_start..].iter().rev() { + if let Some(value) = line.strip_prefix("Signed-by-DID:") { + let value = value.trim(); + if value.starts_with("did:") { + return Some( + value + .split(['#', '?', '/']) + .next() + .unwrap_or(value) + .to_string(), + ); + } + } + } + None +} + +fn is_trailer_line(line: &str) -> bool { + let Some((key, _)) = line.split_once(':') else { + return false; + }; + !key.is_empty() && key.chars().all(|c| c.is_ascii_alphanumeric() || c == '-') +} + #[cfg(test)] mod tests { #![allow(clippy::unwrap_used)] @@ -203,4 +267,121 @@ mod tests { "did:webvh:QmAbc:example.com" ); } + + fn commit_with_trailer(committer: &str, trailer: &str) -> String { + format!( + "tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\ + author A U Thor 1700000000 +0000\n\ + committer {committer} 1700000000 +0000\n\ + \n\ + a message\n\ + \n\ + {trailer}\n" + ) + } + + #[test] + fn signer_did_prefers_trailer_over_committer() { + let commit = commit_with_trailer( + "Alice ", + "Signed-by-DID: did:webvh:QmNew:new.example#key-0", + ); + assert_eq!( + signer_did(commit.as_bytes()).unwrap(), + "did:webvh:QmNew:new.example", + "trailer must take precedence over committer email" + ); + } + + #[test] + fn signer_did_falls_back_to_committer_for_legacy_commits() { + let commit = commit_with_committer("Alice "); + assert_eq!( + signer_did(commit.as_bytes()).unwrap(), + "did:webvh:QmAbc:example.com", + "legacy commits with DID in committer email must still work" + ); + } + + #[test] + fn signer_did_reads_trailer_with_normal_email_committer() { + let commit = commit_with_trailer( + "Alice ", + "Signed-by-DID: did:webvh:QmAbc:example.com#key-0", + ); + assert_eq!( + signer_did(commit.as_bytes()).unwrap(), + "did:webvh:QmAbc:example.com", + ); + } + + #[test] + fn signer_did_returns_none_without_did_anywhere() { + let commit = commit_with_committer("Alice "); + assert!(signer_did(commit.as_bytes()).is_none()); + } + + #[test] + fn trailer_strips_fragment() { + let commit = commit_with_trailer( + "Alice ", + "Signed-by-DID: did:webvh:QmAbc:example.com#key-1", + ); + assert_eq!( + signer_did(commit.as_bytes()).unwrap(), + "did:webvh:QmAbc:example.com", + ); + } + + #[test] + fn trailer_ignores_non_did_values() { + let commit = commit_with_trailer("Alice ", "Signed-by-DID: not-a-did"); + assert!(signer_did(commit.as_bytes()).is_none()); + } + + #[test] + fn signer_did_ignores_body_line_outside_final_trailer_block() { + let commit = "tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\ + author A U Thor 1700000000 +0000\n\ + committer Alice 1700000000 +0000\n\ + \n\ + This line only discusses a trailer.\n\ + Signed-by-DID: did:webvh:QmBody:example.com#key-0\n\ + \n\ + final prose, not a trailer block\n"; + assert!(signer_did(commit.as_bytes()).is_none()); + } + + #[test] + fn signer_did_reads_final_trailer_block_only() { + let commit = "tree 4b825dc642cb6eb9a060e54bf8d69288fbee4904\n\ + author A U Thor 1700000000 +0000\n\ + committer Alice 1700000000 +0000\n\ + \n\ + Signed-by-DID: did:webvh:QmBody:ignored.example#key-0\n\ + \n\ + body text\n\ + \n\ + Signed-off-by: Alice \n\ + Signed-by-DID: did:webvh:QmTrailer:example.com#key-0\n"; + assert_eq!( + signer_did(commit.as_bytes()).unwrap(), + "did:webvh:QmTrailer:example.com" + ); + } + + #[test] + fn conflicting_signer_dids_reports_trailer_and_committer_disagreement() { + let commit = commit_with_trailer( + "Alice ", + "Signed-by-DID: did:webvh:QmTrailer:example.com#key-0", + ); + assert_eq!( + conflicting_signer_dids(commit.as_bytes()).unwrap(), + ( + "did:webvh:QmTrailer:example.com".to_string(), + "did:webvh:QmCommitter:example.com".to_string(), + ) + ); + } } diff --git a/crates/vgi-core/src/lib.rs b/crates/vgi-core/src/lib.rs index 127d0a3..a908c2f 100644 --- a/crates/vgi-core/src/lib.rs +++ b/crates/vgi-core/src/lib.rs @@ -17,6 +17,9 @@ mod commit; mod did; mod sshsig; -pub use commit::{committer_did, committer_identity, normalize_sshsig_armor, split_signed_commit}; +pub use commit::{ + committer_did, committer_identity, conflicting_signer_dids, normalize_sshsig_armor, signer_did, + split_signed_commit, +}; pub use did::{ED25519_MULTICODEC_PREFIX, ed25519_keys_from_doc}; pub use sshsig::{GIT_SSHSIG_NAMESPACE, create_ssh_signature}; diff --git a/docs/RUNBOOK.md b/docs/RUNBOOK.md index 0546480..9de5186 100644 --- a/docs/RUNBOOK.md +++ b/docs/RUNBOOK.md @@ -68,26 +68,37 @@ authorise the setup session, press Enter, then pick the persona and signing key. It configures git: - `gpg.format = ssh`, `gpg.ssh.program = did-git-sign`, `commit.gpgsign = true` -- **`user.email = `** — this is load-bearing. It is the only place - a commit states which identity signed it. A repo that overrides `user.email` - with an ordinary address will fail `noSignerDid` even with a valid signature. +- **`did-git-sign.key = `** — this is load-bearing. It selects the + signing persona, and a `commit-msg` hook writes it into a `Signed-by-DID:` + trailer, which is the only place a commit states which identity signed it. +- **`core.hooksPath`** — points at the directory holding that hook. The + directory also carries a delegating stub for every other standard hook, each + execing the repo's own `.git/hooks/`, so existing hooks keep running. + `init` refuses to take `core.hooksPath` from a tool that already owns it + (husky, lefthook, pre-commit) rather than silently disabling it. + +`user.email` is deliberately left alone: it stays an ordinary address so GitHub +and GitLab can attribute commits to the author's account. A commit that reaches +CI with no `Signed-by-DID:` trailer and a non-DID `user.email` fails +`noSignerDid` even with a valid signature — that means the hook did not run +(`--no-verify`, or a `core.hooksPath` taken by something else). Use `--global` for all repositories, or plain `init` for one. Verify with `did-git-sign health` before the first push, not after the PR check fails. -`--global` also sets `user.email` machine-wide. Fine for a contributor in one -community; if they are in two, use §3a instead — `init` prints that alternative -when run with `--global`. +`--global` also sets `did-git-sign.key` and `core.hooksPath` machine-wide. Fine +for a contributor in one community; if they are in two, use §3a instead — `init` +prints that alternative when run with `--global`. -`did-git-sign` refuses to sign a commit whose committer names a different DID -than the key it is about to use, so a mismatch fails at `git commit` with both -halves named rather than in CI as `unknownKey`. +`did-git-sign` refuses to sign a commit whose DID claim differs from the key it +is about to use, so a mismatch fails at `git commit` with both halves named +rather than in CI as `unknownKey`. ## 3a. Contributors in more than one community -Two settings pick an identity, and they must agree: `user.email` becomes the -commit's claim, and the persona selection picks the key. `did-git-sign` -resolves the key in this order — +One setting picks the identity: it selects the key *and*, read by the +`commit-msg` hook, becomes the commit's claim. `did-git-sign` and the hook +resolve it in the same order — 1. `DID_GIT_SIGN_KEY` (per-invocation), 2. `did-git-sign.key` in git config (per-repo), @@ -112,7 +123,7 @@ identity and the key selection together so they cannot drift: ```ini # ~/.config/git/community-openvtc [user] - email = did:webvh:QmAbc:openvtc.example#key-0 + email = you@openvtc.example name = Your Name [did-git-sign] key = did:webvh:QmAbc:openvtc.example#key-0 @@ -124,12 +135,12 @@ to clone it — and a throwaway clone outside your usual tree still gets the rig persona. Use `includeIf "gitdir:~/devel/openvtc/"` instead if your layout is authoritative and you prefer path matching. -Keep `user.email` in the same file as `did-git-sign.key`. Splitting them is -what lets them drift, and the pair is what the commit's verifiability rests on. +`did-git-sign.key` is the whole of it — there is no second setting to keep in +step, which is what used to drift. Set `user.name` and `user.email` however you +like alongside it; they affect forge attribution, not verifiability. -Reserve `DID_GIT_SIGN_KEY` for one-off overrides — and note that it moves the -key without moving `user.email`, so the sign-time check will refuse unless you -override both. +`DID_GIT_SIGN_KEY` is fine for one-off overrides: the hook honours it too, so +`DID_GIT_SIGN_KEY=… git commit` moves the key and the claim together. ## 4. Set up the repository @@ -227,7 +238,8 @@ the remediation is unambiguous: | Verdict | Cause | Fix | |---|---|---| | `unsigned` | no `gpgsig` header | signing is off — `did-git-sign health` | -| `noSignerDid` | signed, committer is not a DID | `user.email` was overridden; re-run `init` | +| `noSignerDid` | signed, but no DID in the trailer or committer | the `commit-msg` hook did not run — `--no-verify`, or `core.hooksPath` taken by another tool; check `did-git-sign health`, then re-run `init` | +| `conflictingSignerDids` | `Signed-by-DID:` trailer and DID committer name different identities | a hand-written trailer, or a rebase carrying an old one; amend so one claim remains | | `unresolvedSigner` | the claimed DID would not resolve | DID document unreachable, or publishes no Ed25519 method | | `unknownKey` | the claimed DID publishes no such key | signed by a key that identity does not hold | | `badSignature` | key is published, signature fails | the commit was altered after signing |