starred/tty7
1
0
Fork 0
mirror of https://github.com/l0ng-ai/tty7.git synced 2026-09-22 00:02:23 +00:00
Code Issues Packages Projects Releases Wiki Activity

test(ssh): pin the two host-key decisions nothing was checking

Browse Source 
Mutating the host-key policy table showed it is well covered: accepting a
revoked key with verification off, and silently accepting an unknown or a
changed key, each fail the suite. The carrying-out was not.

**Saying no had nothing holding it.** Deleting the `if !accept` guard in
`apply_decision` — so a person shown a changed host key declines and connects
anyway — left every test in the repo green. `accepted_and_remembered` is
tested on its own; nothing checked that the caller acts on its answer. Now
four responses are refused: no, no-with-the-checkbox-still-ticked, cancelled,
and a response of the wrong shape. It touches no filesystem, because none of
them reach the recording branch.

**Dropping the superseded line had nothing holding it either.** The order is
the point — `check` answers `Known` on any same-algorithm match, so appending
without dropping leaves the key this one replaces trusted for good, which is
how a host that rotated away from a compromised key goes on accepting the old
one. There was a test named after that bug, but it called the two halves
itself, so removing the `forget_superseded` call from `apply_decision` changed
nothing it could see.

The sequence is now one function, `record_trusted`, with the order and the
"if the drop fails, append nothing" rule stated where it happens. Production
and that test call the same thing, so neither can drop a half alone. Verified
by re-running both mutations against the new tests; each fails, naming what
went wrong.
This commit is contained in:
l0ng-ai
2026-08-23 04:25:53 +08:00
parent 69ac479c97
commit a9faf7cf07
2 changed files with 87 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/tty7-core/src/daemon/ssh/handler.rs
+55 -17
View File
@@ -105,23 +105,13 @@ impl ClientHandler {
if !accept {
return false;
}
if remember {
// The superseded line has to go before the new one lands.
// `known_hosts::check` answers `Known` on any same-algorithm match,
// so an override that only appended left the key the user had just
// rejected trusted for good. If it cannot be dropped, do not append
// either: being asked again next time is the better half of that
// trade.
match known_hosts::forget_superseded(&self.host, self.port, key) {
Ok(()) => {
if let Err(e) = known_hosts::append_trusted(&self.host, self.port, key) {
log::warn!("failed to record host key in known_hosts: {e}");
}
}
Err(e) => log::warn!(
"not recording host key: the superseded known_hosts line could not be removed: {e}"
),
}
// `record_trusted` drops the line this key supersedes before adding it,
// and adds nothing if that drop fails — see its comment for why the
// order is not optional.
if remember
&& let Err(e) = known_hosts::record_trusted(&self.host, self.port, key)
{
log::warn!("not recording host key in known_hosts: {e}");
}
true
}
@@ -263,6 +253,54 @@ mod tests {
}
}
/// Saying no to a host-key prompt refuses the connection.
///
/// The most direct failure this code could have: a person is shown a
/// changed host key, declines, and connects anyway. `accepted_and_remembered`
/// is tested on its own, but nothing checked that `apply_decision` acts on
/// what it answers — deleting the `if !accept` guard left the whole suite
/// green.
///
/// No filesystem is touched: every response here answers `remember: false`
/// once refused, so the recording branch is not reached. `remember: true`
/// alongside `accept: false` is the dialog's checkbox state, not consent,
/// and is here for exactly that reason.
#[test]
fn a_refused_host_key_prompt_refuses_the_connection() {
let handler = ClientHandler {
host: "example.com".into(),
port: 2222,
verify_host_keys: true,
skip_banner: false,
broker: crate::daemon::ssh::broker::PromptBroker::new(Box::new(|_| true)),
remote_forwards: crate::daemon::ssh::forward::RemoteForwardTable::default(),
};
let key = russh::keys::PublicKey::from_openssh(
"ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIPXO/kBX63iuiTczoR6uNdl3wAFK7tGWz70jCKkKlw5r",
)
.expect("a key to offer");
for resp in [
AuthResponse::HostKeyDecision {
accept: false,
remember: false,
},
// The checkbox left ticked while answering no.
AuthResponse::HostKeyDecision {
accept: false,
remember: true,
},
AuthResponse::Cancelled,
// A response of the wrong shape is not consent either.
AuthResponse::Secret("yes".into()),
] {
assert!(
!handler.apply_decision(resp.clone(), &key),
"{resp:?} must not accept the key"
);
}
}
#[test]
fn an_unknown_host_is_asked_about_with_no_prior_algorithm() {
let HostKeyAction::Ask(prompt) = action(HostKeyStatus::Unknown, true) else {
crates/tty7-core/src/daemon/ssh/known_hosts.rs
+32 -11
View File
@@ -177,11 +177,36 @@ pub(crate) fn known_algorithms_in_str(contents: &str, host: &str, port: u16) ->
out
}
pub(crate) fn append_trusted(host: &str, port: u16, key: &PublicKey) -> std::io::Result<()> {
/// Trust `key` for `host:port`, dropping the line it supersedes first.
///
/// One function rather than two calls at the call site, because the order is
/// the whole point and a call site can get it wrong silently. `check` answers
/// `Known` on any same-algorithm match, so appending without dropping leaves
/// the key this one replaces trusted for good — which is how a host that
/// rotated away from a compromised key would go on accepting the old one.
///
/// And if the drop fails, nothing is appended: being asked again next time is
/// the better half of that trade.
pub(crate) fn record_trusted(host: &str, port: u16, key: &PublicKey) -> std::io::Result<()> {
let path = default_path().ok_or_else(|| {
std::io::Error::new(std::io::ErrorKind::NotFound, "no home dir for known_hosts")
})?;
append_trusted_to(&path, host, port, key)
record_trusted_in_file(&path, host, port, key)
}
pub(crate) fn record_trusted_in_file(
path: &Path,
host: &str,
port: u16,
key: &PublicKey,
) -> std::io::Result<()> {
forget_superseded_in_file(path, host, port, key).map_err(|e| {
std::io::Error::new(
e.kind(),
format!("the superseded known_hosts line could not be removed: {e}"),
)
})?;
append_trusted_to(path, host, port, key)
}
pub(crate) fn append_trusted_to(
@@ -331,13 +356,6 @@ pub(crate) fn delete_in_str(contents: &str, id: &KnownHostId) -> (String, bool)
///
/// A no-op for a host that was merely unknown, which by definition has no
/// same-algorithm line to drop.
pub(crate) fn forget_superseded(host: &str, port: u16, key: &PublicKey) -> std::io::Result<()> {
match default_path() {
Some(path) => forget_superseded_in_file(&path, host, port, key),
None => Ok(()),
}
}
pub(crate) fn forget_superseded_in_file(
path: &Path,
host: &str,
@@ -1032,9 +1050,12 @@ mod tests {
let path = dir.join("known_hosts");
std::fs::write(&path, format!("example.com {KEY_A}\n")).unwrap();
// Through `record_trusted_in_file`, which is what `apply_decision`
// calls: the two halves in the wrong order, or one of them missing, is
// the bug this test is named after, and calling them separately here
// would let the call site drop one without anything noticing.
let kb = key(KEY_B);
forget_superseded_in_file(&path, "example.com", 22, &kb).unwrap();
append_trusted_to(&path, "example.com", 22, &kb).unwrap();
record_trusted_in_file(&path, "example.com", 22, &kb).unwrap();
let contents = std::fs::read_to_string(&path).unwrap();
assert!(