PB-1068 refactor 1: Replace addRepositoryHostToSSHKnownHosts with StrictHostKeyChecking=accept-new

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

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=

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

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 == "" {

internal/job/integration/checkout_git_mirrors_integration_test.go

@@ -358,23 +358,33 @@ 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", "--", "[email protected]: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{
 		"[email protected]:buildkite/agent.git",
 		"BUILDKITE_SSH_KEYSCAN=true",
 	}
 
 	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,19 +400,36 @@ 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",
 		"BUILDKITE_SSH_KEYSCAN=false",
 	}
 
 	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,22 +442,34 @@ 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",
 		"BUILDKITE_SSH_KEYSCAN=true",
 	}
 
 	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) {

internal/job/integration/checkout_integration_test.go

@@ -722,23 +722,33 @@ 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", "--", "[email protected]: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{
 		"[email protected]:buildkite/agent.git",
 		"BUILDKITE_SSH_KEYSCAN=true",
 	}
 
 	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,19 +760,36 @@ 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",
 		"BUILDKITE_SSH_KEYSCAN=false",
 	}
 
 	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,22 +798,34 @@ 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",
 		"BUILDKITE_SSH_KEYSCAN=true",
 	}
 
 	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) {

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