fix(git): verify SSH host keys against known_hosts by default#379
fix(git): verify SSH host keys against known_hosts by default#379Adam-D-Lewis wants to merge 1 commit into
Conversation
Git operations over SSH previously used InsecureIgnoreHostKey(), accepting any host key and leaving the GitOps repository — the source of truth for what ArgoCD deploys — open to man-in-the-middle impersonation during clone and push. Host keys are now verified against the standard known_hosts locations (SSH_KNOWN_HOSTS, ~/.ssh/known_hosts, /etc/ssh/ssh_known_hosts), so most users need no new configuration. Verification failures produce platform-neutral, actionable errors that distinguish an unknown host (with a hint to record its key) from a changed key (a possible MITM). For ephemeral environments without a known_hosts file, the previous behavior remains available as an explicit opt-in via insecure_skip_host_key_verification: true under git auth config. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adam-D-Lewis
added
needs: review 👀
Adam-D-Lewis
requested review from
aktech,
dcmcand,
dharhas,
marcelovilla,
tylerpotts and
viniciusdc
as code owners
Adam-D-Lewis
added
needs: review 👀
dcmcand
left a comment
dcmcand
left a comment
There was a problem hiding this comment.
Approving. This is a clean, correct, well-tested security fix that fully resolves #378: host key verification is on by default now, with an explicit, well-named opt-out for ephemeral CI.
I verified the fix against the library internals, not just the tests: go-git builds its known_hosts callback on skeema/knownhosts under the hood, so the IsHostUnknown/IsHostKeyChanged classification here is reliable rather than string-matching. Tests pass, go build and go vet are clean, and go mod tidy leaves the module graph unchanged.
I left a few inline comments. None of them block this merge - Adam, they're yours to triage, and you can fix now, open follow-up issues, or ignore them at your discretion:
- Two optional nits (em dash in the example config; the remediation hint degrades for non-standard SSH ports).
- One non-blocking architectural thread about OTel instrumentation in
config.gothat's really a docs/convention decision for the maintainers.
One thing that isn't line-specific: this flips a default. SSH git ops now require a known_hosts entry, so anyone running NIC over SSH in a runner without one will see deploys fail until they add the entry or set insecure_skip_host_key_verification: true. That's the right posture, just worth a line in the release notes so upgraders aren't surprised.
Genuine kudos inline too - the library choice, the hermetic TestMain, and the opt-out naming were all done well.
| switch { | ||
| case err == nil: | ||
| return nil | ||
| case knownhosts.IsHostUnknown(err): |
There was a problem hiding this comment.
Kudos: this is the robust way to do it. go-git v5.16.4 builds its NewKnownHostsCallback on skeema/knownhosts internally, so reusing that same package's IsHostUnknown/IsHostKeyChanged here detects the *knownhosts.KeyError via errors.As instead of matching on message text. The three distinct, actionable errors (missing file, unknown host, and changed key with the MITM warning plus ssh-keygen -R recovery) are exactly what an operator needs when this fires.
| // TestMain points host key verification at a hermetic empty known_hosts file | ||
| // so package tests don't depend on the machine they run on. Individual tests | ||
| // override SSH_KNOWN_HOSTS via t.Setenv as needed. | ||
| func TestMain(m *testing.M) { |
There was a problem hiding this comment.
Nice catch here. Pinning SSH_KNOWN_HOSTS to a hermetic empty file doesn't just isolate the new tests, it removes a latent dependency the existing tests had on the runner's ~/.ssh/known_hosts. This hardens CI on a bare runner, not just the code under test.
| // removing protection against man-in-the-middle attacks. Only intended | ||
| // for ephemeral environments (e.g. CI) where maintaining a known_hosts | ||
| // file is impractical. Has no effect on token (HTTPS) authentication. | ||
| InsecureSkipHostKeyVerification bool `yaml:"insecure_skip_host_key_verification,omitempty" json:"insecure_skip_host_key_verification,omitempty"` |
There was a problem hiding this comment.
Good naming: the Insecure prefix mirrors golang.org/x/crypto/ssh.InsecureIgnoreHostKey, so the risk is legible at every call site, and omitempty keeps it out of normal configs.
| # token_env: GIT_TOKEN | ||
| # SSH host keys are verified against known_hosts (SSH_KNOWN_HOSTS, | ||
| # ~/.ssh/known_hosts, /etc/ssh/ssh_known_hosts). For ephemeral environments | ||
| # (e.g. CI) without a known_hosts file, verification can be disabled — |
There was a problem hiding this comment.
Nit: this line uses an em dash. This project leans toward regular dashes or colons, so a one-character swap would match the house style. Non-blocking.
|
|
||
| return func(hostname string, remote net.Addr, key cryptossh.PublicKey) error { | ||
| err := callback(hostname, remote, key) | ||
| host := strings.TrimSuffix(hostname, ":22") |
There was a problem hiding this comment.
Nit (non-blocking): TrimSuffix(hostname, ":22") only strips the default port. For a self-hosted forge on a non-standard port (e.g. :2222), host stays git.example.com:2222, so the hints render as ssh git@git.example.com:2222 (ssh wants -p 2222) and ssh-keygen -R git.example.com:2222 (which won't match the [host]:port known_hosts format). Verification itself is unaffected; this is cosmetic in the guidance text only. net.SplitHostPort would make the hint correct for any port.
| // newHostKeyCallback returns a host key callback backed by the standard | ||
| // known_hosts files (SSH_KNOWN_HOSTS, ~/.ssh/known_hosts, /etc/ssh/ssh_known_hosts), | ||
| // wrapping verification failures with actionable guidance. | ||
| func newHostKeyCallback() (cryptossh.HostKeyCallback, error) { |
There was a problem hiding this comment.
Non-blocking architectural thread: the project convention is that new pkg/ functions are OpenTelemetry span-instrumented, but the established local pattern in this file is the opposite - every credential/config helper here (GetAuth, GetSSHKey, Validate, ...) is ctx-free and span-free, and only the operation methods in client_impl.go carry spans. newHostKeyCallback follows that local pattern faithfully. The clean resolution is probably to document a config.go exemption rather than thread ctx through the CredentialProvider interface, but that's a maintainer call. Flagging it so the gray area gets a decision, not asking you to change this PR.
2 participants
Fixes #378
Problem
Git operations over SSH used
InsecureIgnoreHostKey(), accepting any host key during clone/push of the GitOps repository — the source of truth for what ArgoCD deploys. A network MITM could impersonate the remote, capture pushed manifests, and serve tampered repo content.Change
SSH_KNOWN_HOSTSenv,~/.ssh/known_hosts,/etc/ssh/ssh_known_hosts) using go-git's built-in knownhosts support. Anyone who has ever SSH'd to their git forge already has the entry, so no new configuration is needed for typical use — this includes Windows, where the bundled OpenSSH client uses the same file layout.ssh git@<host>, platform-neutral); a changed key gets an explicit man-in-the-middle warning withssh-keygen -Rrecovery steps.insecure_skip_host_key_verification: trueundergit_repository.authrestores the old behavior where maintaining a known_hosts file is impractical. The insecure mode is now a visible per-config choice instead of a silent universal default.Tests
TestMainpinsSSH_KNOWN_HOSTSto a hermetic file — existing tests silently depended on the machine's~/.ssh/known_hostsand fail on a bare runner without this.go vet ./...and the full test suite pass.Notes
skeema/knownhostsmoves from indirect to direct dependency (already in the module graph via go-git).examples/aws-config.yamldocuments the new flag.argocd-ssh-known-hosts-cm(which already protects the in-cluster sync path).