ec323e8c

Debug-quote unauthenticated cert input and warn on cadir/keydir drift

a73x 2026-08-18 19:09

Commit message

Debug-quote unauthenticated cert input and warn on cadir/keydir drift

Nine small pragmatic-review fixes to the delegate certificate feature:

- delegate.rs Debug-quotes `person` in error messages, since everything
  up to validate_at is cert-claimed input, not yet authenticated.
- The delegate release-mutation denial now names the refs/collab/*
  ceiling instead of replying NOT_FOUND, which only made sense before
  the delegate had authenticated.
- The refs/collab/ ceiling comment states the assumption it rests on:
  git-receive-pack rejects unsafe refnames before the update hook runs,
  so a traversal refname can never reach the prefix check.
- New governance::cadir_warnings surfaces two silent misconfigurations
  operators would otherwise not learn about: a cadir/ CA fingerprint
  that doubles as its own person's keydir/ key (self-mint capability),
  and a cadir/ entry naming nobody enrolled in keydir/ (dead config).
  Checked once per config change (settings-push validation and server
  startup), not per request, so it can't spam the log.
- New tests cover the README's claim that RW (vs RW+) on refs/collab/
  keeps rewind and delete out of a delegate's reach.
- README documents that a delegate's read surface is unclipped: it
  reads whatever the person it acts for reads.
- Assorted test-helper cleanup: settings_work_dir alias removed in
  favor of a public settings_work, comments moved to where their
  rationale belongs.

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

README.md

Old		New
@@ -488,6 +488,11 @@ under two names is an authorization coin-flip): a certificate names its
488	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
489	opt-ins.	489	opt-ins.
490		490
		491	The clip is write-only: a delegate reads whatever the person it acts for
		492	reads, with no narrowing — it could not otherwise prepare a patch against
		493	anything the person can see — so a leaked certificate exposes the person's
		494	whole read surface until it expires.
		495
491	Revocation is the roster: remove `cadir/mint/alex.pub` and the delegates it	496	Revocation is the roster: remove `cadir/mint/alex.pub` and the delegates it
492	minted die on their next command; remove the person's keys and their	497	minted die on their next command; remove the person's keys and their
493	delegates die with them. The cert's own expiry does the rest — there is no	498	delegates die with them. The cert's own expiry does the rest — there is no

src/server/governance/cadir.rs

Old		New
@@ -71,6 +71,12 @@ impl CaDir {
71	pub fn fingerprints_for(&self, name: &str) -> &[Fingerprint] {	71	pub fn fingerprints_for(&self, name: &str) -> &[Fingerprint] {
72	self.by_name.get(name).map(Vec::as_slice).unwrap_or(&[])	72	self.by_name.get(name).map(Vec::as_slice).unwrap_or(&[])
73	}	73	}
		74
		75	/// Every name `cadir/` enrolls a CA for, for hygiene checks that walk the
		76	/// whole roster (e.g. a name with no matching `keydir/` entry).
		77	pub fn names(&self) -> impl Iterator<Item = &str> {
		78	self.by_name.keys().map(String::as_str)
		79	}
74	}	80	}
75		81
76	#[cfg(test)]	82	#[cfg(test)]

src/server/governance/delegate.rs

Old		New
@@ -35,9 +35,13 @@ pub fn validate(
35	many => return Err(format!("certificate names {} principals", many.len())),	35	many => return Err(format!("certificate names {} principals", many.len())),
36	};	36	};
37		37
		38	// Everything from here up to validate_at below is unauthenticated input —
		39	// the cert's own claims, taken before its signature is checked — so it
		40	// must be Debug-quoted wherever it reaches an error message, the same as
		41	// `name` is at the critical-options check below.
38	// cadir/ delegates identity; it never creates it. The person must exist.	42	// cadir/ delegates identity; it never creates it. The person must exist.
39	if !governance.keys.names().contains(&person) {	43	if !governance.keys.names().contains(&person) {
40	return Err(format!("{person} is not enrolled in keydir/"));	44	return Err(format!("{person:?} is not enrolled in keydir/"));
41	}	45	}
42		46
43	// Per PROTOCOL.certkeys, an implementation MUST refuse a certificate	47	// Per PROTOCOL.certkeys, an implementation MUST refuse a certificate
@@ -50,7 +54,7 @@ pub fn validate(
50	// timestamp is inside the validity window — all three via validate_at.	54	// timestamp is inside the validity window — all three via validate_at.
51	let fingerprints = governance.cas.fingerprints_for(&person);	55	let fingerprints = governance.cas.fingerprints_for(&person);
52	if fingerprints.is_empty() {	56	if fingerprints.is_empty() {
53	return Err(format!("no CA is enrolled in cadir/ for {person}"));	57	return Err(format!("no CA is enrolled in cadir/ for {person:?}"));
54	}	58	}
55	cert.validate_at(unix_now, fingerprints.iter())	59	cert.validate_at(unix_now, fingerprints.iter())
56	.map_err(\|e\| format!("certificate did not validate for {person}: {e}"))?;	60	.map_err(\|e\| format!("certificate did not validate for {person}: {e}"))?;

src/server/governance/hook.rs

Old		New
@@ -129,6 +129,14 @@ pub fn run(refname: &str, old: &str, new: &str) -> Result<(), String> {
129	// goes briefly unreadable-as-Absent between the session's regime check	129	// goes briefly unreadable-as-Absent between the session's regime check
130	// and this hook's own re-read skip the ceiling entirely — hard-coded	130	// and this hook's own re-read skip the ceiling entirely — hard-coded
131	// rather than configured, so no line in access.conf can widen it either.	131	// rather than configured, so no line in access.conf can widen it either.
		132	//
		133	// Assumption written down: `starts_with("refs/collab/")` is sufficient to
		134	// confine a delegate only because a refname containing `..` — e.g.
		135	// `refs/collab/../heads/main` — can never reach this comparison in the
		136	// first place. `git-receive-pack` validates every pushed refname with
		137	// `check_refname_format` (`ref_name_is_safe` further down that call
		138	// chain) before invoking the update hook at all, so a path-traversing
		139	// refname is rejected upstream of this code, not by it.
132	let delegate = std::env::var(ENV_DELEGATE).ok().filter(\|v\| !v.is_empty());	140	let delegate = std::env::var(ENV_DELEGATE).ok().filter(\|v\| !v.is_empty());
133	if let Some(key_id) = &delegate {	141	if let Some(key_id) = &delegate {
134	if !refname.starts_with("refs/collab/") {	142	if !refname.starts_with("refs/collab/") {
@@ -244,8 +252,18 @@ fn validate_settings_push(
244	return Ok(());	252	return Ok(());
245	}	253	}
246		254
247	validate_settings_tree(repo, &tree, refname)	255	let governance = validate_settings_tree(repo, &tree, refname)
248	.map_err(\|e\| format!("rejecting this configuration; the previous one stays live\n {e}"))?;	256	.map_err(\|e\| format!("rejecting this configuration; the previous one stays live\n {e}"))?;
		257
		258	// Warnings, not errors: this push still lands. Printed here (rather than
		259	// returned as part of the Err path above) because a valid config that
		260	// merely looks suspicious should reach the operator without blocking
		261	// them — stderr from the update hook is relayed to the pusher as
		262	// `remote:` lines regardless of the hook's exit status.
		263	for warning in super::cadir_warnings(&governance) {
		264	eprintln!("warning: {warning}");
		265	}
		266
249	Ok(())	267	Ok(())
250	}	268	}
251		269

src/server/governance/mod.rs

+    check_roster_has_rules(&tree).err()
+/// Operator-visible warnings about `cadir/` entries that are not outright
+/// errors but are easy to get wrong silently:
+///
+/// - A CA fingerprint that is *also* someone's own key in `keydir/`: that
+///   person can self-mint a delegate certificate naming whoever the `cadir/`
+///   entry names. Legitimate for a solo operator running their own CA;
+///   silent privilege escalation for anyone else, so it is surfaced rather
+///   than judged here.
+/// - A `cadir/` entry for a name with no `keydir/` enrolment: dead config,
+///   typo-shaped — no certificate naming it can ever validate, since
+///   `delegate::validate` requires the principal to exist in `keydir/`
+///   first.
+///
+/// Warnings only: nothing here is rejected, and the config it describes is
+/// otherwise usable as pushed. Deliberately *not* called from `load`, which
+/// runs on every request; a caller invokes this once per config change —
+/// at push validation and at server startup — the same two moments
+/// `check_roster_has_rules` and `unruled_roster` cover for the roster/rules
+/// split.
+pub fn cadir_warnings(governance: &Governance) -> Vec<String> {
+    let mut ca_names: Vec<&str> = governance.cas.names().collect();
+    ca_names.sort_unstable();
+    let mut warnings = Vec::new();
+    for ca_name in &ca_names {
+        if !governance.keys.names().iter().any(|name| name == ca_name) {
+            warnings.push(format!(
+                "cadir/ is enrolled for {ca_name:?}, which is not enrolled in keydir/; \
+                 no certificate naming it can ever validate"
+            ));
+        }
+        for fingerprint in governance.cas.fingerprints_for(ca_name) {
+            let principal = format!("key:{fingerprint}");
+            if let Some(owner) = governance.keys.name_for(&principal) {
+                warnings.push(format!(
+                    "the CA enrolled in cadir/ for {ca_name:?} is also {owner:?}'s own key in \
+                     keydir/: {owner:?} can self-mint delegate certificates naming {ca_name:?}"
+                ));
+            }
+        }
+    }
+    warnings
+}
+/// Record which principal created a repository, for `CREATOR` rules.
+pub fn record_creator(repo_path: &Path, name: &str) -> std::io::Result<()> {
+    let dir = repo_path.join(".collab");
@@ @@ -581,4 +625,57 @@ mod tests { @@
+        let err = validate_settings_tree(&repo, &tree, "refs/heads/main").unwrap_err();
+        assert!(err.contains("cadir/alex.pub"), "got {err}");
+    #[test]
+    fn a_ca_fingerprint_that_is_also_a_keydir_key_is_warned_about() {
+        let mut keys = KeyDir::new();
+        keys.insert("keydir/alex.pub", PERSON_KEY).unwrap();
+        let mut cas = CaDir::new();
+        // alex's own key, enrolled as a CA for bob: alex can self-mint a
+        // delegate certificate naming bob.
+        cas.insert("cadir/mint/bob.pub", PERSON_KEY).unwrap();
+        let conf = AccessConf::parse("repo settings\n    RW+ = alex\n").unwrap();
+        let governance = Governance { conf, keys, cas };
+        let warnings = cadir_warnings(&governance);
+        assert!(
+            warnings
+                .iter()
+                .any(|w| w.contains("alex") && w.contains("bob")),
+            "got {warnings:?}"
+        );
+    }
+    #[test]
+    fn a_ca_named_for_nobody_in_keydir_is_warned_about() {
+        let mut keys = KeyDir::new();
+        keys.insert("keydir/alex.pub", PERSON_KEY).unwrap();
+        let mut cas = CaDir::new();
+        // "ghost" has no keydir/ entry at all — typo-shaped dead config.
+        cas.insert("cadir/mint/ghost.pub", CA_KEY).unwrap();
+        let conf = AccessConf::parse("repo settings\n    RW+ = alex\n").unwrap();
+        let governance = Governance { conf, keys, cas };
+        let warnings = cadir_warnings(&governance);
+        assert!(
+            warnings
+                .iter()
+                .any(|w| w.contains("ghost") && w.contains("not enrolled in keydir")),
+            "got {warnings:?}"
+        );
+    }
+    #[test]
+    fn a_well_formed_cadir_has_no_warnings() {
+        let mut keys = KeyDir::new();
+        keys.insert("keydir/alex.pub", PERSON_KEY).unwrap();
+        let mut cas = CaDir::new();
+        // A distinct CA key, enrolled for the person it names — the
+        // ordinary, unremarkable case.
+        cas.insert("cadir/mint/alex.pub", CA_KEY).unwrap();
+        let conf = AccessConf::parse("repo settings\n    RW+ = alex\n").unwrap();
+        let governance = Governance { conf, keys, cas };
+        assert!(cadir_warnings(&governance).is_empty());
+    }

src/server/main.rs

Old		New
@@ -217,6 +217,15 @@ async fn main() {
217	);	217	);
218	}	218	}
219		219
		220	// cadir/ hygiene: a config already on disk gets no push to validate
		221	// against, so it is checked here too, once, at the only moment an
		222	// operator is looking — the same reasoning as `unruled_roster` above.
		223	if let governance::GovernanceState::Active(governance) = governance::load(&config.repos_dir) {
		224	for warning in governance::cadir_warnings(&governance) {
		225	tracing::warn!("{warning}");
		226	}
		227	}
		228
220	let app_state = http::AppState {	229	let app_state = http::AppState {
221	repos_dir: config.repos_dir.clone(),	230	repos_dir: config.repos_dir.clone(),
222	site_title: config.site_title.clone(),	231	site_title: config.site_title.clone(),

src/server/ssh/session.rs

Old		New
@@ -246,7 +246,11 @@ impl SshHandler {
246	ReleaseCmd::Upload { .. } \| ReleaseCmd::Delete { .. } => Access::Rewind,	246	ReleaseCmd::Upload { .. } \| ReleaseCmd::Delete { .. } => Access::Rewind,
247	};	247	};
248	// Artifacts are not collab refs. A delegate may list what its person	248	// Artifacts are not collab refs. A delegate may list what its person
249	// may see; publishing and deleting are outside the ceiling.	249	// may see; publishing and deleting are outside the ceiling. Unlike the
		250	// unknown/unauthorized-repo case above, a delegate has already
		251	// authenticated and can `release list` this same repo, so there is no
		252	// repository existence to hide here — naming the ceiling is honest,
		253	// not a probe.
250	if let Regime::Governed {	254	if let Regime::Governed {
251	delegate: Some(key_id),	255	delegate: Some(key_id),
252	..	256	..
@@ -254,7 +258,15 @@ impl SshHandler {
254	{	258	{
255	if needed != Access::Read {	259	if needed != Access::Read {
256	warn!("Rejected release command from delegate {key_id}");	260	warn!("Rejected release command from delegate {key_id}");
257	return reply_and_close(session, channel, NOT_FOUND, 1);	261	return reply_and_close(
		262	session,
		263	channel,
		264	&format!(
		265	"error: delegate {key_id} may only write refs/collab/*; \
		266	releases are out of reach\n"
		267	),
		268	1,
		269	);
258	}	270	}
259	}	271	}
260	let authorized = match regime {	272	let authorized = match regime {

tests/common/mod.rs

Old		New
@@ -1092,10 +1092,9 @@ 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	1095	// Tests assert on `server_log()` content, so the level is
1096	// no-op today — it's here so a test asserting on	1096	// pinned here rather than left to inherit whatever the
1097	// `server_log()` content keeps working if that default ever	1097	// binary's own default happens to be.
1098	// tightens.
1099	.env("RUST_LOG", "info")	1098	.env("RUST_LOG", "info")
1100	.stdout(Stdio::piped())	1099	.stdout(Stdio::piped())
1101	.stderr(Stdio::piped())	1100	.stderr(Stdio::piped())
@@ -1308,14 +1307,10 @@ impl ServerHarness {
1308	self.repos_dir().join("settings.git")	1307	self.repos_dir().join("settings.git")
1309	}	1308	}
1310		1309
1311	fn settings_work(&self) -> PathBuf {
1312	self.root.path().join("settings-work")
1313	}
1314
1315	/// The settings working tree, for tests that stage a change by hand	1310	/// The settings working tree, for tests that stage a change by hand
1316	/// rather than through one of the `stage_`/`bootstrap_` helpers.	1311	/// rather than through one of the `stage_`/`bootstrap_` helpers.
1317	pub fn settings_work_dir(&self) -> PathBuf {	1312	pub fn settings_work(&self) -> PathBuf {
1318	self.settings_work()	1313	self.root.path().join("settings-work")
1319	}	1314	}
1320		1315
1321	/// Create `settings.git` and a working tree for it, the way an init	1316	/// Create `settings.git` and a working tree for it, the way an init
@@ -1344,6 +1339,11 @@ impl ServerHarness {
1344	/// `keys` maps a path within `keydir/` (e.g. `laptop/alex.pub`) to the	1339	/// `keys` maps a path within `keydir/` (e.g. `laptop/alex.pub`) to the
1345	/// name of a `named_key`, so a test can put one key at two paths and see	1340	/// name of a `named_key`, so a test can put one key at two paths and see
1346	/// them collapse to one identity.	1341	/// them collapse to one identity.
		1342	///
		1343	/// A call describes the roster in full: `keydir/` and `cadir/` are
		1344	/// cleared before writing, so a key or CA omitted from the arguments
		1345	/// actually disappears from the tree rather than merely failing to be
		1346	/// added — the same call can therefore both enrol and revoke.
1347	pub fn stage_settings(&self, access_conf: &str, keys: &[(&str, &str)]) {	1347	pub fn stage_settings(&self, access_conf: &str, keys: &[(&str, &str)]) {
1348	self.stage_settings_with_cas(access_conf, keys, &[]);	1348	self.stage_settings_with_cas(access_conf, keys, &[]);
1349	}	1349	}
@@ -1364,9 +1364,8 @@ impl ServerHarness {
1364	std::fs::create_dir_all(conf_path.parent().unwrap()).unwrap();	1364	std::fs::create_dir_all(conf_path.parent().unwrap()).unwrap();
1365	std::fs::write(&conf_path, access_conf).unwrap();	1365	std::fs::write(&conf_path, access_conf).unwrap();
1366		1366
1367	// Clear both dirs first: this call describes the roster and the CA	1367	// See stage_settings's doc comment: both dirs are cleared first so
1368	// enrolment in full, so a key or CA dropped from the arguments	1368	// this call fully describes the roster and CA enrolment.
1369	// must actually disappear from the tree, not just fail to be added.
1370	let keydir = work.join("keydir");	1369	let keydir = work.join("keydir");
1371	if keydir.exists() {	1370	if keydir.exists() {
1372	std::fs::remove_dir_all(&keydir).unwrap();	1371	std::fs::remove_dir_all(&keydir).unwrap();

tests/delegate_test.rs

@@ @@ -343,7 +343,7 @@ fn a_malformed_cadir_file_rejects_the_settings_push() { @@
+    );
+    // Stage a broken CA file in the settings work tree and push over SSH.
+    let work = harness.settings_work();
+    std::fs::write(work.join("cadir").join("junk.pub"), "not a key").unwrap();
+    common::git_cmd(&work, &["add", "-A"]);
+    common::git_cmd(&work, &["commit", "-q", "-m", "break cadir"]);
@@ @@ -379,7 +379,7 @@ fn a_delegate_may_not_push_settings_itself() { @@
+    // A harmless change in the settings work tree — the push itself, not its
+    // content, is what must be refused.
+    let work = harness.settings_work();
+    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"]);
@@ @@ -439,3 +439,153 @@ fn a_delegate_may_list_releases_but_not_publish_them() { @@
+        stderr(&list)
+    );
+/// Mint a delegate cert of `alex` for `harness`, having bootstrapped settings
+/// with `access_conf`. Shared by the RW/RW+ collab-ref pair below, which
+/// differ only in that one line of config.
+fn delegate_of_alex(
+    harness: &ServerHarness,
+    access_conf: &str,
+) -> (std::path::PathBuf, std::path::PathBuf) {
+    harness.bootstrap_settings_with_cas(
+        access_conf,
+        &[("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");
+    (agent_key, cert)
+}
+/// The README's claim that `RW refs/collab/` (as opposed to `RW+`) keeps
+/// rewind and delete of collab refs out of a delegate's reach, even though
+/// the delegate's write into that namespace is otherwise unrestricted.
+#[test]
+fn a_delegate_under_rw_may_write_but_not_rewind_or_delete_collab_refs() {
+    let harness = ServerHarness::new("delegate-rw-ceiling");
+    harness.push_head();
+    let conf = format!(
+        "repo settings\n    RW+ = alex\n\nrepo {}\n    RW refs/collab/ = alex\n",
+        harness.repo_name()
+    );
+    let (agent_key, cert) = delegate_of_alex(&harness, &conf);
+    harness.work_repo().issue_open("RW ceiling");
+    let push = harness.ssh_push_from_cert(
+        harness.work_repo().dir.path(),
+        &agent_key,
+        &cert,
+        harness.repo_name(),
+        "refs/collab/*:refs/collab/*",
+    );
+    assert!(
+        push.status.success(),
+        "a delegate under RW could not write a collab ref: {}",
+        stderr(&push)
+    );
+    let victim = harness
+        .work_repo()
+        .git(&["for-each-ref", "--format=%(refname)", "refs/collab/"])
+        .lines()
+        .next()
+        .expect("a collab ref to target")
+        .trim()
+        .to_string();
+    // Rewind: force-push an unrelated commit onto the same ref name, which is
+    // not a fast-forward of it.
+    harness
+        .work_repo()
+        .commit_file("unrelated.txt", "x", "unrelated commit");
+    let rewind = harness.ssh_push_from_cert(
+        harness.work_repo().dir.path(),
+        &agent_key,
+        &cert,
+        harness.repo_name(),
+        &format!("+HEAD:{victim}"),
+    );
+    assert!(
+        !rewind.status.success(),
+        "a delegate under RW rewound a collab ref"
+    );
+    let delete = harness.ssh_push_from_cert(
+        harness.work_repo().dir.path(),
+        &agent_key,
+        &cert,
+        harness.repo_name(),
+        &format!(":{victim}"),
+    );
+    assert!(
+        !delete.status.success(),
+        "a delegate under RW deleted a collab ref"
+    );
+}
+/// The contrast: under `RW+`, the same operations succeed — a delegate
+/// inherits rewind and delete inside `refs/collab/*` along with everything
+/// else the person holds there.
+#[test]
+fn a_delegate_under_rw_plus_may_rewind_and_delete_collab_refs() {
+    let harness = ServerHarness::new("delegate-rw-plus-ceiling");
+    harness.push_head();
+    let conf = format!(
+        "repo settings\n    RW+ = alex\n\nrepo {}\n    RW+ refs/collab/ = alex\n",
+        harness.repo_name()
+    );
+    let (agent_key, cert) = delegate_of_alex(&harness, &conf);
+    harness.work_repo().issue_open("RW+ ceiling");
+    let push = harness.ssh_push_from_cert(
+        harness.work_repo().dir.path(),
+        &agent_key,
+        &cert,
+        harness.repo_name(),
+        "refs/collab/*:refs/collab/*",
+    );
+    assert!(
+        push.status.success(),
+        "a delegate under RW+ could not write a collab ref: {}",
+        stderr(&push)
+    );
+    let victim = harness
+        .work_repo()
+        .git(&["for-each-ref", "--format=%(refname)", "refs/collab/"])
+        .lines()
+        .next()
+        .expect("a collab ref to target")
+        .trim()
+        .to_string();
+    harness
+        .work_repo()
+        .commit_file("unrelated.txt", "x", "unrelated commit");
+    let rewind = harness.ssh_push_from_cert(
+        harness.work_repo().dir.path(),
+        &agent_key,
+        &cert,
+        harness.repo_name(),
+        &format!("+HEAD:{victim}"),
+    );
+    assert!(
+        rewind.status.success(),
+        "a delegate under RW+ could not rewind a collab ref: {}",
+        stderr(&rewind)
+    );
+    let delete = harness.ssh_push_from_cert(
+        harness.work_repo().dir.path(),
+        &agent_key,
+        &cert,
+        harness.repo_name(),
+        &format!(":{victim}"),
+    );
+    assert!(
+        delete.status.success(),
+        "a delegate under RW+ could not delete a collab ref: {}",
+        stderr(&delete)
+    );
+}