f0ec3c92

Fix the four Important findings from the whole-branch delegate review

a73x 2026-08-18 18:17

Commit message

Fix the four Important findings from the whole-branch delegate review

- Pin two untested denials: a delegate pushing settings itself, and a
  delegate publishing/deleting a release (list still works, it's Read).
- Make exec/release attribution logs say "<person> (via <key-id>)" like
  the connection-level auth log already does, instead of a bare key ID.
  Also route server logs to stderr (they were silently going to stdout)
  so a test can actually assert on them.
- Reject a certificate key ID that's empty, over 64 bytes, or outside
  printable ASCII before it reaches logs, a client-visible hook string,
  or a child env var — it's CA-controlled and ssh-keygen places no limit
  on it.
- README: note that the collab-refs clip is namespace-scoped, not
  operation-scoped, so RW+ lends a delegate rewind/delete over collab
  history; RW is the mitigation.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

README.md

Old		New
@@ -477,8 +477,13 @@ The cert's principal must name an enrolled person and its CA must be enrolled
477	for that name — `cadir/` lends identity, it never creates it. `access.conf`	477	for that name — `cadir/` lends identity, it never creates it. `access.conf`
478	never mentions delegates — the person holds the grants and the certificate	478	never mentions delegates — the person holds the grants and the certificate
479	borrows them, clipped to `refs/collab/*`. No rule can widen that clip: none	479	borrows them, clipped to `refs/collab/*`. No rule can widen that clip: none
480	grants a certificate a branch, a release, or a repository creation. The same	480	grants a certificate a branch, a release, or a repository creation. The clip
481	CA key enrolled under two names is allowed (unlike `keydir/`, where one key	481	is namespace-scoped, not operation-scoped, though: inside `refs/collab/*` a
		482	delegate inherits whatever the person holds there, rewind and delete
		483	included, so `RW+` on a repo lends every delegate of that person the power to
		484	rewind or delete collab refs — destroying issue and patch history — where
		485	`RW` keeps that out of reach. The same CA key enrolled under two names is
		486	allowed (unlike `keydir/`, where one key
482	under two names is an authorization coin-flip): a certificate names its	487	under two names is an authorization coin-flip): a certificate names its
483	principal, so the lookup runs the other way, and a shared CA is two explicit	488	principal, so the lookup runs the other way, and a shared CA is two explicit
484	opt-ins.	489	opt-ins.

src/server/governance/delegate.rs

@@ @@ -55,10 +55,26 @@ pub fn validate( @@
+    cert.validate_at(unix_now, fingerprints.iter())
+        .map_err(|e| format!("certificate did not validate for {person}: {e}"))?;
+    // key_id is the CA's choice, not ours, and it reaches a client-visible
+    // hook string, a child process env var, and the log — all unescaped. An
+    // OpenSSH cert places no limit on it (ssh-keygen -I takes any string,
+    // newlines included), so refuse anything that isn't a short, printable
+    // ASCII token before it goes anywhere.
+    let key_id = cert.key_id().to_string();
+    if key_id.is_empty() {
+        return Err("certificate key ID is empty".to_string());
+    }
+    if key_id.len() > 64 {
+        return Err(format!(
+            "certificate key ID is {} bytes, longer than 64",
+            key_id.len()
+        ));
+    }
+    if !key_id.bytes().all(|b| (0x20..=0x7E).contains(&b)) {
+        return Err("certificate key ID contains a character outside printable ASCII".to_string());
+    }
+    Ok(Delegate { person, key_id })
+#[cfg(test)]
@@ @@ -315,4 +331,66 @@ mod tests { @@
+        let err = validate(&cert, &gov, NOW).unwrap_err();
+        assert!(err.contains("did not validate"), "got {err}");
+    #[test]
+    fn an_empty_key_id_is_rejected() {
+        let tmp = tempfile::TempDir::new().unwrap();
+        let (ca, ca_pub) = keygen(tmp.path(), "ca");
+        let (person_key, person_pub) = keygen(tmp.path(), "alex");
+        let gov = governance(&person_pub, "alex", Some((&ca_pub, "alex")));
+        let cert = mint(
+            &ca,
+            &person_key.with_extension("pub"),
+            "",
+            Some("alex"),
+            VALID_WINDOW,
+            &[],
+        );
+        let err = validate(&cert, &gov, NOW).unwrap_err();
+        assert!(err.contains("key ID"), "got {err}");
+    }
+    #[test]
+    fn a_key_id_over_64_bytes_is_rejected() {
+        let tmp = tempfile::TempDir::new().unwrap();
+        let (ca, ca_pub) = keygen(tmp.path(), "ca");
+        let (person_key, person_pub) = keygen(tmp.path(), "alex");
+        let gov = governance(&person_pub, "alex", Some((&ca_pub, "alex")));
+        let long_id = "x".repeat(65);
+        let cert = mint(
+            &ca,
+            &person_key.with_extension("pub"),
+            &long_id,
+            Some("alex"),
+            VALID_WINDOW,
+            &[],
+        );
+        let err = validate(&cert, &gov, NOW).unwrap_err();
+        assert!(err.contains("key ID"), "got {err}");
+    }
+    #[test]
+    fn a_key_id_containing_a_newline_is_rejected() {
+        let tmp = tempfile::TempDir::new().unwrap();
+        let (ca, ca_pub) = keygen(tmp.path(), "ca");
+        let (person_key, person_pub) = keygen(tmp.path(), "alex");
+        let gov = governance(&person_pub, "alex", Some((&ca_pub, "alex")));
+        // ssh-keygen -I takes any string, newline included — no shell is
+        // involved (it's a single argv element), so this is the external
+        // oracle for "a hostile CA can put a newline in a key ID", not
+        // something our own test setup invented.
+        let cert = mint(
+            &ca,
+            &person_key.with_extension("pub"),
+            "evil\nfake-line",
+            Some("alex"),
+            VALID_WINDOW,
+            &[],
+        );
+        let err = validate(&cert, &gov, NOW).unwrap_err();
+        assert!(err.contains("key ID"), "got {err}");
+    }

src/server/main.rs

Old		New
@@ -108,7 +108,13 @@ enum Command {
108		108
109	#[tokio::main]	109	#[tokio::main]
110	async fn main() {	110	async fn main() {
111	tracing_subscriber::fmt::init();	111	// stderr, not the default stdout: this server's stdout is otherwise free
		112	// for a future machine-readable use, and callers already expect
		113	// diagnostics on stderr (the "server exited before becoming ready"
		114	// failure path reads it).
		115	tracing_subscriber::fmt()
		116	.with_writer(std::io::stderr)
		117	.init();
112		118
113	let args = Args::parse();	119	let args = Args::parse();
114		120

src/server/ssh/session.rs

Old		New
@@ -748,6 +748,20 @@ impl Handler for SshHandler {
748		748
749	let regime = self.regime();	749	let regime = self.regime();
750		750
		751	// The regime is what proves a delegate cert's principal is real and
		752	// still enrolled, so only from here on is there a person to name.
		753	// Swap the bare-key-ID fallback above for the same "<person> (via
		754	// <key-id>)" form the connection-level auth log already uses, so a
		755	// delegate's exec/release log lines read the same way.
		756	let principal = match &regime {
		757	Regime::Governed {
		758	name,
		759	delegate: Some(key_id),
		760	..
		761	} => format!("{name} (via {key_id})"),
		762	_ => principal,
		763	};
		764
751	let git_cmd = match exec_cmd {	765	let git_cmd = match exec_cmd {
752	ExecCommand::Git { cmd, .. } => cmd,	766	ExecCommand::Git { cmd, .. } => cmd,
753	ExecCommand::Release(rel) => {	767	ExecCommand::Release(rel) => {

tests/common/mod.rs

Old		New
@@ -1092,6 +1092,11 @@ impl ServerHarness {
1092		1092
1093	let mut server = Command::new(env!("CARGO_BIN_EXE_git-collab-server"))	1093	let mut server = Command::new(env!("CARGO_BIN_EXE_git-collab-server"))
1094	.args(["--config", config_path.to_str().unwrap()])	1094	.args(["--config", config_path.to_str().unwrap()])
		1095	// The server's default level is already INFO, so this is a
		1096	// no-op today — it's here so a test asserting on
		1097	// `server_log()` content keeps working if that default ever
		1098	// tightens.
		1099	.env("RUST_LOG", "info")
1095	.stdout(Stdio::piped())	1100	.stdout(Stdio::piped())
1096	.stderr(Stdio::piped())	1101	.stderr(Stdio::piped())
1097	.spawn()	1102	.spawn()
@@ -1578,25 +1583,56 @@ impl ServerHarness {
1578	/// than the `authorized_keys` one — the only kind that authenticates on a	1583	/// than the `authorized_keys` one — the only kind that authenticates on a
1579	/// governed server.	1584	/// governed server.
1580	pub fn ssh_exec_as(&self, key: &Path, remote_cmd: &str, stdin: &[u8]) -> Output {	1585	pub fn ssh_exec_as(&self, key: &Path, remote_cmd: &str, stdin: &[u8]) -> Output {
		1586	self.ssh_exec_with_identity(key, &[], remote_cmd, stdin)
		1587	}
		1588
		1589	/// Like `ssh_exec_as`, but authenticating with a delegate certificate
		1590	/// rather than the key's own enrollment.
		1591	pub fn ssh_exec_as_cert(
		1592	&self,
		1593	key: &Path,
		1594	cert: &Path,
		1595	remote_cmd: &str,
		1596	stdin: &[u8],
		1597	) -> Output {
		1598	let cert_flag = format!("CertificateFile={}", cert.display());
		1599	self.ssh_exec_with_identity(key, &["-o", &cert_flag], remote_cmd, stdin)
		1600	}
		1601
		1602	/// Shared plumbing for `ssh_exec_as`/`ssh_exec_as_cert`: the base ssh
		1603	/// options common to both, plus whatever `extra_args` the caller needs
		1604	/// for identity (e.g. a certificate file). See `ssh_exec_with_stdin` for
		1605	/// why stdin is written from a separate thread and a watchdog is needed.
		1606	fn ssh_exec_with_identity(
		1607	&self,
		1608	key: &Path,
		1609	extra_args: &[&str],
		1610	remote_cmd: &str,
		1611	stdin: &[u8],
		1612	) -> Output {
		1613	let port = self.ssh_addr.port().to_string();
		1614	let mut args = vec![
		1615	"-p",
		1616	port.as_str(),
		1617	"-i",
		1618	key.to_str().unwrap(),
		1619	"-o",
		1620	"StrictHostKeyChecking=no",
		1621	"-o",
		1622	"UserKnownHostsFile=/dev/null",
		1623	"-o",
		1624	"IdentitiesOnly=yes",
		1625	"-o",
		1626	"BatchMode=yes",
		1627	"-o",
		1628	"ConnectTimeout=5",
		1629	];
		1630	args.extend_from_slice(extra_args);
		1631	args.push("git@127.0.0.1");
		1632	args.push(remote_cmd);
		1633
1581	let mut child = Command::new("ssh")	1634	let mut child = Command::new("ssh")
1582	.args([	1635	.args(args)
1583	"-p",
1584	&self.ssh_addr.port().to_string(),
1585	"-i",
1586	key.to_str().unwrap(),
1587	"-o",
1588	"StrictHostKeyChecking=no",
1589	"-o",
1590	"UserKnownHostsFile=/dev/null",
1591	"-o",
1592	"IdentitiesOnly=yes",
1593	"-o",
1594	"BatchMode=yes",
1595	"-o",
1596	"ConnectTimeout=5",
1597	"git@127.0.0.1",
1598	remote_cmd,
1599	])
1600	.stdin(Stdio::piped())	1636	.stdin(Stdio::piped())
1601	.stdout(Stdio::piped())	1637	.stdout(Stdio::piped())
1602	.stderr(Stdio::piped())	1638	.stderr(Stdio::piped())

tests/delegate_test.rs

+        "delegate collab push failed: {}",
+        stderr(&push)
+    );
+    assert!(
+        harness.server_log().contains("claude-a"),
+        "the delegate's key id should be attributed in the server log, got: {}",
+        harness.server_log()
+    );
+    harness
+        .work_repo()
@@ @@ -354,3 +359,83 @@ fn a_malformed_cadir_file_rejects_the_settings_push() { @@
+        stderr(&push)
+    );
+/// The threat-model case by name: a delegate must not rewrite `settings`
+/// itself, the config that governs it — even though the person it acts for
+/// holds RW+ there.
+#[test]
+fn a_delegate_may_not_push_settings_itself() {
+    let harness = ServerHarness::new("delegate-settings-ceiling");
+    harness.push_head();
+    harness.bootstrap_settings_with_cas(
+        &access_conf(harness.repo_name()),
+        &[("alex.pub", "alex")],
+        &[("mint/alex.pub", "mint")],
+    );
+    let agent_key = harness.named_key("agent-key");
+    let ca = harness.delegate_ca("mint");
+    let cert = harness.mint_cert(&ca, &agent_key, "claude-a", "alex", "-1m:+30m");
+    // A harmless change in the settings work tree — the push itself, not its
+    // content, is what must be refused.
+    let work = harness.settings_work_dir();
+    std::fs::write(work.join("note.txt"), "n/a").unwrap();
+    common::git_cmd(&work, &["add", "-A"]);
+    common::git_cmd(&work, &["commit", "-q", "-m", "settings tweak"]);
+    let push = harness.ssh_push_from_cert(&work, &agent_key, &cert, "settings", "main:main");
+    assert!(
+        !push.status.success(),
+        "a delegate rewrote settings, the config that governs it"
+    );
+    assert!(
+        stderr(&push).contains("refs/collab"),
+        "the refusal should name the ceiling, got: {}",
+        stderr(&push)
+    );
+}
+/// Release upload/delete are outside the ceiling even though the person a
+/// delegate acts for may hold RW+ on the repo; List is Read and stays open.
+#[test]
+fn a_delegate_may_list_releases_but_not_publish_them() {
+    let harness = ServerHarness::new("delegate-release-ceiling");
+    harness.push_head();
+    harness.bootstrap_settings_with_cas(
+        &access_conf(harness.repo_name()),
+        &[("alex.pub", "alex")],
+        &[("mint/alex.pub", "mint")],
+    );
+    let agent_key = harness.named_key("agent-key");
+    let ca = harness.delegate_ca("mint");
+    let cert = harness.mint_cert(&ca, &agent_key, "claude-a", "alex", "-1m:+30m");
+    let upload = harness.ssh_exec_as_cert(
+        &agent_key,
+        &cert,
+        &format!(
+            "collab-release upload '{}.git' 'v1' 'a.tar.gz'",
+            harness.repo_name()
+        ),
+        b"fake tarball bytes",
+    );
+    assert!(
+        !upload.status.success(),
+        "a delegate published a release: {}",
+        stderr(&upload)
+    );
+    let list = harness.ssh_exec_as_cert(
+        &agent_key,
+        &cert,
+        &format!("collab-release list '{}.git'", harness.repo_name()),
+        b"",
+    );
+    assert!(
+        list.status.success(),
+        "a delegate could not list releases: {}",
+        stderr(&list)
+    );
+}