diff --git a/src/cmd/repo/checkout.rs b/src/cmd/repo/checkout.rs index 89b3016..4f10637 100644 --- a/src/cmd/repo/checkout.rs +++ b/src/cmd/repo/checkout.rs @@ -877,7 +877,7 @@ pub(super) async fn configure(args: ConfigureArgs) -> Result<()> { crate::term::say::step!(Ssh, "skipped (--no-ssh-config)"); Ok(()) } else { - configure_ssh(here, &did, &who, args.dry_run).await + configure_ssh(here, &did, &who, written, args.dry_run).await }; // The hook is the reason a stack can be created from a plain-git branch @@ -951,12 +951,87 @@ fn advise_here(did: &str, handle: Option<&str>) { } } +/// What a failed push-key lookup leaves `repo configure` able to do, given +/// what this checkout has pinned already. +#[derive(Debug, PartialEq)] +pub(super) enum KeyLookupFailure { + /// Say the keys could not be checked and exit 0. + Warn, + /// Refuse, in these words. + Refuse(String), +} + +/// Decide between the two when `push_key_for` itself fails — a PDS that did +/// not answer, a rate-limited plc.directory, a `listRecords` that timed out — +/// as opposed to answering that this account has no key here. +/// +/// The judgement is about what the checkout is left holding, not about how +/// bad the network was. With nothing pinned, a warning is honest: no pin was +/// promised to anyone, the next push falls back to whatever ssh offers first, +/// and that is the state the checkout was already in before this command ran. +/// With a pin already in place it is not, because `core.sshCommand` is a +/// claim about *which account pushes from here*, and the `[user]` identity +/// for a different account has just been written above it. Continuing leaves +/// `IdentitiesOnly=yes -i ` authenticating to the knot as the old +/// account while every commit is authored as the new one, and says so with a +/// warning that only mentions the lookup. So it is refused, the same way the +/// confirmed-no-key branch below refuses and the same way `repo clone --ssh` +/// refuses an unverified key — and unlike `clone`, `configure` has no https +/// fallback that would make going ahead defensible. +/// +/// Because the identity write lands first, the refusal has to describe a +/// half-applied checkout rather than a command that did nothing, in the +/// manner of [`orphan_note`](super::write::orphan_note) — which is why it +/// takes the [`Identity`] outcome rather than assuming one. Under +/// `--dry-run` nothing was written and the refusal says that instead: it +/// still fails, because predicting this failure is what the dry run is for. +pub(super) fn key_lookup_failure( + who: &str, + error: &str, + pinned: Option<&str>, + identity: Identity, + dry_run: bool, +) -> KeyLookupFailure { + let Some(pinned) = pinned else { + return KeyLookupFailure::Warn; + }; + // Written or already correct are the two states in which the `[user]` + // section names this account and the pin contradicts it. `LeftAlone` and + // `Skipped` are not: the checkout goes on naming whoever it named before, + // this command has changed nothing, and a warning describes that exactly. + let state = match (identity, dry_run) { + (Identity::Skipped | Identity::LeftAlone, _) => return KeyLookupFailure::Warn, + (_, true) => format!( + "nothing was written; this is what a real run would stop on, with the git \ + identity already naming {who} and core.sshCommand still pointing at" + ), + (Identity::AlreadySet, false) => format!( + "the git identity here already names {who}, so commits are authored as \ + that account, while core.sshCommand still points at" + ), + (Identity::Written, false) => format!( + "the git identity is written, so commits here are already authored as {who}, \ + while core.sshCommand still points at" + ), + }; + KeyLookupFailure::Refuse(format!( + "could not check registered SSH keys for {who}: {error}\n\ + {state} the key it held before ({pinned}), which is a claim about a \ + different account: a push from here would authenticate as whoever that key \ + belongs to\n\ + retry when the lookup works, or configure the identity alone and clear the \ + stale pin: atgc repo configure --no-ssh-config && git config --local --unset \ + core.sshCommand" + )) +} + /// Pin `core.sshCommand` to the key matching the account's registered /// `sh.tangled.publicKey` records — the other half of "configure this /// checkout for this account", and the same lookup `repo clone` does. /// -/// Never clears an existing value: if the PDS cannot be reached, whatever is -/// already configured is better than nothing and is left alone. +/// Never clears an existing value: what is already configured is left alone +/// rather than blanked, though when the lookup fails it may be refused as a +/// stale claim — see [`key_lookup_failure`]. /// /// An account with no key this machine holds *is* fatal, though, and that is /// a change. Pinning a key is half of what this command does, and the half @@ -971,16 +1046,31 @@ fn advise_here(did: &str, handle: Option<&str>) { /// asking for `--force`. It is transport configuration rather than something /// git bakes irreversibly into commits, the previous value is printed, and /// `--no-ssh-config` keeps a hand-written one. -async fn configure_ssh(dir: &Path, did: &str, who: &str, dry_run: bool) -> Result<()> { +async fn configure_ssh( + dir: &Path, + did: &str, + who: &str, + identity: Identity, + dry_run: bool, +) -> Result<()> { let key = match push_key_for(did).await { Ok(key) => key, - // The lookup itself failing is a different thing from having no key, - // and it stays a warning: the network is not the user's mistake, and - // the pin that is already there keeps working. + // The lookup itself failing is a different thing from having no key. + // Whether that difference is survivable depends on what is pinned + // here already, which is why the existing value is read before the + // decision instead of after it. Err(e) => { crate::logging::debug::dump_err("push identity lookup failed", &e); - crate::term::say::warning!(Ssh, "could not check registered SSH keys: {e}"); - return Ok(()); + let pinned = gitconfig::local_config(dir, "core.sshCommand"); + match key_lookup_failure(who, &format!("{e:#}"), pinned.as_deref(), identity, dry_run) { + KeyLookupFailure::Warn => { + crate::term::say::warning!(Ssh, "could not check registered SSH keys: {e}"); + return Ok(()); + } + KeyLookupFailure::Refuse(message) => { + return Err(crate::exit::fail(crate::exit::classify(&e), message)); + } + } } }; let Some(identity) = key.identity() else { diff --git a/src/cmd/repo/mod.rs b/src/cmd/repo/mod.rs index e8451bc..e42c0c0 100644 --- a/src/cmd/repo/mod.rs +++ b/src/cmd/repo/mod.rs @@ -436,7 +436,10 @@ mod tests { use super::branch::{ delete_branch_input, is_current_default, repo_record_uri, set_default_branch_input, }; - use super::checkout::{GitIdentity, Overwrite, parse_repo_ref, write_git_identity}; + use super::checkout::{ + GitIdentity, Identity, KeyLookupFailure, Overwrite, key_lookup_failure, parse_repo_ref, + write_git_identity, + }; use super::read::{ Count, KnotState, PAGE, branch_count, default_branch_from, language_summary, languages_from, newest_first, repo_row_json, repo_view_json, tag_names, tag_summary, @@ -1085,6 +1088,118 @@ mod tests { } } + // ----------------------------------------------------------------------- + // repo configure: a push-key lookup that failed + // ----------------------------------------------------------------------- + + /// With no `core.sshCommand` in this checkout, a lookup that could not be + /// made leaves nothing wrong behind, so `repo configure` warns and carries + /// on rather than failing a run whose identity half succeeded. + #[test] + fn an_unpinned_checkout_survives_a_lookup_that_could_not_be_made() { + for dry_run in [false, true] { + assert_eq!( + key_lookup_failure( + "@b.example", + "connection timed out", + None, + Identity::Written, + dry_run + ), + KeyLookupFailure::Warn, + "dry_run: {dry_run}" + ); + } + } + + /// A refusal claiming the identity landed would be a lie when it did not: + /// `--no-git-config` and an ordinary git identity left alone both leave + /// the checkout naming exactly whoever it named before this command ran, + /// so the stale pin contradicts nothing new and the warning stands. + #[test] + fn an_identity_this_run_did_not_write_leaves_the_pin_a_warning() { + for identity in [Identity::Skipped, Identity::LeftAlone] { + assert_eq!( + key_lookup_failure( + "@b.example", + "connection timed out", + Some("ssh -i /home/a/.ssh/a_key"), + identity, + false + ), + KeyLookupFailure::Warn, + "identity: {identity:?}" + ); + } + } + + /// The bug this pins: `repo configure --account B` against an unreachable + /// PDS wrote B into `[user]`, warned that the keys could not be checked, + /// and exited 0 with account A's key still pinned — so the next push + /// authenticated as A while every commit claimed B. An existing pin makes + /// the lookup failure fatal, the way a confirmed missing key already was. + #[test] + fn a_pin_left_over_from_another_account_is_refused_not_warned_about() { + let failure = key_lookup_failure( + "@b.example", + "connection timed out", + Some("ssh -o IdentitiesOnly=yes -i \"/home/a/.ssh/a_key\""), + Identity::Written, + false, + ); + let KeyLookupFailure::Refuse(message) = failure else { + panic!("an existing pin has to be refused, not warned about: {failure:?}"); + }; + assert!(message.contains("connection timed out"), "{message}"); + assert!(message.contains("/home/a/.ssh/a_key"), "{message}"); + } + + /// The refusal comes after the identity write, so it has to say what the + /// checkout is now — half-configured — rather than reading as a command + /// that did nothing, the way [`orphan_note`] does for `repo delete`. + #[test] + fn the_refusal_says_the_identity_is_already_written() { + let KeyLookupFailure::Refuse(message) = key_lookup_failure( + "@b.example", + "connection timed out", + Some("ssh -i /home/a/.ssh/a_key"), + Identity::Written, + false, + ) else { + panic!("an existing pin has to be refused"); + }; + assert!( + message.contains("identity is written"), + "the refusal has to name the half that already landed: {message}" + ); + assert!( + message.contains("authored as @b.example"), + "and whose name commits now carry: {message}" + ); + assert!( + message.contains("retry"), + "unlike the orphan note, a retry is the fix here: {message}" + ); + } + + /// A dry run writes no identity, so the same refusal must not claim one + /// landed — but it stays a refusal, because predicting this failure is + /// what the dry run is for. + #[test] + fn the_dry_run_refusal_does_not_claim_an_identity_was_written() { + let KeyLookupFailure::Refuse(message) = key_lookup_failure( + "@b.example", + "connection timed out", + Some("ssh -i /home/a/.ssh/a_key"), + Identity::Written, + true, + ) else { + panic!("a dry run predicts the refusal rather than skipping it"); + }; + assert!(message.contains("nothing was written"), "{message}"); + assert!(!message.contains("identity is written"), "{message}"); + } + #[test] fn shorthand_forms() { let expected = ("permadeath.com".to_string(), "atgc".to_string());