-
Notifications
You must be signed in to change notification settings - Fork 9
fix(git): verify SSH host keys against known_hosts by default #379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,13 +2,15 @@ package git | |
|
|
||
| import ( | ||
| "fmt" | ||
| "net" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
||
| "github.com/go-git/go-git/v5/plumbing/transport" | ||
| "github.com/go-git/go-git/v5/plumbing/transport/http" | ||
| "github.com/go-git/go-git/v5/plumbing/transport/ssh" | ||
| "github.com/skeema/knownhosts" | ||
| cryptossh "golang.org/x/crypto/ssh" | ||
| ) | ||
|
|
||
|
|
@@ -58,6 +60,12 @@ type AuthConfig struct { | |
| // TokenEnv is the name of the environment variable containing the personal access token | ||
| // Used for HTTPS authentication | ||
| TokenEnv string `yaml:"token_env" json:"token_env"` | ||
|
|
||
| // InsecureSkipHostKeyVerification disables SSH host key verification, | ||
| // 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"` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good naming: the |
||
| } | ||
|
|
||
| // Validate checks that the configuration is valid. | ||
|
|
@@ -215,15 +223,26 @@ func (a *AuthConfig) GetAuth() (transport.AuthMethod, error) { | |
| return nil, fmt.Errorf("failed to parse SSH private key: %w", err) | ||
| } | ||
|
|
||
| if a.InsecureSkipHostKeyVerification { | ||
| return &ssh.PublicKeys{ | ||
| User: "git", | ||
| Signer: signer, | ||
| HostKeyCallbackHelper: ssh.HostKeyCallbackHelper{ | ||
| HostKeyCallback: cryptossh.InsecureIgnoreHostKey(), //nolint:gosec // G106: explicit opt-in via insecure_skip_host_key_verification | ||
| }, | ||
| }, nil | ||
| } | ||
|
|
||
| callback, err := newHostKeyCallback() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &ssh.PublicKeys{ | ||
| User: "git", | ||
| Signer: signer, | ||
| // Accept any host key - appropriate for automated systems | ||
| // where we trust the configured repository URL. | ||
| // This is intentional for CI/CD environments where known_hosts | ||
| // may not be available or maintained. | ||
| HostKeyCallbackHelper: ssh.HostKeyCallbackHelper{ | ||
| HostKeyCallback: cryptossh.InsecureIgnoreHostKey(), //nolint:gosec // G106: Intentional for automated CI/CD systems | ||
| HostKeyCallback: callback, | ||
| }, | ||
| }, nil | ||
|
|
||
|
|
@@ -242,3 +261,34 @@ func (a *AuthConfig) GetAuth() (transport.AuthMethod, error) { | |
| return nil, fmt.Errorf("no authentication configured") | ||
| } | ||
| } | ||
|
|
||
| // 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking architectural thread: the project convention is that new |
||
| callback, err := ssh.NewKnownHostsCallback() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("ssh host key verification requires a known_hosts file: %w\n"+ | ||
| "connect to the git host once with your SSH client (e.g. `ssh git@github.com`) to record its key, "+ | ||
| "or set insecure_skip_host_key_verification: true under git_repository auth to disable verification (not recommended)", err) | ||
| } | ||
|
|
||
| return func(hostname string, remote net.Addr, key cryptossh.PublicKey) error { | ||
| err := callback(hostname, remote, key) | ||
| host := strings.TrimSuffix(hostname, ":22") | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (non-blocking): |
||
| switch { | ||
| case err == nil: | ||
| return nil | ||
| case knownhosts.IsHostUnknown(err): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Kudos: this is the robust way to do it. go-git v5.16.4 builds its |
||
| return fmt.Errorf("ssh host key verification failed: %s is not in known_hosts\n"+ | ||
| "to trust this host, connect to it once with your SSH client (e.g. `ssh git@%s`) and accept its key, "+ | ||
| "or set insecure_skip_host_key_verification: true under git_repository auth to disable verification (not recommended)", host, host) | ||
| case knownhosts.IsHostKeyChanged(err): | ||
| return fmt.Errorf("ssh host key verification failed: the key presented by %s does not match known_hosts\n"+ | ||
| "this could indicate a man-in-the-middle attack; if the host key legitimately changed, "+ | ||
| "remove the old entry (`ssh-keygen -R %s`) and connect once to record the new one: %w", host, host, err) | ||
| default: | ||
| return err | ||
| } | ||
| }, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,16 @@ | ||
| package git | ||
|
|
||
| import ( | ||
| "crypto/ed25519" | ||
| "crypto/rand" | ||
| "net" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/go-git/go-git/v5/plumbing/transport" | ||
| cryptossh "golang.org/x/crypto/ssh" | ||
| ) | ||
|
|
||
| func TestConfigValidate(t *testing.T) { | ||
|
|
@@ -448,6 +453,141 @@ func TestAuthConfigGetAuth(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| // 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) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice catch here. Pinning |
||
| dir, err := os.MkdirTemp("", "git-test-known-hosts-*") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| path := filepath.Join(dir, "known_hosts") | ||
| if err := os.WriteFile(path, nil, 0o600); err != nil { | ||
| panic(err) | ||
| } | ||
| if err := os.Setenv("SSH_KNOWN_HOSTS", path); err != nil { | ||
| panic(err) | ||
| } | ||
| code := m.Run() | ||
| _ = os.RemoveAll(dir) | ||
| os.Exit(code) | ||
| } | ||
|
|
||
| // writeTestKnownHosts writes a known_hosts file with the given lines to a | ||
| // temp directory and returns its path. | ||
| func writeTestKnownHosts(t *testing.T, lines ...string) string { | ||
| t.Helper() | ||
| path := filepath.Join(t.TempDir(), "known_hosts") | ||
| if err := os.WriteFile(path, []byte(strings.Join(lines, "\n")+"\n"), 0o600); err != nil { | ||
| t.Fatalf("failed to write known_hosts: %v", err) | ||
| } | ||
| return path | ||
| } | ||
|
|
||
| // generateTestHostKey generates an Ed25519 SSH public key for use as a fake | ||
| // server host key in tests. | ||
| func generateTestHostKey(t *testing.T) cryptossh.PublicKey { | ||
| t.Helper() | ||
| pub, _, err := ed25519.GenerateKey(rand.Reader) | ||
| if err != nil { | ||
| t.Fatalf("failed to generate key: %v", err) | ||
| } | ||
| sshPub, err := cryptossh.NewPublicKey(pub) | ||
| if err != nil { | ||
| t.Fatalf("failed to convert key: %v", err) | ||
| } | ||
| return sshPub | ||
| } | ||
|
|
||
| func TestGetAuthHostKeyVerification(t *testing.T) { | ||
| remote := &net.TCPAddr{IP: net.ParseIP("140.82.112.3"), Port: 22} | ||
|
|
||
| t.Run("fails without known_hosts by default", func(t *testing.T) { | ||
| t.Setenv("SSH_KNOWN_HOSTS", filepath.Join(t.TempDir(), "nonexistent")) | ||
| t.Setenv("TEST_SSH_KEY", testSSHKeyForConfig) | ||
|
|
||
| auth := AuthConfig{SSHKeyEnv: "TEST_SSH_KEY"} | ||
| _, err := auth.GetAuth() | ||
| if err == nil { | ||
| t.Fatal("GetAuth() expected error when no known_hosts file exists, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "known_hosts") { | ||
| t.Errorf("GetAuth() error = %v, want guidance mentioning known_hosts", err) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("insecure opt-out succeeds without known_hosts", func(t *testing.T) { | ||
| t.Setenv("SSH_KNOWN_HOSTS", filepath.Join(t.TempDir(), "nonexistent")) | ||
| t.Setenv("TEST_SSH_KEY", testSSHKeyForConfig) | ||
|
|
||
| auth := AuthConfig{SSHKeyEnv: "TEST_SSH_KEY", InsecureSkipHostKeyVerification: true} | ||
| got, err := auth.GetAuth() | ||
| if err != nil { | ||
| t.Fatalf("GetAuth() unexpected error: %v", err) | ||
| } | ||
| if got == nil { | ||
| t.Fatal("GetAuth() returned nil auth") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("accepts key matching known_hosts", func(t *testing.T) { | ||
| hostKey := generateTestHostKey(t) | ||
| t.Setenv("SSH_KNOWN_HOSTS", writeTestKnownHosts(t, | ||
| "github.com "+strings.TrimSpace(string(cryptossh.MarshalAuthorizedKey(hostKey))))) | ||
|
|
||
| callback, err := newHostKeyCallback() | ||
| if err != nil { | ||
| t.Fatalf("newHostKeyCallback() unexpected error: %v", err) | ||
| } | ||
| if err := callback("github.com:22", remote, hostKey); err != nil { | ||
| t.Errorf("callback() unexpected error for matching key: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| t.Run("rejects unknown host with guidance", func(t *testing.T) { | ||
| hostKey := generateTestHostKey(t) | ||
| t.Setenv("SSH_KNOWN_HOSTS", writeTestKnownHosts(t, | ||
| "gitlab.com "+strings.TrimSpace(string(cryptossh.MarshalAuthorizedKey(hostKey))))) | ||
|
|
||
| callback, err := newHostKeyCallback() | ||
| if err != nil { | ||
| t.Fatalf("newHostKeyCallback() unexpected error: %v", err) | ||
| } | ||
| err = callback("github.com:22", remote, hostKey) | ||
| if err == nil { | ||
| t.Fatal("callback() expected error for unknown host, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "not in known_hosts") { | ||
| t.Errorf("callback() error = %v, want message containing %q", err, "not in known_hosts") | ||
| } | ||
| if !strings.Contains(err.Error(), "ssh git@github.com") { | ||
| t.Errorf("callback() error = %v, want remediation hint mentioning %q", err, "ssh git@github.com") | ||
| } | ||
| }) | ||
|
|
||
| t.Run("rejects changed host key with MITM warning", func(t *testing.T) { | ||
| recordedKey := generateTestHostKey(t) | ||
| presentedKey := generateTestHostKey(t) | ||
| t.Setenv("SSH_KNOWN_HOSTS", writeTestKnownHosts(t, | ||
| "github.com "+strings.TrimSpace(string(cryptossh.MarshalAuthorizedKey(recordedKey))))) | ||
|
|
||
| callback, err := newHostKeyCallback() | ||
| if err != nil { | ||
| t.Fatalf("newHostKeyCallback() unexpected error: %v", err) | ||
| } | ||
| err = callback("github.com:22", remote, presentedKey) | ||
| if err == nil { | ||
| t.Fatal("callback() expected error for changed host key, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "does not match known_hosts") { | ||
| t.Errorf("callback() error = %v, want message containing %q", err, "does not match known_hosts") | ||
| } | ||
| if !strings.Contains(err.Error(), "man-in-the-middle") { | ||
| t.Errorf("callback() error = %v, want message containing %q", err, "man-in-the-middle") | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| // mockCredentialProvider demonstrates how to mock CredentialProvider for testing. | ||
| type mockCredentialProvider struct { | ||
| auth transport.AuthMethod | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.