2551fecd

Name the resolved person, not just the fingerprint, in governance rejections

a73x 2026-08-19 10:31

Commit message

Name the resolved person, not just the fingerprint, in governance rejections

A governed key session authenticated as a bare `key:SHA256:...` fingerprint
even after `governance.name_for()` resolved a name for it, so every
rejection line made the operator map a fingerprint back to a person by
hand — in the one message where knowing who was refused is the point.
Delegate sessions already carried a resolved display name; key sessions
now do too, via a `log_principal` helper that ACL checks don't use (they
still key on the raw fingerprint form policies are written against).

Relatedly, "may not create" was misleading for the common case: a typo
or a repo never pushed here, not an ACL gap. A git-upload-pack (fetch or
clone) of an absent path was never going to create anything regardless
of the `C` rules, so evaluating them there just mislabeled "no such
repository" as a permission refusal. Only git-receive-pack still asks
`C`; a fetch of a missing repo now says plainly that it does not exist.

Finally, the git subprocess spawned per exec request runs on its own
tokio task, which does not inherit the connection's tracing span, so its
"Git subprocess error" line logged with no identity attached — the one
gap left by the earlier per-connection span work. Capture the ambient
span at spawn time and instrument the task with it.

Fixes 46c1bb64.

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

src/server/ssh/session.rs

Old		New
@@ -36,6 +36,25 @@ enum Regime {
36	Closed,	36	Closed,
37	}	37	}
38		38
		39	/// The principal string a log line should show.
		40	///
		41	/// A governed key session authenticates as a bare fingerprint but resolves
		42	/// to a name in `keydir/` — the name rules are written against, and the one
		43	/// an operator reading a rejection wants to see. Everyone else already
		44	/// carries the right string in `principal`: a delegate's is set once at
		45	/// `auth_openssh_certificate` as `"{person} (via {key_id})"`, and an
		46	/// ungoverned or closed session has no name to resolve to.
		47	fn log_principal(regime: &Regime, principal: &str) -> String {
		48	match regime {
		49	Regime::Governed {
		50	name,
		51	delegate: None,
		52	..
		53	} => format!("{name} ({principal})"),
		54	_ => principal.to_string(),
		55	}
		56	}
		57
