feat: verify SSH host keys against known_hosts - #334
Conversation
|
I did not check the security part, only the implementation, which looks good. Note that we do have some issues around proxies and also PRs. @daiyam: What's your take on this? As that is a security issues, I feel that after this is pulled in, a new version should be released with accompanying news.. |
|
Fair point. It's load-bearing here — the verifier looks the key up under That's also the first half of #109: Splitting it out with a proxy e2e and rebasing this on top. |
The port of a jump host was computed in two places with different fallbacks: connecting to the hop fell back to the target's port, while the forwardOut reaching the next hop fell back to 22. A jump host does not inherit the target's Port, so a target declared on a non-default port had its first hop dialed on that port. Use one helper for both, falling back to 22 like ssh does, and cover the ProxyJump path with unit tests for the resolution order plus a two container e2e where the target is only reachable through the hop.
Connections previously accepted whatever key the server presented: no hostVerifier was passed to ssh2, so a man-in-the-middle was undetectable (jeanp413#266). Every connection — the target and each ProxyJump hop — is now verified against known_hosts. A new knownHosts module replaces the never-wired hostfile.ts. It reads UserKnownHostsFile/GlobalKnownHostsFile (plaintext and |1| hashed entries, [host]:port, wildcards, negations, HostKeyAlias), compares keys per type like OpenSSH, lowercases hostnames the way ssh does, and appends accepted keys to the user file. Behavior is governed by two settings, as discussed in jeanp413#266: - remote.SSH.verifyUnknownHosts (accept|ask|reject, default accept): accept mirrors OpenSSH's accept-new, so existing users see no change while gaining protection from the second connection on. - remote.SSH.verifyKnownHosts (accept|ask|reject, default ask): a changed key shows both fingerprints, with cancel as the default. Safety details: a changed key is only rewritten in place when its line covers just that host — rewriting a shared or wildcard line would re-pin every other host it names — otherwise the accepted key is appended to the user file, which also covers read-only global entries; a known_hosts read failure downgrades auto-accept to an explicit prompt instead of failing open; UserKnownHostsFile 'none' connects without persisting.
A key marked @Revoked is refused before any other consideration and no setting overrides it, matching OpenSSH. A revoked entry for a different key doesn't affect the presented one, and a lone revoked entry doesn't count as a recorded key. @cert-authority remains unsupported since ssh2 has no certificate support.
The changed-key e2e assertion assumed an 11-character key type when slicing the blob; compare against the forged base64 itself instead, and also assert the update happened in place. Comments added where the matching semantics aren't obvious: hashed-entry format, OpenSSH negation rules, the stale-line guard doubling as @Revoked protection, and why an approval isn't persisted while the trust store is unreadable.
Approving a changed key only rewrote the offending line when it named a single host in a user-owned file; otherwise the new key was appended and the old entry left in place. Since any matching same-type entry verifies, both keys stayed trusted — an attacker holding the old key could connect silently, which is exactly what the warning is about. Now every conflicting entry is cleared: a single-host line is rewritten, a line naming several hosts loses just this host (what ssh-keygen -R does). If anything survives — a wildcard line, or a file the user doesn't own — the connection is refused and the surviving file:line is reported, following ssh, which refuses on a changed key rather than half-trusting. Also from review: - lowercase HostKeyAlias, so a mixed-case alias matches hashed entries - one port per ProxyJump hop, shared by the tunnel and the host key lookup; a jump host no longer inherits the target's Port - report the 'none' UserKnownHostsFile case instead of logging a write "to undefined", and don't promise to remember a key that can't be Tests: the reject case now asserts no prompt was shown (it passed with the policy removed before), the wildcard case asserts the old key is refused rather than that the new one works, and readFailures, removeHostFromEntry and findConflictingEntries are covered.
aadeb86 to
47558e3
Compare
|
Split done: the proxy-port change is now #337, with the ProxyJump tests that were missing — unit tests for the port resolution order plus a two-container e2e where the target is only reachable through the hop. This branch is rebased on top of it and uses the shared helper, so its diff will shrink to the host key work alone once #337 lands. |
2 participants
Closes #266. Implements the plan discussed there, with the settings shape proposed in the threads.
What this does
Every connection — the target and each
ProxyJumphop — now verifies the server's host key againstknown_hoststhrough ssh2'shostVerifier(raw key blob, so nohostHash). Previously no verifier was passed at all, so any key was accepted silently.The never-wired
hostfile.tsis replaced by aknownHostsmodule that follows OpenSSH behavior:|1|hashed entries,[host]:port, comma lists,*/?wildcards,!negationsUserKnownHostsFile/GlobalKnownHostsFilerespected (including thenonekeyword → connect without persisting),HostKeyAliasused for lookup and recording when setSettings
remote.SSH.verifyUnknownHostsaccept/ask/rejectacceptremote.SSH.verifyKnownHostsaccept/ask/rejectaskaccepton unknown hosts mirrors OpenSSH'saccept-new: existing users see no change on their first connect, and get mismatch protection from the second on. A changed key shows both fingerprints with cancel as the default action.Safety details worth reviewing
ssh-keygen -Rdoes). Shared lines are never re-keyed wholesale — that would re-pin every other host they name. If anything survives (a wildcard line, or a file the user doesn't own) the connection is refused and the survivingfile:lineis reported, following ssh, which refuses on a changed key rather than half-trusting it. Appending alongside a stale entry would leave both keys valid, which is a fail-open.unknowncan't be trusted, so auto-accept is downgraded to an explicit prompt that names the unreadable file.Tests
[host]:port, CRLF, negations, aliases, quoted paths with spaces,none, unreadable files, shared-line protection, comment preservation,@revokedprecedence, conflict detectionrejectrefuses without prompting → cancelled prompt refuses → accepted prompt updates in place and connects → a wildcard line that can't be cleared refuses and names the linessh-keygen -F: 100 lookups over a real 86-entry known_hosts in both plaintext andssh-keygen -Hhashed form, plus synthetic multi-name/wildcard/negation/port/case/comment lines — no divergence from OpenSSH's own matcher. Also exercised end to end against a real host: silent match, tampered key rejected,@revokedrefused even withaccept, and an auto-recorded entry that is byte-identical to the one ssh writes and thatssh-keygen -Ffinds.Known limitation
The
askmodals run inside ssh2's handshake, which is bounded byreadyTimeout(default 60s). Answering after the timeout still records the decision, and the retry then connects against the recorded key — the approval isn't lost, but that first attempt reports a failed connection. Noted in a comment; solving it properly means pausing/retrying around the handshake and felt like its own PR.@revokedentries are honored: a revoked key is refused unconditionally, no setting overrides it.@cert-authorityis skipped for now — verifying CA-signed host certificates needs certificate support in ssh2, which doesn't exist yet (#261, #95).