Sort secrets longest-first when redacting#340
Conversation
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.
|
|
|
Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement Learn more about why HashiCorp requires a CLA and what the CLA includes Have you signed the CLA already but the status is still pending? Recheck it. |
|
While this is not related to the hashicorp/packer#13659 issue, this is still a good find. |
There was a problem hiding this comment.
Pull request overview
This PR makes secret redaction deterministic and safer when secrets overlap by sorting registered secrets longest-first before performing replacements, preventing partial leaks when a shorter secret is a substring of a longer one.
Changes:
- Add a
secrets()helper that returns non-empty secrets sorted longest-first (and deterministic for equal lengths). - Update
WriteandFilterStringto use the sorted secret list andReplaceAllAPIs. - Add tests covering overlapping secrets for both
FilterStringandWrite.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
packer/logs.go |
Introduces longest-first deterministic ordering for secret replacement and uses it in string/byte redaction paths. |
packer/logs_test.go |
Adds regression tests ensuring overlapping secrets do not partially leak. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (l *secretFilter) secrets() []string { | ||
| secrets := make([]string, 0, len(l.s)) | ||
| for s := range l.s { | ||
| if s != "" { | ||
| message = strings.Replace(message, s, "<sensitive>", -1) | ||
| secrets = append(secrets, s) | ||
| } | ||
| } |
| func (l *secretFilter) Write(p []byte) (n int, err error) { | ||
| for s := range l.s { | ||
| if s != "" { | ||
| p = bytes.Replace(p, []byte(s), []byte("<sensitive>"), -1) | ||
| } | ||
| for _, s := range l.secrets() { | ||
| p = bytes.ReplaceAll(p, []byte(s), []byte("<sensitive>")) | ||
| } | ||
| return l.w.Write(p) | ||
| } |
| // the filtered string. | ||
| func (l *secretFilter) FilterString(message string) string { | ||
| for _, s := range l.secrets() { | ||
| message = strings.ReplaceAll(message, s, "<sensitive>") |
There was a problem hiding this comment.
Replacements are applied to the result of previous replacements, so a later secret can match text introduced by the <sensitive> marker.
For example, with secrets "my_token" and "sitive", longest-first changes "my_token" into "<sensitive>" and then into "<sen<sensitive>>".
This PR makes that malformed output deterministic and may break consumers that recognize the exact redaction marker.
Please compute matches against the original input, or otherwise ensure generated markers cannot be filtered again, and cover both FilterString and Write with a regression test.
| } | ||
| return secrets[i] < secrets[j] | ||
| }) | ||
| return secrets |
There was a problem hiding this comment.
Could we avoid allocating and sorting the complete secret set for every message? FilterString runs for every BasicUi.Say and BasicUi.Error, and Write may be installed in a logging path.
This changes each call from direct map iteration to a slice allocation plus an O(NlogN) sort.
Since secrets change only through Set, please maintain a sorted snapshot when Set mutates the set under the mutex, then let readers use that snapshot. This would also provide a race-free solution to the concurrent map access noted above.
| const in = "connecting to ubuntu-22.04 now" | ||
| const want = "connecting to <sensitive> now" | ||
|
|
||
| for i := 0; i < 100; i++ { |
There was a problem hiding this comment.
This test relies on unspecified map iteration eventually selecting the shorter secret first within 100 attempts. That is probabilistic and is not guaranteed to fail against the old implementation on every Go version or platform. Please assert the sorted snapshot/order directly, while retaining one end-to-end redaction assertion.
tanmay-hc
left a comment
tanmay-hc
left a comment
There was a problem hiding this comment.
Refer to the copilot's Race condition comments too.
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" -> "<sen<sensitive>>"). 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.
|
Reworked it, covers all five: Races — Marker re-matching — Per-message sort — the sort happens in Test — asserts the sorted order directly now, plus deterministic redaction cases and a Passes |
Description
Redaction ranged over a map, so the replace order was random. When one sensitive value is a substring of another (
ubuntuandubuntu-22.04both sensitive), the short one could get replaced first and leave the tail of the longer secret in the log:Same line came out either way run to run. Sorting longest-first before replacing fixes it. Tests for
FilterStringandWritefail on the old code and pass with this.Found while checking at hashicorp/packer#13659. cc @tanmay-hc
Resolved Issues
None.
Rollback Plan
If a change needs to be reverted, we will roll out an update to the code within 7 days.
Changes to Security Controls
Yes, log redaction. Makes it stricter: overlapping secrets now redact the longest match first, so less can slip through.