Merge pull request #2834 from buildkite/redact-jobapi-token

Redact Job API token like other env vars

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

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
 

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 {

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