Merge pull request #3676 from buildkite/ming/pb-1068-refactor-1

A-1118: Replace addRepositoryHostToSSHKnownHosts with StrictHostKeyChecking=accept-new

Author: zhming0

go.mod

@@ -35,7 +35,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-20260215035315-f0c533e9ce9b
-	github.com/gliderlabs/ssh v0.3.8
 	github.com/go-chi/chi/v5 v5.3.0
 	github.com/gofrs/flock v0.13.0
 	github.com/google/go-cmp v0.7.0
@@ -60,7 +59,6 @@ require (
 	go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.43.0
 	go.opentelemetry.io/otel/sdk v1.43.0
 	go.opentelemetry.io/otel/trace v1.43.0
-	golang.org/x/crypto v0.52.0
 	golang.org/x/net v0.55.0
 	golang.org/x/oauth2 v0.36.0
 	golang.org/x/sync v0.20.0
@@ -85,7 +83,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.10 // indirect
 	github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.23 // indirect
 	github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.23 // indirect
@@ -149,6 +146,7 @@ require (
 	go.opentelemetry.io/proto/otlp v1.10.0 // indirect
 	go.uber.org/multierr v1.11.0 // indirect
 	go.yaml.in/yaml/v2 v2.4.4 // indirect
+	golang.org/x/crypto v0.52.0 // indirect
 	golang.org/x/mod v0.35.0 // indirect
 	golang.org/x/text v0.37.0 // indirect
 	golang.org/x/time v0.15.0 // indirect

go.sum

@@ -51,8 +51,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.7 h1:DWpAJt66FmnnaRIOT/8ASTucrvuDPZASqhhLey6tLY8=
@@ -162,8 +160,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.3.0 h1:halUjDxhshgXHMrao5bB8eNBXo/rnzwr8m5m36glehM=
 github.com/go-chi/chi/v5 v5.3.0/go.mod h1:R+tYY2hNuVUUjxoPtqUdgBqevM9s9njzkTLutVsOCto=
 github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=

internal/job/checkout.go

@@ -843,10 +843,6 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) {
 	})
 	defer func() { tracetools.FinishWithError(span, retErr) }()
 
-	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
@@ -995,11 +991,6 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) {
 		} else {
 			mirrorSubmodules := e.GitMirrorsPath != ""
 			for _, repository := range submoduleRepos {
-				// submodules might need their fingerprints verified too
-				if e.SSHKeyscan {
-					addRepositoryHostToSSHKnownHosts(ctx, e.shell, repository)
-				}
-
 				if !mirrorSubmodules {
 					continue
 				}

internal/job/executor.go

@@ -829,24 +829,6 @@ func dirForRepository(repository string) string {
 	return badCharsRE.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) (retErr error) {
@@ -899,6 +881,13 @@ func (e *Executor) setUp(ctx context.Context) (retErr 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.configureSSHKeyChecking()
+	}
+
 	// Fetch and set secrets before environment hook execution
 	if e.Secrets != "" {
 		if err := e.fetchAndSetSecrets(ctx); err != nil {

internal/job/integration/checkout_git_mirrors_integration_test.go

@@ -330,11 +330,6 @@ 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)
-
 	git := tester.MustMock(t, "git")
 	git.IgnoreUnexpectedInvocations()
 
@@ -362,10 +357,6 @@ func TestCheckingOutWithoutSSHKeyscan_WithGitMirrors(t *testing.T) {
 		t.Fatalf("EnableGitMirrors() error = %v", err)
 	}
 
-	tester.MustMock(t, "ssh-keyscan").
-		Expect("github.com").
-		NotCalled()
-
 	env := []string{
 		"BUILDKITE_REPO=https://github.com/buildkite/bash-example.git",
 		"BUILDKITE_SSH_KEYSCAN=false",
@@ -387,10 +378,6 @@ func TestCheckingOutWithSSHKeyscanAndUnscannableRepo_WithGitMirrors(t *testing.T
 		t.Fatalf("EnableGitMirrors() error = %v", err)
 	}
 
-	tester.MustMock(t, "ssh-keyscan").
-		Expect("github.com").
-		NotCalled()
-
 	git := tester.MustMock(t, "git")
 	git.IgnoreUnexpectedInvocations()
 

internal/job/integration/checkout_integration_test.go

@@ -765,11 +765,6 @@ func TestCheckingOutWithSSHKeyscan(t *testing.T) {
 	}
 	defer tester.Close()
 
-	tester.MustMock(t, "ssh-keyscan").
-		Expect("github.com").
-		AndWriteToStdout("github.com ssh-rsa xxx=").
-		AndExitWith(0)
-
 	git := tester.MustMock(t, "git")
 	git.IgnoreUnexpectedInvocations()
 
@@ -793,10 +788,6 @@ func TestCheckingOutWithoutSSHKeyscan(t *testing.T) {
 	}
 	defer tester.Close()
 
-	tester.MustMock(t, "ssh-keyscan").
-		Expect("github.com").
-		NotCalled()
-
 	env := []string{
 		"BUILDKITE_REPO=https://github.com/buildkite/bash-example.git",
 		"BUILDKITE_SSH_KEYSCAN=false",
@@ -814,10 +805,6 @@ func TestCheckingOutWithSSHKeyscanAndUnscannableRepo(t *testing.T) {
 	}
 	defer tester.Close()
 
-	tester.MustMock(t, "ssh-keyscan").
-		Expect("github.com").
-		NotCalled()
-
 	git := tester.MustMock(t, "git")
 	git.IgnoreUnexpectedInvocations()
 

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, causing an env mismatch between the Job API and the process.
+	tester.RunAndCheck(t, "BUILDKITE_SSH_KEYSCAN=false")
 }