Fix redaction leak from ssh_secret within checkout

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