From ace3fbbcb3dda17262e16c6317788bea76a8460d Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Mon, 3 Apr 2023 15:57:09 +1200 Subject: [PATCH 01/11] WIP support polyglot hooks --- bootstrap/bootstrap.go | 49 +++++++++++++++++++++++-- bootstrap/shell/shell.go | 28 ++++++++++++++- env/environment.go | 4 +++ hook/scriptwrapper.go | 6 ++-- shellscript/shellscript.go | 73 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 154 insertions(+), 6 deletions(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index e412c26dc8..184708d3b5 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -297,11 +297,56 @@ func (b *Bootstrap) executeHook(ctx context.Context, hookCfg HookConfig) error { b.shell.Headerf("Running %s hook", hookName) + isBinary, err := shellscript.IsBinaryExecutable(hookCfg.Path) + if err != nil { + return fmt.Errorf("checking if %s hook is a binary: %w", hookName, err) + } + + shebangLine, err := shellscript.ShebangLine(hookCfg.Path) + if err != nil { + return fmt.Errorf("reading shebang line for %s hook: %w", hookName, err) + } + + switch { + case isBinary: + // It's a binary, so we'll just run it directly, no wrapping needed or possible + return b.runUnwrappedHook(ctx, hookName, hookCfg) + case shellscript.IsPOSIXShell(shebangLine): + // It's definitely a shell script, wrap it so that we can snaffle the changed environment variables + return b.runWrappedShellScriptHook(ctx, hookName, hookCfg) + case shebangLine != "" && !shellscript.IsPOSIXShell(shebangLine): + // It's an interpretable-something with a shebang line, and it almost certainly isn't a shell script, + // so it won't be capable of setting environment variables (outside of the job api), so just run it directly + return b.runUnwrappedHook(ctx, hookName, hookCfg) + case shebangLine == "": + // If there's no shebang line in the hook, and it's not a binary, so let's just assume that it's a shell script + // The OS will complain if we do something stupid, so we'll just give it a shot + return b.runWrappedShellScriptHook(ctx, hookName, hookCfg) + default: + return fmt.Errorf(`the buildkite agent wasn't able to determine the type of hook it was trying to run. +This is a bug, please contact support@buildkite.com and/or submit an issue on the github.com/buildkite/agent repo with the following information: +Hook scope: %q +Hook path: %q +Hook type: %q +Shebang line: %q +Is binary?: %t`, hookCfg.Scope, hookCfg.Path, hookName, shebangLine, isBinary) + } +} + +func (b *Bootstrap) runUnwrappedHook(ctx context.Context, hookName string, hookCfg HookConfig) error { + environ := hookCfg.Env.Copy() + + environ.Set("BUILDKITE_HOOK_PHASE", hookCfg.Name) + environ.Set("BUILDKITE_HOOK_PATH", hookCfg.Path) + environ.Set("BUILDKITE_HOOK_TRIGGERED_FROM", hookCfg.Scope) + + return b.shell.RunWithEnv(ctx, environ, hookCfg.Path) +} + +func (b *Bootstrap) runWrappedShellScriptHook(ctx context.Context, hookName string, hookCfg HookConfig) error { redactors := b.setupRedactors() defer redactors.Flush() - // We need a script to wrap the hook script so that we can snaffle the changed - // environment variables script, err := hook.NewScriptWrapper(hook.WithHookPath(hookCfg.Path)) if err != nil { b.shell.Errorf("Error creating hook script: %v", err) diff --git a/bootstrap/shell/shell.go b/bootstrap/shell/shell.go index ec9e7cfa7f..a29e325371 100644 --- a/bootstrap/shell/shell.go +++ b/bootstrap/shell/shell.go @@ -282,6 +282,30 @@ func (s *Shell) Run(ctx context.Context, command string, arg ...string) error { return s.RunWithoutPrompt(ctx, command, arg...) } +func (s *Shell) RunWithEnv(ctx context.Context, environ *env.Environment, command string, arg ...string) error { + formatted := process.FormatCommand(command, arg) + if s.stdin == nil { + s.Promptf("%s", formatted) + } else { + // bash-syntax-compatible indication that input is coming from somewhere + s.Promptf("%s < /dev/stdin", formatted) + } + + cmd, err := s.buildCommand(command, arg...) + if err != nil { + s.Errorf("Error building command: %v", err) + return err + } + + cmd.Env = append(cmd.Env, environ.ToSlice()...) + + return s.executeCommand(ctx, cmd, s.Writer, executeFlags{ + Stdout: true, + Stderr: true, + PTY: s.PTY, + }) +} + // RunWithoutPrompt runs a command, writes stdout and err to the logger, // and returns an error if it fails. It doesn't show a prompt. func (s *Shell) RunWithoutPrompt(ctx context.Context, command string, arg ...string) error { @@ -379,6 +403,8 @@ func (s *Shell) RunScript(ctx context.Context, path string, extra *env.Environme case !isWindows && isSh: // If the script contains a shebang line, it can be run directly, // with the shebang line choosing the interpreter. + // note that this means that it isn't necessarily a shell script in this case! + // #!/usr/bin/env python would be totally valid, and would execute as a python script sb, err := shellscript.ShebangLine(path) if err == nil && sb != "" { command = path @@ -397,7 +423,7 @@ func (s *Shell) RunScript(ctx context.Context, path string, extra *env.Environme s.Warningf("Couldn't find bash (%v). Attempting to fall back to sh. This may cause issues for hooks and plugins that assume Bash features.", err) shPath, err = s.AbsolutePath("sh") if err != nil { - return fmt.Errorf("Error finding a shell, needed to run scripts: %v.", err) + return fmt.Errorf("error finding a shell, needed to run scripts: %v", err) } } command = shPath diff --git a/env/environment.go b/env/environment.go index 37674f98c2..a62c5dc7ea 100644 --- a/env/environment.go +++ b/env/environment.go @@ -197,6 +197,10 @@ func (e *Environment) Apply(diff Diff) { // Copy returns a copy of the env func (e *Environment) Copy() *Environment { + if e == nil { + return New() + } + c := New() e.underlying.Range(func(k, v string) bool { diff --git a/hook/scriptwrapper.go b/hook/scriptwrapper.go index 20e121e707..667b390660 100644 --- a/hook/scriptwrapper.go +++ b/hook/scriptwrapper.go @@ -145,9 +145,9 @@ func NewScriptWrapper(opts ...scriptWrapperOpt) (*ScriptWrapper, error) { // // But if the shebang specifies something weird like Ruby 🤪 // the wrapper won't work. Stick to POSIX shells for now. - if shebang != "" && !shellscript.IsPOSIXShell(shebang) { - return nil, fmt.Errorf("hook starts with an unsupported shebang line %q", shebang) - } + // if shebang != "" && !shellscript.IsPOSIXShell(shebang) { + // return nil, fmt.Errorf("hook starts with an unsupported shebang line %q", shebang) + // } var isPOSIXHook, isPwshHook bool diff --git a/shellscript/shellscript.go b/shellscript/shellscript.go index 1ea9d5ab92..ef528c824f 100644 --- a/shellscript/shellscript.go +++ b/shellscript/shellscript.go @@ -3,6 +3,8 @@ package shellscript import ( "bufio" + "bytes" + "fmt" "os" "path/filepath" "strings" @@ -10,6 +12,77 @@ import ( "github.com/buildkite/shellwords" ) +// Magic numbers for various types of executable files +var ( + // Linux and many other Unix-like systems use ELF binaries + ELFMagic = []byte{0x7F, 0x45, 0x4C, 0x46} + + // Windows uses MS-DOS MZ-style executables. This includes PE32 and PE32+ (which is 64-bit 🙃) binaries, + // which a are what modern windowses tend to use. + MZMagic = []byte{0x4D, 0x5A} + + // macOS uses Mach-O binaries. There are two variants: fat and thin. + // Thin binaries are further divided into 32-bit and 64-bit variants, + // which are furtherer subdivided by processor endianess. It's a mess. + + // For "fat" binaries, which contain multiple architectures + MachOFatMagic = []byte{0xCA, 0xFE, 0xBA, 0xBE} + + // For "thin" binaries, which contain a single architecture + // (ie 32 and 64 bit variants, on top of different processor architectures) + MachOThin32Magic = []byte{0xFE, 0xED, 0xFA, 0xCE} + MachOThin64Magic = []byte{0xFE, 0xED, 0xFA, 0xCF} + + // Mach-O thin binaries can also be in reverse byte order, apparently? + // I think this is for big endian processors like PowerPC, in which case we don't need these here, + // but we might as well include them for completeness. + MachOThin32ReverseMagic = []byte{0xCE, 0xFA, 0xED, 0xFE} + MachOThin64ReverseMagic = []byte{0xCF, 0xFA, 0xED, 0xFE} + + BinaryMagicks = [][]byte{ + ELFMagic, + MZMagic, + MachOFatMagic, + MachOThin32Magic, + MachOThin64Magic, + MachOThin32ReverseMagic, + MachOThin64ReverseMagic, + } +) + +// ExecutableType returns the type of file executable at the given path. +// The file at the given path is assumed to be an executable, and executability is not checked. +func IsBinaryExecutable(path string) (bool, error) { + f, err := os.Open(path) + if err != nil { + return false, fmt.Errorf("open file %q: %w", path, err) + } + + defer f.Close() + r := bufio.NewReader(f) + firstFour, err := r.Peek(4) + if err != nil { + return false, fmt.Errorf("reading first four bytes of file %q: %w", path, err) + } + + if len(firstFour) < 4 { + // there are less than four bytes in the file, there's nothing that we can do with it + return false, nil + } + + for _, magicNumber := range BinaryMagicks { + if sliceHasPrefix(firstFour, magicNumber) { + return true, nil + } + } + + return false, nil +} + +func sliceHasPrefix(s, prefix []byte) bool { + return len(s) >= len(prefix) && bytes.Equal(s[:len(prefix)], prefix) +} + // ShebangLine extracts the shebang line from the file, if present. If the file // is readable but contains no shebang line, it will return an empty string. // Non-nil errors only reflect an inability to read the file. From d8b5c36d9c5428534f1b104aa187ed4151f635bc Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Thu, 27 Apr 2023 16:36:38 +1000 Subject: [PATCH 02/11] Add tests for hook type detection --- bootstrap/bootstrap.go | 31 ++----- hook/binary.go | 75 +++++++++++++++++ hook/type.go | 54 ++++++++++++ hook/type_test.go | 130 +++++++++++++++++++++++++++++ scripts/generate-test-binaries.sh | 12 +++ shellscript/shellscript.go | 73 ---------------- test/fixtures/hook/hook.rb | 4 + test/fixtures/hook/main.go | 6 ++ test/fixtures/hook/shebanged.sh | 4 + test/fixtures/hook/un-shebanged.sh | 3 + 10 files changed, 294 insertions(+), 98 deletions(-) create mode 100644 hook/binary.go create mode 100644 hook/type.go create mode 100644 hook/type_test.go create mode 100755 scripts/generate-test-binaries.sh create mode 100644 test/fixtures/hook/hook.rb create mode 100644 test/fixtures/hook/main.go create mode 100644 test/fixtures/hook/shebanged.sh create mode 100644 test/fixtures/hook/un-shebanged.sh diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index 184708d3b5..8a77616a30 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -297,39 +297,20 @@ func (b *Bootstrap) executeHook(ctx context.Context, hookCfg HookConfig) error { b.shell.Headerf("Running %s hook", hookName) - isBinary, err := shellscript.IsBinaryExecutable(hookCfg.Path) + hookType, err := hook.Type(hookCfg.Path) if err != nil { - return fmt.Errorf("checking if %s hook is a binary: %w", hookName, err) + return fmt.Errorf("determining hook type for %q hook: %w", hookName, err) } - shebangLine, err := shellscript.ShebangLine(hookCfg.Path) - if err != nil { - return fmt.Errorf("reading shebang line for %s hook: %w", hookName, err) - } - - switch { - case isBinary: + switch hookType { + case hook.TypeBinary, hook.TypeScript: // It's a binary, so we'll just run it directly, no wrapping needed or possible return b.runUnwrappedHook(ctx, hookName, hookCfg) - case shellscript.IsPOSIXShell(shebangLine): + case hook.TypeShell: // It's definitely a shell script, wrap it so that we can snaffle the changed environment variables return b.runWrappedShellScriptHook(ctx, hookName, hookCfg) - case shebangLine != "" && !shellscript.IsPOSIXShell(shebangLine): - // It's an interpretable-something with a shebang line, and it almost certainly isn't a shell script, - // so it won't be capable of setting environment variables (outside of the job api), so just run it directly - return b.runUnwrappedHook(ctx, hookName, hookCfg) - case shebangLine == "": - // If there's no shebang line in the hook, and it's not a binary, so let's just assume that it's a shell script - // The OS will complain if we do something stupid, so we'll just give it a shot - return b.runWrappedShellScriptHook(ctx, hookName, hookCfg) default: - return fmt.Errorf(`the buildkite agent wasn't able to determine the type of hook it was trying to run. -This is a bug, please contact support@buildkite.com and/or submit an issue on the github.com/buildkite/agent repo with the following information: -Hook scope: %q -Hook path: %q -Hook type: %q -Shebang line: %q -Is binary?: %t`, hookCfg.Scope, hookCfg.Path, hookName, shebangLine, isBinary) + return fmt.Errorf("unknown hook type %q for %q hook", hookType, hookName) } } diff --git a/hook/binary.go b/hook/binary.go new file mode 100644 index 0000000000..eae7b325b9 --- /dev/null +++ b/hook/binary.go @@ -0,0 +1,75 @@ +package hook + +import ( + "bufio" + "bytes" + "fmt" + "os" +) + +// Magic numbers for various types of executable files +var ( + // Linux and many other Unix-like systems use ELF binaries + ELFMagic = []byte{0x7F, 0x45, 0x4C, 0x46} + + // Windows uses MS-DOS MZ-style executables. This includes PE32 and PE32+ (which is 64-bit 🙃) binaries, + // which are what modern windowses tend to use. + MZMagic = []byte{0x4D, 0x5A} + + // macOS uses Mach-O binaries. There are two variants: fat and thin. + // Thin binaries are further divided into 32-bit and 64-bit variants, + // which are furtherer subdivided by processor endianess. It's a mess. + + // For "fat" binaries, which contain multiple architectures + MachOFatMagic = []byte{0xCA, 0xFE, 0xBA, 0xBE} + + // For "thin" binaries, which contain a single architecture + // (ie 32 and 64 bit variants, on top of different processor architectures) + MachOThin32Magic = []byte{0xFE, 0xED, 0xFA, 0xCE} + MachOThin64Magic = []byte{0xFE, 0xED, 0xFA, 0xCF} + + // Mach-O thin binaries can also be in reverse byte order, apparently? + // I think this is for big endian processors like PowerPC, in which case we don't need these here, + // but we might as well include them for completeness. + MachOThin32ReverseMagic = []byte{0xCE, 0xFA, 0xED, 0xFE} + MachOThin64ReverseMagic = []byte{0xCF, 0xFA, 0xED, 0xFE} + + BinaryMagicks = [][]byte{ + ELFMagic, + MZMagic, + MachOFatMagic, + MachOThin32Magic, + MachOThin64Magic, + MachOThin32ReverseMagic, + MachOThin64ReverseMagic, + } +) + +// ExecutableType returns the type of file executable at the given path. +// The file at the given path is assumed to be an executable, and executability is not checked. +func isBinaryExecutable(path string) (bool, error) { + f, err := os.Open(path) + if err != nil { + return false, fmt.Errorf("open file %q: %w", path, err) + } + + defer f.Close() + r := bufio.NewReader(f) + firstFour, err := r.Peek(4) + if err != nil { + return false, fmt.Errorf("reading first four bytes of file %q: %w", path, err) + } + + if len(firstFour) < 4 { + // there are less than four bytes in the file, there's nothing that we can do with it + return false, nil + } + + for _, magicNumber := range BinaryMagicks { + if bytes.HasPrefix(firstFour, magicNumber) { + return true, nil + } + } + + return false, nil +} diff --git a/hook/type.go b/hook/type.go new file mode 100644 index 0000000000..60dc096a43 --- /dev/null +++ b/hook/type.go @@ -0,0 +1,54 @@ +package hook + +import ( + "fmt" + "os" + + "github.com/buildkite/agent/v3/shellscript" +) + +const ( + TypeShell = "shell" + TypeBinary = "binary" + TypeScript = "script" // Something with a shebang that isn't a shell script, but is an interpretable something-or-other + TypeUnknown = "unknown" +) + +func Type(path string) (string, error) { + _, err := os.Open(path) + if err != nil { + return TypeUnknown, fmt.Errorf("opening hook: %w", err) + } + + isBinary, err := isBinaryExecutable(path) + if err != nil { + return TypeUnknown, fmt.Errorf("determining if %q is a binary: %w", path, err) + } + + if isBinary { + return TypeBinary, nil + } + + shebangLine, err := shellscript.ShebangLine(path) + if err != nil { + return TypeUnknown, fmt.Errorf("reading shebang line for hook: %w", err) + } + + switch { + case shellscript.IsPOSIXShell(shebangLine): + return TypeShell, nil + + case shebangLine != "" && !shellscript.IsPOSIXShell(shebangLine): + return TypeScript, nil + + case shebangLine == "": + return TypeShell, nil // Assume shell if no shebang line is present and it's not a binary + + default: + return TypeUnknown, fmt.Errorf(`the buildkite agent wasn't able to determine the type of hook it was trying to run. +This is a bug, please contact support@buildkite.com and/or submit an issue on the github.com/buildkite/agent repo with the following information: +Hook path: %q +Shebang line: %q +Is binary?: %t`, path, shebangLine, isBinary) + } +} diff --git a/hook/type_test.go b/hook/type_test.go new file mode 100644 index 0000000000..2fcb5c8cfd --- /dev/null +++ b/hook/type_test.go @@ -0,0 +1,130 @@ +package hook_test + +import ( + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "testing" + + "github.com/buildkite/agent/v3/hook" +) + +type testCase struct { + name string + hookPath string + expectedHookType string + errCheck func(*testing.T, error) bool +} + +func noErr(t *testing.T, err error) bool { + if err != nil { + t.Errorf("unexpected error: %v", err) + return true + } + return false +} + +func isNotExist(t *testing.T, err error) bool { + if !errors.Is(err, os.ErrNotExist) { + t.Errorf("expected os.ErrNotExist, got %v", err) + return true + } + + return false +} + +func TestHookType(t *testing.T) { + // The test working dir is at $REPO_ROOT/hook, but the fixtures are in $REPO_ROOT/test/fixtures/hook, so we need to + // go up a directory to get to the root + wd, _ := os.Getwd() + root := filepath.Join(wd, "..") + + cases := []testCase{ + { + name: "non-shell script with shebang", + hookPath: hookFixture(root, "hook.rb"), + expectedHookType: hook.TypeScript, + errCheck: noErr, + }, + { + name: "shell script with shebang", + hookPath: hookFixture(root, "shebanged.sh"), + expectedHookType: hook.TypeShell, + errCheck: noErr, + }, + { + name: "shell script without shebang", + hookPath: hookFixture(root, "un-shebanged.sh"), + expectedHookType: hook.TypeShell, + errCheck: noErr, + }, + { + name: "non-existent hook", + hookPath: hookFixture(root, "some", "path", "that", "doesnt", "exist"), + expectedHookType: hook.TypeUnknown, + errCheck: isNotExist, + }, + } + + for _, operatingSystem := range []string{"linux", "darwin", "windows"} { + for _, arch := range []string{"amd64", "arm64"} { + + binaryName := fmt.Sprintf("test-binary-%s-%s", operatingSystem, arch) + binaryPath := hookFixture(root, binaryName) + sourcePath := hookFixture(root) + + fmt.Println(binaryPath) + + cmd := exec.Command("go", "build", "-o", binaryPath, sourcePath) + extraEnv := []string{ + fmt.Sprintf("GOOS=%s", operatingSystem), + fmt.Sprintf("GOARCH=%s", arch), + "CGO_ENABLED=0", + } + + cmd.Env = append(os.Environ(), extraEnv...) + + output, err := cmd.CombinedOutput() + fmt.Println(string(output)) + if err != nil { + t.Fatalf("Failed to build test-binary-hook: %v, output: %s", err, string(output)) + } + + t.Cleanup(func() { + err = os.Remove(binaryPath) + if err != nil { + t.Fatalf("Failed to remove test-binary-hook: %v", err) + } + }) + + cases = append(cases, testCase{ + name: fmt.Sprintf("binary for %s/%s", operatingSystem, arch), + hookPath: binaryPath, + expectedHookType: hook.TypeBinary, + errCheck: noErr, + }) + } + } + + for _, c := range cases { + c := c + t.Run(c.name, func(t *testing.T) { + t.Parallel() + + hookType, err := hook.Type(c.hookPath) + if c.errCheck(t, err) { + t.Fatalf("error check failed") + } + + if hookType != c.expectedHookType { + t.Fatalf("Expected hook type %q, got %q", c.expectedHookType, hookType) + } + }) + } +} + +func hookFixture(wd string, fixturePath ...string) string { + return filepath.Join(append([]string{wd, "test", "fixtures", "hook"}, fixturePath...)...) +} diff --git a/scripts/generate-test-binaries.sh b/scripts/generate-test-binaries.sh new file mode 100755 index 0000000000..b8ccb08141 --- /dev/null +++ b/scripts/generate-test-binaries.sh @@ -0,0 +1,12 @@ +#!/usr/bin/env bash + +indir=$1 +outdir=$2 +binary_name=$3 + +for os in "darwin" "linux" "windows"; do + for arch in "amd64" "arm64"; do + echo "Building test binary for $os/$arch" + GOOS=$os GOARCH=$arch go build -o "$outdir/$binary_name-$os-$arch" "$indir" + done +done diff --git a/shellscript/shellscript.go b/shellscript/shellscript.go index ef528c824f..1ea9d5ab92 100644 --- a/shellscript/shellscript.go +++ b/shellscript/shellscript.go @@ -3,8 +3,6 @@ package shellscript import ( "bufio" - "bytes" - "fmt" "os" "path/filepath" "strings" @@ -12,77 +10,6 @@ import ( "github.com/buildkite/shellwords" ) -// Magic numbers for various types of executable files -var ( - // Linux and many other Unix-like systems use ELF binaries - ELFMagic = []byte{0x7F, 0x45, 0x4C, 0x46} - - // Windows uses MS-DOS MZ-style executables. This includes PE32 and PE32+ (which is 64-bit 🙃) binaries, - // which a are what modern windowses tend to use. - MZMagic = []byte{0x4D, 0x5A} - - // macOS uses Mach-O binaries. There are two variants: fat and thin. - // Thin binaries are further divided into 32-bit and 64-bit variants, - // which are furtherer subdivided by processor endianess. It's a mess. - - // For "fat" binaries, which contain multiple architectures - MachOFatMagic = []byte{0xCA, 0xFE, 0xBA, 0xBE} - - // For "thin" binaries, which contain a single architecture - // (ie 32 and 64 bit variants, on top of different processor architectures) - MachOThin32Magic = []byte{0xFE, 0xED, 0xFA, 0xCE} - MachOThin64Magic = []byte{0xFE, 0xED, 0xFA, 0xCF} - - // Mach-O thin binaries can also be in reverse byte order, apparently? - // I think this is for big endian processors like PowerPC, in which case we don't need these here, - // but we might as well include them for completeness. - MachOThin32ReverseMagic = []byte{0xCE, 0xFA, 0xED, 0xFE} - MachOThin64ReverseMagic = []byte{0xCF, 0xFA, 0xED, 0xFE} - - BinaryMagicks = [][]byte{ - ELFMagic, - MZMagic, - MachOFatMagic, - MachOThin32Magic, - MachOThin64Magic, - MachOThin32ReverseMagic, - MachOThin64ReverseMagic, - } -) - -// ExecutableType returns the type of file executable at the given path. -// The file at the given path is assumed to be an executable, and executability is not checked. -func IsBinaryExecutable(path string) (bool, error) { - f, err := os.Open(path) - if err != nil { - return false, fmt.Errorf("open file %q: %w", path, err) - } - - defer f.Close() - r := bufio.NewReader(f) - firstFour, err := r.Peek(4) - if err != nil { - return false, fmt.Errorf("reading first four bytes of file %q: %w", path, err) - } - - if len(firstFour) < 4 { - // there are less than four bytes in the file, there's nothing that we can do with it - return false, nil - } - - for _, magicNumber := range BinaryMagicks { - if sliceHasPrefix(firstFour, magicNumber) { - return true, nil - } - } - - return false, nil -} - -func sliceHasPrefix(s, prefix []byte) bool { - return len(s) >= len(prefix) && bytes.Equal(s[:len(prefix)], prefix) -} - // ShebangLine extracts the shebang line from the file, if present. If the file // is readable but contains no shebang line, it will return an empty string. // Non-nil errors only reflect an inability to read the file. diff --git a/test/fixtures/hook/hook.rb b/test/fixtures/hook/hook.rb new file mode 100644 index 0000000000..cf7197e6df --- /dev/null +++ b/test/fixtures/hook/hook.rb @@ -0,0 +1,4 @@ +#!/usr/bin/env ruby + +puts "hi there world" +puts "this script isn't meant to be run, only checked for its signature" diff --git a/test/fixtures/hook/main.go b/test/fixtures/hook/main.go new file mode 100644 index 0000000000..a8c11ac622 --- /dev/null +++ b/test/fixtures/hook/main.go @@ -0,0 +1,6 @@ +package main + +func main() { + println("Hello, world!") + println("This file is not meant to be run, just generate a couple of binaries") +} diff --git a/test/fixtures/hook/shebanged.sh b/test/fixtures/hook/shebanged.sh new file mode 100644 index 0000000000..eb7a980857 --- /dev/null +++ b/test/fixtures/hook/shebanged.sh @@ -0,0 +1,4 @@ +#!/usr/bin/env bash + +echo "Hello, world!" +echo "This is a shebanged script." diff --git a/test/fixtures/hook/un-shebanged.sh b/test/fixtures/hook/un-shebanged.sh new file mode 100644 index 0000000000..01972a5c59 --- /dev/null +++ b/test/fixtures/hook/un-shebanged.sh @@ -0,0 +1,3 @@ +# shellcheck disable=all +echo "Hello world!" +echo "This is an un-shebanged script." From af4656fe5b84fcc6241705c858d1f9518f3880a2 Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Mon, 1 May 2023 15:36:57 +1000 Subject: [PATCH 03/11] Separate structs used by PATCH /env client lib and server The server needs to be able to handle JSON nulls as values, so it should use a map[string]*string, but the client library should just not allow anyone to send nulls, so it should use a map[string]string. This has the side-benefit of making the DX of actually using the library much better --- bootstrap/integration/job_api_integration_test.go | 2 +- clicommand/env_set.go | 4 ++-- jobapi/client_test.go | 9 ++++----- jobapi/payloads.go | 5 +++++ jobapi/routes.go | 2 +- jobapi/server_test.go | 8 ++++---- 6 files changed, 17 insertions(+), 13 deletions(-) diff --git a/bootstrap/integration/job_api_integration_test.go b/bootstrap/integration/job_api_integration_test.go index e419ffa728..554e06447d 100644 --- a/bootstrap/integration/job_api_integration_test.go +++ b/bootstrap/integration/job_api_integration_test.go @@ -87,7 +87,7 @@ func TestBootstrapRunsJobAPI(t *testing.T) { } mtn := "chimborazo" - b, err := json.Marshal(jobapi.EnvUpdateRequest{Env: map[string]*string{"MOUNTAIN": &mtn}}) + b, err := json.Marshal(jobapi.EnvUpdateRequest{Env: map[string]string{"MOUNTAIN": mtn}}) if err != nil { t.Errorf("marshaling env update request: %v", err) c.Exit(1) diff --git a/clicommand/env_set.go b/clicommand/env_set.go index d2f559b2bf..4c12fa0c23 100644 --- a/clicommand/env_set.go +++ b/clicommand/env_set.go @@ -72,7 +72,7 @@ func envSetAction(c *cli.Context) error { } req := &jobapi.EnvUpdateRequest{ - Env: make(map[string]*string), + Env: make(map[string]string), } var parse func(string) error @@ -84,7 +84,7 @@ func envSetAction(c *cli.Context) error { if !ok { return fmt.Errorf("%q is not in key=value format", input) } - req.Env[e] = &v + req.Env[e] = v return nil } diff --git a/jobapi/client_test.go b/jobapi/client_test.go index 7106022122..730aaa5075 100644 --- a/jobapi/client_test.go +++ b/jobapi/client_test.go @@ -64,7 +64,7 @@ func (f *fakeServer) ServeHTTP(w http.ResponseWriter, r *http.Request) { } case "PATCH": - var req EnvUpdateRequest + var req EnvUpdateRequestPayload var resp EnvUpdateResponse if err := json.NewDecoder(r.Body).Decode(&req); err != nil { socket.WriteError(w, fmt.Sprintf("decoding request: %v", err), http.StatusBadRequest) @@ -181,11 +181,10 @@ func TestClientEnvUpdate(t *testing.T) { t.Fatalf("NewClient(%q, %q) error = %v", svr.sock, svr.token, err) } - sp := func(s string) *string { return &s } req := &EnvUpdateRequest{ - Env: map[string]*string{ - "PACHA": sp("Friend"), - "YZMA": sp("Kitten"), + Env: map[string]string{ + "PACHA": "Friend", + "YZMA": "Kitten", }, } diff --git a/jobapi/payloads.go b/jobapi/payloads.go index f3a0d9ac62..c6e51bb25e 100644 --- a/jobapi/payloads.go +++ b/jobapi/payloads.go @@ -16,6 +16,11 @@ type EnvGetResponse struct { // EnvUpdateRequest is the request body for the PATCH /env endpoint type EnvUpdateRequest struct { + Env map[string]string `json:"env"` +} + +// EnvUpdateRequestPayload is the request body that the PATCH /env endpoint unmarshalls requests into +type EnvUpdateRequestPayload struct { Env map[string]*string `json:"env"` } diff --git a/jobapi/routes.go b/jobapi/routes.go index ddc3f2e6df..80d21d9956 100644 --- a/jobapi/routes.go +++ b/jobapi/routes.go @@ -45,7 +45,7 @@ func (s *Server) getEnv(w http.ResponseWriter, _ *http.Request) { } func (s *Server) patchEnv(w http.ResponseWriter, r *http.Request) { - var req EnvUpdateRequest + var req EnvUpdateRequestPayload err := json.NewDecoder(r.Body).Decode(&req) defer r.Body.Close() if err != nil { diff --git a/jobapi/server_test.go b/jobapi/server_test.go index 7c8f0c47a9..49f1fc5ad1 100644 --- a/jobapi/server_test.go +++ b/jobapi/server_test.go @@ -213,10 +213,10 @@ func TestDeleteEnv(t *testing.T) { func TestPatchEnv(t *testing.T) { t.Parallel() - cases := []apiTestCase[jobapi.EnvUpdateRequest, jobapi.EnvUpdateResponse]{ + cases := []apiTestCase[jobapi.EnvUpdateRequestPayload, jobapi.EnvUpdateResponse]{ { name: "happy case", - requestBody: &jobapi.EnvUpdateRequest{ + requestBody: &jobapi.EnvUpdateRequestPayload{ Env: map[string]*string{ "MOUNTAIN": pt("chimborazo"), "CAPITAL": pt("quito"), @@ -236,7 +236,7 @@ func TestPatchEnv(t *testing.T) { }, { name: "setting to nil returns a 422", - requestBody: &jobapi.EnvUpdateRequest{ + requestBody: &jobapi.EnvUpdateRequestPayload{ Env: map[string]*string{ "NATIONAL_PARKS": nil, "MOUNTAIN": pt("chimborazo"), @@ -250,7 +250,7 @@ func TestPatchEnv(t *testing.T) { }, { name: "setting protected variables returns a 422", - requestBody: &jobapi.EnvUpdateRequest{ + requestBody: &jobapi.EnvUpdateRequestPayload{ Env: map[string]*string{ "BUILDKITE_AGENT_PID": pt("12345"), "MOUNTAIN": pt("antisana"), From 8879ad555a4a01b0419772af220e1a732f19257a Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Wed, 3 May 2023 14:58:11 +1000 Subject: [PATCH 04/11] Add WithUndo to experiments package --- .../integration/checkout_integration_test.go | 18 +++--------------- .../integration/job_api_integration_test.go | 2 +- experiments/experiments.go | 5 +++++ 3 files changed, 9 insertions(+), 16 deletions(-) diff --git a/bootstrap/integration/checkout_integration_test.go b/bootstrap/integration/checkout_integration_test.go index 96e916c941..7518f36d81 100644 --- a/bootstrap/integration/checkout_integration_test.go +++ b/bootstrap/integration/checkout_integration_test.go @@ -26,21 +26,9 @@ var commitPattern = bintest.MatchPattern(`(?ms)\Acommit [0-9a-f]+\nabbrev-commit // We expect this arg multiple times, just define it once. var gitShowFormatArg = "--format=commit %H%nabbrev-commit %h%nAuthor: %an <%ae>%n%n%w(0,4,4)%B" -// Enable an experiment, returning a function to restore the previous state. -// Usage: defer experimentWithUndo("foo")() -func experimentWithUndo(name string) func() { - prev := experiments.IsEnabled(name) - experiments.Enable(name) - return func() { - if !prev { - experiments.Disable(name) - } - } -} - func TestCheckingOutGitHubPullRequestsWithGitMirrorsExperiment(t *testing.T) { // t.Parallel() cannot be used with experiments.Enable() - defer experimentWithUndo(experiments.GitMirrors)() + defer experiments.EnableWithUndo(experiments.GitMirrors)() tester, err := NewBootstrapTester() if err != nil { @@ -81,7 +69,7 @@ func TestCheckingOutGitHubPullRequestsWithGitMirrorsExperiment(t *testing.T) { func TestWithResolvingCommitExperiment(t *testing.T) { // t.Parallel() cannot be used with experiments.Enable() - defer experimentWithUndo(experiments.ResolveCommitAfterCheckout)() + defer experiments.EnableWithUndo(experiments.ResolveCommitAfterCheckout)() tester, err := NewBootstrapTester() if err != nil { @@ -691,7 +679,7 @@ func TestRepositorylessCheckout(t *testing.T) { func TestGitMirrorEnv(t *testing.T) { // t.Parallel() cannot test experiment flags in parallel - defer experimentWithUndo(experiments.GitMirrors)() + defer experiments.EnableWithUndo(experiments.GitMirrors)() tester, err := NewBootstrapTester() if err != nil { diff --git a/bootstrap/integration/job_api_integration_test.go b/bootstrap/integration/job_api_integration_test.go index 554e06447d..6bab7fbe74 100644 --- a/bootstrap/integration/job_api_integration_test.go +++ b/bootstrap/integration/job_api_integration_test.go @@ -16,7 +16,7 @@ import ( ) func TestBootstrapRunsJobAPI(t *testing.T) { - defer experimentWithUndo(experiments.JobAPI)() + defer experiments.EnableWithUndo(experiments.JobAPI)() tester, err := NewBootstrapTester() if err != nil { diff --git a/experiments/experiments.go b/experiments/experiments.go index 05ccd389cf..351b619547 100644 --- a/experiments/experiments.go +++ b/experiments/experiments.go @@ -34,6 +34,11 @@ var ( experiments = make(map[string]bool, len(Available)) ) +func EnableWithUndo(key string) func() { + Enable(key) + return func() { Disable(key) } +} + // Enable a particular experiment in the agent. func Enable(key string) (known bool) { experiments[key] = true From fa98c76a7d53589887489dc30d8cf7e9df6110ad Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Wed, 3 May 2023 15:50:33 +1000 Subject: [PATCH 05/11] Add polyglot hook integration tests --- .buildkite/Dockerfile-compile | 3 + .../integration/hooks_integration_test.go | 74 +++++++++++++++++++ .../integration/test-binary-hook/main.go | 31 ++++++++ 3 files changed, 108 insertions(+) create mode 100644 bootstrap/integration/test-binary-hook/main.go diff --git a/.buildkite/Dockerfile-compile b/.buildkite/Dockerfile-compile index 580674964d..3b6b06406a 100644 --- a/.buildkite/Dockerfile-compile +++ b/.buildkite/Dockerfile-compile @@ -1,3 +1,6 @@ FROM golang:1.20.4@sha256:6876eff5b20336c5c2896b0c3055f3258bdeba7aa38bbdabcb5a4abb5cdd39c7 COPY build/ssh.conf /etc/ssh/ssh_config.d/ RUN go install github.com/google/go-licenses@latest + +# Ruby used for polyglot hook integration tests +RUN apt update && apt install -y ruby diff --git a/bootstrap/integration/hooks_integration_test.go b/bootstrap/integration/hooks_integration_test.go index 119aa872d4..f918036541 100644 --- a/bootstrap/integration/hooks_integration_test.go +++ b/bootstrap/integration/hooks_integration_test.go @@ -3,6 +3,7 @@ package integration import ( "fmt" "os" + "os/exec" "path/filepath" "runtime" "strings" @@ -488,3 +489,76 @@ func TestPreExitHooksFireAfterCancel(t *testing.T) { tester.CheckMocks(t) } + +func TestPolyglotScriptHooksCanBeRun(t *testing.T) { + path, err := exec.LookPath("ruby") + if err != nil { + t.Fatal("error finding path to ruby executable: %w", err) + } + + if path == "" { + t.Fatal("ruby not found in $PATH. This test requires ruby to be installed on the host") + } + + defer experiments.WithUndo(experiments.PolyglotHooks)() + + tester, err := NewBootstrapTester() + if err != nil { + t.Fatalf("NewBootstrapTester() error = %v", err) + } + defer tester.Close() + + filename := "environment" + script := []string{ + "#!/usr/bin/env ruby", + `puts "ohai, it's ruby!"`, + } + + if err := os.WriteFile(filepath.Join(tester.HooksDir, filename), []byte(strings.Join(script, "\n")), 0755); err != nil { + t.Fatalf("os.WriteFile(%q, script, 0755 = %v", filename, err) + } + + tester.RunAndCheck(t) + + if !strings.Contains(tester.Output, "ohai, it's ruby!") { + t.Fatalf("tester.Output %s does not contain expected output: %q", tester.Output, "ohai, it's ruby!") + } +} + +func TestPolyglotBinaryHooksCanBeRun(t *testing.T) { + defer experiments.WithUndo(experiments.PolyglotHooks)() + defer experiments.WithUndo(experiments.JobAPI)() + + tester, err := NewBootstrapTester() + if err != nil { + t.Fatalf("NewBootstrapTester() error = %v", err) + } + defer tester.Close() + + // We build a binary as part of this test so that we can produce a binary hook + // Forgive me for my sins, RSC, but it's better than the alternatives. + // The code that we're building is in ./test-binary-hook/main.go, it's pretty straightforward. + + fmt.Println("Building test-binary-hook") + hookPath := filepath.Join(tester.HooksDir, "environment") + output, err := exec.Command("go", "build", "-o", hookPath, "./test-binary-hook").CombinedOutput() + if err != nil { + t.Fatalf("Failed to build test-binary-hook: %v, output: %s", err, string(output)) + } + + tester.ExpectGlobalHook("post-command").Once().AndExitWith(0).AndCallFunc(func(c *bintest.Call) { + // Set via ./test-binary-hook/main.go + if c.GetEnv("OCEAN") != "Pacífico" { + fmt.Fprintf(c.Stderr, "Expected OCEAN to be Pacífico, got %q", c.GetEnv("OCEAN")) + c.Exit(1) + } else { + c.Exit(0) + } + }) + + tester.RunAndCheck(t) + + if !strings.Contains(tester.Output, "hi there from golang 🌊") { + t.Fatalf("tester.Output %s does not contain expected output: %q", tester.Output, "hi there from golang 🌊") + } +} diff --git a/bootstrap/integration/test-binary-hook/main.go b/bootstrap/integration/test-binary-hook/main.go new file mode 100644 index 0000000000..7eaf02b7b9 --- /dev/null +++ b/bootstrap/integration/test-binary-hook/main.go @@ -0,0 +1,31 @@ +package main + +import ( + "context" + "fmt" + "log" + + "github.com/buildkite/agent/v3/jobapi" +) + +// This file gets built and commited to this repo, then used as part of the hooks integration test to ensure that the +// bootstrap can run binary hooks +func main() { + c, err := jobapi.NewDefaultClient(context.TODO()) + if err != nil { + log.Fatalf("error: %v", fmt.Errorf("creating job api client: %w", err)) + } + + _, err = c.EnvUpdate(context.TODO(), &jobapi.EnvUpdateRequest{ + Env: map[string]string{ + "OCEAN": "Pacífico", + "MOUNTAIN": "chimborazo", + }, + }) + + if err != nil { + log.Fatalf("error: %v", fmt.Errorf("updating env: %w", err)) + } + + fmt.Println("hi there from golang 🌊") +} From 775cb16dfb9e1d1737aecb56c2fbd88383be8db2 Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Wed, 3 May 2023 19:48:57 +1000 Subject: [PATCH 06/11] Wrap polyglot hooks in an experiment --- bootstrap/bootstrap.go | 4 ++++ bootstrap/integration/hooks_integration_test.go | 7 ++++--- experiments/experiments.go | 2 ++ 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index 8a77616a30..59da2aa123 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -297,6 +297,10 @@ func (b *Bootstrap) executeHook(ctx context.Context, hookCfg HookConfig) error { b.shell.Headerf("Running %s hook", hookName) + if !experiments.IsEnabled(experiments.PolyglotHooks) { + return b.runWrappedShellScriptHook(ctx, hookName, hookCfg) + } + hookType, err := hook.Type(hookCfg.Path) if err != nil { return fmt.Errorf("determining hook type for %q hook: %w", hookName, err) diff --git a/bootstrap/integration/hooks_integration_test.go b/bootstrap/integration/hooks_integration_test.go index f918036541..18b2180ed9 100644 --- a/bootstrap/integration/hooks_integration_test.go +++ b/bootstrap/integration/hooks_integration_test.go @@ -12,6 +12,7 @@ import ( "time" "github.com/buildkite/agent/v3/bootstrap/shell" + "github.com/buildkite/agent/v3/experiments" "github.com/buildkite/bintest/v3" ) @@ -500,7 +501,7 @@ func TestPolyglotScriptHooksCanBeRun(t *testing.T) { t.Fatal("ruby not found in $PATH. This test requires ruby to be installed on the host") } - defer experiments.WithUndo(experiments.PolyglotHooks)() + defer experiments.EnableWithUndo(experiments.PolyglotHooks)() tester, err := NewBootstrapTester() if err != nil { @@ -526,8 +527,8 @@ func TestPolyglotScriptHooksCanBeRun(t *testing.T) { } func TestPolyglotBinaryHooksCanBeRun(t *testing.T) { - defer experiments.WithUndo(experiments.PolyglotHooks)() - defer experiments.WithUndo(experiments.JobAPI)() + defer experiments.EnableWithUndo(experiments.PolyglotHooks)() + defer experiments.EnableWithUndo(experiments.JobAPI)() tester, err := NewBootstrapTester() if err != nil { diff --git a/experiments/experiments.go b/experiments/experiments.go index 351b619547..50bf98e102 100644 --- a/experiments/experiments.go +++ b/experiments/experiments.go @@ -5,6 +5,7 @@ package experiments const ( + PolyglotHooks = "polyglot-hooks" JobAPI = "job-api" KubernetesExec = "kubernetes-exec" ANSITimestamps = "ansi-timestamps" @@ -19,6 +20,7 @@ const ( var ( Available = map[string]struct{}{ + PolyglotHooks: {}, JobAPI: {}, KubernetesExec: {}, ANSITimestamps: {}, From 6634945ce18875ad3ec26271847ca4b998cc5669 Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Thu, 18 May 2023 12:41:12 +1000 Subject: [PATCH 07/11] Disable script hooks on windows --- bootstrap/bootstrap.go | 40 +++++++++++++++++-- .../integration/hooks_integration_test.go | 4 ++ 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index 59da2aa123..c4adcf8019 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "regexp" + "runtime" "strconv" "strings" "time" @@ -307,12 +308,45 @@ func (b *Bootstrap) executeHook(ctx context.Context, hookCfg HookConfig) error { } switch hookType { - case hook.TypeBinary, hook.TypeScript: + case hook.TypeScript: + if runtime.GOOS == "windows" { + // We use shebangs to figure out how to run scripts, and Windows has no way to interpret a shebang + // ie, on linux, we can just point the OS to a file of some sort and say "run that", and as part of that it will try to + // read a shebang, and run the script using the interpreter specified. Windows can't do this, so we can't run scripts + // directly on Windows + + // Potentially there's something that we could do with file extensions, but that's a bit of a minefield, and would + // probably mean that we have to have a list of blessed hook runtimes on windows only... or something. + + // Regardless: not supported right now, or potentially ever. + sheb, _ := shellscript.ShebangLine(hookCfg.Path) // we know this won't error because it must have a shebang to be a script + + err := fmt.Sprintf(`when trying to run the hook at %q, the agent found that it was a script with a shebang that isn't for a shellscripting language - in this case, %q. +Hooks of this kind are unfortunately not supported on Windows, as we have no way of interpreting a shebang on Windows`, hookCfg.Path, sheb) + return fmt.Errorf(err) + } + + // It's a script, and we can rely on the OS to figure out how to run it (because we're not on windows), so run it + // directly without wrapping + if err := b.runUnwrappedHook(ctx, hookName, hookCfg); err != nil { + return fmt.Errorf("running %q script hook: %w", hookName, err) + } + + return nil + case hook.TypeBinary: // It's a binary, so we'll just run it directly, no wrapping needed or possible - return b.runUnwrappedHook(ctx, hookName, hookCfg) + if err := b.runUnwrappedHook(ctx, hookName, hookCfg); err != nil { + return fmt.Errorf("running %q binary hook: %w", hookName, err) + } + + return nil case hook.TypeShell: // It's definitely a shell script, wrap it so that we can snaffle the changed environment variables - return b.runWrappedShellScriptHook(ctx, hookName, hookCfg) + if err := b.runWrappedShellScriptHook(ctx, hookName, hookCfg); err != nil { + return fmt.Errorf("running %q shell hook: %w", hookName, err) + } + + return nil default: return fmt.Errorf("unknown hook type %q for %q hook", hookType, hookName) } diff --git a/bootstrap/integration/hooks_integration_test.go b/bootstrap/integration/hooks_integration_test.go index 18b2180ed9..c32f20c2bd 100644 --- a/bootstrap/integration/hooks_integration_test.go +++ b/bootstrap/integration/hooks_integration_test.go @@ -492,6 +492,10 @@ func TestPreExitHooksFireAfterCancel(t *testing.T) { } func TestPolyglotScriptHooksCanBeRun(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("script hooks aren't supported on windows") + } + path, err := exec.LookPath("ruby") if err != nil { t.Fatal("error finding path to ruby executable: %w", err) From aee1191cf9ca50335d6ef1b984563ca1a7a32e50 Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Thu, 18 May 2023 12:43:56 +1000 Subject: [PATCH 08/11] Executable hooks on windows probably have to be .exe, ay? --- bootstrap/integration/hooks_integration_test.go | 5 +++++ hook/hook.go | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/bootstrap/integration/hooks_integration_test.go b/bootstrap/integration/hooks_integration_test.go index c32f20c2bd..4acf69f34a 100644 --- a/bootstrap/integration/hooks_integration_test.go +++ b/bootstrap/integration/hooks_integration_test.go @@ -546,6 +546,11 @@ func TestPolyglotBinaryHooksCanBeRun(t *testing.T) { fmt.Println("Building test-binary-hook") hookPath := filepath.Join(tester.HooksDir, "environment") + + if runtime.GOOS == "windows" { + hookPath += ".exe" + } + output, err := exec.Command("go", "build", "-o", hookPath, "./test-binary-hook").CombinedOutput() if err != nil { t.Fatalf("Failed to build test-binary-hook: %v, output: %s", err, string(output)) diff --git a/hook/hook.go b/hook/hook.go index d0c2283d57..e974ecc036 100644 --- a/hook/hook.go +++ b/hook/hook.go @@ -18,7 +18,7 @@ import ( func Find(hookDir string, name string) (string, error) { if runtime.GOOS == "windows" { // check for windows types first - if p, err := shell.LookPath(name, hookDir, ".BAT;.CMD;.PS1"); err == nil { + if p, err := shell.LookPath(name, hookDir, ".BAT;.CMD;.PS1;.EXE"); err == nil { return p, nil } } From 1a5f05029cc363a984188c441f938dd316dedd16 Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Fri, 19 May 2023 16:09:36 +1000 Subject: [PATCH 09/11] Ensure that the scriptwrapper won't try to run anything that's not a shell script --- hook/scriptwrapper.go | 9 +++++---- hook/scriptwrapper_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 4 deletions(-) diff --git a/hook/scriptwrapper.go b/hook/scriptwrapper.go index 667b390660..4c6bcfec03 100644 --- a/hook/scriptwrapper.go +++ b/hook/scriptwrapper.go @@ -144,10 +144,11 @@ func NewScriptWrapper(opts ...scriptWrapperOpt) (*ScriptWrapper, error) { // responsibility of the job executor. // // But if the shebang specifies something weird like Ruby 🤪 - // the wrapper won't work. Stick to POSIX shells for now. - // if shebang != "" && !shellscript.IsPOSIXShell(shebang) { - // return nil, fmt.Errorf("hook starts with an unsupported shebang line %q", shebang) - // } + // the wrapper won't work. We do support ruby (and other interpreted) hooks via polyglot hooks (see: https://github.com/buildkite/agent/pull/2040), + // but they should never be wrapped, and if they have been, something has gone wrong. + if shebang != "" && !shellscript.IsPOSIXShell(shebang) { + return nil, fmt.Errorf("scriptwrapper tried to wrap hook with invalid shebang: %q", shebang) + } var isPOSIXHook, isPwshHook bool diff --git a/hook/scriptwrapper_test.go b/hook/scriptwrapper_test.go index 0dd2dcc1e0..7550222480 100644 --- a/hook/scriptwrapper_test.go +++ b/hook/scriptwrapper_test.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "github.com/buildkite/agent/v3/bootstrap/shell" @@ -332,3 +333,26 @@ func mockAgent() (*bintest.Mock, func(), error) { return agent, cleanup, nil } + +func TestScriptWrapperFailsOnHookWithInvalidShebang(t *testing.T) { + t.Parallel() + + hookFile, err := shell.TempFileWithExtension("hookName") + assert.NoError(t, err) + + script := strings.Join([]string{ + "#!/usr/bin/env ruby", + "puts 'Hello There!'", + }, "\n") + + _, err = fmt.Fprintln(hookFile, script) + assert.NoError(t, err) + + hookFile.Close() + + _, err = NewScriptWrapper( + WithHookPath(hookFile.Name()), + WithOS("linux"), + ) + assert.Error(t, err, `scriptwrapper tried to wrap hook with invalid shebang: "#!/usr/bin/env ruby"`) +} From 251b72f9f5edfd5fd4a927b6b819c5bd8a10b245 Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Fri, 19 May 2023 17:27:26 +1000 Subject: [PATCH 10/11] Put hook type detection test binaries in the temp dir --- hook/type_test.go | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/hook/type_test.go b/hook/type_test.go index 2fcb5c8cfd..c2fb5fec8d 100644 --- a/hook/type_test.go +++ b/hook/type_test.go @@ -72,11 +72,9 @@ func TestHookType(t *testing.T) { for _, arch := range []string{"amd64", "arm64"} { binaryName := fmt.Sprintf("test-binary-%s-%s", operatingSystem, arch) - binaryPath := hookFixture(root, binaryName) + binaryPath := filepath.Join(os.TempDir(), binaryName) sourcePath := hookFixture(root) - fmt.Println(binaryPath) - cmd := exec.Command("go", "build", "-o", binaryPath, sourcePath) extraEnv := []string{ fmt.Sprintf("GOOS=%s", operatingSystem), @@ -87,18 +85,10 @@ func TestHookType(t *testing.T) { cmd.Env = append(os.Environ(), extraEnv...) output, err := cmd.CombinedOutput() - fmt.Println(string(output)) if err != nil { t.Fatalf("Failed to build test-binary-hook: %v, output: %s", err, string(output)) } - t.Cleanup(func() { - err = os.Remove(binaryPath) - if err != nil { - t.Fatalf("Failed to remove test-binary-hook: %v", err) - } - }) - cases = append(cases, testCase{ name: fmt.Sprintf("binary for %s/%s", operatingSystem, arch), hookPath: binaryPath, From 3971f6d3e9511f81550cc12e5547770c53ff18af Mon Sep 17 00:00:00 2001 From: Ben Moskovitz Date: Mon, 22 May 2023 13:02:35 +1000 Subject: [PATCH 11/11] Minor edits for clarity --- bootstrap/bootstrap.go | 4 ++-- bootstrap/integration/hooks_integration_test.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/bootstrap/bootstrap.go b/bootstrap/bootstrap.go index c4adcf8019..21e87bc25a 100644 --- a/bootstrap/bootstrap.go +++ b/bootstrap/bootstrap.go @@ -321,9 +321,9 @@ func (b *Bootstrap) executeHook(ctx context.Context, hookCfg HookConfig) error { // Regardless: not supported right now, or potentially ever. sheb, _ := shellscript.ShebangLine(hookCfg.Path) // we know this won't error because it must have a shebang to be a script - err := fmt.Sprintf(`when trying to run the hook at %q, the agent found that it was a script with a shebang that isn't for a shellscripting language - in this case, %q. + err := fmt.Errorf(`when trying to run the hook at %q, the agent found that it was a script with a shebang that isn't for a shellscripting language - in this case, %q. Hooks of this kind are unfortunately not supported on Windows, as we have no way of interpreting a shebang on Windows`, hookCfg.Path, sheb) - return fmt.Errorf(err) + return err } // It's a script, and we can rely on the OS to figure out how to run it (because we're not on windows), so run it diff --git a/bootstrap/integration/hooks_integration_test.go b/bootstrap/integration/hooks_integration_test.go index 4acf69f34a..eba98596fa 100644 --- a/bootstrap/integration/hooks_integration_test.go +++ b/bootstrap/integration/hooks_integration_test.go @@ -498,7 +498,7 @@ func TestPolyglotScriptHooksCanBeRun(t *testing.T) { path, err := exec.LookPath("ruby") if err != nil { - t.Fatal("error finding path to ruby executable: %w", err) + t.Fatalf("error finding path to ruby executable: %v", err) } if path == "" { @@ -520,13 +520,13 @@ func TestPolyglotScriptHooksCanBeRun(t *testing.T) { } if err := os.WriteFile(filepath.Join(tester.HooksDir, filename), []byte(strings.Join(script, "\n")), 0755); err != nil { - t.Fatalf("os.WriteFile(%q, script, 0755 = %v", filename, err) + t.Fatalf("os.WriteFile(%q, script, 0755) = %v", filename, err) } tester.RunAndCheck(t) if !strings.Contains(tester.Output, "ohai, it's ruby!") { - t.Fatalf("tester.Output %s does not contain expected output: %q", tester.Output, "ohai, it's ruby!") + t.Fatalf("tester.Output %q does not contain expected output: %q", tester.Output, "ohai, it's ruby!") } }