Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions examples/aws-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ git_repository:
ssh_key_env: GIT_SSH_PRIVATE_KEY # Environment variable containing SSH private key
# OR use token for HTTPS:
# 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 —

Copy link
Copy Markdown
Contributor

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.

# this removes man-in-the-middle protection:
# insecure_skip_host_key_verification: true
# Optional: separate read-only credentials for ArgoCD (falls back to auth if not set)
# argocd_auth:
# ssh_key_env: ARGOCD_SSH_KEY
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ require (
github.com/hashicorp/terraform-exec v0.24.0
github.com/joho/godotenv v1.5.1
github.com/opentofu/tofudl v0.0.1
github.com/skeema/knownhosts v1.3.1
github.com/spf13/afero v1.15.0
github.com/spf13/cobra v1.10.1
go.opentelemetry.io/otel v1.38.0
Expand Down Expand Up @@ -150,7 +151,6 @@ require (
github.com/sergi/go-diff v1.3.2-0.20230802210424-5b0b94c5c0d3 // indirect
github.com/shopspring/decimal v1.4.0 // indirect
github.com/sirupsen/logrus v1.9.3 // indirect
github.com/skeema/knownhosts v1.3.1 // indirect
github.com/spf13/cast v1.7.0 // indirect
github.com/spf13/pflag v1.0.10 // indirect
github.com/tidwall/gjson v1.14.4 // indirect
Expand Down
60 changes: 55 additions & 5 deletions pkg/git/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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"`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Validate checks that the configuration is valid.
Expand Down Expand Up @@ -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

Expand All @@ -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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

switch {
case err == nil:
return nil
case knownhosts.IsHostUnknown(err):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 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.

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
}
140 changes: 140 additions & 0 deletions pkg/git/config_test.go
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) {
Expand Down Expand Up @@ -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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Expand Down
Loading