From f55be2d89b3d8cf86ddbf1852de7902f26cb6f2a Mon Sep 17 00:00:00 2001 From: apoorva-01 Date: Sat, 11 Jul 2026 03:57:26 +0530 Subject: [PATCH 1/2] Sort secrets longest-first when redacting Redaction ranged over a map, so when one secret was a substring of another the shorter one could be replaced first and leave the tail of the longer secret in the output. Longest-first makes it deterministic. --- packer/logs.go | 31 +++++++++++++++++++++------ packer/logs_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 6 deletions(-) create mode 100644 packer/logs_test.go diff --git a/packer/logs.go b/packer/logs.go index dde95e738..f9cbab6e7 100644 --- a/packer/logs.go +++ b/packer/logs.go @@ -6,6 +6,7 @@ package packer import ( "bytes" "io" + "sort" "strings" "sync" ) @@ -31,10 +32,8 @@ func (l *secretFilter) SetOutput(output io.Writer) { } func (l *secretFilter) Write(p []byte) (n int, err error) { - for s := range l.s { - if s != "" { - p = bytes.Replace(p, []byte(s), []byte(""), -1) - } + for _, s := range l.secrets() { + p = bytes.ReplaceAll(p, []byte(s), []byte("")) } return l.w.Write(p) } @@ -42,12 +41,32 @@ func (l *secretFilter) Write(p []byte) (n int, err error) { // FilterString will overwrite any senstitive variables in a string, returning // the filtered string. func (l *secretFilter) FilterString(message string) string { + for _, s := range l.secrets() { + message = strings.ReplaceAll(message, s, "") + } + return message +} + +// secrets returns the non-empty registered secrets ordered longest first. +// Ranging over the map directly is randomly ordered, so when one secret is a +// substring of another (e.g. "ubuntu" and "ubuntu-22.04") the shorter one could +// be replaced first, leaving the remainder of the longer secret in the output. +// Redacting the longest match first keeps that from happening and makes the +// result deterministic. +func (l *secretFilter) secrets() []string { + secrets := make([]string, 0, len(l.s)) for s := range l.s { if s != "" { - message = strings.Replace(message, s, "", -1) + secrets = append(secrets, s) } } - return message + sort.Slice(secrets, func(i, j int) bool { + if len(secrets[i]) != len(secrets[j]) { + return len(secrets[i]) > len(secrets[j]) + } + return secrets[i] < secrets[j] + }) + return secrets } var LogSecretFilter secretFilter diff --git a/packer/logs_test.go b/packer/logs_test.go new file mode 100644 index 000000000..0fe8b4208 --- /dev/null +++ b/packer/logs_test.go @@ -0,0 +1,51 @@ +// Copyright IBM Corp. 2013, 2025 +// SPDX-License-Identifier: MPL-2.0 + +package packer + +import ( + "bytes" + "testing" +) + +// When one registered secret is a substring of another, the shorter value must +// not be replaced first. Ranging over the map is randomly ordered, so without +// longest-first ordering the short secret ("ubuntu") can be redacted before the +// long one ("ubuntu-22.04"), leaving the tail "-22.04" (part of a real secret) +// in the output. The loop makes the random map order show up reliably. +func TestSecretFilterFilterStringOverlapping(t *testing.T) { + const in = "connecting to ubuntu-22.04 now" + const want = "connecting to now" + + for i := 0; i < 100; i++ { + l := &secretFilter{s: map[string]struct{}{ + "ubuntu-22.04": {}, + "ubuntu": {}, + }} + if got := l.FilterString(in); got != want { + t.Fatalf("secret partially leaked: got %q, want %q", got, want) + } + } +} + +func TestSecretFilterWriteOverlapping(t *testing.T) { + const in = "connecting to ubuntu-22.04 now" + const want = "connecting to now" + + for i := 0; i < 100; i++ { + var buf bytes.Buffer + l := &secretFilter{ + s: map[string]struct{}{ + "ubuntu-22.04": {}, + "ubuntu": {}, + }, + w: &buf, + } + if _, err := l.Write([]byte(in)); err != nil { + t.Fatal(err) + } + if got := buf.String(); got != want { + t.Fatalf("secret partially leaked: got %q, want %q", got, want) + } + } +} From 9c1d520bb75c16949e48ff6f4f26dfa0bf83d6fd Mon Sep 17 00:00:00 2001 From: apoorva-01 Date: Sun, 26 Jul 2026 05:24:14 +0530 Subject: [PATCH 2/2] Make secret redaction race-free and single-pass Snapshotting the secret set without the lock raced with Set, and chaining ReplaceAll let a secret match text inside a marker a previous replacement wrote ("my_token" -> ">"). Keep a longest-first snapshot built in Set under the lock, read it and the writer together under the lock, and redact in a single pass over the original input. --- packer/logs.go | 79 +++++++++++++++++++++++++++++----------- packer/logs_test.go | 89 ++++++++++++++++++++++++++++++--------------- 2 files changed, 118 insertions(+), 50 deletions(-) diff --git a/packer/logs.go b/packer/logs.go index f9cbab6e7..def9579e5 100644 --- a/packer/logs.go +++ b/packer/logs.go @@ -4,7 +4,6 @@ package packer import ( - "bytes" "io" "sort" "strings" @@ -12,9 +11,10 @@ import ( ) type secretFilter struct { - s map[string]struct{} - m sync.Mutex - w io.Writer + s map[string]struct{} + sorted []string // non-empty secrets, longest first; rebuilt on Set + m sync.Mutex + w io.Writer } func (l *secretFilter) Set(secrets ...string) { @@ -23,6 +23,7 @@ func (l *secretFilter) Set(secrets ...string) { for _, s := range secrets { l.s[s] = struct{}{} } + l.sorted = sortedSecrets(l.s) } func (l *secretFilter) SetOutput(output io.Writer) { @@ -32,30 +33,66 @@ func (l *secretFilter) SetOutput(output io.Writer) { } func (l *secretFilter) Write(p []byte) (n int, err error) { - for _, s := range l.secrets() { - p = bytes.ReplaceAll(p, []byte(s), []byte("")) - } - return l.w.Write(p) + secrets, w := l.snapshot() + return w.Write([]byte(redact(string(p), secrets))) } -// FilterString will overwrite any senstitive variables in a string, returning +// FilterString will overwrite any sensitive variables in a string, returning // the filtered string. func (l *secretFilter) FilterString(message string) string { - for _, s := range l.secrets() { - message = strings.ReplaceAll(message, s, "") + secrets, _ := l.snapshot() + return redact(message, secrets) +} + +// snapshot reads the longest-first secret list and the output writer together +// under the mutex, so a concurrent Set or SetOutput can't race with a redaction +// in progress. Set replaces the slice wholesale rather than mutating it, so the +// returned slice stays safe to range over after the mutex is released. +func (l *secretFilter) snapshot() ([]string, io.Writer) { + l.m.Lock() + defer l.m.Unlock() + return l.sorted, l.w +} + +// redact replaces every occurrence of a secret with "". It scans the +// original input once, so a "" marker it writes is never itself +// searched for secrets. Feeding each replacement's output into the next lets a +// secret like "sitive" match text a previous replacement introduced, turning +// "my_token" into ">"; scanning the original avoids that. +// secrets must be ordered longest first so the longest match wins where secrets +// overlap at a position. +func redact(s string, secrets []string) string { + if len(secrets) == 0 { + return s + } + var b strings.Builder + b.Grow(len(s)) + for i := 0; i < len(s); { + matched := false + for _, secret := range secrets { + if strings.HasPrefix(s[i:], secret) { + b.WriteString("") + i += len(secret) + matched = true + break + } + } + if !matched { + b.WriteByte(s[i]) + i++ + } } - return message + return b.String() } -// secrets returns the non-empty registered secrets ordered longest first. -// Ranging over the map directly is randomly ordered, so when one secret is a -// substring of another (e.g. "ubuntu" and "ubuntu-22.04") the shorter one could -// be replaced first, leaving the remainder of the longer secret in the output. -// Redacting the longest match first keeps that from happening and makes the -// result deterministic. -func (l *secretFilter) secrets() []string { - secrets := make([]string, 0, len(l.s)) - for s := range l.s { +// sortedSecrets returns the non-empty secrets ordered longest first, and +// lexicographically for equal lengths so the order is deterministic. Redacting +// the longest match first keeps a shorter secret that is a substring of a +// longer one (e.g. "ubuntu" and "ubuntu-22.04") from leaving the remainder of +// the longer secret in the output. +func sortedSecrets(set map[string]struct{}) []string { + secrets := make([]string, 0, len(set)) + for s := range set { if s != "" { secrets = append(secrets, s) } diff --git a/packer/logs_test.go b/packer/logs_test.go index 0fe8b4208..bbd5b15c9 100644 --- a/packer/logs_test.go +++ b/packer/logs_test.go @@ -5,47 +5,78 @@ package packer import ( "bytes" + "io" + "reflect" + "sync" "testing" ) -// When one registered secret is a substring of another, the shorter value must -// not be replaced first. Ranging over the map is randomly ordered, so without -// longest-first ordering the short secret ("ubuntu") can be redacted before the -// long one ("ubuntu-22.04"), leaving the tail "-22.04" (part of a real secret) -// in the output. The loop makes the random map order show up reliably. +func newSecretFilter(secrets ...string) *secretFilter { + l := &secretFilter{s: map[string]struct{}{}} + l.Set(secrets...) + return l +} + +// Set must record secrets longest first (lexicographically for equal lengths) +// and drop empty values, so redaction is deterministic regardless of the map's +// iteration order. +func TestSecretFilterSetSortsLongestFirst(t *testing.T) { + l := newSecretFilter("ubuntu", "", "ubuntu-22.04", "bb", "aa") + want := []string{"ubuntu-22.04", "ubuntu", "aa", "bb"} + if !reflect.DeepEqual(l.sorted, want) { + t.Fatalf("sorted secrets: got %q, want %q", l.sorted, want) + } +} + +// When one secret is a substring of another, the longest match must be redacted +// so the tail of the longer secret ("-22.04") can't leak. func TestSecretFilterFilterStringOverlapping(t *testing.T) { - const in = "connecting to ubuntu-22.04 now" + l := newSecretFilter("ubuntu-22.04", "ubuntu") const want = "connecting to now" + if got := l.FilterString("connecting to ubuntu-22.04 now"); got != want { + t.Fatalf("secret partially leaked: got %q, want %q", got, want) + } +} - for i := 0; i < 100; i++ { - l := &secretFilter{s: map[string]struct{}{ - "ubuntu-22.04": {}, - "ubuntu": {}, - }} - if got := l.FilterString(in); got != want { - t.Fatalf("secret partially leaked: got %q, want %q", got, want) - } +// Redaction runs against the original input, so a secret that happens to appear +// inside the "" marker ("sitive") must not match text the marker +// introduced and turn "my_token" into ">". +func TestSecretFilterFilterStringDoesNotRefilterMarker(t *testing.T) { + l := newSecretFilter("my_token", "sitive") + if got := l.FilterString("my_token"); got != "" { + t.Fatalf("marker was re-filtered: got %q, want %q", got, "") } } func TestSecretFilterWriteOverlapping(t *testing.T) { - const in = "connecting to ubuntu-22.04 now" + var buf bytes.Buffer + l := newSecretFilter("ubuntu-22.04", "ubuntu") + l.SetOutput(&buf) + + if _, err := l.Write([]byte("connecting to ubuntu-22.04 now")); err != nil { + t.Fatal(err) + } const want = "connecting to now" + if got := buf.String(); got != want { + t.Fatalf("secret partially leaked: got %q, want %q", got, want) + } +} + +// Registering secrets while a redaction reads them must not race. Run under +// `go test -race`: reading the map without the mutex trips the detector. +func TestSecretFilterConcurrentSetAndRead(t *testing.T) { + l := newSecretFilter() + l.SetOutput(io.Discard) + var wg sync.WaitGroup for i := 0; i < 100; i++ { - var buf bytes.Buffer - l := &secretFilter{ - s: map[string]struct{}{ - "ubuntu-22.04": {}, - "ubuntu": {}, - }, - w: &buf, - } - if _, err := l.Write([]byte(in)); err != nil { - t.Fatal(err) - } - if got := buf.String(); got != want { - t.Fatalf("secret partially leaked: got %q, want %q", got, want) - } + wg.Add(3) + go func() { defer wg.Done(); l.Set("secret") }() + go func() { defer wg.Done(); l.FilterString("a secret in a message") }() + go func() { + defer wg.Done() + _, _ = l.Write([]byte("a secret in a message")) + }() } + wg.Wait() }