diff --git a/go.mod b/go.mod index f8e6df72a2..a8cac89aad 100644 --- a/go.mod +++ b/go.mod @@ -31,7 +31,6 @@ require ( github.com/denisbrodbeck/machineid v1.0.1 github.com/dustin/go-humanize v1.0.1 github.com/dustinkirkland/golang-petname v0.0.0-20231002161417-6a283f1aaaf2 - github.com/gliderlabs/ssh v0.3.8 github.com/go-chi/chi/v5 v5.2.3 github.com/gofrs/flock v0.13.0 github.com/google/go-cmp v0.7.0 @@ -56,7 +55,6 @@ require ( go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.39.0 go.opentelemetry.io/otel/sdk v1.39.0 go.opentelemetry.io/otel/trace v1.39.0 - golang.org/x/crypto v0.46.0 golang.org/x/net v0.48.0 golang.org/x/oauth2 v0.34.0 golang.org/x/sync v0.19.0 @@ -94,7 +92,6 @@ require ( github.com/agnivade/levenshtein v1.2.1 // indirect github.com/alexflint/go-arg v1.5.1 // indirect github.com/alexflint/go-scalar v1.2.0 // indirect - github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be // indirect github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.7.4 // indirect github.com/aws/aws-sdk-go-v2/credentials v1.19.6 // indirect github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.16 // indirect @@ -197,6 +194,7 @@ require ( go.uber.org/multierr v1.11.0 // indirect go.uber.org/zap v1.27.0 // indirect go.yaml.in/yaml/v2 v2.4.2 // indirect + golang.org/x/crypto v0.46.0 // indirect golang.org/x/exp v0.0.0-20250606033433-dcc06ee1d476 // indirect golang.org/x/mod v0.30.0 // indirect golang.org/x/text v0.32.0 // indirect diff --git a/go.sum b/go.sum index 452e1bec59..cc01b1a57f 100644 --- a/go.sum +++ b/go.sum @@ -72,8 +72,6 @@ github.com/alexflint/go-scalar v1.2.0 h1:WR7JPKkeNpnYIOfHRa7ivM21aWAdHD0gEWHCx+W github.com/alexflint/go-scalar v1.2.0/go.mod h1:LoFvNMqS1CPrMVltza4LvnGKhaSpc3oyLEBUZVhhS2o= github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883 h1:bvNMNQO63//z+xNgfBlViaCIJKLlCJ6/fmUseuG0wVQ= github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo29Kk6CurOXKm700vrz8f0KW0JNfpkRJY/8= -github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be h1:9AeTilPcZAjCFIImctFaOjnTIavg87rW78vTPkQqLI8= -github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be/go.mod h1:ySMOLuWl6zY27l47sB3qLNK6tF2fkHG55UZxx8oIVo4= github.com/arbovm/levenshtein v0.0.0-20160628152529-48b4e1c0c4d0 h1:jfIu9sQUG6Ig+0+Ap1h4unLjW6YQJpKZVmUzxsD4E/Q= github.com/arbovm/levenshtein v0.0.0-20160628152529-48b4e1c0c4d0/go.mod h1:t2tdKJDJF9BV14lnkjHmOQgcvEKgtqs5a1N3LNdJhGE= github.com/aws/aws-sdk-go-v2 v1.41.0 h1:tNvqh1s+v0vFYdA1xq0aOJH+Y5cRyZ5upu6roPgPKd4= @@ -189,8 +187,6 @@ github.com/fortytw2/leaktest v1.3.0 h1:u8491cBMTQ8ft8aeV+adlcytMZylmA5nnwwkRZjI8 github.com/fortytw2/leaktest v1.3.0/go.mod h1:jDsjWgpAGjm2CA7WthBh/CdZYEPF31XHquHwclZch5g= github.com/fsnotify/fsnotify v1.9.0 h1:2Ml+OJNzbYCTzsxtv8vKSFD9PbJjmhYF14k/jKC7S9k= github.com/fsnotify/fsnotify v1.9.0/go.mod h1:8jBTzvmWwFyi3Pb8djgCCO5IBqzKJ/Jwo8TRcHyHii0= -github.com/gliderlabs/ssh v0.3.8 h1:a4YXD1V7xMF9g5nTkdfnja3Sxy1PVDCj1Zg4Wb8vY6c= -github.com/gliderlabs/ssh v0.3.8/go.mod h1:xYoytBv1sV0aL3CavoDuJIQNURXkkfPA/wxQ1pL1fAU= github.com/go-chi/chi/v5 v5.2.3 h1:WQIt9uxdsAbgIYgid+BpYc+liqQZGMHRaUwp0JUcvdE= github.com/go-chi/chi/v5 v5.2.3/go.mod h1:L2yAIGWB3H+phAw1NxKwWM+7eUH/lU8pOMm5hHcoops= github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A= diff --git a/internal/job/checkout.go b/internal/job/checkout.go index 1836c0b948..2b401a7189 100644 --- a/internal/job/checkout.go +++ b/internal/job/checkout.go @@ -594,10 +594,6 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error { var err error defer func() { span.FinishWithError(err) }() - if e.SSHKeyscan { - addRepositoryHostToSSHKnownHosts(ctx, e.shell, e.Repository) - } - var mirrorDir string // If we can, get a mirror of the git repository to use for reference later @@ -785,10 +781,6 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error { mirrorSubmodules := e.GitMirrorsPath != "" for _, repository := range submoduleRepos { submoduleArgs := slices.Clone(args) - // submodules might need their fingerprints verified too - if e.SSHKeyscan { - addRepositoryHostToSSHKnownHosts(ctx, e.shell, repository) - } if !mirrorSubmodules { continue diff --git a/internal/job/executor.go b/internal/job/executor.go index 4010db97bb..acd471ee52 100644 --- a/internal/job/executor.go +++ b/internal/job/executor.go @@ -799,24 +799,6 @@ func dirForRepository(repository string) string { return badCharsPattern.ReplaceAllString(repository, "-") } -// Given a repository, it will add the host to the set of SSH known_hosts on the machine -func addRepositoryHostToSSHKnownHosts(ctx context.Context, sh *shell.Shell, repository string) { - if osutil.FileExists(repository) { - return - } - - knownHosts, err := findKnownHosts(sh) - if err != nil { - sh.Warningf("Failed to find SSH known_hosts file: %v", err) - return - } - - if err = knownHosts.AddFromRepository(ctx, repository); err != nil { - sh.Warningf("Error adding to known_hosts: %v", err) - return - } -} - // setUp is run before all the phases run. It's responsible for initializing the // job environment func (e *Executor) setUp(ctx context.Context) error { @@ -870,6 +852,13 @@ func (e *Executor) setUp(ctx context.Context) error { // Disable any interactive Git/SSH prompting e.shell.Env.Set("GIT_TERMINAL_PROMPT", "0") + // Configure SSH to automatically accept new host keys (TOFU - Trust On First Use) + // This replaces the previous ssh-keyscan approach with a simpler mechanism that + // achieves the same security model: accept new hosts, reject changed keys. + if e.SSHKeyscan { + e.configureSSHKeyscan() + } + // Fetch and set secrets before environment hook execution if e.Secrets != "" { if err := e.fetchAndSetSecrets(ctx); err != nil { @@ -884,6 +873,37 @@ func (e *Executor) setUp(ctx context.Context) error { return err } +// configureSSHKeyscan sets up GIT_SSH_COMMAND with host key checking options. +// If GIT_SSH is set (a binary path), we skip configuration since we can't add flags. +// For SSH >= 7.6, uses StrictHostKeyChecking=accept-new (TOFU: accept new, reject changed). +// For older SSH, falls back to StrictHostKeyChecking=no with ephemeral known_hosts. +func (e *Executor) configureSSHKeyscan() { + // If GIT_SSH is set, it's a path to a binary - we can't add flags to it + if _, hasGitSSH := e.shell.Env.Get("GIT_SSH"); hasGitSSH { + e.shell.Commentf("GIT_SSH is set, skipping SSH host key configuration") + return + } + + // Determine the SSH options based on version + var sshOptions string + if sshSupportsAcceptNew() { + // OpenSSH 7.6+ supports accept-new: accept new host keys, reject changed ones + sshOptions = "-o StrictHostKeyChecking=accept-new" + } else { + // Older SSH: disable host key checking entirely + // Use /dev/null for known_hosts to avoid polluting the user's file + sshOptions = "-o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null" + e.shell.Commentf("SSH version < 7.6 detected, using StrictHostKeyChecking=no") + } + + // Append to existing GIT_SSH_COMMAND or create new one + existingSSHCommand, _ := e.shell.Env.Get("GIT_SSH_COMMAND") + if existingSSHCommand == "" { + existingSSHCommand = "ssh" + } + e.shell.Env.Set("GIT_SSH_COMMAND", existingSSHCommand+" "+sshOptions) +} + // fetchAndSetSecrets handles secrets fetching and processing directly func (e *Executor) fetchAndSetSecrets(ctx context.Context) error { if e.Secrets == "" { diff --git a/internal/job/integration/checkout_git_mirrors_integration_test.go b/internal/job/integration/checkout_git_mirrors_integration_test.go index 2a00ad2d01..f5b72a4244 100644 --- a/internal/job/integration/checkout_git_mirrors_integration_test.go +++ b/internal/job/integration/checkout_git_mirrors_integration_test.go @@ -358,16 +358,21 @@ func TestCheckingOutWithSSHKeyscan_WithGitMirrors(t *testing.T) { t.Fatalf("EnableGitMirrors() error = %v", err) } - tester.MustMock(t, "ssh-keyscan"). - Expect("github.com"). - AndWriteToStdout("github.com ssh-rsa xxx="). - AndExitWith(0) - + var gitSSHCommand string git := tester.MustMock(t, "git") git.IgnoreUnexpectedInvocations() git.Expect("clone", "--mirror", "-v", "--", "git@github.com:buildkite/agent.git", bintest.MatchAny()). - AndExitWith(0) + AndCallFunc(func(c *bintest.Call) { + // Capture GIT_SSH_COMMAND for verification + for _, env := range c.Env { + if strings.HasPrefix(env, "GIT_SSH_COMMAND=") { + gitSSHCommand = env + break + } + } + c.Exit(0) + }) env := []string{ "BUILDKITE_REPO=git@github.com:buildkite/agent.git", @@ -375,6 +380,11 @@ func TestCheckingOutWithSSHKeyscan_WithGitMirrors(t *testing.T) { } tester.RunAndCheck(t, env...) + + // Verify GIT_SSH_COMMAND was set with accept-new + if !strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") { + t.Errorf("Expected GIT_SSH_COMMAND to contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand) + } } func TestCheckingOutWithoutSSHKeyscan_WithGitMirrors(t *testing.T) { @@ -390,9 +400,21 @@ func TestCheckingOutWithoutSSHKeyscan_WithGitMirrors(t *testing.T) { t.Fatalf("EnableGitMirrors() error = %v", err) } - tester.MustMock(t, "ssh-keyscan"). - Expect("github.com"). - NotCalled() + var gitSSHCommand string + git := tester.MustMock(t, "git") + git.IgnoreUnexpectedInvocations() + + git.Expect("clone", "--mirror", "-v", "--", "https://github.com/buildkite/bash-example.git", bintest.MatchAny()). + AndCallFunc(func(c *bintest.Call) { + // Capture GIT_SSH_COMMAND for verification + for _, env := range c.Env { + if strings.HasPrefix(env, "GIT_SSH_COMMAND=") { + gitSSHCommand = env + break + } + } + c.Exit(0) + }) env := []string{ "BUILDKITE_REPO=https://github.com/buildkite/bash-example.git", @@ -400,9 +422,14 @@ func TestCheckingOutWithoutSSHKeyscan_WithGitMirrors(t *testing.T) { } tester.RunAndCheck(t, env...) + + // Verify GIT_SSH_COMMAND does NOT contain accept-new when SSHKeyscan is disabled + if strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") { + t.Errorf("Expected GIT_SSH_COMMAND to NOT contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand) + } } -func TestCheckingOutWithSSHKeyscanAndUnscannableRepo_WithGitMirrors(t *testing.T) { +func TestCheckingOutWithSSHKeyscanAndHTTPSRepo_WithGitMirrors(t *testing.T) { t.Parallel() tester, err := NewExecutorTester(mainCtx) @@ -415,15 +442,22 @@ func TestCheckingOutWithSSHKeyscanAndUnscannableRepo_WithGitMirrors(t *testing.T t.Fatalf("EnableGitMirrors() error = %v", err) } - tester.MustMock(t, "ssh-keyscan"). - Expect("github.com"). - NotCalled() - + var gitSSHCommand string git := tester.MustMock(t, "git") git.IgnoreUnexpectedInvocations() + // Even with HTTPS repos, GIT_SSH_COMMAND is set (it just won't be used) git.Expect("clone", "--mirror", "-v", "--", "https://github.com/buildkite/bash-example.git", bintest.MatchAny()). - AndExitWith(0) + AndCallFunc(func(c *bintest.Call) { + // Capture GIT_SSH_COMMAND for verification + for _, env := range c.Env { + if strings.HasPrefix(env, "GIT_SSH_COMMAND=") { + gitSSHCommand = env + break + } + } + c.Exit(0) + }) env := []string{ "BUILDKITE_REPO=https://github.com/buildkite/bash-example.git", @@ -431,6 +465,11 @@ func TestCheckingOutWithSSHKeyscanAndUnscannableRepo_WithGitMirrors(t *testing.T } tester.RunAndCheck(t, env...) + + // Verify GIT_SSH_COMMAND was set with accept-new even for HTTPS repos + if !strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") { + t.Errorf("Expected GIT_SSH_COMMAND to contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand) + } } func TestCleaningAnExistingCheckout_WithGitMirrors(t *testing.T) { diff --git a/internal/job/integration/checkout_integration_test.go b/internal/job/integration/checkout_integration_test.go index e1fd8f9791..ea6bde02ee 100644 --- a/internal/job/integration/checkout_integration_test.go +++ b/internal/job/integration/checkout_integration_test.go @@ -722,16 +722,21 @@ func TestCheckingOutWithSSHKeyscan(t *testing.T) { } defer tester.Close() - tester.MustMock(t, "ssh-keyscan"). - Expect("github.com"). - AndWriteToStdout("github.com ssh-rsa xxx="). - AndExitWith(0) - + var gitSSHCommand string git := tester.MustMock(t, "git") git.IgnoreUnexpectedInvocations() git.Expect("clone", "-v", "--", "git@github.com:buildkite/agent.git", "."). - AndExitWith(0) + AndCallFunc(func(c *bintest.Call) { + // Capture GIT_SSH_COMMAND for verification + for _, env := range c.Env { + if strings.HasPrefix(env, "GIT_SSH_COMMAND=") { + gitSSHCommand = env + break + } + } + c.Exit(0) + }) env := []string{ "BUILDKITE_REPO=git@github.com:buildkite/agent.git", @@ -739,6 +744,11 @@ func TestCheckingOutWithSSHKeyscan(t *testing.T) { } tester.RunAndCheck(t, env...) + + // Verify GIT_SSH_COMMAND was set with accept-new + if !strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") { + t.Errorf("Expected GIT_SSH_COMMAND to contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand) + } } func TestCheckingOutWithoutSSHKeyscan(t *testing.T) { @@ -750,9 +760,21 @@ func TestCheckingOutWithoutSSHKeyscan(t *testing.T) { } defer tester.Close() - tester.MustMock(t, "ssh-keyscan"). - Expect("github.com"). - NotCalled() + var gitSSHCommand string + git := tester.MustMock(t, "git") + git.IgnoreUnexpectedInvocations() + + git.Expect("clone", "-v", "--", "https://github.com/buildkite/bash-example.git", "."). + AndCallFunc(func(c *bintest.Call) { + // Capture GIT_SSH_COMMAND for verification + for _, env := range c.Env { + if strings.HasPrefix(env, "GIT_SSH_COMMAND=") { + gitSSHCommand = env + break + } + } + c.Exit(0) + }) env := []string{ "BUILDKITE_REPO=https://github.com/buildkite/bash-example.git", @@ -760,9 +782,14 @@ func TestCheckingOutWithoutSSHKeyscan(t *testing.T) { } tester.RunAndCheck(t, env...) + + // Verify GIT_SSH_COMMAND does NOT contain accept-new when SSHKeyscan is disabled + if strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") { + t.Errorf("Expected GIT_SSH_COMMAND to NOT contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand) + } } -func TestCheckingOutWithSSHKeyscanAndUnscannableRepo(t *testing.T) { +func TestCheckingOutWithSSHKeyscanAndHTTPSRepo(t *testing.T) { t.Parallel() tester, err := NewExecutorTester(mainCtx) @@ -771,15 +798,22 @@ func TestCheckingOutWithSSHKeyscanAndUnscannableRepo(t *testing.T) { } defer tester.Close() - tester.MustMock(t, "ssh-keyscan"). - Expect("github.com"). - NotCalled() - + var gitSSHCommand string git := tester.MustMock(t, "git") git.IgnoreUnexpectedInvocations() + // Even with HTTPS repos, GIT_SSH_COMMAND is set (it just won't be used) git.Expect("clone", "-v", "--", "https://github.com/buildkite/bash-example.git", "."). - AndExitWith(0) + AndCallFunc(func(c *bintest.Call) { + // Capture GIT_SSH_COMMAND for verification + for _, env := range c.Env { + if strings.HasPrefix(env, "GIT_SSH_COMMAND=") { + gitSSHCommand = env + break + } + } + c.Exit(0) + }) env := []string{ "BUILDKITE_REPO=https://github.com/buildkite/bash-example.git", @@ -787,6 +821,11 @@ func TestCheckingOutWithSSHKeyscanAndUnscannableRepo(t *testing.T) { } tester.RunAndCheck(t, env...) + + // Verify GIT_SSH_COMMAND was set with accept-new even for HTTPS repos + if !strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") { + t.Errorf("Expected GIT_SSH_COMMAND to contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand) + } } func TestCleaningAnExistingCheckout(t *testing.T) { diff --git a/internal/job/integration/job_api_integration_test.go b/internal/job/integration/job_api_integration_test.go index 07149e300a..7b0b6758f9 100644 --- a/internal/job/integration/job_api_integration_test.go +++ b/internal/job/integration/job_api_integration_test.go @@ -142,5 +142,7 @@ func TestBootstrapRunsJobAPI(t *testing.T) { } }) - tester.RunAndCheck(t) + // Disable SSH keyscan since this test isn't about SSH and it modifies + // GIT_SSH_COMMAND which would cause env mismatch between Job API and process + tester.RunAndCheck(t, "BUILDKITE_SSH_KEYSCAN=false") } diff --git a/internal/job/knownhosts.go b/internal/job/knownhosts.go deleted file mode 100644 index 15e2adea6c..0000000000 --- a/internal/job/knownhosts.go +++ /dev/null @@ -1,157 +0,0 @@ -package job - -import ( - "bufio" - "context" - "fmt" - "os" - "path/filepath" - "strings" - "time" - - "github.com/buildkite/agent/v3/internal/osutil" - "github.com/buildkite/agent/v3/internal/shell" - "golang.org/x/crypto/ssh/knownhosts" -) - -type knownHosts struct { - Shell *shell.Shell - Path string -} - -func findKnownHosts(sh *shell.Shell) (*knownHosts, error) { - userHomePath, err := osutil.UserHomeDir() - if err != nil { - return nil, fmt.Errorf("Could not find the current users home directory (%s)", err) - } - - // Construct paths to the known_hosts file - sshDirectory := filepath.Join(userHomePath, ".ssh") - knownHostPath := filepath.Join(sshDirectory, "known_hosts") - - // Ensure ssh directory exists - if err := os.MkdirAll(sshDirectory, 0o700); err != nil { - return nil, err - } - - // Ensure file exists - if _, err := os.Stat(knownHostPath); err != nil { - f, err := os.OpenFile(knownHostPath, os.O_CREATE|os.O_WRONLY, 0o600) - if err != nil { - return nil, fmt.Errorf("create %q: %w", knownHostPath, err) - } - if err = f.Close(); err != nil { - return nil, err - } - } - - return &knownHosts{Shell: sh, Path: knownHostPath}, nil -} - -func (kh *knownHosts) Contains(host string) (bool, error) { - file, err := os.Open(kh.Path) - if err != nil { - return false, err - } - defer file.Close() - - normalized := knownhosts.Normalize(host) - - // There don't appear to be any libraries to parse known_hosts that don't also want to - // validate the IP's and host keys. Shelling out to ssh-keygen doesn't support custom ports - // so I guess we'll do it ourselves. - // - // known_host format is defined at https://man.openbsd.org/sshd#SSH_KNOWN_HOSTS_FILE_FORMAT - // A basic example is: - // # Comments allowed at start of line - // closenet,...,192.0.2.53 1024 37 159...93 closenet.example.net - // cvs.example.net,192.0.2.10 ssh-rsa AAAA1234.....= - // # A hashed hostname - // |1|JfKTdBh7rNbXkVAQCRp4OQoPfmI=|USECr3SWf1JUPsms5AqfD5QfxkM= ssh-rsa - // AAAA1234.....= - // # A revoked key - // @revoked * ssh-rsa AAAAB5W... - // # A CA key, accepted for any host in *.mydomain.com or *.mydomain.org - // @cert-authority *.mydomain.org,*.mydomain.com ssh-rsa AAAAB5W... - scanner := bufio.NewScanner(file) - for scanner.Scan() { - fields := strings.Split(scanner.Text(), " ") - if len(fields) != 3 { - continue - } - for addr := range strings.SplitSeq(fields[0], ",") { - if addr == normalized || addr == knownhosts.HashHostname(normalized) { - return true, nil - } - } - } - - return false, nil -} - -func (kh *knownHosts) Add(ctx context.Context, host string) error { - // Use a lockfile to prevent parallel processes stepping on each other - lockCtx, canc := context.WithTimeout(ctx, 30*time.Second) - defer canc() - lock, err := kh.Shell.LockFile(lockCtx, kh.Path+".lock") - if err != nil { - return err - } - defer func() { - if err := lock.Unlock(); err != nil { - kh.Shell.Warningf("Failed to release known_hosts file lock: %#v", err) - } - }() - - // If the keygen output already contains the host, we can skip! - if contains, _ := kh.Contains(host); contains { - kh.Shell.Commentf("Host %q already in list of known hosts at \"%s\"", host, kh.Path) - return nil - } - - // Scan the key and then write it to the known_host file - keyscanOutput, err := sshKeyScan(ctx, kh.Shell, host) - if err != nil { - return fmt.Errorf("Could not `ssh-keyscan`: %w", err) - } - - kh.Shell.Commentf("Added host %q to known hosts at \"%s\"", host, kh.Path) - - // Try and open the existing hostfile in (append_only) mode - f, err := os.OpenFile(kh.Path, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o700) - if err != nil { - return fmt.Errorf("Could not open %q for appending: %w", kh.Path, err) - } - defer f.Close() //nolint:errcheck // Best-effort cleanup - primary Close error is checked below. - - if _, err := fmt.Fprintf(f, "%s\n", keyscanOutput); err != nil { - return fmt.Errorf("Could not write to %q: %w", kh.Path, err) - } - - if err := f.Close(); err != nil { - return fmt.Errorf("Could not close %q: %w", kh.Path, err) - } - return nil -} - -// AddFromRepository takes a git repo url, extracts the host and adds it -func (kh *knownHosts) AddFromRepository(ctx context.Context, repository string) error { - u, err := parseGittableURL(repository) - if err != nil { - kh.Shell.Warningf("Could not parse %q as a URL - skipping adding host to SSH known_hosts", repository) - return err - } - - // We only need to keyscan ssh repository urls - if u.Scheme != "ssh" { - return nil - } - - host := resolveGitHost(ctx, kh.Shell, u.Host) - - if err := kh.Add(ctx, host); err != nil { - return fmt.Errorf("Failed to add %q to known_hosts file %q: %w", host, u, err) - } - - return nil -} diff --git a/internal/job/knownhosts_test.go b/internal/job/knownhosts_test.go deleted file mode 100644 index b20988dad5..0000000000 --- a/internal/job/knownhosts_test.go +++ /dev/null @@ -1,77 +0,0 @@ -package job - -import ( - "context" - "fmt" - "net" - "os" - "testing" - - "github.com/buildkite/agent/v3/internal/shell" - "github.com/gliderlabs/ssh" -) - -func TestAddingToKnownHosts(t *testing.T) { - t.Parallel() - - // Start a fake SSH server for ssh-keyscan to poke at. - // It will generate its own host key. - // This uses ':0' to pick an ephemeral port to listen on. - svr := &ssh.Server{ - Addr: "localhost:0", - Version: "Fake SSH Server v0.1", - } - ln, err := net.Listen("tcp", svr.Addr) - if err != nil { - t.Fatalf("net.Listen(tcp, %q) error = %v", svr.Addr, err) - } - go svr.Serve(ln) - defer svr.Close() - - hostAddr := ln.Addr().String() - repoURL := fmt.Sprintf("ssh://git@%s/var/cache/git/project.git", hostAddr) - t.Logf("Fake SSH server listening at address %s", hostAddr) - - // Create a new empty known-hosts file to add to. - f, err := os.CreateTemp("", "known-hosts") - if err != nil { - t.Fatalf(`os.CreateTemp("", "known-hosts") error = %v`, err) - } - t.Cleanup(func() { - os.RemoveAll(f.Name()) //nolint:errcheck // Best-effort cleanup. - }) - if err := f.Close(); err != nil { - t.Fatalf("f.Close() = %v", err) - } - - sh := shell.NewTestShell(t) - sh.Env.Set("PATH", os.Getenv("PATH")) - kh := knownHosts{ - Shell: sh, - Path: f.Name(), - } - - // The file should not contain this host - exists, err := kh.Contains(hostAddr) - if err != nil { - t.Errorf("kh.Contains(%q) error = %v", hostAddr, err) - } - if got, want := exists, false; got != want { - t.Errorf("kh.Contains(%q) = %t, want %t", hostAddr, got, want) - } - - // Add the host - this resolves any SSH client configuration - // (in case the URL contains an alias), and runs ssh-keyscan to get its key. - if err := kh.AddFromRepository(context.Background(), repoURL); err != nil { - t.Errorf("kh.AddFromRespository(%q) = %v", repoURL, err) - } - - // The file should now contain the host key - exists, err = kh.Contains(hostAddr) - if err != nil { - t.Errorf("kh.Contains(%q) error = %v", hostAddr, err) - } - if got, want := exists, true; got != want { - t.Errorf("kh.Contains(%q) = %t, want %t", hostAddr, got, want) - } -} diff --git a/internal/job/plugin.go b/internal/job/plugin.go index 91345b8f2d..0f5bf348f9 100644 --- a/internal/job/plugin.go +++ b/internal/job/plugin.go @@ -341,8 +341,7 @@ func (e *Executor) checkoutPlugin(ctx context.Context, p *plugin.Plugin) (*plugi // existing repos, which is probably fast, but it's _surprisingly hard_ to write a really robust // chain of Git commands that'll definitely get you a clean version of a given upstream branch // ref (the branch might have been force-pushed, the checkout might have become dirty and - // unmergeable, etc.). Plus, then we're duplicating a bunch of fetch/checkout machinery and - // perhaps missing things (like `addRepositoryHostToSSHKnownHosts` which is called down below). + // unmergeable, etc.). Plus, then we're duplicating a bunch of fetch/checkout machinery. // Alternatively, we can DRY it up and simply `rm -rf` the plugin directory if it exists, but // that means a potentially slow and unnecessary clone on every build step. Sigh. I think the // tradeoff is favourable for just blowing away an existing clone if we want least-hassle @@ -388,10 +387,6 @@ func (e *Executor) checkoutPlugin(ctx context.Context, p *plugin.Plugin) (*plugi return nil, err } - if e.SSHKeyscan { - addRepositoryHostToSSHKnownHosts(ctx, e.shell, repo) - } - // Make the directory. Use a random name that _doesn't_ look like a plugin // name, to avoid the `cd ...` line looking like it contains the final path. e.shell.Promptf("mktemp -dp %s", shellwords.Quote(e.PluginsPath)) diff --git a/internal/job/ssh.go b/internal/job/ssh.go index 5eea97b58f..e0dc33c71b 100644 --- a/internal/job/ssh.go +++ b/internal/job/ssh.go @@ -1,89 +1,50 @@ package job import ( - "context" - "fmt" - "os" - "path/filepath" - "runtime" - "strings" - "time" - - "github.com/buildkite/agent/v3/internal/shell" - "github.com/buildkite/roko" + "os/exec" + "regexp" + "strconv" ) -var sshKeyscanRetryInterval = 2 * time.Second +// sshSupportsAcceptNew checks if the installed SSH version supports +// StrictHostKeyChecking=accept-new (requires OpenSSH 7.6+, released Oct 2017). +func sshSupportsAcceptNew() bool { + major, minor, ok := parseSSHVersion() + if !ok { + return false + } + return major > 7 || (major == 7 && minor >= 6) +} -func sshKeyScan(ctx context.Context, sh *shell.Shell, host string) (string, error) { - toolsDir, err := findPathToSSHTools(ctx, sh) +// parseSSHVersion runs "ssh -V" and parses the version number. +// Returns (major, minor, true) on success, (0, 0, false) on failure. +func parseSSHVersion() (major, minor int, ok bool) { + cmd := exec.Command("ssh", "-V") + output, err := cmd.CombinedOutput() if err != nil { - return "", err + return 0, 0, false } - - sshKeyScanPath := filepath.Join(toolsDir, "ssh-keyscan") - hostParts := strings.Split(host, ":") - - r := roko.NewRetrier( - roko.WithMaxAttempts(3), - roko.WithStrategy(roko.Constant(sshKeyscanRetryInterval)), - ) - return roko.DoFunc(ctx, r, func(r *roko.Retrier) (string, error) { - sshKeyScanCommand := fmt.Sprintf("ssh-keyscan %q", host) - args := []string{host} - - // `ssh-keyscan` needs `-p` when scanning a host with a port - if len(hostParts) == 2 { - sshKeyScanCommand = fmt.Sprintf("ssh-keyscan -p %q %q", hostParts[1], hostParts[0]) - args = []string{"-p", hostParts[1], hostParts[0]} - } - - out, err := sh.Command(sshKeyScanPath, args...).RunAndCaptureStdout(ctx) - if err != nil { - keyScanError := fmt.Errorf("`%s` failed", sshKeyScanCommand) - sh.Warningf("%s (%s)", keyScanError, r) - return "", keyScanError - } - if strings.TrimSpace(out) == "" { - // Older versions of ssh-keyscan would exit 0 but not - // return anything, and we've observed newer versions - // of ssh-keyscan - just sometimes return no data - // (maybe networking related?). In any case, no - // response, means an error. - keyScanError := fmt.Errorf("`%s` returned nothing", sshKeyScanCommand) - sh.Warningf("%s (%s)", keyScanError, r) - return "", keyScanError - } - - return out, nil - }) + return extractSSHVersion(string(output)) } -// On Windows, there are many horrible different versions of the ssh tools. Our -// preference is the one bundled with git for windows which is generally MinGW. -// Often this isn't in the path, so we go looking for it specifically. -// -// Some more details on the relative paths at -// https://stackoverflow.com/a/11771907 -func findPathToSSHTools(ctx context.Context, sh *shell.Shell) (string, error) { - sshKeyscan, err := sh.AbsolutePath("ssh-keyscan") - if err == nil { - return filepath.Dir(sshKeyscan), nil +// extractSSHVersion parses an SSH version string like "OpenSSH_8.9p1" or "OpenSSH_7.6p1". +// Returns (major, minor, true) on success. +func extractSSHVersion(output string) (major, minor int, ok bool) { + re := regexp.MustCompile(`OpenSSH[_\s](\d+)\.(\d+)`) + matches := re.FindStringSubmatch(output) + if len(matches) < 3 { + return 0, 0, false + } + + major, err := strconv.Atoi(matches[1]) + if err != nil { + return 0, 0, false } - if runtime.GOOS == "windows" { - execPath, _ := sh.Command("git", "--exec-path").RunAndCaptureStdout(ctx) - if len(execPath) > 0 { - for _, path := range []string{ - filepath.Join(execPath, "..", "..", "..", "usr", "bin", "ssh-keygen.exe"), - filepath.Join(execPath, "..", "..", "bin", "ssh-keygen.exe"), - } { - if _, err := os.Stat(path); err == nil { - return filepath.Dir(path), nil - } - } - } + minor, err = strconv.Atoi(matches[2]) + if err != nil { + return 0, 0, false } - return "", fmt.Errorf("Unable to find ssh-keyscan: %w", err) + return major, minor, true } diff --git a/internal/job/ssh_test.go b/internal/job/ssh_test.go index cd97d8458c..a3f1aa8b6c 100644 --- a/internal/job/ssh_test.go +++ b/internal/job/ssh_test.go @@ -1,129 +1,115 @@ package job -import ( - "context" - "path/filepath" - "testing" - "time" +import "testing" - "github.com/buildkite/agent/v3/internal/shell" - "github.com/buildkite/bintest/v3" - "github.com/stretchr/testify/assert" -) - -func init() { - sshKeyscanRetryInterval = time.Millisecond -} - -func TestFindingSSHTools(t *testing.T) { +func TestExtractSSHVersion(t *testing.T) { t.Parallel() - sh, err := shell.New() - if err != nil { - t.Fatalf("shell.New() error = %v", err) + tests := []struct { + name string + output string + wantMajor int + wantMinor int + wantOK bool + }{ + { + name: "OpenSSH 8.9", + output: "OpenSSH_8.9p1 Ubuntu-3ubuntu0.1, OpenSSL 3.0.2 15 Mar 2022", + wantMajor: 8, + wantMinor: 9, + wantOK: true, + }, + { + name: "OpenSSH 7.6", + output: "OpenSSH_7.6p1 Ubuntu-4ubuntu0.7, OpenSSL 1.0.2n 7 Dec 2017", + wantMajor: 7, + wantMinor: 6, + wantOK: true, + }, + { + name: "OpenSSH 7.5 (before accept-new)", + output: "OpenSSH_7.5p1, OpenSSL 1.0.2k 26 Jan 2017", + wantMajor: 7, + wantMinor: 5, + wantOK: true, + }, + { + name: "OpenSSH 9.0", + output: "OpenSSH_9.0p1, LibreSSL 3.3.6", + wantMajor: 9, + wantMinor: 0, + wantOK: true, + }, + { + name: "macOS format with space", + output: "OpenSSH 9.6p1, LibreSSL 3.3.6", + wantMajor: 9, + wantMinor: 6, + wantOK: true, + }, + { + name: "invalid output", + output: "not ssh output", + wantMajor: 0, + wantMinor: 0, + wantOK: false, + }, + { + name: "empty output", + output: "", + wantMajor: 0, + wantMinor: 0, + wantOK: false, + }, } - sh.Logger = shell.TestingLogger{T: t} - - if _, err := findPathToSSHTools(context.Background(), sh); err != nil { - t.Errorf("findPathToSSHTools(sh) error = %v", err) - } -} - -func TestSSHKeyscanReturnsOutput(t *testing.T) { - t.Parallel() - - sh := shell.NewTestShell(t) - - keyScan, err := bintest.NewMock("ssh-keyscan") - if err != nil { - t.Fatalf("bintest.NewMock(ssh-keyscan) error = %v", err) - } - defer keyScan.CheckAndClose(t) //nolint:errcheck // bintest logs to t - - sh.Env.Set("PATH", filepath.Dir(keyScan.Path)) - - keyScan. - Expect("github.com"). - AndWriteToStdout("github.com ssh-rsa xxx="). - AndExitWith(0) - - keyScanOutput, err := sshKeyScan(context.Background(), sh, "github.com") - - assert.Equal(t, keyScanOutput, "github.com ssh-rsa xxx=") - assert.NoError(t, err) -} - -func TestSSHKeyscanWithHostAndPortReturnsOutput(t *testing.T) { - t.Parallel() - - sh := shell.NewTestShell(t) - - keyScan, err := bintest.NewMock("ssh-keyscan") - if err != nil { - t.Fatalf("bintest.NewMock(ssh-keyscan) error = %v", err) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + major, minor, ok := extractSSHVersion(tc.output) + if ok != tc.wantOK { + t.Errorf("extractSSHVersion(%q) ok = %v, want %v", tc.output, ok, tc.wantOK) + } + if major != tc.wantMajor { + t.Errorf("extractSSHVersion(%q) major = %d, want %d", tc.output, major, tc.wantMajor) + } + if minor != tc.wantMinor { + t.Errorf("extractSSHVersion(%q) minor = %d, want %d", tc.output, minor, tc.wantMinor) + } + }) } - defer keyScan.CheckAndClose(t) //nolint:errcheck // bintest logs to t - - sh.Env.Set("PATH", filepath.Dir(keyScan.Path)) - - keyScan. - Expect("-p", "123", "github.com"). - AndWriteToStdout("github.com ssh-rsa xxx="). - AndExitWith(0) - - keyScanOutput, err := sshKeyScan(context.Background(), sh, "github.com:123") - - assert.Equal(t, keyScanOutput, "github.com ssh-rsa xxx=") - assert.NoError(t, err) } -func TestSSHKeyscanRetriesOnExit1(t *testing.T) { +func TestSSHVersionSupportsAcceptNew(t *testing.T) { t.Parallel() - sh := shell.NewTestShell(t) - - keyScan, err := bintest.NewMock("ssh-keyscan") - if err != nil { - t.Fatalf("bintest.NewMock(ssh-keyscan) error = %v", err) + tests := []struct { + name string + output string + want bool + }{ + {"7.5 - no", "OpenSSH_7.5p1", false}, + {"7.6 - yes", "OpenSSH_7.6p1", true}, + {"7.7 - yes", "OpenSSH_7.7p1", true}, + {"8.0 - yes", "OpenSSH_8.0p1", true}, + {"9.0 - yes", "OpenSSH_9.0p1", true}, + {"6.9 - no", "OpenSSH_6.9p1", false}, } - defer keyScan.CheckAndClose(t) //nolint:errcheck // bintest logs to t - - sh.Env.Set("PATH", filepath.Dir(keyScan.Path)) - keyScan. - Expect("github.com"). - AndWriteToStderr("it failed"). - Exactly(3). - AndExitWith(1) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() - keyScanOutput, err := sshKeyScan(context.Background(), sh, "github.com") + major, minor, ok := extractSSHVersion(tc.output) + if !ok { + t.Fatalf("failed to parse version from %q", tc.output) + } - assert.Equal(t, keyScanOutput, "") - assert.EqualError(t, err, "`ssh-keyscan \"github.com\"` failed") -} - -func TestSSHKeyscanRetriesOnBlankOutputAndExit0(t *testing.T) { - t.Parallel() - - sh := shell.NewTestShell(t) - - keyScan, err := bintest.NewMock("ssh-keyscan") - if err != nil { - t.Fatalf("bintest.NewMock(ssh-keyscan) error = %v", err) + got := major > 7 || (major == 7 && minor >= 6) + if got != tc.want { + t.Errorf("version %d.%d supportsAcceptNew = %v, want %v", major, minor, got, tc.want) + } + }) } - defer keyScan.CheckAndClose(t) - //nolint:errcheck // bintest logs to t - sh.Env.Set("PATH", filepath.Dir(keyScan.Path)) - - keyScan. - Expect("github.com"). - AndWriteToStdout(""). - Exactly(3). - AndExitWith(0) - - keyScanOutput, err := sshKeyScan(context.Background(), sh, "github.com") - - assert.Equal(t, keyScanOutput, "") - assert.EqualError(t, err, "`ssh-keyscan \"github.com\"` returned nothing") }