From 3f7530e4306e7ba5c892f984d5128ebd3c8c3440 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Fri, 9 Oct 2026 13:16:36 +0200 Subject: [PATCH 1/3] fix(redaction): reduce false positives in AI coding session redaction Honor inline gitleaks:allow and betterleaks:allow markers when redacting AI coding sessions and captured specs. The marker covers only the line of the decoded string it is on, so a marker in one line of a file read by a tool keeps the secrets next to it and nothing else. The scanner still ignores markers itself, because it sees a whole string leaf as one line. Improve rule precision with an embedded ruleset that extends the betterleaks defaults with allowlists for UUIDs, sha digests, code expressions, the bare password key word, documentation placeholders in curl examples and ports read as URI passwords. Optional components of a composite finding, such as the username next to a generic password, are no longer redacted. A generic-password match is cut at an escaped line break, so a placeholder the transcript already shows is not redacted again. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: f7d0aeca-fbc0-4795-ad11-21847f1d012f --- app/cli/internal/trace/README.md | 1 + internal/redaction/betterleaks.go | 71 ++++-- internal/redaction/betterleaks.toml | 44 ++++ internal/redaction/betterleaks_test.go | 5 +- internal/redaction/precision_test.go | 225 ++++++++++++++++++ internal/redaction/redaction.go | 159 +++++++++++-- internal/redaction/redaction_test.go | 123 ++++++++++ .../materials/aicodingsession/redact.go | 14 +- .../testdata/session-fp-shaped.json | 30 +++ 9 files changed, 634 insertions(+), 38 deletions(-) create mode 100644 internal/redaction/betterleaks.toml create mode 100644 internal/redaction/precision_test.go diff --git a/app/cli/internal/trace/README.md b/app/cli/internal/trace/README.md index 3df2f4458..6072463ae 100644 --- a/app/cli/internal/trace/README.md +++ b/app/cli/internal/trace/README.md @@ -85,6 +85,7 @@ How the evidence is protected before it leaves the machine. |---------|---------------|-------------|--------|--------------|------------| | Secret redaction | Secrets in the session evidence and in the spec materials are replaced with a placeholder before upload. | Yes | Yes | Yes | Yes | | Media skipped by redaction | The secret redaction does not scan base64 media (images, PDFs) in the transcript, so it is faster and does not break the media. | Yes | Unknown | Not applicable | Not applicable | +| Allow markers in redaction | A secret on a line that carries an inline `gitleaks:allow` or `betterleaks:allow` marker, such as a test fixture the agent read, is not redacted. The marker covers only its own line. | Yes | Yes | Yes | Yes | Why not Yes: diff --git a/internal/redaction/betterleaks.go b/internal/redaction/betterleaks.go index 357312c33..540e6de71 100644 --- a/internal/redaction/betterleaks.go +++ b/internal/redaction/betterleaks.go @@ -18,6 +18,7 @@ package redaction import ( "context" "crypto/sha256" + _ "embed" "fmt" "runtime" "slices" @@ -25,6 +26,7 @@ import ( "strings" "sync" + "github.com/betterleaks/betterleaks/config" "github.com/betterleaks/betterleaks/detect" betterleaksregexp "github.com/betterleaks/betterleaks/regexp" "github.com/betterleaks/betterleaks/regexp/re2" @@ -67,6 +69,12 @@ type betterleaksScanner struct { chunkSize int } +// rulesConfig is the betterleaks default ruleset, extended with allowlists for +// values the generic rules match but which are not credentials. +// +//go:embed betterleaks.toml +var rulesConfig string + var ( defaultScanner = sync.OnceValues(newBetterleaksScanner) // useRE2 selects the regex engine. It is process-wide and only affects @@ -88,21 +96,30 @@ func DefaultScanner() (Scanner, error) { } func newBetterleaksScanner() (*betterleaksScanner, error) { + return newScannerWithRules(rulesConfig) +} + +// newScannerWithRules builds a scanner from a betterleaks TOML configuration. +func newScannerWithRules(rules string) (*betterleaksScanner, error) { // The library defaults to the standard library engine, which is several // times slower on this ruleset; the betterleaks CLI defaults to RE2 for the // same reason. useRE2() - // Validation stays off, which is what the default constructor gives us: - // validating would reach out to third-party APIs to check whether a candidate - // credential is live, and crafting a material must not do that. - d, err := detect.NewDetectorDefaultConfig() + cfg, err := config.ParseTOMLString(rules, "") if err != nil { - return nil, fmt.Errorf("loading the default secret scanning rules: %w", err) + return nil, fmt.Errorf("loading the secret scanning rules: %w", err) } - // A session transcript is text the model was free to write, so an in-band - // "betterleaks:allow" or "gitleaks:allow" must not switch redaction off. + // Validation stays off: validating would reach out to third-party APIs to + // check whether a candidate credential is live, and crafting a material + // must not do that. + d := detect.NewDetectorContext(context.Background(), cfg, detect.ValidationOptions{}) + + // The detector checks an allow marker against the whole line of its match. + // A string leaf of the scanned document is a single line, so one marker + // would suppress every finding in a whole tool result. Redactor checks the + // marker against the line of each occurrence instead, when asked to. d.IgnoreGitleaksAllow = true // Findings must carry the verbatim secret: it is what we search for in the // document in order to replace it. @@ -169,10 +186,12 @@ func (s *betterleaksScanner) scanChunks(ctx context.Context, text string, chunks // and the rule for that component is marked as not independently // reportable — so the primary finding names the harmless public // identifier while the actual credential arrives only as a component. - // Both halves have to be redacted. + // Both halves have to be redacted. An optional component is context + // that only raises the confidence of the finding, such as the username + // next to a generic password, and is not a secret. for _, set := range result.Finding.ComponentSets { for _, component := range set.Components { - if component == nil { + if component == nil || component.Optional { continue } perChunk[chunk] = appendSecret(perChunk[chunk], component.RuleID, component.Secret) @@ -332,8 +351,11 @@ func byteSpans(text string, ranges []lineSpan) []span { // appendSecret records a locatable secret. A finding without one cannot be // searched for in the document, so it is dropped rather than reported. func appendSecret(dst []Finding, ruleID, secret string) []Finding { - if ruleID == jwtRuleID { + switch ruleID { + case jwtRuleID: secret = cutAtTerminatorEscape(secret) + case genericPasswordRuleID: + secret = cutAtLineEscape(secret) } if secret == "" { return dst @@ -341,8 +363,17 @@ func appendSecret(dst []Finding, ruleID, secret string) []Finding { return append(dst, Finding{RuleID: ruleID, Secret: secret}) } -// jwtRuleID is the default ruleset's rule for JSON Web Tokens. -const jwtRuleID = "jwt" +const ( + // jwtRuleID is the default ruleset's rule for JSON Web Tokens. + jwtRuleID = "jwt" + // genericPasswordRuleID is the default ruleset's rule for a password + // assigned to a password key. Its unquoted value stops at whitespace in + // source text, but in JSON-encoded text a line break is the escape `\n`, so + // the match runs into the next line. That swallowed, for instance, the line + // break after a placeholder the transcript already showed, and reported the + // placeholder as a new secret. + genericPasswordRuleID = "generic-password" +) // cutAtTerminatorEscape removes the text from the first whitespace or quote // escape onwards. @@ -357,12 +388,24 @@ const jwtRuleID = "jwt" // Escapes are read as units from the left, so the second backslash of a `\\` // escape is never taken as the start of another escape. func cutAtTerminatorEscape(secret string) string { + return cutAtEscape(secret, `"nrt`) +} + +// cutAtLineEscape removes the text from the first whitespace escape onwards. A +// password is not cut at a quote escape: its value often starts with one, +// `password: \"...\"`, which is part of it. +func cutAtLineEscape(secret string) string { + return cutAtEscape(secret, "nrt") +} + +// cutAtEscape removes the text from the first escape of one of the given +// characters onwards. +func cutAtEscape(secret, terminators string) string { for i := 0; i+1 < len(secret); i++ { if secret[i] != '\\' { continue } - switch secret[i+1] { - case '"', 'n', 'r', 't': + if strings.IndexByte(terminators, secret[i+1]) >= 0 { return secret[:i] } // Skip the escaped character, so that it is not read as the start of diff --git a/internal/redaction/betterleaks.toml b/internal/redaction/betterleaks.toml new file mode 100644 index 000000000..a72df6cdd --- /dev/null +++ b/internal/redaction/betterleaks.toml @@ -0,0 +1,44 @@ +# Secret scanning rules for redaction: the betterleaks default ruleset, with +# allowlists for values the generic rules match in AI coding sessions but which +# are not credentials. +# +# Every allowlist targets one or more low-precision rules and tests the found +# secret, or the rule's match. None tests the whole line: a string leaf of the +# scanned document is a single line, so a line allowlist would drop every +# finding in a whole tool result. + +[extend] +useDefault = true + +[[allowlists]] +description = "UUIDs are identifiers, not credentials" +targetRules = ["generic-api-key", "generic-password"] +regexes = ['''^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$'''] + +[[allowlists]] +description = "Content digests, such as sha256:" +targetRules = ["generic-api-key"] +regexTarget = "match" +regexes = ['''(?i)\bsha(?:1|224|256|384|512):[0-9a-f]{40,128}\b'''] + +[[allowlists]] +description = "Code expressions and the bare key word, which are not password values" +targetRules = ["generic-password"] +regexes = [ + # An expression such as t.password||n.password in minified JavaScript. + '''\|\||&&|\?\?''', + # A property access such as e.password or config.db_password. + '''^[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)*\.\w*(?i:passw(?:or)?d|pwd)\w*$''', + # Documentation such as (password: `password`). + '''(?i)^(?:password|passwd|pwd)$''', +] + +[[allowlists]] +description = "Placeholders in documentation, such as Bearer YOUR_API_TOKEN" +targetRules = ["curl-auth-header"] +regexes = ['''^[A-Z][A-Z0-9]*(?:_[A-Z0-9]+)+$'''] + +[[allowlists]] +description = "A port number read as the password of a URI, such as http://localhost:8000@example.com" +targetRules = ["generic-credential-uri"] +regexes = ['''^[0-9]{1,5}$'''] diff --git a/internal/redaction/betterleaks_test.go b/internal/redaction/betterleaks_test.go index 6101ed724..62dbca7c7 100644 --- a/internal/redaction/betterleaks_test.go +++ b/internal/redaction/betterleaks_test.go @@ -79,8 +79,9 @@ func TestDefaultScannerDetects(t *testing.T) { wantRule: "github-pat", }, { - // A session transcript is attacker-influenceable text, so neither - // in-band bypass marker may suppress a finding. + // The detector would apply a marker to its whole line, which is a + // whole string leaf of the document. Redactor applies it to the + // line of each occurrence instead, so the scanner reports it. name: "gitleaks:allow must not suppress the finding", text: `"content": "` + awsPair + ` // gitleaks:allow"`, wantRule: "aws-access-token", diff --git a/internal/redaction/precision_test.go b/internal/redaction/precision_test.go new file mode 100644 index 000000000..1eb09e234 --- /dev/null +++ b/internal/redaction/precision_test.go @@ -0,0 +1,225 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package redaction + +import ( + "context" + "strings" + "testing" + + betterleaksconfig "github.com/betterleaks/betterleaks/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Values shaped like the ones the generic rules matched in recorded AI coding +// sessions, and real secrets of the same shape. Assembled from fragments, like +// the other synthetic credentials in this package. +const ( + sampleUUID = "3bf79921-3c03-81b6-" + "afff-cb246849866f" + sampleDigest = "9f8e7d6c5b4a39281706f5e4d3c2b1a0" + "f9e8d7c69f8e7d6c5b4a39281706f5e4" + // samplePassword is a password with enough entropy to look real. + samplePassword = "Xk9mQ2vL" + "p7wRt4zN" + // sampleAPIKey is a random-looking key the generic-api-key rule matches. + sampleAPIKey = "q8Zr2LmX7v" + "Pk4TnW9sYb" + sampleEmail = "sarah@chainloop.local" + + ruleGenericAPIKey = "generic-api-key" + ruleGitHubPAT = "github-pat" +) + +// defaultRulesScanner returns a scanner with the betterleaks default ruleset and +// none of the allowlists of rulesConfig. It shows that a value the allowlists +// keep is one the default ruleset matches, so that a test of an allowlist +// cannot pass for the wrong reason. +func defaultRulesScanner(t *testing.T) *betterleaksScanner { + t.Helper() + s, err := newScannerWithRules(betterleaksconfig.DefaultConfig) + require.NoError(t, err) + return s +} + +// TestRulesConfigKeepsNonSecrets covers the values that the generic rules of +// the default ruleset replace in AI coding sessions but which are not secrets. +// Each case is a line of a tool result or a message, followed by a line break, +// which is the shape it has in a transcript. +func TestRulesConfigKeepsNonSecrets(t *testing.T) { + scanner, err := DefaultScanner() + require.NoError(t, err) + unfiltered := defaultRulesScanner(t) + + testCases := []struct { + name string + text string + // wantDefaultRule is the rule of the default ruleset that replaces part + // of the text. + wantDefaultRule string + }{ + { + name: "uuid as the value of a key", + text: "workflow key: " + sampleUUID + "\n", + wantDefaultRule: ruleGenericAPIKey, + }, + { + name: "uuid as an idempotency key", + text: "idempotency_key=" + sampleUUID + "\n", + wantDefaultRule: ruleGenericAPIKey, + }, + { + name: "sha256 evidence digest in cli output", + text: "attestation pushed, access sha256:" + sampleDigest + "\n", + wantDefaultRule: ruleGenericAPIKey, + }, + { + name: "minified javascript in a tool result", + text: "host: localhost\nuser: admin\n!function(e){var t={};e.password=t.password||n.password,e.user=u}\n", + wantDefaultRule: genericPasswordRuleID, + }, + { + name: "documentation of development credentials", + text: "- **Dex OIDC**: localhost:5556, users " + sampleEmail + " / john@chainloop.local (password: `password`)\n- **Database**: host: localhost, user: postgres\n", + wantDefaultRule: genericPasswordRuleID, + }, + { + name: "placeholder token in a curl example of a document", + text: "curl -H 'Authorization: Bearer REPLACE_ME' https://api.example.com\n", + wantDefaultRule: "curl-auth-header", + }, + { + name: "port read as the password of a uri", + text: "issuer: http://localhost:8000@chainloop.local\n", + wantDefaultRule: "generic-credential-uri", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + _, report, err := New(unfiltered).RedactText(context.Background(), tc.text) + require.NoError(t, err) + require.Contains(t, report.RuleIDs(), tc.wantDefaultRule, "the default ruleset must replace part of this text, or this case tests nothing") + + got, report, err := New(scanner).RedactText(context.Background(), tc.text) + require.NoError(t, err) + assert.Equal(t, tc.text, got) + assert.False(t, report.Changed(), "replaced by %v", report.RuleIDs()) + }) + } +} + +// TestRulesConfigStillRedactsSecrets is the counterpart of +// TestRulesConfigKeepsNonSecrets: a real secret of each shape is still +// replaced. +func TestRulesConfigStillRedactsSecrets(t *testing.T) { + scanner, err := DefaultScanner() + require.NoError(t, err) + + testCases := []struct { + name string + text string + secret string + wantRule string + // keep is text next to the secret that must survive. + keep string + }{ + { + name: "api key that is not a uuid", + text: "workflow key: " + sampleAPIKey + "\n", + secret: sampleAPIKey, + wantRule: ruleGenericAPIKey, + }, + { + name: "hex key without a digest prefix", + text: "access_token: " + sampleDigest + "\n", + secret: sampleDigest, + wantRule: ruleGenericAPIKey, + }, + { + name: "password next to an email username", + text: "host: db.internal\nuser: " + sampleEmail + "\npassword: " + samplePassword + "\n", + secret: samplePassword, + wantRule: genericPasswordRuleID, + keep: sampleEmail, + }, + { + name: "bearer token in a curl command", + text: "curl -H 'Authorization: Bearer " + sampleAPIKey + "' https://api.example.com\n", + secret: sampleAPIKey, + wantRule: "curl-auth-header", + }, + { + name: "password in a connection uri", + text: "dsn: postgres://admin:" + samplePassword + "@db.internal:5432/app\n", + secret: samplePassword, + wantRule: "generic-credential-uri", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got, report, err := New(scanner).RedactText(context.Background(), tc.text) + require.NoError(t, err) + assert.NotContains(t, got, tc.secret) + assert.Contains(t, report.RuleIDs(), tc.wantRule) + if tc.keep != "" { + assert.Contains(t, got, tc.keep) + } + }) + } +} + +// TestRedactKeepsEchoedPlaceholders covers transcripts that show placeholders +// of an earlier redaction: test assertions, a pull request check table, or a +// policy result. They are left as they are, and not counted as replacements. +func TestRedactKeepsEchoedPlaceholders(t *testing.T) { + scanner, err := DefaultScanner() + require.NoError(t, err) + + texts := []string{ + "wantURI := \"https://tracker.example.com/issue/1?token=[CHAINLOOP_TRACE_REDACTED:github-pat]\"\n", + "assert.Contains(t, string(content), \"[CHAINLOOP_TRACE_REDACTED:aws-access-token]\")\n", + "| ai-config-no-secrets | fail | Secret (generic-password) detected [CHAINLOOP_TRACE_REDACTED:generic-password] |\n", + "password: [CHAINLOOP_TRACE_REDACTED:generic-password]\napi_key: [CHAINLOOP_TRACE_REDACTED:generic-api-key]\n", + // The log line the CLI prints after a redaction. + "10:04AM INF redacted secrets from the AI coding session before upload count=3 rules=[\"generic-password\",\"generic-username\"]\n", + } + + for _, text := range texts { + t.Run(strings.SplitN(text, " ", 2)[0], func(t *testing.T) { + got, report, err := New(scanner).RedactText(context.Background(), text) + require.NoError(t, err) + assert.Equal(t, text, got) + assert.False(t, report.Changed(), "replaced by %v", report.RuleIDs()) + }) + } +} + +// TestRedactAllowMarkerWithDefaultRules checks an allow marker against the real +// ruleset: a test fixture whose line carries one is kept, the same value on a +// line without one is not. +func TestRedactAllowMarkerWithDefaultRules(t *testing.T) { + scanner, err := DefaultScanner() + require.NoError(t, err) + + fixture := "\tconst pat = \"" + fakeGitHubPAT + "\" // gitleaks:allow\n" + text := "119" + fixture + "120\twantURI := \"https://tracker.example.com/issue/1?token=\" + pat\nexport GITHUB_TOKEN=" + fakeGitHubPAT + "\n" + + got, report, err := New(scanner, WithAllowMarkers()).RedactText(context.Background(), text) + require.NoError(t, err) + assert.Contains(t, got, fixture, "the line with the marker is kept") + assert.Contains(t, got, "export GITHUB_TOKEN=[CHAINLOOP_TRACE_REDACTED:github-pat]\n") + assert.Equal(t, map[string]int{ruleGitHubPAT: 1}, report.Allowed) + assert.Equal(t, map[string]int{ruleGitHubPAT: 1}, report.ByRule) +} diff --git a/internal/redaction/redaction.go b/internal/redaction/redaction.go index 382a35687..5395e175c 100644 --- a/internal/redaction/redaction.go +++ b/internal/redaction/redaction.go @@ -32,6 +32,7 @@ import ( "fmt" "io" "regexp" + "slices" "sort" "strconv" "strings" @@ -106,6 +107,9 @@ type Report struct { // lies in a path the filter protects, or when it spans the JSON punctuation // between two adjacent leaves. Unlocated map[string]int + // Allowed counts, per rule id, occurrences left in place because their line + // carries an allow marker. Empty unless the Redactor honours markers. + Allowed map[string]int // Passes is the number of detection passes performed. Passes int } @@ -136,6 +140,7 @@ type Redactor struct { maxPasses int placeholder func(ruleID string) string isPlaceholder func(string) bool + allowMarkers bool } // Option customises a Redactor. @@ -159,6 +164,19 @@ func WithOpaque(f OpaqueFilter) Option { } } +// WithAllowMarkers leaves in place a secret whose line carries an inline +// "gitleaks:allow" or "betterleaks:allow" marker, as a repository scan with +// either tool would. The default redacts it. +// +// A line is a line of the decoded string, such as one line of a file in a tool +// result, so the marker only covers the secrets next to it. An occurrence that +// spans more than one line is always redacted. +func WithAllowMarkers() Option { + return func(r *Redactor) { + r.allowMarkers = true + } +} + // WithMaxPasses bounds the convergence loop. func WithMaxPasses(n int) Option { return func(r *Redactor) { @@ -273,7 +291,7 @@ func (r *Redactor) Redact(ctx context.Context, doc []byte) ([]byte, *Report, err detachOpaque(root, "", r.opaque, &detached) } - report := &Report{ByRule: map[string]int{}, Unlocated: map[string]int{}} + report := &Report{ByRule: map[string]int{}, Unlocated: map[string]int{}, Allowed: map[string]int{}} // Secrets no eligible leaf contains, so that the loop stops chasing them. skip := make(map[string]struct{}) converged := false @@ -308,16 +326,26 @@ func (r *Redactor) Redact(ctx context.Context, doc []byte) ([]byte, *Report, err } w := &rewriter{ - pathFilter: r.pathFilter, - placeholder: r.placeholder, - secrets: pending, - byRule: map[string]int{}, - located: map[string]struct{}{}, + pathFilter: r.pathFilter, + placeholder: r.placeholder, + allowMarkers: r.allowMarkers, + secrets: pending, + byRule: map[string]int{}, + allowed: map[string]int{}, + located: map[string]struct{}{}, + kept: map[string]struct{}{}, } w.rewriteMap(root, "") for _, s := range pending { - if _, ok := w.located[s.secret]; !ok { + _, located := w.located[s.secret] + _, kept := w.kept[s.secret] + switch { + case kept: + // It is only left on allowed lines after this pass, so a further + // pass would only find it again. + skip[s.secret] = struct{}{} + case !located: skip[s.secret] = struct{}{} report.Unlocated[s.ruleID]++ } @@ -325,6 +353,9 @@ func (r *Redactor) Redact(ctx context.Context, doc []byte) ([]byte, *Report, err for rule, n := range w.byRule { report.ByRule[rule] += n } + for rule, n := range w.allowed { + report.Allowed[rule] += n + } report.Replacements += w.count if w.count == 0 { @@ -449,12 +480,17 @@ func trimPartialEscape(secret string) string { // rewriter walks a decoded JSON value tree replacing secrets in eligible string // leaves. type rewriter struct { - pathFilter PathFilter - placeholder func(string) string - secrets []secretRule - byRule map[string]int - located map[string]struct{} - count int + pathFilter PathFilter + placeholder func(string) string + allowMarkers bool + secrets []secretRule + byRule map[string]int + // allowed counts, per rule id, the occurrences kept on allowed lines. + allowed map[string]int + located map[string]struct{} + // kept holds the secrets with an occurrence kept on an allowed line. + kept map[string]struct{} + count int } func (w *rewriter) rewriteMap(m map[string]any, path string) { @@ -498,15 +534,22 @@ func (w *rewriter) redactLeaf(s, path string) string { var ( n int + eligible bool lastRule string + // Replacing a secret never adds or removes a marker, so this holds for + // the whole leaf. + marked = w.allowMarkers && hasAllowMarker(body) ) for _, sr := range w.secrets { c := strings.Count(body, sr.secret) if c == 0 { continue } - if n == 0 && !w.pathFilter(path) { - return s + if !eligible { + if !w.pathFilter(path) { + return s + } + eligible = true } // The placeholder goes into the escaped form of the string, so it is // escaped too. A raw quote or backslash in it would otherwise break @@ -515,9 +558,22 @@ func (w *rewriter) redactLeaf(s, path string) string { if err != nil { return w.placeholder(sr.ruleID) } - body = strings.ReplaceAll(body, sr.secret, placeholder) - n += c - w.byRule[sr.ruleID] += c + + replaced, kept := c, 0 + if marked { + body, replaced, kept = replaceOutsideAllowedLines(body, sr.secret, placeholder) + } else { + body = strings.ReplaceAll(body, sr.secret, placeholder) + } + if kept > 0 { + w.allowed[sr.ruleID] += kept + w.kept[sr.secret] = struct{}{} + } + if replaced == 0 { + continue + } + n += replaced + w.byRule[sr.ruleID] += replaced w.located[sr.secret] = struct{}{} lastRule = sr.ruleID } @@ -535,6 +591,73 @@ func (w *rewriter) redactLeaf(s, path string) string { return out } +// allowSignatures are the inline markers that exempt a line from secret +// scanning in betterleaks and gitleaks. +var allowSignatures = []string{"betterleaks:allow", "gitleaks:allow"} + +// hasAllowMarker reports whether s carries one of allowSignatures. +func hasAllowMarker(s string) bool { + return slices.ContainsFunc(allowSignatures, func(sig string) bool { return strings.Contains(s, sig) }) +} + +// replaceOutsideAllowedLines substitutes placeholder for every occurrence of +// secret in body, the JSON-encoded form of a leaf, except the occurrences that +// lie within a line carrying an allow marker. It returns the new body and the +// number of occurrences replaced and kept. +func replaceOutsideAllowedLines(body, secret, placeholder string) (string, int, int) { + allowed := allowedLines(body) + + var ( + b strings.Builder + replaced, kept int + pos int + ) + for { + i := strings.Index(body[pos:], secret) + if i < 0 { + break + } + start, end := pos+i, pos+i+len(secret) + b.WriteString(body[pos:start]) + if within(allowed, start, end) { + b.WriteString(secret) + kept++ + } else { + b.WriteString(placeholder) + replaced++ + } + pos = end + } + b.WriteString(body[pos:]) + return b.String(), replaced, kept +} + +// allowedLines returns the lines of body, the JSON-encoded form of a string, +// that carry an allow marker. Lines are those of the decoded string: they end at +// an escaped line feed or carriage return. +func allowedLines(body string) []span { + var out []span + for start := 0; start <= len(body); { + line := cutAtEscape(body[start:], "nr") + if hasAllowMarker(line) { + out = append(out, span{start: start, end: start + len(line)}) + } + // Past the line and the two characters of its line break. + start += len(line) + 2 + } + return out +} + +// within reports whether [start, end) lies inside one of spans. +func within(spans []span, start, end int) bool { + for _, s := range spans { + if s.start <= start && end <= s.end { + return true + } + } + return false +} + // decodeObject parses doc into a value tree, keeping numbers in their original // textual form so re-encoding does not reformat them. func decodeObject(doc []byte) (map[string]any, error) { diff --git a/internal/redaction/redaction_test.go b/internal/redaction/redaction_test.go index abe76fc6b..7a030806e 100644 --- a/internal/redaction/redaction_test.go +++ b/internal/redaction/redaction_test.go @@ -470,6 +470,129 @@ func TestRedactText(t *testing.T) { } } +// TestRedactAllowMarkers checks that an allow marker keeps only the secrets on +// its own line, the line of the decoded string, and nothing else. +func TestRedactAllowMarkers(t *testing.T) { + const ( + secret = "s3cr3t-value-0123456789" + placeholder = "[CHAINLOOP_TRACE_REDACTED:test-token]" + ) + + testCases := []struct { + name string + // leaves are the string leaves of the document, in keys a, b, ... + leaves []string + opts []Option + want []string + wantByRule map[string]int + wantAllowed map[string]int + }{ + { + name: "marker on the line of the secret keeps it", + leaves: []string{"token := \"" + secret + "\" // gitleaks:allow"}, + opts: []Option{WithAllowMarkers()}, + want: []string{"token := \"" + secret + "\" // gitleaks:allow"}, + wantByRule: map[string]int{}, + wantAllowed: map[string]int{"test-token": 1}, + }, + { + name: "betterleaks marker keeps it too", + leaves: []string{"token: " + secret + " # betterleaks:allow"}, + opts: []Option{WithAllowMarkers()}, + want: []string{"token: " + secret + " # betterleaks:allow"}, + wantByRule: map[string]int{}, + wantAllowed: map[string]int{"test-token": 1}, + }, + { + name: "marker on another line of the same leaf does not", + leaves: []string{"1\ttoken := \"" + secret + "\"\n2\tother := x // gitleaks:allow"}, + opts: []Option{WithAllowMarkers()}, + want: []string{"1\ttoken := \"" + placeholder + "\"\n2\tother := x // gitleaks:allow"}, + wantByRule: map[string]int{"test-token": 1}, + wantAllowed: map[string]int{}, + }, + { + name: "carriage returns end a line too", + leaves: []string{"token := " + secret + "\r\nx // gitleaks:allow"}, + opts: []Option{WithAllowMarkers()}, + want: []string{"token := " + placeholder + "\r\nx // gitleaks:allow"}, + wantByRule: map[string]int{"test-token": 1}, + wantAllowed: map[string]int{}, + }, + { + name: "the same secret without a marker elsewhere in the leaf is replaced", + leaves: []string{"a := \"" + secret + "\" // gitleaks:allow\nexport TOKEN=" + secret}, + opts: []Option{WithAllowMarkers()}, + want: []string{"a := \"" + secret + "\" // gitleaks:allow\nexport TOKEN=" + placeholder}, + wantByRule: map[string]int{"test-token": 1}, + wantAllowed: map[string]int{"test-token": 1}, + }, + { + name: "marker in another leaf does not", + leaves: []string{"export TOKEN=" + secret, "x // gitleaks:allow"}, + opts: []Option{WithAllowMarkers()}, + want: []string{"export TOKEN=" + placeholder, "x // gitleaks:allow"}, + wantByRule: map[string]int{"test-token": 1}, + wantAllowed: map[string]int{}, + }, + { + name: "an escaped backslash before n is not a line break", + leaves: []string{`path C:\new ` + secret + ` // gitleaks:allow`}, + opts: []Option{WithAllowMarkers()}, + want: []string{`path C:\new ` + secret + ` // gitleaks:allow`}, + wantByRule: map[string]int{}, + wantAllowed: map[string]int{"test-token": 1}, + }, + { + name: "markers are ignored unless enabled", + leaves: []string{"token := \"" + secret + "\" // gitleaks:allow"}, + want: []string{"token := \"" + placeholder + "\" // gitleaks:allow"}, + wantByRule: map[string]int{"test-token": 1}, + wantAllowed: map[string]int{}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + scanner := &fakeScanner{findings: []Finding{{RuleID: "test-token", Secret: secret}}, requirePresent: true} + doc := map[string]string{} + for i, leaf := range tc.leaves { + doc[string(rune('a'+i))] = leaf + } + in, err := json.Marshal(doc) + require.NoError(t, err) + + out, report, err := New(scanner, tc.opts...).Redact(context.Background(), in) + require.NoError(t, err) + + var got map[string]string + require.NoError(t, json.Unmarshal(out, &got)) + for i, want := range tc.want { + assert.Equal(t, want, got[string(rune('a'+i))]) + } + assert.Equal(t, tc.wantByRule, report.ByRule) + assert.Equal(t, tc.wantAllowed, report.Allowed) + assert.Empty(t, report.Unlocated, "a kept secret is not an unlocated one") + }) + } +} + +// TestRedactTextAllowMarker checks a kept secret next to a replaced one in plain +// text, the shape of a captured spec. +func TestRedactTextAllowMarker(t *testing.T) { + scanner := &fakeScanner{findings: []Finding{ + {RuleID: "kept", Secret: "KEPT-0123456789"}, + {RuleID: "other", Secret: "OTHER-0123456789"}, + }, requirePresent: true} + + out, report, err := New(scanner, WithAllowMarkers()). + RedactText(context.Background(), "a KEPT-0123456789 // gitleaks:allow\nb OTHER-0123456789") + + require.NoError(t, err) + assert.Equal(t, "a KEPT-0123456789 // gitleaks:allow\nb [CHAINLOOP_TRACE_REDACTED:other]", out) + assert.Equal(t, 1, report.Replacements) +} + // TestRedactPlaceholderNeedingEscape checks that a placeholder is inserted in // its JSON-escaped form. A raw quote or backslash in it would break the string // the leaf is rebuilt from, and the whole leaf would then be lost to the diff --git a/pkg/attestation/crafter/materials/aicodingsession/redact.go b/pkg/attestation/crafter/materials/aicodingsession/redact.go index 0e0d20df8..8d07c96a7 100644 --- a/pkg/attestation/crafter/materials/aicodingsession/redact.go +++ b/pkg/attestation/crafter/materials/aicodingsession/redact.go @@ -76,13 +76,19 @@ var protectedPaths = []string{ // its original digest. The result is guaranteed to still validate against the AI // coding session schema; if it does not, redaction fails rather than uploading // either an invalid document or an unredacted one. +// +// A secret on a line with an inline gitleaks:allow or betterleaks:allow marker +// is kept, as a repository scan would keep it: an agent reading a test fixture +// marked that way is the common case. The marker is text in the transcript, so +// the agent could also write one next to a real secret; it covers only its +// own line, which is the same trust a repository scan gives it. func Redact(ctx context.Context, evidence []byte) ([]byte, *redaction.Report, error) { scanner, err := redaction.DefaultScanner() if err != nil { return nil, nil, fmt.Errorf("initialising the secret scanner: %w", err) } - redacted, report, err := redaction.New(scanner, redaction.WithPathFilter(eligible), redaction.WithOpaque(isMedia)).Redact(ctx, evidence) + redacted, report, err := redaction.New(scanner, redaction.WithPathFilter(eligible), redaction.WithOpaque(isMedia), redaction.WithAllowMarkers()).Redact(ctx, evidence) if err != nil { return nil, nil, err } @@ -99,8 +105,8 @@ func Redact(ctx context.Context, evidence []byte) ([]byte, *redaction.Report, er // RedactSpecText removes detected secrets from the text of one captured spec. // // A spec is stored as its own material rather than inside the session -// document, so it does not pass through Redact. It gets the same scanner and -// the same placeholders here, so a credential pasted into a ticket body is +// document, so it does not pass through Redact. It gets the same scanner, the +// same allow markers and the same placeholders here, so a credential pasted into a ticket body is // removed exactly as it would be from the transcript. Text with no detected // secrets is returned unchanged. func RedactSpecText(ctx context.Context, text string) (string, *redaction.Report, error) { @@ -109,7 +115,7 @@ func RedactSpecText(ctx context.Context, text string) (string, *redaction.Report return "", nil, fmt.Errorf("initialising the secret scanner: %w", err) } - return redaction.New(scanner).RedactText(ctx, text) + return redaction.New(scanner, redaction.WithAllowMarkers()).RedactText(ctx, text) } // eligible reports whether the string leaf at path may be rewritten. diff --git a/pkg/attestation/crafter/materials/aicodingsession/testdata/session-fp-shaped.json b/pkg/attestation/crafter/materials/aicodingsession/testdata/session-fp-shaped.json index 2f780f9a3..bc47ac637 100644 --- a/pkg/attestation/crafter/materials/aicodingsession/testdata/session-fp-shaped.json +++ b/pkg/attestation/crafter/materials/aicodingsession/testdata/session-fp-shaped.json @@ -79,6 +79,36 @@ "type": "assistant", "message": {"role": "assistant", "content": "Reading pkg/attestation/crafter/materials/aicodingsession/aicodingsession.go and running `go test ./... -run TestRedact`. Base64 of hello: aGVsbG8=. Escaped: & \"quotes\" and a tab\there."}, "timestamp": "2026-03-25T15:13:40.100Z" + }, + { + "type": "user", + "message": {"role": "user", "content": "Pushed the attestation.\n digest: sha256:9f8e7d6c5b4a39281706f5e4d3c2b1a0f9e8d7c69f8e7d6c5b4a39281706f5e4\n access sha256:9f8e7d6c5b4a39281706f5e4d3c2b1a0f9e8d7c69f8e7d6c5b4a39281706f5e4\nworkflow key: 3bf79921-3c03-81b6-afff-cb246849866f\nidempotency_key=3bf79921-3c03-81b6-afff-cb246849866f\n"}, + "timestamp": "2026-03-25T15:14:02.000Z" + }, + { + "type": "user", + "message": {"role": "user", "content": "- **Dex OIDC**: localhost:5556, users sarah@chainloop.local / john@chainloop.local (password: `password`)\n- **Database**: host: localhost, user: postgres\n"}, + "timestamp": "2026-03-25T15:14:03.000Z" + }, + { + "type": "user", + "message": {"role": "user", "content": "host: localhost\nuser: admin\n!function(e){var t={};e.password=t.password||n.password,e.user=u}\n"}, + "timestamp": "2026-03-25T15:14:04.000Z" + }, + { + "type": "assistant", + "message": {"role": "assistant", "content": "curl -H 'Authorization: Bearer REPLACE_ME' https://api.example.com\nissuer: http://localhost:8000@chainloop.local\n"}, + "timestamp": "2026-03-25T15:14:05.000Z" + }, + { + "type": "user", + "message": {"role": "user", "content": "| ai-config-no-secrets | fail | Secret (generic-password) detected [CHAINLOOP_TRACE_REDACTED:generic-password] |\npassword: [CHAINLOOP_TRACE_REDACTED:generic-password]\napi_key: [CHAINLOOP_TRACE_REDACTED:generic-api-key]\n"}, + "timestamp": "2026-03-25T15:14:06.000Z" + }, + { + "type": "user", + "message": {"role": "user", "content": "119\tconst pat = \"__GITHUB_PAT__\" // gitleaks:allow\n120\twantURI := \"https://tracker.example.com/issue/1?token=\" + pat\n"}, + "timestamp": "2026-03-25T15:14:07.000Z" } ] }, From 823fa199bfdeba99c6855591480199b96421167c Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Fri, 9 Oct 2026 13:34:56 +0200 Subject: [PATCH 2/3] fix(redaction): tighten allowlists and record the ruleset version Narrow two allowlists so real secrets of the same shape are still redacted: a password is exempt as a code expression only when it is made of property accesses joined by logical operators, and a numeric URI password only when the user part is a host name, as in http://localhost:8000@host. Report a copy of a secret in a protected field as unlocated even when another copy was kept by an allow marker. Exempt further values seen in real AI coding sessions: Go test durations, placeholders a transcript shows again (also in the legacy format), elided URI passwords, and matches that run on through an escaped line break or a \n in source code. Record the version of the detection rules in the new chainloop.material.redaction.ruleset annotation, so a policy can tell placeholders from the earlier default ruleset apart from current ones. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: f7d0aeca-fbc0-4795-ad11-21847f1d012f --- internal/redaction/betterleaks.go | 6 +++ internal/redaction/betterleaks.toml | 36 ++++++++++--- internal/redaction/precision_test.go | 52 ++++++++++++++++++- internal/redaction/redaction.go | 30 ++++++++--- internal/redaction/redaction_test.go | 15 ++++++ .../api/attestation/v1/crafting_state.go | 5 ++ pkg/attestation/crafter/crafter_test.go | 3 ++ .../materials/chainloop_ai_coding_session.go | 1 + ...inloop_ai_coding_session_redaction_test.go | 3 ++ 9 files changed, 133 insertions(+), 18 deletions(-) diff --git a/internal/redaction/betterleaks.go b/internal/redaction/betterleaks.go index 540e6de71..439952cb0 100644 --- a/internal/redaction/betterleaks.go +++ b/internal/redaction/betterleaks.go @@ -75,6 +75,12 @@ type betterleaksScanner struct { //go:embed betterleaks.toml var rulesConfig string +// RulesetVersion identifies what the scanner detects. Bump it whenever +// betterleaks.toml, the betterleaks version, or the handling of findings in +// this file changes what gets redacted, so that policies can tell placeholders +// from different rulesets apart. Version 1 is the plain default ruleset. +const RulesetVersion = 2 + var ( defaultScanner = sync.OnceValues(newBetterleaksScanner) // useRE2 selects the regex engine. It is process-wide and only affects diff --git a/internal/redaction/betterleaks.toml b/internal/redaction/betterleaks.toml index a72df6cdd..2e10355b8 100644 --- a/internal/redaction/betterleaks.toml +++ b/internal/redaction/betterleaks.toml @@ -22,15 +22,26 @@ regexTarget = "match" regexes = ['''(?i)\bsha(?:1|224|256|384|512):[0-9a-f]{40,128}\b'''] [[allowlists]] -description = "Code expressions and the bare key word, which are not password values" +description = "Code expressions, test durations and the bare key word, which are not password values" targetRules = ["generic-password"] +# The scanned text is JSON-encoded, so a match of this rule can run on through +# an escaped line break, or through a \n written in source code, which reaches +# the scanner as \\n. The scanner cuts the secret at the first, but only after +# these regexes ran, so each one also accepts such a tail. regexes = [ - # An expression such as t.password||n.password in minified JavaScript. - '''\|\||&&|\?\?''', - # A property access such as e.password or config.db_password. - '''^[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)*\.\w*(?i:passw(?:or)?d|pwd)\w*$''', + # An expression such as t.password||n.password in minified JavaScript: + # property accesses, and only those, joined by logical operators. + '''^[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)+(?:(?:\|\||&&|\?\?)[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)+)+(?:\\+[nrt].*)?$''', + # A property access such as e.password or config.db_password, alone or + # followed by logical operators and identifiers, as in e.password||n. + '''^[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)*\.\w*(?i:passw(?:or)?d|pwd)\w*(?:(?:\|\||&&|\?\?)[A-Za-z_$][\w$]*(?:\.[A-Za-z_$][\w$]*)*)*(?:\\+[nrt].*)?$''', + # A redaction placeholder the transcript shows again, in the current or + # the legacy format. + '''^\[(?:CHAINLOOP_TRACE_)?REDACTED(?::[^\]\s]*)?\](?:\\+[nrt].*)?$''', # Documentation such as (password: `password`). - '''(?i)^(?:password|passwd|pwd)$''', + '''(?i)^(?:password|passwd|pwd)(?:\\+[nrt].*)?$''', + # The duration of a Go test, as in "--- PASS: TestPassword: (0.01s)". + '''^\([0-9]+(?:\.[0-9]+)?(?:ns|us|µs|ms|s|m|h)\)(?:\\+[nrt].*)?$''', ] [[allowlists]] @@ -39,6 +50,15 @@ targetRules = ["curl-auth-header"] regexes = ['''^[A-Z][A-Z0-9]*(?:_[A-Z0-9]+)+$'''] [[allowlists]] -description = "A port number read as the password of a URI, such as http://localhost:8000@example.com" +description = "A host and port read as the user and password of a URI, such as http://localhost:8000@example.com" targetRules = ["generic-credential-uri"] -regexes = ['''^[0-9]{1,5}$'''] +regexTarget = "match" +# The "user" must be localhost, an IPv4 address or a dotted host name, so a +# numeric password next to a plain user name, as in admin:12345@, is still +# redacted. +regexes = ['''(?i)://(?:localhost|[0-9]{1,3}(?:\.[0-9]{1,3}){3}|[a-z0-9-]+(?:\.[a-z0-9-]+)+):[0-9]{1,5}@'''] + +[[allowlists]] +description = "An elided password in documentation, such as http://user:…@example.com" +targetRules = ["generic-credential-uri"] +regexes = ['''^(?:…|\.{3})$'''] diff --git a/internal/redaction/precision_test.go b/internal/redaction/precision_test.go index 1eb09e234..ef0d2b292 100644 --- a/internal/redaction/precision_test.go +++ b/internal/redaction/precision_test.go @@ -39,6 +39,7 @@ const ( ruleGenericAPIKey = "generic-api-key" ruleGitHubPAT = "github-pat" + ruleCredentialURI = "generic-credential-uri" ) // defaultRulesScanner returns a scanner with the betterleaks default ruleset and @@ -93,6 +94,41 @@ func TestRulesConfigKeepsNonSecrets(t *testing.T) { text: "- **Dex OIDC**: localhost:5556, users " + sampleEmail + " / john@chainloop.local (password: `password`)\n- **Database**: host: localhost, user: postgres\n", wantDefaultRule: genericPasswordRuleID, }, + { + name: "key word as the value, followed by another line", + text: "host: localhost\nuser: postgres\npassword: password\nport: 5432\n", + wantDefaultRule: genericPasswordRuleID, + }, + { + name: "key word as the value in a go string literal", + text: `text: "host: localhost\nuser: postgres\npassword: password\nport: 5432\n",` + "\n", + wantDefaultRule: genericPasswordRuleID, + }, + { + name: "minified javascript with a plain identifier operand", + text: "host: localhost\nuser: admin\n!function(e){e.password=e.password||n,e.user=u}\n", + wantDefaultRule: genericPasswordRuleID, + }, + { + name: "legacy placeholder shown again", + text: "host: localhost\nuser: admin\npassword: [REDACTED:generic-password]\nport: 5432\n", + wantDefaultRule: genericPasswordRuleID, + }, + { + name: "placeholder shown again in a go string literal", + text: `text: "host: localhost\nuser: admin\npassword: [CHAINLOOP_TRACE_REDACTED:generic-password]\nport: 5432\n",` + "\n", + wantDefaultRule: genericPasswordRuleID, + }, + { + name: "elided password of a uri in documentation", + text: "the issuer is `http://localhost:…@chainloop.local`\n", + wantDefaultRule: ruleCredentialURI, + }, + { + name: "duration of a go test", + text: "=== RUN TestRedactPassword\n--- PASS: TestRedactPassword: (0.01s)\nPASS\n", + wantDefaultRule: genericPasswordRuleID, + }, { name: "placeholder token in a curl example of a document", text: "curl -H 'Authorization: Bearer REPLACE_ME' https://api.example.com\n", @@ -101,7 +137,7 @@ func TestRulesConfigKeepsNonSecrets(t *testing.T) { { name: "port read as the password of a uri", text: "issuer: http://localhost:8000@chainloop.local\n", - wantDefaultRule: "generic-credential-uri", + wantDefaultRule: ruleCredentialURI, }, } @@ -159,11 +195,23 @@ func TestRulesConfigStillRedactsSecrets(t *testing.T) { secret: sampleAPIKey, wantRule: "curl-auth-header", }, + { + name: "password with a logical operator in it", + text: "host: db.internal\nuser: " + sampleEmail + "\npassword: Xk9m||Q2vL" + "p7wRt4z\n", + secret: "Xk9m||Q2vL" + "p7wRt4z", + wantRule: genericPasswordRuleID, + }, + { + name: "numeric password in a connection uri", + text: "dsn: postgres://admin:" + "12345@db.internal:5432/app\n", + secret: "admin:" + "12345@", + wantRule: ruleCredentialURI, + }, { name: "password in a connection uri", text: "dsn: postgres://admin:" + samplePassword + "@db.internal:5432/app\n", secret: samplePassword, - wantRule: "generic-credential-uri", + wantRule: ruleCredentialURI, }, } diff --git a/internal/redaction/redaction.go b/internal/redaction/redaction.go index 5395e175c..cd0687f9f 100644 --- a/internal/redaction/redaction.go +++ b/internal/redaction/redaction.go @@ -334,20 +334,24 @@ func (r *Redactor) Redact(ctx context.Context, doc []byte) ([]byte, *Report, err allowed: map[string]int{}, located: map[string]struct{}{}, kept: map[string]struct{}{}, + protected: map[string]struct{}{}, } w.rewriteMap(root, "") for _, s := range pending { _, located := w.located[s.secret] _, kept := w.kept[s.secret] + _, protected := w.protected[s.secret] switch { + // A copy in a protected leaf stays in the document whatever happened + // to the others, so it is reported even when another was kept. + case protected || (!located && !kept): + skip[s.secret] = struct{}{} + report.Unlocated[s.ruleID]++ case kept: // It is only left on allowed lines after this pass, so a further // pass would only find it again. skip[s.secret] = struct{}{} - case !located: - skip[s.secret] = struct{}{} - report.Unlocated[s.ruleID]++ } } for rule, n := range w.byRule { @@ -489,8 +493,11 @@ type rewriter struct { allowed map[string]int located map[string]struct{} // kept holds the secrets with an occurrence kept on an allowed line. - kept map[string]struct{} - count int + kept map[string]struct{} + // protected holds the secrets with an occurrence in a leaf the path filter + // protects, which therefore stays in the document. + protected map[string]struct{} + count int } func (w *rewriter) rewriteMap(m map[string]any, path string) { @@ -524,8 +531,9 @@ func (w *rewriter) rewrite(node any, path string) any { // characters `\n`. // // The path filter is consulted only once the leaf is known to hold a secret, and -// nothing is recorded for a leaf it protects. Nearly every leaf holds none, and -// the filter runs for each leaf on every pass otherwise. +// a leaf it protects is left as it is, with its secrets recorded as protected. +// Nearly every leaf holds none, and the filter runs for each leaf on every pass +// otherwise. func (w *rewriter) redactLeaf(s, path string) string { body, err := encodeStringBody(s) if err != nil { @@ -540,13 +548,19 @@ func (w *rewriter) redactLeaf(s, path string) string { // the whole leaf. marked = w.allowMarkers && hasAllowMarker(body) ) - for _, sr := range w.secrets { + for i, sr := range w.secrets { c := strings.Count(body, sr.secret) if c == 0 { continue } if !eligible { if !w.pathFilter(path) { + // The secrets before this one are not in the leaf. + for _, rest := range w.secrets[i:] { + if strings.Contains(body, rest.secret) { + w.protected[rest.secret] = struct{}{} + } + } return s } eligible = true diff --git a/internal/redaction/redaction_test.go b/internal/redaction/redaction_test.go index 7a030806e..8cea6ef43 100644 --- a/internal/redaction/redaction_test.go +++ b/internal/redaction/redaction_test.go @@ -577,6 +577,21 @@ func TestRedactAllowMarkers(t *testing.T) { } } +// TestRedactAllowMarkerDoesNotHideProtectedCopy checks that a copy of a secret +// in a protected leaf is still reported when another copy was kept by an allow +// marker. +func TestRedactAllowMarkerDoesNotHideProtectedCopy(t *testing.T) { + scanner := &fakeScanner{findings: []Finding{{RuleID: "r1", Secret: "SEC-0123456789"}}, requirePresent: true} + doc := `{"keepme":"SEC-0123456789","other":"x SEC-0123456789 // gitleaks:allow"}` + + _, report, err := New(scanner, WithAllowMarkers(), WithPathFilter(func(p string) bool { return p != "/keepme" })). + Redact(context.Background(), []byte(doc)) + + require.NoError(t, err) + assert.Equal(t, map[string]int{"r1": 1}, report.Allowed) + assert.Equal(t, map[string]int{"r1": 1}, report.Unlocated) +} + // TestRedactTextAllowMarker checks a kept secret next to a replaced one in plain // text, the shape of a captured spec. func TestRedactTextAllowMarker(t *testing.T) { diff --git a/pkg/attestation/crafter/api/attestation/v1/crafting_state.go b/pkg/attestation/crafter/api/attestation/v1/crafting_state.go index cb623d993..07ea4766e 100644 --- a/pkg/attestation/crafter/api/attestation/v1/crafting_state.go +++ b/pkg/attestation/crafter/api/attestation/v1/crafting_state.go @@ -73,6 +73,11 @@ var ( // AnnotationMaterialRedactionRules lists the detection rules that matched, // so a policy can act on the kind of credential that was present. AnnotationMaterialRedactionRules = CreateAnnotation("material.redaction.rules") + // AnnotationMaterialRedactionRuleset is the version of the detection rules + // the scan ran with, so a policy can tell placeholders from an older, less + // precise ruleset apart from current ones. Absent on materials redacted + // before it was introduced, which used version 1. + AnnotationMaterialRedactionRuleset = CreateAnnotation("material.redaction.ruleset") // AnnotationMaterialRedactionSkipped marks a material uploaded without // redaction because the operator explicitly asked for it. Recorded so the // bypass is visible to policies rather than silent. diff --git a/pkg/attestation/crafter/crafter_test.go b/pkg/attestation/crafter/crafter_test.go index 842f1bf39..4ab4b0700 100644 --- a/pkg/attestation/crafter/crafter_test.go +++ b/pkg/attestation/crafter/crafter_test.go @@ -27,6 +27,7 @@ import ( "os" "path/filepath" "slices" + "strconv" "strings" "testing" "time" @@ -35,6 +36,7 @@ import ( schemaapi "github.com/chainloop-dev/chainloop/app/controlplane/api/workflowcontract/v1" "github.com/chainloop-dev/chainloop/app/controlplane/pkg/unmarshal" + "github.com/chainloop-dev/chainloop/internal/redaction" "github.com/chainloop-dev/chainloop/pkg/attestation/crafter" v1 "github.com/chainloop-dev/chainloop/pkg/attestation/crafter/api/attestation/v1" "github.com/chainloop-dev/chainloop/pkg/attestation/crafter/materials" @@ -838,6 +840,7 @@ func (s *crafterSuite) TestAddMaterialCleanSessionIsMarkedScanned() { assert.Equal(s.T(), v1.AnnotationValueTrue, m.Annotations[v1.AnnotationMaterialRedacted]) assert.Equal(s.T(), "0", m.Annotations[v1.AnnotationMaterialRedactionCount]) + assert.Equal(s.T(), strconv.Itoa(redaction.RulesetVersion), m.Annotations[v1.AnnotationMaterialRedactionRuleset]) assert.NotContains(s.T(), m.Annotations, v1.AnnotationMaterialRedactionSkipped) // Nothing was replaced, so the digest is still that of the source file. diff --git a/pkg/attestation/crafter/materials/chainloop_ai_coding_session.go b/pkg/attestation/crafter/materials/chainloop_ai_coding_session.go index 891592063..852e8b986 100644 --- a/pkg/attestation/crafter/materials/chainloop_ai_coding_session.go +++ b/pkg/attestation/crafter/materials/chainloop_ai_coding_session.go @@ -200,6 +200,7 @@ func (c *ChainloopAICodingSessionCrafter) annotateRedaction(material *api.Attest material.Annotations[api.AnnotationMaterialRedacted] = api.AnnotationValueTrue material.Annotations[api.AnnotationMaterialRedactionCount] = strconv.Itoa(report.Replacements) + material.Annotations[api.AnnotationMaterialRedactionRuleset] = strconv.Itoa(redaction.RulesetVersion) if !report.Changed() { return diff --git a/pkg/attestation/crafter/materials/chainloop_ai_coding_session_redaction_test.go b/pkg/attestation/crafter/materials/chainloop_ai_coding_session_redaction_test.go index 3837cb16f..331d02150 100644 --- a/pkg/attestation/crafter/materials/chainloop_ai_coding_session_redaction_test.go +++ b/pkg/attestation/crafter/materials/chainloop_ai_coding_session_redaction_test.go @@ -27,6 +27,7 @@ import ( "testing" schemaapi "github.com/chainloop-dev/chainloop/app/controlplane/api/workflowcontract/v1" + "github.com/chainloop-dev/chainloop/internal/redaction" api "github.com/chainloop-dev/chainloop/pkg/attestation/crafter/api/attestation/v1" "github.com/chainloop-dev/chainloop/pkg/casclient" mUploader "github.com/chainloop-dev/chainloop/pkg/casclient/mocks" @@ -285,6 +286,7 @@ func TestChainloopAICodingSessionCrafterRedaction(t *testing.T) { if tc.skipRedaction { assert.Equal(t, "true", got.Annotations[api.AnnotationMaterialRedactionSkipped]) assert.NotContains(t, got.Annotations, api.AnnotationMaterialRedacted) + assert.NotContains(t, got.Annotations, api.AnnotationMaterialRedactionRuleset) assert.Contains(t, string(stored), awsKey) assert.Equal(t, sha256Digest(string(original)), got.GetArtifact().Digest) // Nothing was transformed, so nothing is held in memory for the @@ -293,6 +295,7 @@ func TestChainloopAICodingSessionCrafterRedaction(t *testing.T) { } else { // Marked redacted whenever the scan ran, even when it found nothing. assert.Equal(t, "true", got.Annotations[api.AnnotationMaterialRedacted]) + assert.Equal(t, strconv.Itoa(redaction.RulesetVersion), got.Annotations[api.AnnotationMaterialRedactionRuleset]) assert.NotContains(t, got.Annotations, api.AnnotationMaterialRedactionSkipped) // The scanned copy is what policies must be handed, since a From 49021d8455a092bab7629ec3eedcbc4af9aab288 Mon Sep 17 00:00:00 2001 From: Miguel Martinez Trivino Date: Fri, 9 Oct 2026 16:44:06 +0200 Subject: [PATCH 3/3] test(redaction): compare session redaction with a betterleaks file scan Run the real detector over files and over the same text as session leaves, and record where the two agree and where they differ: allow markers match line by line, while multi-line secrets, double-quoted values, values on the next line and some line filters differ because the session is scanned as JSON. Assisted-by: Claude Code Signed-off-by: Miguel Martinez Trivino Chainloop-Trace-Sessions: fbcf9691-c7f6-4c78-87be-6f91f64c34f1 --- internal/redaction/parity_test.go | 277 ++++++++++++++++++++++++++++++ 1 file changed, 277 insertions(+) create mode 100644 internal/redaction/parity_test.go diff --git a/internal/redaction/parity_test.go b/internal/redaction/parity_test.go new file mode 100644 index 000000000..7c4f03b91 --- /dev/null +++ b/internal/redaction/parity_test.go @@ -0,0 +1,277 @@ +// +// Copyright 2026 The Chainloop Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package redaction + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/betterleaks/betterleaks/config" + "github.com/betterleaks/betterleaks/detect" + "github.com/betterleaks/betterleaks/sources" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakePrivateKey is a private key block the private-key rule matches. Its +// lines are assembled from fragments, like the other synthetic credentials in +// this package. +var fakePrivateKey = "-----BEGIN RSA " + "PRIVATE KEY-----\n" + + strings.Repeat("MIIEvQIBADANBgkqhkiG9w0BAQEFAASC"+"BKcwggSjAgEAAoIBAQC7\n", 3) + + "-----END RSA " + "PRIVATE KEY-----" + +// fileVisible scans each text as its own file, the way betterleaks scans a +// directory, with the ruleset of the session scanner and allow markers +// honoured. It returns, for each of secrets in order, how many of its occurrences the +// scan does not report: the ones it does not detect and the ones an allow +// marker keeps. +func fileVisible(t *testing.T, texts, secrets []string) []int { + t.Helper() + useRE2() + cfg, err := config.ParseTOMLString(rulesConfig, "") + require.NoError(t, err) + d := detect.NewDetectorContext(context.Background(), cfg, detect.ValidationOptions{}) + d.Redact = 0 + d.MaxDecodeDepth = 0 + d.SkipFindingAppend = true + + visible := make([]int, len(secrets)) + for _, text := range texts { + // An occurrence is identified by its line, so that a secret two rules + // report on the same line counts once. + reported := map[string]map[int]struct{}{} + report := func(secret string, line int) { + if reported[secret] == nil { + reported[secret] = map[int]struct{}{} + } + reported[secret][line] = struct{}{} + } + for r := range d.Run(context.Background(), fileSource{text: text}) { + require.NoError(t, r.Err) + report(r.Finding.Secret, r.Finding.StartLine) + for _, set := range r.Finding.ComponentSets { + for _, c := range set.Components { + if c != nil && !c.Optional { + report(c.Secret, c.StartLine) + } + } + } + } + for i, s := range secrets { + visible[i] += strings.Count(text, s) - len(reported[s]) + } + } + return visible +} + +// fileSource is a single file's content. +type fileSource struct{ text string } + +func (s fileSource) Fragments(_ context.Context, yield sources.FragmentsFunc) error { + return yield(sources.Fragment{Raw: s.text}, nil) +} + +// sessionVisible redacts a session document whose string leaves are texts, the +// way an AI coding session is redacted, and returns how many occurrences of +// each of secrets, in order, are left in it. +func sessionVisible(t *testing.T, texts, secrets []string) []int { + t.Helper() + scanner, err := DefaultScanner() + require.NoError(t, err) + + doc, err := json.Marshal(map[string][]string{"content": texts}) + require.NoError(t, err) + + out, _, err := New(scanner, WithAllowMarkers()).Redact(context.Background(), doc) + require.NoError(t, err) + + var got map[string][]string + require.NoError(t, json.Unmarshal(out, &got)) + visible := make([]int, len(secrets)) + for _, leaf := range got["content"] { + for i, s := range secrets { + visible[i] += strings.Count(leaf, s) + } + } + return visible +} + +// TestSessionRedactionMatchesFileScan compares the real detector over files +// with the session redaction of the same text as string leaves. The ruleset is +// the same on both sides, so the comparison is about where the text is: a file +// has real line breaks, a session leaf is a JSON string where a line break is +// the escape \n and a quote is \". +// +// Each case gives, per secret, the occurrences left visible. A case with a +// difference documents why the session does not do what a file scan does. +func TestSessionRedactionMatchesFileScan(t *testing.T) { + testCases := []struct { + name string + // texts are the files of the file scan, and the leaves of the session. + texts []string + secrets []string + // wantFile and wantSession are the occurrences of each of secrets left + // visible, in the same order. + wantFile []int + wantSession []int + // difference explains why wantSession is not wantFile. + difference string + }{ + { + name: "a secret without a marker is reported", + texts: []string{"export GITHUB_TOKEN=" + fakeGitHubPAT + "\n"}, + secrets: []string{fakeGitHubPAT}, + wantFile: []int{0}, + }, + { + name: "a marker on the line of the secret keeps it", + texts: []string{"const pat = '" + fakeGitHubPAT + "' // gitleaks:allow\n"}, + secrets: []string{fakeGitHubPAT}, + wantFile: []int{1}, + }, + { + name: "the betterleaks marker keeps it too", + texts: []string{"token: " + fakeGitHubPAT + " # betterleaks:allow\n"}, + secrets: []string{fakeGitHubPAT}, + wantFile: []int{1}, + }, + { + name: "a marker on the next line does not keep it", + texts: []string{"const pat = '" + fakeGitHubPAT + "'\n// gitleaks:allow\n"}, + secrets: []string{fakeGitHubPAT}, + wantFile: []int{0}, + }, + { + name: "a marker on the previous line does not keep it", + texts: []string{"// gitleaks:allow\nconst pat = '" + fakeGitHubPAT + "'\n"}, + secrets: []string{fakeGitHubPAT}, + wantFile: []int{0}, + }, + { + name: "a marker keeps only the occurrence on its line", + texts: []string{"a = '" + fakeGitHubPAT + "' # gitleaks:allow\nexport GITHUB_TOKEN=" + fakeGitHubPAT + "\n"}, + secrets: []string{fakeGitHubPAT}, + wantFile: []int{1}, + }, + { + name: "a marker in another file or leaf does not keep it", + texts: []string{"export GITHUB_TOKEN=" + fakeGitHubPAT + "\n", "x // gitleaks:allow\n"}, + secrets: []string{fakeGitHubPAT}, + wantFile: []int{0}, + }, + { + name: "an unquoted password", + texts: []string{"password: " + samplePassword + "\n"}, + secrets: []string{samplePassword}, + wantFile: []int{0}, + }, + { + name: "a single-quoted key", + texts: []string{"api_key = '" + sampleAPIKey + "'\n"}, + secrets: []string{sampleAPIKey}, + wantFile: []int{0}, + }, + { + name: "an AWS key pair", + texts: []string{"AWS_ACCESS_KEY_ID=" + fakeAWSKey + "\nAWS_SECRET_ACCESS_KEY=" + fakeAWSSecret + "\n"}, + secrets: []string{fakeAWSKey, fakeAWSSecret}, + wantFile: []int{0, 0}, + }, + { + // The file scan drops the whole pair, then reports the secret key with + // the generic rule. The session keeps only the key id. Either way + // only the id is left. + name: "a marker on the AWS key id line keeps the id only", + texts: []string{"AWS_ACCESS_KEY_ID=" + fakeAWSKey + " # gitleaks:allow\nAWS_SECRET_ACCESS_KEY=" + fakeAWSSecret + "\n"}, + secrets: []string{fakeAWSKey, fakeAWSSecret}, + wantFile: []int{1, 0}, + }, + { + name: "a private key", + texts: []string{fakePrivateKey + "\n"}, + secrets: []string{fakePrivateKey}, + wantFile: []int{0}, + }, + { + name: "a marker on the last line of a private key", + texts: []string{fakePrivateKey + " # gitleaks:allow\n"}, + secrets: []string{fakePrivateKey}, + wantFile: []int{1}, + wantSession: []int{0}, + difference: "betterleaks checks the marker against every line of a match, so one marker " + + "keeps a whole multi-line secret; the session redacts a secret that spans lines", + }, + { + name: "a double-quoted key", + texts: []string{"const api_key = \"" + sampleAPIKey + "\"\n"}, + secrets: []string{sampleAPIKey}, + wantFile: []int{0}, + wantSession: []int{1}, + difference: "known gap: the session scans JSON, where the quote is \\\" and the generic rules do not allow a backslash before the value", + }, + { + name: "a double-quoted password", + texts: []string{"password = \"" + samplePassword + "\"\n"}, + secrets: []string{samplePassword}, + wantFile: []int{0}, + wantSession: []int{1}, + difference: "known gap: the session scans JSON, where the quote is \\\"", + }, + { + name: "a value on the line after its key", + texts: []string{"api_key:\n " + sampleAPIKey + "\n"}, + secrets: []string{sampleAPIKey}, + wantFile: []int{0}, + wantSession: []int{1}, + difference: "known gap: in JSON the line break is the escape \\n, which the rules do not read as whitespace", + }, + { + name: "an import on another line of the text", + texts: []string{"import { a } from 'b'\napi_key = " + sampleAPIKey + "\n"}, + secrets: []string{sampleAPIKey}, + wantFile: []int{0}, + wantSession: []int{1}, + difference: "known gap: the generic-api-key rule drops a finding whose line is an import, " + + "and a whole session leaf is one line", + }, + { + name: "an AWS key id at the start of a line", + texts: []string{"x\n" + fakeAWSKey + "\naws_secret = " + fakeAWSSecret + "\n"}, + secrets: []string{fakeAWSKey, fakeAWSSecret}, + wantFile: []int{0, 0}, + wantSession: []int{1, 0}, + difference: "known gap: after the escape \\n the id follows the letter n, so the rule's word boundary does not match", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + wantSession := tc.wantSession + if tc.difference == "" { + wantSession = tc.wantFile + } else { + require.NotEqual(t, tc.wantFile, wantSession, "a case with a difference expects one") + } + + // Compared as counts by position, so that a failure does not print a + // secret. + assert.Equal(t, tc.wantFile, fileVisible(t, tc.texts, tc.secrets), "file scan") + assert.Equal(t, wantSession, sessionVisible(t, tc.texts, tc.secrets), "session") + }) + } +}