Fix redaction leak from ssh_secret within checkout

clicommand/meta_data_set_batch.go

@@ -81,7 +81,8 @@ var MetaDataSetBatchCommand = cli.Command{
 			}
 		}
 
-		return setMetaDataBatch(ctx, cfg, l, items)
+		client := api.NewClient(l, loadAPIClientConfig(cfg, "AgentAccessToken"))
+		return setMetaDataBatch(ctx, client, cfg.Job, l, items)
 	},
 }
 
@@ -121,16 +122,14 @@ func parseMetaDataBatchArgs(args []string) ([]api.MetaData, error) {
 	return items, nil
 }
 
-func setMetaDataBatch(ctx context.Context, cfg MetaDataSetBatchConfig, l logger.Logger, items []api.MetaData) error {
-	client := api.NewClient(l, loadAPIClientConfig(cfg, "AgentAccessToken"))
-
+func setMetaDataBatch(ctx context.Context, client *api.Client, jobID string, l logger.Logger, items []api.MetaData) error {
 	batch := &api.MetaDataBatch{Items: items}
 
 	if err := roko.NewRetrier(
 		roko.WithMaxAttempts(10),
 		roko.WithStrategy(roko.ExponentialSubsecond(2*time.Second)),
 	).DoWithContext(ctx, func(r *roko.Retrier) error {
-		resp, err := client.SetMetaDataBatch(ctx, cfg.Job, batch)
+		resp, err := client.SetMetaDataBatch(ctx, jobID, batch)
 		if resp != nil && (resp.StatusCode == 401 || resp.StatusCode == 404) {
 			r.Break()
 		}

clicommand/meta_data_set_batch_test.go

@@ -4,8 +4,8 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"io"
 	"net/http"
-	"net/http/httptest"
 	"strings"
 	"testing"
 	"time"
@@ -90,14 +90,38 @@ func TestParseMetaDataBatchArgs(t *testing.T) {
 	}
 }
 
+type roundTripperFunc func(*http.Request) (*http.Response, error)
+
+func (f roundTripperFunc) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) }
+
+func stubAPIClient(l logger.Logger, rt roundTripperFunc) *api.Client {
+	return api.NewClient(l, api.Config{
+		Endpoint:   "http://api.stub",
+		Token:      "agentaccesstoken",
+		HTTPClient: &http.Client{Transport: rt},
+	})
+}
+
+func stubResponse(req *http.Request, status int) *http.Response {
+	return &http.Response{
+		StatusCode: status,
+		Status:     http.StatusText(status),
+		Body:       io.NopCloser(strings.NewReader("")),
+		Header:     make(http.Header),
+		Request:    req,
+	}
+}
+
 func TestSetMetaDataBatch(t *testing.T) {
 	t.Parallel()
 
 	t.Run("success", func(t *testing.T) {
 		t.Parallel()
 
+		l := logger.NewBuffer()
+
 		var receivedBatch api.MetaDataBatch
-		server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		client := stubAPIClient(l, func(req *http.Request) (*http.Response, error) {
 			if req.Method != "POST" {
 				t.Errorf("req.Method = %q, want %q", req.Method, "POST")
 			}
@@ -107,25 +131,15 @@ func TestSetMetaDataBatch(t *testing.T) {
 			if err := json.NewDecoder(req.Body).Decode(&receivedBatch); err != nil {
 				t.Errorf("decoding request body: %v", err)
 			}
-			rw.WriteHeader(http.StatusNoContent)
-		}))
-		defer server.Close()
-
-		cfg := MetaDataSetBatchConfig{
-			Job: "jobid",
-			APIConfig: APIConfig{
-				AgentAccessToken: "agentaccesstoken",
-				Endpoint:         server.URL,
-			},
-		}
+			return stubResponse(req, http.StatusNoContent), nil
+		})
 
 		items := []api.MetaData{
 			{Key: "foo", Value: "bar"},
 			{Key: "baz", Value: "qux"},
 		}
 
-		l := logger.NewBuffer()
-		if err := setMetaDataBatch(t.Context(), cfg, l, items); err != nil {
+		if err := setMetaDataBatch(t.Context(), client, "jobid", l, items); err != nil {
 			t.Fatalf("setMetaDataBatch error = %v, want nil", err)
 		}
 		if diff := cmp.Diff(receivedBatch.Items, items); diff != "" {
@@ -136,26 +150,17 @@ func TestSetMetaDataBatch(t *testing.T) {
 	t.Run("server error gives up when context is cancelled", func(t *testing.T) {
 		t.Parallel()
 
-		server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
-			rw.WriteHeader(http.StatusInternalServerError)
-		}))
-		defer server.Close()
-
-		cfg := MetaDataSetBatchConfig{
-			Job: "jobid",
-			APIConfig: APIConfig{
-				AgentAccessToken: "agentaccesstoken",
-				Endpoint:         server.URL,
-			},
-		}
+		l := logger.NewBuffer()
+		client := stubAPIClient(l, func(req *http.Request) (*http.Response, error) {
+			return stubResponse(req, http.StatusInternalServerError), nil
+		})
 
 		items := []api.MetaData{{Key: "a", Value: "1"}}
 
 		ctx, cancel := context.WithTimeout(t.Context(), 100*time.Millisecond)
 		defer cancel()
 
-		l := logger.NewBuffer()
-		err := setMetaDataBatch(ctx, cfg, l, items)
+		err := setMetaDataBatch(ctx, client, "jobid", l, items)
 		if err == nil {
 			t.Fatal("setMetaDataBatch error = nil, want error")
 		}
@@ -167,25 +172,16 @@ func TestSetMetaDataBatch(t *testing.T) {
 	t.Run("401 does not retry", func(t *testing.T) {
 		t.Parallel()
 
+		l := logger.NewBuffer()
 		callCount := 0
-		server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		client := stubAPIClient(l, func(req *http.Request) (*http.Response, error) {
 			callCount++
-			rw.WriteHeader(http.StatusUnauthorized)
-		}))
-		defer server.Close()
-
-		cfg := MetaDataSetBatchConfig{
-			Job: "jobid",
-			APIConfig: APIConfig{
-				AgentAccessToken: "agentaccesstoken",
-				Endpoint:         server.URL,
-			},
-		}
+			return stubResponse(req, http.StatusUnauthorized), nil
+		})
 
 		items := []api.MetaData{{Key: "a", Value: "1"}}
 
-		l := logger.NewBuffer()
-		if err := setMetaDataBatch(t.Context(), cfg, l, items); err == nil {
+		if err := setMetaDataBatch(t.Context(), client, "jobid", l, items); err == nil {
 			t.Fatal("setMetaDataBatch error = nil, want error")
 		}
 		if callCount != 1 {
@@ -196,25 +192,16 @@ func TestSetMetaDataBatch(t *testing.T) {
 	t.Run("404 does not retry", func(t *testing.T) {
 		t.Parallel()
 
+		l := logger.NewBuffer()
 		callCount := 0
-		server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+		client := stubAPIClient(l, func(req *http.Request) (*http.Response, error) {
 			callCount++
-			rw.WriteHeader(http.StatusNotFound)
-		}))
-		defer server.Close()
-
-		cfg := MetaDataSetBatchConfig{
-			Job: "jobid",
-			APIConfig: APIConfig{
-				AgentAccessToken: "agentaccesstoken",
-				Endpoint:         server.URL,
-			},
-		}
+			return stubResponse(req, http.StatusNotFound), nil
+		})
 
 		items := []api.MetaData{{Key: "a", Value: "1"}}
 
-		l := logger.NewBuffer()
-		if err := setMetaDataBatch(t.Context(), cfg, l, items); err == nil {
+		if err := setMetaDataBatch(t.Context(), client, "jobid", l, items); err == nil {
 			t.Fatal("setMetaDataBatch error = nil, want error")
 		}
 		if callCount != 1 {

internal/job/executor.go

@@ -679,15 +679,15 @@ func (e *Executor) applyEnvironmentChanges(changes hook.EnvChanges) {
 	for k, v := range changes.Diff.Added {
 		if _, ok := executorConfigEnvChanges[k]; ok {
 			executorConfigEnvChangesLogged[k] = true
-			e.shell.Commentf("%s is now %q", k, v)
+			e.shell.Commentf("%s is now %s", k, e.redactedValue(v))
 		} else {
 			e.shell.Commentf("%s added", k)
 		}
 	}
 	for k, v := range changes.Diff.Changed {
 		if _, ok := executorConfigEnvChanges[k]; ok {
 			executorConfigEnvChangesLogged[k] = true
-			e.shell.Commentf("%s was %q and is now %q", k, v.Old, v.New)
+			e.shell.Commentf("%s was %s and is now %s", k, e.redactedValue(v.Old), e.redactedValue(v.New))
 		} else {
 			e.shell.Commentf("%s changed", k)
 		}
@@ -705,11 +705,18 @@ func (e *Executor) applyEnvironmentChanges(changes hook.EnvChanges) {
 	// it might not appear in the script "changes".
 	for k, v := range executorConfigEnvChanges {
 		if !executorConfigEnvChangesLogged[k] {
-			e.shell.Commentf("%s is now %q", k, v)
+			e.shell.Commentf("%s is now %s", k, e.redactedValue(v))
 		}
 	}
 }
 
+// redactedValue strips known secrets from a value before %q formatting.
+// %q escapes newlines, so the output redactor can't match a multiline secret
+// like an SSH key once it's formatted.
+func (e *Executor) redactedValue(value string) string {
+	return fmt.Sprintf("%q", redact.String(value, e.redactors.Needles()))
+}
+
 // Should be called whenever we updated our e.shell.Env.
 func (e *Executor) addOutputRedactors() {
 	// reset output redactors based on new environment variable values

internal/job/integration/secrets_integration_test.go

@@ -366,6 +366,64 @@ func TestSecretsIntegration_MultilineSecretRedaction(t *testing.T) {
 	}
 }
 
+// A secret mapped onto a config var like BUILDKITE_GIT_SSH_KEY used to be
+// printed by the env change log after a hook ran. %q escaped its newlines so
+// the output redactor missed it.
+func TestSecretsIntegration_ConfigVarSecretNotLoggedByEnvChange(t *testing.T) {
+	t.Parallel()
+
+	tester, err := NewExecutorTester(mainCtx)
+	if err != nil {
+		t.Fatalf("setting up executor tester: %v", err)
+	}
+	defer tester.Close()
+
+	// The token makes any leaked rendering of the key easy to spot, raw or
+	// %q escaped.
+	const keyToken = "SYNTHETICKEYDONOTUSEFAKEFAKEFAKE"
+	multilineKey := "-----BEGIN OPENSSH PRIVATE KEY-----\n" +
+		"b3BlbnNzaC1rZXktdjEAAAAABG5vbmUAAAAEbm9uZQAAAAAAAAABAAAAMwAAAAtz\n" +
+		"c2gtZWQyNTUxOQAAACD" + keyToken + "0123456789abcdefghij\n" +
+		"-----END OPENSSH PRIVATE KEY-----"
+
+	secretsMap := map[string]string{"REPO_DEPLOY_KEY": multilineKey}
+	apiServer := setupSecretsAPIServer(t, secretsMap)
+	defer apiServer.Close()
+
+	secrets := []pipeline.Secret{
+		{
+			Key:                 "REPO_DEPLOY_KEY",
+			EnvironmentVariable: "BUILDKITE_GIT_SSH_KEY",
+		},
+	}
+	secretsJSON, err := json.Marshal(secrets)
+	if err != nil {
+		t.Fatalf("marshaling secrets: %v", err)
+	}
+
+	// Running any hook is what triggers the env change log for the key.
+	tester.ExpectGlobalHook("environment").AndCallFunc(func(c *bintest.Call) {
+		c.Exit(0)
+	})
+	tester.ExpectGlobalHook("command").AndCallFunc(func(c *bintest.Call) {
+		c.Exit(0)
+	})
+
+	err = tester.Run(t, fmt.Sprintf("BUILDKITE_SECRETS_CONFIG=%s", string(secretsJSON)), fmt.Sprintf("BUILDKITE_AGENT_ENDPOINT=%s", apiServer.URL))
+	if err != nil {
+		t.Fatalf("running executor tester: %v", err)
+	}
+
+	if strings.Contains(tester.Output, keyToken) {
+		t.Fatalf("SSH key leaked into job log in cleartext. Full output:\n%s", tester.Output)
+	}
+
+	// Prove the log line actually fired, so the test can't pass silently.
+	if !strings.Contains(tester.Output, `BUILDKITE_GIT_SSH_KEY is now "[REDACTED]"`) {
+		t.Fatalf("expected redacted env-change log for BUILDKITE_GIT_SSH_KEY. Full output:\n%s", tester.Output)
+	}
+}
+
 func TestSecretsIntegration_LocalHookAccess(t *testing.T) {
 	t.Parallel()
 

internal/replacer/mux.go

@@ -32,6 +32,22 @@ func (m *Mux) Add(needles ...string) {
 	}
 }
 
+// Needles returns the deduplicated set of needles across all replacers.
+func (m *Mux) Needles() []string {
+	seen := make(map[string]struct{})
+	needles := make([]string, 0)
+	for _, r := range m.underlying {
+		for _, n := range r.Needles() {
+			if _, ok := seen[n]; ok {
+				continue
+			}
+			seen[n] = struct{}{}
+			needles = append(needles, n)
+		}
+	}
+	return needles
+}
+
 // Append adds a replacer to the Mux.
 func (m *Mux) Append(r *Replacer) {
 	m.underlying = append(m.underlying, r)

internal/replacer/mux_test.go

@@ -0,0 +1,32 @@
+package replacer
+
+import (
+	"io"
+	"slices"
+	"testing"
+)
+
+func passthrough(b []byte) []byte { return b }
+
+func TestMuxNeedlesDeduplicates(t *testing.T) {
+	t.Parallel()
+
+	r1 := New(io.Discard, []string{"alpha", "bravo"}, passthrough)
+	r2 := New(io.Discard, []string{"bravo", "charlie"}, passthrough)
+	m := NewMux(r1, r2)
+
+	got := m.Needles()
+	slices.Sort(got)
+	want := []string{"alpha", "bravo", "charlie"}
+	if !slices.Equal(got, want) {
+		t.Errorf("Mux.Needles() = %v, want %v (deduplicated across replacers)", got, want)
+	}
+}
+
+func TestMuxNeedlesEmpty(t *testing.T) {
+	t.Parallel()
+
+	if got := NewMux().Needles(); len(got) != 0 {
+		t.Errorf("empty Mux.Needles() = %v, want empty", got)
+	}
+}