39	/// Configuration shared across all SSH connections.	58	/// Configuration shared across all SSH connections.
40	#[derive(Debug, Clone)]	59	#[derive(Debug, Clone)]
41	pub struct SshServerConfig {	60	pub struct SshServerConfig {
@@ -278,7 +297,8 @@ impl SshHandler {
278	if !authorized {	297	if !authorized {
279	warn!(	298	warn!(
280	"Rejected release command: principal {} not authorized on {:?}",	299	"Rejected release command: principal {} not authorized on {:?}",
281	principal, resolved_path	300	log_principal(regime, principal),
		301	resolved_path
282	);	302	);
283	return reply_and_close(session, channel, NOT_FOUND, 1);	303	return reply_and_close(session, channel, NOT_FOUND, 1);
284	}	304	}
@@ -763,6 +783,10 @@ impl Handler for SshHandler {
763	};	783	};
764		784
765	let regime = self.regime();	785	let regime = self.regime();
		786	// What a rejection line should call this session. ACL checks
		787	// below keep using `principal` (the fingerprint form policies are
		788	// keyed on); this is only for what an operator reads.
		789	let log_principal = log_principal(&regime, &principal);
766		790
767	// The delegate ceiling, asked once for whatever the command turns	791	// The delegate ceiling, asked once for whatever the command turns
768	// out to be — see governance::delegate::permits. Per-command sites	792	// out to be — see governance::delegate::permits. Per-command sites
@@ -853,32 +877,46 @@ impl Handler for SshHandler {
853	if !authorized {	877	if !authorized {
854	warn!(	878	warn!(
855	"Rejected exec request: principal {} is not authorized for {} on {:?}",	879	"Rejected exec request: principal {} is not authorized for {} on {:?}",
856	principal,	880	log_principal,
857	git_cmd.as_str(),	881	git_cmd.as_str(),
858	resolved_path	882	resolved_path
859	);	883	);
860	return reply_and_close(session, channel, "", 1);	884	return reply_and_close(session, channel, "", 1);
861	}	885	}
862	} else {	886	} else {
863	// Auto-creation is what makes wild repos work without a central	887	// Auto-creation is what makes wild repos work without a
864	// allocator, so under governance it is a permission of its own:	888	// central allocator, so under governance it is a permission
865	// `C` on a pattern the requested name matches.	889	// of its own: `C` on a pattern the requested name matches —
		890	// but only for git-receive-pack, the only command that could
		891	// ever create anything. A git-upload-pack (fetch/clone) of an
		892	// absent path was never going to create it either way, so
		893	// asking `C` here would answer a question the client never
		894	// raised and mislabel a typo or an unpushed repo as a
		895	// permission refusal — the confusing case this distinguishes.
866	if let Regime::Governed {	896	if let Regime::Governed {
867	governance, name, ..	897	governance, name, ..
868	} = &regime	898	} = &regime
869	{	899	{
870	// A delegate never reaches this: creating a repository is	900	if git_cmd == GitCmd::ReceivePack {
871	// refused at the ceiling gate above, whatever `C` its	901	// A delegate never reaches this: creating a
872	// person may hold here.	902	// repository is refused at the ceiling gate above,
873	let allowed = repo_key.as_deref().is_some_and(\|key\| {	903	// whatever `C` its person may hold here.
874	governance	904	let allowed = repo_key.as_deref().is_some_and(\|key\| {
875	.conf	905	governance
876	.allows_repo(key, &Subject::new(name), Access::Create)	906	.conf
877	});	907	.allows_repo(key, &Subject::new(name), Access::Create)
878	if !allowed {	908	});
		909	if !allowed {
		910	warn!(
		911	"Rejected exec request: no such repository {:?}, and {} may not create it",
		912	resolved_path, log_principal
		913	);
		914	return reply_and_close(session, channel, "", 1);
		915	}
		916	} else {
879	warn!(	917	warn!(
880	"Rejected exec request: principal {} may not create {:?}",	918	"Rejected exec request: no such repository {:?}",
881	principal, resolved_path	919	resolved_path
882	);	920	);
883	return reply_and_close(session, channel, "", 1);	921	return reply_and_close(session, channel, "", 1);
884	}	922	}
@@ -901,7 +939,7 @@ impl Handler for SshHandler {
901	}	939	}
902	Ok(false) => {	940	Ok(false) => {
903	warn!(	941	warn!(
904	"Rejected exec request: repo path does not exist: {:?}",	942	"Rejected exec request: no such repository {:?}",
905	resolved_path	943	resolved_path
906	);	944	);
907	return reply_and_close(session, channel, "", 1);	945	return reply_and_close(session, channel, "", 1);
@@ -929,15 +967,23 @@ impl Handler for SshHandler {
929	let (tx, rx) = mpsc::channel::<Vec<u8>>(64);	967	let (tx, rx) = mpsc::channel::<Vec<u8>>(64);
930	self.stdin_tx = Some((channel, tx));	968	self.stdin_tx = Some((channel, tx));
931		969
932	// Spawn the git subprocess	970	// Spawn the git subprocess. `tokio::spawn` does not inherit the
		971	// current span, so without `.instrument` every "Git subprocess
		972	// error" line would log with no connection attached — carry it
		973	// over explicitly.
933	let handle = session.handle();	974	let handle = session.handle();
934	tokio::spawn(async move {	975	let spawn_span = Span::current();
935	if let Err(e) =	976	tokio::spawn(
936	run_git_command(handle, channel, git_cmd, &resolved_path, child_env, rx).await	977	async move {
937	{	978	if let Err(e) =
938	error!("Git subprocess error: {}", e);	979	run_git_command(handle, channel, git_cmd, &resolved_path, child_env, rx)
		980	.await
		981	{
		982	error!("Git subprocess error: {}", e);
		983	}
939	}	984	}
940	});	985	.instrument(spawn_span),
		986	);
941		987
942	Ok(())	988	Ok(())
943	}	989	}

tests/governance_test.rs

@@ @@ -9,7 +9,7 @@ @@
+mod common;
+use common::ServerHarness;
+use std::process::{Command, Output};
+/// The configuration most of these tests run under.
+///
+        "server.toml must not be able to publish what access.conf withholds"
+    );
+// ---- Log identity: rejections name the person, not just the key --------
+/// The `key:SHA256:...` principal string OpenSSH's own `ssh-keygen -lf`
+/// would print for `pubkey_path` — an external oracle, not the server's own
+/// fingerprint code, so this proves agreement with an independent source
+/// rather than self-consistency.
+fn fingerprint_oracle(pubkey_path: &std::path::Path) -> String {
+    let out = Command::new("ssh-keygen")
+        .args(["-lf"])
+        .arg(pubkey_path)
+        .output()
+        .expect("failed to run ssh-keygen -lf");
+    assert!(
+        out.status.success(),
+        "ssh-keygen -lf failed: {}",
+        String::from_utf8_lossy(&out.stderr)
+    );
+    let stdout = String::from_utf8_lossy(&out.stdout);
+    // "256 SHA256:Gzb/mmBlyDmoWq3hMyPjb3SRWklPJ/yDAoD1K+ATvJA name (ED25519)"
+    let fp = stdout
+        .split_whitespace()
+        .nth(1)
+        .expect("ssh-keygen -lf output missing fingerprint field");
+    format!("key:{fp}")
+}
+/// A refused create names the person `settings.git` resolved the key to,
+/// with the fingerprint kept alongside for the surprising-resolution case —
+/// not just the bare fingerprint an operator has to map back to someone by
+/// hand.
+#[test]
+fn a_refused_create_names_the_resolved_person_and_the_fingerprint() {
+    let harness = ServerHarness::new("governed-create-identity");
+    harness.bootstrap_settings(ACCESS_CONF, KEYS);
+    let admin = harness.named_key("alex");
+    let fingerprint = fingerprint_oracle(&admin.with_extension("pub"));
+    // "eitri" matches no `repo` pattern in ACCESS_CONF (not "settings", not
+    // "governed", not "agents/[a-z-]+"), so no rule grants alex `C` on it.
+    harness.work_repo().commit_file("e.txt", "1", "seed");
+    let push = harness.ssh_push_from(harness.work_repo().dir.path(), &admin, "eitri", "main:main");
+    assert_refused(&push, "a push creating a repo no rule grants C on");
+    assert!(
+        !harness.repos_dir().join("eitri.git").exists(),
+        "the repository must not exist after a refused create"
+    );
+    let log = harness.server_log();
+    let rejection = log
+        .lines()
+        .find(|line| line.contains("Rejected exec request") && line.contains("eitri"))
+        .unwrap_or_else(|| panic!("expected a create-rejection line, got: {log}"));
+    assert!(
+        rejection.contains("alex") && rejection.contains(&fingerprint),
+        "rejection should name both the resolved person and the fingerprint, got: {rejection}"
+    );
+    assert!(
+        rejection.contains("no such repository") && rejection.contains("may not create"),
+        "a denied create should say so, not just \"may not create\", got: {rejection}"
+    );
+}
+/// The common case behind "no such repository" is a typo or a repo never
+/// pushed here — not a permission gap. A fetch of an absent repository can
+/// never create anything either way, so it must not be phrased as a denied
+/// create, contrasting with the ACL-denial case above.
+#[test]
+fn a_fetch_of_an_absent_repo_is_not_phrased_as_a_denied_create() {
+    let harness = ServerHarness::new("governed-fetch-missing");
+    harness.bootstrap_settings(ACCESS_CONF, KEYS);
+    let admin = harness.named_key("alex");
+    let fetch = harness.ssh_fetch(harness.work_repo().dir.path(), &admin, "nonexistent");
+    assert!(
+        !fetch.status.success(),
+        "fetching an absent repository should fail"
+    );
+    let log = harness.server_log();
+    let rejection = log
+        .lines()
+        .find(|line| line.contains("Rejected exec request") && line.contains("nonexistent"))
+        .unwrap_or_else(|| panic!("expected a missing-repo rejection line, got: {log}"));
+    assert!(rejection.contains("no such repository"), "got: {rejection}");
+    assert!(
+        !rejection.contains("may not create"),
+        "a fetch was never going to create anything, so it must not be phrased as a denied \
+         create, got: {rejection}"
+    );
+}