From 7e8ee96faddb317dec3ca97561393178dcf10c81 Mon Sep 17 00:00:00 2001 From: Josh Deprez Date: Thu, 13 Jun 2024 10:27:25 +1000 Subject: [PATCH 1/2] Redact the Job API token --- internal/job/api.go | 16 +++++++++++++ internal/redact/redact.go | 47 +++++++++++++++++++++++---------------- 2 files changed, 44 insertions(+), 19 deletions(-) diff --git a/internal/job/api.go b/internal/job/api.go index 108e058bf9..5431cb4dfc 100644 --- a/internal/job/api.go +++ b/internal/job/api.go @@ -3,6 +3,7 @@ package job import ( "fmt" + "github.com/buildkite/agent/v3/internal/redact" "github.com/buildkite/agent/v3/internal/socket" "github.com/buildkite/agent/v3/jobapi" ) @@ -37,6 +38,21 @@ We'll continue to run your job, but you won't be able to use the Job API`) e.shell.Env.Set("BUILDKITE_AGENT_JOB_API_SOCKET", socketPath) e.shell.Env.Set("BUILDKITE_AGENT_JOB_API_TOKEN", token) + if redact.Match(e.shell.Logger, e.RedactedVars, "BUILDKITE_AGENT_JOB_API_TOKEN") { + // The Job API token lets the job talk to this executor. When the job ends, + // the socket should be closed and the token becomes meaningless. Also, the + // socket should only be accessible to the user running the agent on the + // local host. + // So it shouldn't matter if the token is leaked in the logs - in order + // to make any use of it, someone would have to be on the same host as the + // same local user at the same time the job is running. + // However, it looks confusing when an environment variable that looks like + // an access token with a name ending in _TOKEN is *not* redacted. + // Conclusion: if the name matches, redact the Job API token. + // This depends on startJobAPI being called after setupRedactors. + e.redactors.Add(token) + } + if err := srv.Start(); err != nil { return cleanup, fmt.Errorf("starting Job API server: %w", err) } diff --git a/internal/redact/redact.go b/internal/redact/redact.go index 0c1fa258e7..59c596df23 100644 --- a/internal/redact/redact.go +++ b/internal/redact/redact.go @@ -24,6 +24,23 @@ func Redact([]byte) []byte { return redacted } +// Match reports if the name matches any of the patterns. +func Match(logger shell.Logger, patterns []string, name string) bool { + for _, pattern := range patterns { + matched, err := path.Match(pattern, name) + if err != nil { + // path.ErrBadPattern is the only error returned by path.Match + logger.Warningf("Bad redacted vars pattern: %s", pattern) + continue + } + + if matched { + return true + } + } + return false +} + // Values returns the variable Values to be redacted, given a // redaction config string and an environment map. func Values(logger shell.Logger, patterns []string, environment map[string]string) []string { @@ -43,32 +60,24 @@ func Values(logger shell.Logger, patterns []string, environment map[string]strin // Vars returns the variable names and values to be redacted, given a // redaction config string and an environment map. func Vars(logger shell.Logger, patterns []string, environment map[string]string) map[string]string { - // Lifted out of Bootstrap.setupRedactors to facilitate testing vars := make(map[string]string) shortVars := make(map[string]struct{}) for name, val := range environment { - for _, pattern := range patterns { - matched, err := path.Match(pattern, name) - if err != nil { - // path.ErrBadPattern is the only error returned by path.Match - logger.Warningf("Bad redacted vars pattern: %s", pattern) - continue - } + // Does the name match any of the patterns? + if !Match(logger, patterns, name) { + continue + } - if !matched { - continue - } - if len(val) < LengthMin { - if len(val) > 0 { - shortVars[name] = struct{}{} - } - continue + // The name matched, now test the length of the value. + if len(val) < LengthMin { + if len(val) > 0 { + shortVars[name] = struct{}{} } - - vars[name] = val - break // Break pattern loop, continue to next env var + continue } + + vars[name] = val } if len(shortVars) > 0 { From 4ef96756ab93dc1172426579003f502413b58231 Mon Sep 17 00:00:00 2001 From: Josh Deprez Date: Thu, 13 Jun 2024 10:27:37 +1000 Subject: [PATCH 2/2] Move redact tests to the redact package --- internal/job/executor_test.go | 39 ----------------------------- internal/redact/redact_test.go | 45 ++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 39 deletions(-) create mode 100644 internal/redact/redact_test.go diff --git a/internal/job/executor_test.go b/internal/job/executor_test.go index 85cddfdbe5..863bf03647 100644 --- a/internal/job/executor_test.go +++ b/internal/job/executor_test.go @@ -5,9 +5,7 @@ import ( "testing" "github.com/buildkite/agent/v3/internal/job/shell" - "github.com/buildkite/agent/v3/internal/redact" "github.com/buildkite/agent/v3/tracetools" - "github.com/google/go-cmp/cmp" "github.com/opentracing/opentracing-go" "github.com/stretchr/testify/assert" "gopkg.in/DataDog/dd-trace-go.v1/ddtrace/opentracer" @@ -46,43 +44,6 @@ func TestDirForRepository(t *testing.T) { } } -func TestValuesToRedact(t *testing.T) { - t.Parallel() - - redactConfig := []string{ - "*_PASSWORD", - "*_TOKEN", - } - environment := map[string]string{ - "BUILDKITE_PIPELINE": "unit-test", - // These are example values, and are not leaked credentials - "DATABASE_USERNAME": "AzureDiamond", - "DATABASE_PASSWORD": "hunter2", - } - - got := redact.Values(shell.DiscardLogger, redactConfig, environment) - want := []string{"hunter2"} - - if diff := cmp.Diff(got, want); diff != "" { - t.Errorf("redact.Values(%q, %q) diff (-got +want)\n%s", redactConfig, environment, diff) - } -} - -func TestValuesToRedactEmpty(t *testing.T) { - t.Parallel() - - redactConfig := []string{} - environment := map[string]string{ - "FOO": "BAR", - "BUILDKITE_PIPELINE": "unit-test", - } - - got := redact.Values(shell.DiscardLogger, redactConfig, environment) - if len(got) != 0 { - t.Errorf("redact.Values(%q, %q) = %q, want empty slice", redactConfig, environment, got) - } -} - func TestStartTracing_NoTracingBackend(t *testing.T) { var err error diff --git a/internal/redact/redact_test.go b/internal/redact/redact_test.go new file mode 100644 index 0000000000..ed4b027b59 --- /dev/null +++ b/internal/redact/redact_test.go @@ -0,0 +1,45 @@ +package redact + +import ( + "testing" + + "github.com/buildkite/agent/v3/internal/job/shell" + "github.com/google/go-cmp/cmp" +) + +func TestValuesToRedact(t *testing.T) { + t.Parallel() + + redactConfig := []string{ + "*_PASSWORD", + "*_TOKEN", + } + environment := map[string]string{ + "BUILDKITE_PIPELINE": "unit-test", + // These are example values, and are not leaked credentials + "DATABASE_USERNAME": "AzureDiamond", + "DATABASE_PASSWORD": "hunter2", + } + + got := Values(shell.DiscardLogger, redactConfig, environment) + want := []string{"hunter2"} + + if diff := cmp.Diff(got, want); diff != "" { + t.Errorf("Values(%q, %q) diff (-got +want)\n%s", redactConfig, environment, diff) + } +} + +func TestValuesToRedactEmpty(t *testing.T) { + t.Parallel() + + redactConfig := []string{} + environment := map[string]string{ + "FOO": "BAR", + "BUILDKITE_PIPELINE": "unit-test", + } + + got := Values(shell.DiscardLogger, redactConfig, environment) + if len(got) != 0 { + t.Errorf("Values(%q, %q) = %q, want empty slice", redactConfig, environment, got) + } +}