Skip to content

Commit f35f648

Browse files
committed
PB-1068 refactor 1: Replace addRepositoryHostToSSHKnownHosts with StrictHostKeyChecking=accept-new
1 parent ed463eb commit f35f648

File tree

12 files changed

+125
-522
lines changed

12 files changed

+125
-522
lines changed

go.mod

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ require (
3131
github.com/denisbrodbeck/machineid v1.0.1
3232
github.com/dustin/go-humanize v1.0.1
3333
github.com/dustinkirkland/golang-petname v0.0.0-20231002161417-6a283f1aaaf2
34-
github.com/gliderlabs/ssh v0.3.8
3534
github.com/go-chi/chi/v5 v5.2.3
3635
github.com/gofrs/flock v0.13.0
3736
github.com/google/go-cmp v0.7.0
@@ -56,7 +55,6 @@ require (
5655
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracehttp v1.39.0
5756
go.opentelemetry.io/otel/sdk v1.39.0
5857
go.opentelemetry.io/otel/trace v1.39.0
59-
golang.org/x/crypto v0.46.0
6058
golang.org/x/net v0.48.0
6159
golang.org/x/oauth2 v0.34.0
6260
golang.org/x/sync v0.19.0
@@ -94,7 +92,6 @@ require (
9492
github.com/agnivade/levenshtein v1.2.1 // indirect
9593
github.com/alexflint/go-arg v1.5.1 // indirect
9694
github.com/alexflint/go-scalar v1.2.0 // indirect
97-
github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be // indirect
9895
github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.7.4 // indirect
9996
github.com/aws/aws-sdk-go-v2/credentials v1.19.6 // indirect
10097
github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.16 // indirect
@@ -197,6 +194,7 @@ require (
197194
go.uber.org/multierr v1.11.0 // indirect
198195
go.uber.org/zap v1.27.0 // indirect
199196
go.yaml.in/yaml/v2 v2.4.2 // indirect
197+
golang.org/x/crypto v0.46.0 // indirect
200198
golang.org/x/exp v0.0.0-20250606033433-dcc06ee1d476 // indirect
201199
golang.org/x/mod v0.30.0 // indirect
202200
golang.org/x/text v0.32.0 // indirect

go.sum

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,6 @@ github.com/alexflint/go-scalar v1.2.0 h1:WR7JPKkeNpnYIOfHRa7ivM21aWAdHD0gEWHCx+W
7272
github.com/alexflint/go-scalar v1.2.0/go.mod h1:LoFvNMqS1CPrMVltza4LvnGKhaSpc3oyLEBUZVhhS2o=
7373
github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883 h1:bvNMNQO63//z+xNgfBlViaCIJKLlCJ6/fmUseuG0wVQ=
7474
github.com/andreyvit/diff v0.0.0-20170406064948-c7f18ee00883/go.mod h1:rCTlJbsFo29Kk6CurOXKm700vrz8f0KW0JNfpkRJY/8=
75-
github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be h1:9AeTilPcZAjCFIImctFaOjnTIavg87rW78vTPkQqLI8=
76-
github.com/anmitsu/go-shlex v0.0.0-20200514113438-38f4b401e2be/go.mod h1:ySMOLuWl6zY27l47sB3qLNK6tF2fkHG55UZxx8oIVo4=
7775
github.com/arbovm/levenshtein v0.0.0-20160628152529-48b4e1c0c4d0 h1:jfIu9sQUG6Ig+0+Ap1h4unLjW6YQJpKZVmUzxsD4E/Q=
7876
github.com/arbovm/levenshtein v0.0.0-20160628152529-48b4e1c0c4d0/go.mod h1:t2tdKJDJF9BV14lnkjHmOQgcvEKgtqs5a1N3LNdJhGE=
7977
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
189187
github.com/fortytw2/leaktest v1.3.0/go.mod h1:jDsjWgpAGjm2CA7WthBh/CdZYEPF31XHquHwclZch5g=
190188
github.com/fsnotify/fsnotify v1.9.0 h1:2Ml+OJNzbYCTzsxtv8vKSFD9PbJjmhYF14k/jKC7S9k=
191189
github.com/fsnotify/fsnotify v1.9.0/go.mod h1:8jBTzvmWwFyi3Pb8djgCCO5IBqzKJ/Jwo8TRcHyHii0=
192-
github.com/gliderlabs/ssh v0.3.8 h1:a4YXD1V7xMF9g5nTkdfnja3Sxy1PVDCj1Zg4Wb8vY6c=
193-
github.com/gliderlabs/ssh v0.3.8/go.mod h1:xYoytBv1sV0aL3CavoDuJIQNURXkkfPA/wxQ1pL1fAU=
194190
github.com/go-chi/chi/v5 v5.2.3 h1:WQIt9uxdsAbgIYgid+BpYc+liqQZGMHRaUwp0JUcvdE=
195191
github.com/go-chi/chi/v5 v5.2.3/go.mod h1:L2yAIGWB3H+phAw1NxKwWM+7eUH/lU8pOMm5hHcoops=
196192
github.com/go-logr/logr v1.2.2/go.mod h1:jdQByPbusPIv2/zmleS9BjJVeZ6kBagPoEUsqbVz/1A=

internal/job/checkout.go

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -594,10 +594,6 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error {
594594
var err error
595595
defer func() { span.FinishWithError(err) }()
596596

597-
if e.SSHKeyscan {
598-
addRepositoryHostToSSHKnownHosts(ctx, e.shell, e.Repository)
599-
}
600-
601597
var mirrorDir string
602598

603599
// 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 {
785781
mirrorSubmodules := e.GitMirrorsPath != ""
786782
for _, repository := range submoduleRepos {
787783
submoduleArgs := slices.Clone(args)
788-
// submodules might need their fingerprints verified too
789-
if e.SSHKeyscan {
790-
addRepositoryHostToSSHKnownHosts(ctx, e.shell, repository)
791-
}
792784

793785
if !mirrorSubmodules {
794786
continue

internal/job/executor.go

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -799,24 +799,6 @@ func dirForRepository(repository string) string {
799799
return badCharsPattern.ReplaceAllString(repository, "-")
800800
}
801801

802-
// Given a repository, it will add the host to the set of SSH known_hosts on the machine
803-
func addRepositoryHostToSSHKnownHosts(ctx context.Context, sh *shell.Shell, repository string) {
804-
if osutil.FileExists(repository) {
805-
return
806-
}
807-
808-
knownHosts, err := findKnownHosts(sh)
809-
if err != nil {
810-
sh.Warningf("Failed to find SSH known_hosts file: %v", err)
811-
return
812-
}
813-
814-
if err = knownHosts.AddFromRepository(ctx, repository); err != nil {
815-
sh.Warningf("Error adding to known_hosts: %v", err)
816-
return
817-
}
818-
}
819-
820802
// setUp is run before all the phases run. It's responsible for initializing the
821803
// job environment
822804
func (e *Executor) setUp(ctx context.Context) error {
@@ -870,6 +852,18 @@ func (e *Executor) setUp(ctx context.Context) error {
870852
// Disable any interactive Git/SSH prompting
871853
e.shell.Env.Set("GIT_TERMINAL_PROMPT", "0")
872854

855+
// Configure SSH to automatically accept new host keys (TOFU - Trust On First Use)
856+
// This replaces the previous ssh-keyscan approach with a simpler mechanism that
857+
// achieves the same security model: accept new hosts, reject changed keys.
858+
// Requires OpenSSH 7.6+ (released Oct 2017).
859+
if e.SSHKeyscan {
860+
existingSSHCommand, _ := e.shell.Env.Get("GIT_SSH_COMMAND")
861+
if existingSSHCommand == "" {
862+
existingSSHCommand = "ssh"
863+
}
864+
e.shell.Env.Set("GIT_SSH_COMMAND", existingSSHCommand+" -o StrictHostKeyChecking=accept-new")
865+
}
866+
873867
// Fetch and set secrets before environment hook execution
874868
if e.Secrets != "" {
875869
if err := e.fetchAndSetSecrets(ctx); err != nil {

internal/job/integration/checkout_git_mirrors_integration_test.go

Lines changed: 54 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -358,23 +358,33 @@ func TestCheckingOutWithSSHKeyscan_WithGitMirrors(t *testing.T) {
358358
t.Fatalf("EnableGitMirrors() error = %v", err)
359359
}
360360

361-
tester.MustMock(t, "ssh-keyscan").
362-
Expect("github.com").
363-
AndWriteToStdout("github.com ssh-rsa xxx=").
364-
AndExitWith(0)
365-
361+
var gitSSHCommand string
366362
git := tester.MustMock(t, "git")
367363
git.IgnoreUnexpectedInvocations()
368364

369365
git.Expect("clone", "--mirror", "-v", "--", "[email protected]:buildkite/agent.git", bintest.MatchAny()).
370-
AndExitWith(0)
366+
AndCallFunc(func(c *bintest.Call) {
367+
// Capture GIT_SSH_COMMAND for verification
368+
for _, env := range c.Env {
369+
if strings.HasPrefix(env, "GIT_SSH_COMMAND=") {
370+
gitSSHCommand = env
371+
break
372+
}
373+
}
374+
c.Exit(0)
375+
})
371376

372377
env := []string{
373378
"[email protected]:buildkite/agent.git",
374379
"BUILDKITE_SSH_KEYSCAN=true",
375380
}
376381

377382
tester.RunAndCheck(t, env...)
383+
384+
// Verify GIT_SSH_COMMAND was set with accept-new
385+
if !strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") {
386+
t.Errorf("Expected GIT_SSH_COMMAND to contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand)
387+
}
378388
}
379389

380390
func TestCheckingOutWithoutSSHKeyscan_WithGitMirrors(t *testing.T) {
@@ -390,19 +400,36 @@ func TestCheckingOutWithoutSSHKeyscan_WithGitMirrors(t *testing.T) {
390400
t.Fatalf("EnableGitMirrors() error = %v", err)
391401
}
392402

393-
tester.MustMock(t, "ssh-keyscan").
394-
Expect("github.com").
395-
NotCalled()
403+
var gitSSHCommand string
404+
git := tester.MustMock(t, "git")
405+
git.IgnoreUnexpectedInvocations()
406+
407+
git.Expect("clone", "--mirror", "-v", "--", "https://github.com/buildkite/bash-example.git", bintest.MatchAny()).
408+
AndCallFunc(func(c *bintest.Call) {
409+
// Capture GIT_SSH_COMMAND for verification
410+
for _, env := range c.Env {
411+
if strings.HasPrefix(env, "GIT_SSH_COMMAND=") {
412+
gitSSHCommand = env
413+
break
414+
}
415+
}
416+
c.Exit(0)
417+
})
396418

397419
env := []string{
398420
"BUILDKITE_REPO=https://github.com/buildkite/bash-example.git",
399421
"BUILDKITE_SSH_KEYSCAN=false",
400422
}
401423

402424
tester.RunAndCheck(t, env...)
425+
426+
// Verify GIT_SSH_COMMAND does NOT contain accept-new when SSHKeyscan is disabled
427+
if strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") {
428+
t.Errorf("Expected GIT_SSH_COMMAND to NOT contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand)
429+
}
403430
}
404431

405-
func TestCheckingOutWithSSHKeyscanAndUnscannableRepo_WithGitMirrors(t *testing.T) {
432+
func TestCheckingOutWithSSHKeyscanAndHTTPSRepo_WithGitMirrors(t *testing.T) {
406433
t.Parallel()
407434

408435
tester, err := NewExecutorTester(mainCtx)
@@ -415,22 +442,34 @@ func TestCheckingOutWithSSHKeyscanAndUnscannableRepo_WithGitMirrors(t *testing.T
415442
t.Fatalf("EnableGitMirrors() error = %v", err)
416443
}
417444

418-
tester.MustMock(t, "ssh-keyscan").
419-
Expect("github.com").
420-
NotCalled()
421-
445+
var gitSSHCommand string
422446
git := tester.MustMock(t, "git")
423447
git.IgnoreUnexpectedInvocations()
424448

449+
// Even with HTTPS repos, GIT_SSH_COMMAND is set (it just won't be used)
425450
git.Expect("clone", "--mirror", "-v", "--", "https://github.com/buildkite/bash-example.git", bintest.MatchAny()).
426-
AndExitWith(0)
451+
AndCallFunc(func(c *bintest.Call) {
452+
// Capture GIT_SSH_COMMAND for verification
453+
for _, env := range c.Env {
454+
if strings.HasPrefix(env, "GIT_SSH_COMMAND=") {
455+
gitSSHCommand = env
456+
break
457+
}
458+
}
459+
c.Exit(0)
460+
})
427461

428462
env := []string{
429463
"BUILDKITE_REPO=https://github.com/buildkite/bash-example.git",
430464
"BUILDKITE_SSH_KEYSCAN=true",
431465
}
432466

433467
tester.RunAndCheck(t, env...)
468+
469+
// Verify GIT_SSH_COMMAND was set with accept-new even for HTTPS repos
470+
if !strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") {
471+
t.Errorf("Expected GIT_SSH_COMMAND to contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand)
472+
}
434473
}
435474

436475
func TestCleaningAnExistingCheckout_WithGitMirrors(t *testing.T) {

internal/job/integration/checkout_integration_test.go

Lines changed: 54 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -722,23 +722,33 @@ func TestCheckingOutWithSSHKeyscan(t *testing.T) {
722722
}
723723
defer tester.Close()
724724

725-
tester.MustMock(t, "ssh-keyscan").
726-
Expect("github.com").
727-
AndWriteToStdout("github.com ssh-rsa xxx=").
728-
AndExitWith(0)
729-
725+
var gitSSHCommand string
730726
git := tester.MustMock(t, "git")
731727
git.IgnoreUnexpectedInvocations()
732728

733729
git.Expect("clone", "-v", "--", "[email protected]:buildkite/agent.git", ".").
734-
AndExitWith(0)
730+
AndCallFunc(func(c *bintest.Call) {
731+
// Capture GIT_SSH_COMMAND for verification
732+
for _, env := range c.Env {
733+
if strings.HasPrefix(env, "GIT_SSH_COMMAND=") {
734+
gitSSHCommand = env
735+
break
736+
}
737+
}
738+
c.Exit(0)
739+
})
735740

736741
env := []string{
737742
"[email protected]:buildkite/agent.git",
738743
"BUILDKITE_SSH_KEYSCAN=true",
739744
}
740745

741746
tester.RunAndCheck(t, env...)
747+
748+
// Verify GIT_SSH_COMMAND was set with accept-new
749+
if !strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") {
750+
t.Errorf("Expected GIT_SSH_COMMAND to contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand)
751+
}
742752
}
743753

744754
func TestCheckingOutWithoutSSHKeyscan(t *testing.T) {
@@ -750,19 +760,36 @@ func TestCheckingOutWithoutSSHKeyscan(t *testing.T) {
750760
}
751761
defer tester.Close()
752762

753-
tester.MustMock(t, "ssh-keyscan").
754-
Expect("github.com").
755-
NotCalled()
763+
var gitSSHCommand string
764+
git := tester.MustMock(t, "git")
765+
git.IgnoreUnexpectedInvocations()
766+
767+
git.Expect("clone", "-v", "--", "https://github.com/buildkite/bash-example.git", ".").
768+
AndCallFunc(func(c *bintest.Call) {
769+
// Capture GIT_SSH_COMMAND for verification
770+
for _, env := range c.Env {
771+
if strings.HasPrefix(env, "GIT_SSH_COMMAND=") {
772+
gitSSHCommand = env
773+
break
774+
}
775+
}
776+
c.Exit(0)
777+
})
756778

757779
env := []string{
758780
"BUILDKITE_REPO=https://github.com/buildkite/bash-example.git",
759781
"BUILDKITE_SSH_KEYSCAN=false",
760782
}
761783

762784
tester.RunAndCheck(t, env...)
785+
786+
// Verify GIT_SSH_COMMAND does NOT contain accept-new when SSHKeyscan is disabled
787+
if strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") {
788+
t.Errorf("Expected GIT_SSH_COMMAND to NOT contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand)
789+
}
763790
}
764791

765-
func TestCheckingOutWithSSHKeyscanAndUnscannableRepo(t *testing.T) {
792+
func TestCheckingOutWithSSHKeyscanAndHTTPSRepo(t *testing.T) {
766793
t.Parallel()
767794

768795
tester, err := NewExecutorTester(mainCtx)
@@ -771,22 +798,34 @@ func TestCheckingOutWithSSHKeyscanAndUnscannableRepo(t *testing.T) {
771798
}
772799
defer tester.Close()
773800

774-
tester.MustMock(t, "ssh-keyscan").
775-
Expect("github.com").
776-
NotCalled()
777-
801+
var gitSSHCommand string
778802
git := tester.MustMock(t, "git")
779803
git.IgnoreUnexpectedInvocations()
780804

805+
// Even with HTTPS repos, GIT_SSH_COMMAND is set (it just won't be used)
781806
git.Expect("clone", "-v", "--", "https://github.com/buildkite/bash-example.git", ".").
782-
AndExitWith(0)
807+
AndCallFunc(func(c *bintest.Call) {
808+
// Capture GIT_SSH_COMMAND for verification
809+
for _, env := range c.Env {
810+
if strings.HasPrefix(env, "GIT_SSH_COMMAND=") {
811+
gitSSHCommand = env
812+
break
813+
}
814+
}
815+
c.Exit(0)
816+
})
783817

784818
env := []string{
785819
"BUILDKITE_REPO=https://github.com/buildkite/bash-example.git",
786820
"BUILDKITE_SSH_KEYSCAN=true",
787821
}
788822

789823
tester.RunAndCheck(t, env...)
824+
825+
// Verify GIT_SSH_COMMAND was set with accept-new even for HTTPS repos
826+
if !strings.Contains(gitSSHCommand, "StrictHostKeyChecking=accept-new") {
827+
t.Errorf("Expected GIT_SSH_COMMAND to contain 'StrictHostKeyChecking=accept-new', got: %q", gitSSHCommand)
828+
}
790829
}
791830

792831
func TestCleaningAnExistingCheckout(t *testing.T) {

internal/job/integration/job_api_integration_test.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,5 +142,7 @@ func TestBootstrapRunsJobAPI(t *testing.T) {
142142
}
143143
})
144144

145-
tester.RunAndCheck(t)
145+
// Disable SSH keyscan since this test isn't about SSH and it modifies
146+
// GIT_SSH_COMMAND which would cause env mismatch between Job API and process
147+
tester.RunAndCheck(t, "BUILDKITE_SSH_KEYSCAN=false")
146148
}

0 commit comments

Comments
 (0)