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
10 changes: 9 additions & 1 deletion clicommand/bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ type BootstrapConfig struct {
GitFetchFlags string `cli:"git-fetch-flags"`
GitCloneMirrorFlags string `cli:"git-clone-mirror-flags"`
GitCleanFlags string `cli:"git-clean-flags"`
GitSSHKey string `cli:"git-ssh-key"`
GitMirrorsPath string `cli:"git-mirrors-path" normalize:"filepath"`
GitMirrorCheckoutMode string `cli:"git-mirror-checkout-mode"`
GitMirrorsLockTimeout int `cli:"git-mirrors-lock-timeout"`
Expand Down Expand Up @@ -242,6 +243,11 @@ var BootstrapCommand = cli.Command{
GitCloneMirrorFlagsFlag,
GitCleanFlagsFlag,
GitFetchFlagsFlag,
cli.StringFlag{
Name: "git-ssh-key",
Usage: "SSH private key to use for git checkout",
EnvVar: "BUILDKITE_GIT_SSH_KEY",
},
GitMirrorsPathFlag,
GitMirrorCheckoutModeFlag,
GitMirrorsLockTimeoutFlag,
Expand Down Expand Up @@ -443,6 +449,7 @@ var BootstrapCommand = cli.Command{
GitCloneFlags: cfg.GitCloneFlags,
GitCloneMirrorFlags: cfg.GitCloneMirrorFlags,
GitFetchFlags: cfg.GitFetchFlags,
GitSSHKey: cfg.GitSSHKey,
GitMirrorsLockTimeout: cfg.GitMirrorsLockTimeout,
GitMirrorsPath: cfg.GitMirrorsPath,
GitMirrorCheckoutMode: cfg.GitMirrorCheckoutMode,
Expand Down Expand Up @@ -489,7 +496,8 @@ var BootstrapCommand = cli.Command{
defer cancel()

signals := make(chan os.Signal, 1)
signal.Notify(signals, os.Interrupt,
signal.Notify(
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This isn't really a change, it's just a result of using gofumpt for my formatting rather than gofmt, I'm happy to default to the latter, though

signals, os.Interrupt,
syscall.SIGHUP,
syscall.SIGTERM,
syscall.SIGINT,
Expand Down
1 change: 1 addition & 0 deletions clicommand/global.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ var (
"*_SECRET",
"*_TOKEN",
"*_PRIVATE_KEY",
"*_SSH_KEY",
"*_ACCESS_KEY",
"*_SECRET_KEY",
// Connection strings frequently contain passwords, e.g.
Expand Down
88 changes: 88 additions & 0 deletions internal/job/checkout.go
Original file line number Diff line number Diff line change
Expand Up @@ -838,6 +838,23 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) {
addRepositoryHostToSSHKnownHosts(ctx, e.shell, e.Repository)
}

sshKeyPath, cleanupSSHKey, err := e.prepareGitSSHKey()
if err != nil {
return fmt.Errorf("preparing git ssh key: %w", err)
}
if cleanupSSHKey != nil {
defer func() {
if cleanupErr := cleanupSSHKey(); cleanupErr != nil {
cleanupErr = fmt.Errorf("cleaning up git ssh key %q: %w", sshKeyPath, cleanupErr)
if err == nil {
err = cleanupErr
} else {
err = errors.Join(err, cleanupErr)
}
Comment on lines +849 to +853
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.

err has (meanwhile) become retErr, and errors.Join does the right thing with nil (i.e. drops it), so:

Suggested change
if err == nil {
err = cleanupErr
} else {
err = errors.Join(err, cleanupErr)
}
retErr = errors.Join(retErr, cleanupErr)

}
}()
}

var mirrorDir string

// If we can, get a mirror of the git repository to use for reference later
Expand Down Expand Up @@ -1065,6 +1082,77 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) (retErr error) {
return nil
}

// prepareGitSSHKey materialises the configured GitSSHKey into a private
// directory next to the build checkout and points GIT_SSH_COMMAND at it for
// the duration of the checkout phase. It returns the path to the key file, a
// cleanup function that removes the key and restores any previous
// GIT_SSH_COMMAND, and an error. If no key is configured both the path and
// cleanup are zero.
//
// Only the default checkout phase invokes this; custom checkout hooks must
// arrange their own credentials.
func (e *Executor) prepareGitSSHKey() (_ string, _ func() error, retErr 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.

Naming the return args has clear readability and documentation upsides over using _:

Suggested change
func (e *Executor) prepareGitSSHKey() (_ string, _ func() error, retErr error) {
func (e *Executor) prepareGitSSHKey() (sshKeyPath string, cleanup func() error, retErr error) {

if e.GitSSHKey == "" {
return "", nil, nil
}

checkoutPath, exists := e.shell.Env.Get("BUILDKITE_BUILD_CHECKOUT_PATH")
if !exists || checkoutPath == "" {
return "", nil, errors.New("BUILDKITE_BUILD_CHECKOUT_PATH is not set")
}

parentDir := filepath.Dir(checkoutPath)
if err := os.MkdirAll(parentDir, 0o700); err != nil {
return "", nil, fmt.Errorf("creating ssh key parent directory %q: %w", parentDir, err)
}

pattern := ".buildkite-ssh-key-"
if e.PipelineSlug != "" {
pattern += badCharsRE.ReplaceAllString(e.PipelineSlug, "-") + "-"
}

// os.MkdirTemp creates the directory with mode 0o700 on Unix, giving the
// key file inside an additional layer of protection from sibling builds.
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.

Sibling builds would have to be running under a different uid to be prevented from accessing inside a dir with mode 0o700, right?

sshKeyDir, err := os.MkdirTemp(parentDir, pattern)
if err != nil {
return "", nil, fmt.Errorf("creating ssh key directory: %w", err)
}
defer func() {
if retErr != nil {
_ = os.RemoveAll(sshKeyDir)
}
}()

sshKeyPath := filepath.Join(sshKeyDir, "id")
// Most SSH key parsers require a trailing newline; tolerate either form
// of input and always write a single one.
keyBytes := []byte(strings.TrimRight(e.GitSSHKey, "\n") + "\n")
if err := os.WriteFile(sshKeyPath, keyBytes, 0o600); err != nil {
return "", nil, fmt.Errorf("writing ssh key file: %w", err)
}

previousGitSSHCommand, hadPreviousGitSSHCommand := e.shell.Env.Get("GIT_SSH_COMMAND")
e.shell.Env.Set("GIT_SSH_COMMAND", gitSSHCommandForKeyFile(sshKeyPath))

cleanup := func() error {
if hadPreviousGitSSHCommand {
e.shell.Env.Set("GIT_SSH_COMMAND", previousGitSSHCommand)
} else {
e.shell.Env.Remove("GIT_SSH_COMMAND")
}
if err := os.RemoveAll(sshKeyDir); err != nil && !os.IsNotExist(err) {
return err
}
return nil
}

return sshKeyPath, cleanup, nil
}

func gitSSHCommandForKeyFile(path string) string {
return fmt.Sprintf("ssh -i %s -o IdentitiesOnly=yes", shellwords.Quote(path))
}

// gitFetchWithFallback run git fetch for refspecs, when it fails on recoverable reason, it will retry fetching
// all heads and refs.
func gitFetchWithFallback(ctx context.Context, shell *shell.Shell, gitFetchFlags string, refspecs ...string) error {
Expand Down
135 changes: 135 additions & 0 deletions internal/job/checkout_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ package job
import (
"context"
"os"
"path/filepath"
"runtime"
"testing"
"time"

Expand Down Expand Up @@ -153,6 +155,139 @@ func TestDefaultCheckoutPhase(t *testing.T) {
}
}

func TestPrepareGitSSHKey(t *testing.T) {
t.Parallel()

sh, err := shell.New()
if err != nil {
t.Fatalf("shell.New() error = %v, want nil", err)
}

t.Run("no key configured", func(t *testing.T) {
executor := &Executor{shell: sh}

sshKeyPath, cleanup, err := executor.prepareGitSSHKey()
if err != nil {
t.Fatalf("executor.prepareGitSSHKey() error = %v, want nil", err)
}
if sshKeyPath != "" {
t.Fatalf("executor.prepareGitSSHKey() path = %q, want empty", sshKeyPath)
}
if cleanup != nil {
t.Fatal("executor.prepareGitSSHKey() cleanup != nil, want nil")
}
})

t.Run("creates key file and restores environment", func(t *testing.T) {
checkoutParent := t.TempDir()
checkoutPath := filepath.Join(checkoutParent, "checkout")
sh.Env.Set("BUILDKITE_BUILD_CHECKOUT_PATH", checkoutPath)
sh.Env.Set("GIT_SSH_COMMAND", "ssh -F ~/.ssh/config")

executor := &Executor{
shell: sh,
ExecutorConfig: ExecutorConfig{
GitSSHKey: "super-secret-key",
PipelineSlug: "test/pipeline",
},
}

sshKeyPath, cleanup, err := executor.prepareGitSSHKey()
if err != nil {
t.Fatalf("executor.prepareGitSSHKey() error = %v, want nil", err)
}
if cleanup == nil {
t.Fatal("executor.prepareGitSSHKey() cleanup = nil, want non-nil")
}
sshKeyDir := filepath.Dir(sshKeyPath)
if got, want := filepath.Dir(sshKeyDir), checkoutParent; got != want {
t.Fatalf("filepath.Dir(sshKeyDir) = %q, want %q", got, want)
}
if matched, err := filepath.Match(filepath.Join(checkoutParent, ".buildkite-ssh-key-test-pipeline-*"), sshKeyDir); err != nil || !matched {
t.Fatalf("sshKeyDir = %q, want buildkite ssh key pattern match (err=%v)", sshKeyDir, err)
}
contents, err := os.ReadFile(sshKeyPath)
if err != nil {
t.Fatalf("os.ReadFile(%q) error = %v, want nil", sshKeyPath, err)
}
if got, want := string(contents), "super-secret-key\n"; got != want {
t.Fatalf("ssh key contents = %q, want %q", got, want)
}
if runtime.GOOS != "windows" {
info, err := os.Stat(sshKeyPath)
if err != nil {
t.Fatalf("os.Stat(%q) error = %v, want nil", sshKeyPath, err)
}
if got, want := info.Mode().Perm(), os.FileMode(0o600); got != want {
t.Fatalf("ssh key permissions = %o, want %o", got, want)
}
dirInfo, err := os.Stat(sshKeyDir)
if err != nil {
t.Fatalf("os.Stat(%q) error = %v, want nil", sshKeyDir, err)
}
if got, want := dirInfo.Mode().Perm(), os.FileMode(0o700); got != want {
t.Fatalf("ssh key directory permissions = %o, want %o", got, want)
}
}
if got, want := sh.Env.GetString("GIT_SSH_COMMAND", ""), gitSSHCommandForKeyFile(sshKeyPath); got != want {
t.Fatalf("GIT_SSH_COMMAND = %q, want %q", got, want)
}

if err := cleanup(); err != nil {
t.Fatalf("cleanup() error = %v, want nil", err)
}
if _, err := os.Stat(sshKeyDir); !os.IsNotExist(err) {
t.Fatalf("os.Stat(%q) error = %v, want not exist", sshKeyDir, err)
}
if got, want := sh.Env.GetString("GIT_SSH_COMMAND", ""), "ssh -F ~/.ssh/config"; got != want {
t.Fatalf("restored GIT_SSH_COMMAND = %q, want %q", got, want)
}
})
}

func TestDefaultCheckoutPhase_CleansUpGitSSHKeyOnError(t *testing.T) {
t.Parallel()

sh, err := shell.New()
if err != nil {
t.Fatalf("shell.New() error = %v, want nil", err)
}

checkoutParent := t.TempDir()
checkoutPath := filepath.Join(checkoutParent, "checkout")
sh.Env.Set("BUILDKITE_BUILD_CHECKOUT_PATH", checkoutPath)

executor := &Executor{
shell: sh,
ExecutorConfig: ExecutorConfig{
Repository: filepath.Join(checkoutParent, "does-not-exist.git"),
Commit: "HEAD",
Branch: "main",
GitCleanFlags: "-fdq",
GitSSHKey: "super-secret-key",
PipelineSlug: "test-pipeline",
},
}
t.Cleanup(func() {
if executor.checkoutRoot != nil {
_ = executor.checkoutRoot.Close()
executor.checkoutRoot = nil
}
})

if err := executor.defaultCheckoutPhase(context.Background()); err == nil {
t.Fatal("executor.defaultCheckoutPhase(context.Background()) error = nil, want non-nil")
}

matches, err := filepath.Glob(filepath.Join(checkoutParent, ".buildkite-ssh-key-*"))
if err != nil {
t.Fatalf("filepath.Glob() error = %v, want nil", err)
}
if len(matches) != 0 {
t.Fatalf("ssh key files left behind: %v", matches)
}
}

func TestSkipCheckout(t *testing.T) {
t.Parallel()

Expand Down
3 changes: 3 additions & 0 deletions internal/job/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ type ExecutorConfig struct {
// Flags to pass to "git clean" command
GitCleanFlags string `env:"BUILDKITE_GIT_CLEAN_FLAGS"`

// SSH private key to use for git checkout operations
GitSSHKey string `env:"BUILDKITE_GIT_SSH_KEY"`

// Config key=value pairs to pass to "git" when submodule init commands are invoked
GitSubmoduleCloneConfig []string `env:"BUILDKITE_GIT_SUBMODULE_CLONE_CONFIG"`

Expand Down
7 changes: 7 additions & 0 deletions internal/job/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) {
AutomaticArtifactUploadPaths: "llamas/",
GitCloneFlags: "--prune",
GitCleanFlags: "-v",
GitSSHKey: "original-key",
AgentName: "myAgent",
CleanCheckout: false,
PluginsAlwaysCloneFresh: false,
Expand All @@ -27,6 +28,7 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) {
"BUILDKITE_SOMETHING_ELSE=1",
"BUILDKITE_REPO=https://my.mirror/repo.git",
"BUILDKITE_CLEAN_CHECKOUT=true",
"BUILDKITE_GIT_SSH_KEY=new-key",
"BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH=true",
"BUILDKITE_GIT_SUBMODULES=true",
})
Expand All @@ -37,6 +39,7 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) {
"BUILDKITE_GIT_CLONE_FLAGS": "-f",
"BUILDKITE_REPO": "https://my.mirror/repo.git",
"BUILDKITE_CLEAN_CHECKOUT": "true",
"BUILDKITE_GIT_SSH_KEY": "new-key",
"BUILDKITE_PLUGINS_ALWAYS_CLONE_FRESH": "true",
"BUILDKITE_GIT_SUBMODULES": "true",
}
Expand All @@ -53,6 +56,10 @@ func TestEnvVarsAreMappedToConfig(t *testing.T) {
t.Errorf("config.Repository = %q, want %q", got, want)
}

if got, want := config.GitSSHKey, "new-key"; got != want {
t.Errorf("config.GitSSHKey = %q, want %q", got, want)
}

if got, want := config.CleanCheckout, true; got != want {
t.Errorf("config.CleanCheckout = %t, want %t", got, want)
}
Expand Down
Loading