From f5b88ddd0285415852ea5f6d6280ea94a97e345b Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Mon, 31 Aug 2026 19:25:59 +0800 Subject: [PATCH 1/7] fix: redact the default admin trace and stop the shape sweep mangling prose The non-verbose admin trace rendering printed the raw query string, error, message and annotations of every traced request unredacted, in both the text and the JSON form: a presigned URL's signature, an STS session token or a proxy token= parameter of any client the server traced reached the operator's terminal. Only the verbose path went through redactTraceText. Both renderings now work on redacted copies of those fields. The verbose path passed the raw query without its leading "?", so the query shape - anchored on the [?&] before a parameter name - missed a credential in the first parameter. redactTraceQuery puts the anchor back. Three free-text shapes matched ordinary prose and were applied to every error message: "AWS S3 compatible" became "AWS **REDACTED** compatible", "token has expired" became "token **REDACTED** expired", "Basic auth is disabled" became "Basic **REDACTED** is disabled". The SigV4 shape now requires its Credential field, the SigV2 shape the access-key:signature colon, and the scheme shape a payload that looks like a token rather than the next word of a sentence. The registry no longer records placeholder values such as auth_token=off or client_secret=true, which turned every later "off" and "true" into a marker, and admin config set now registers and masks credentials embedded in values whose key names none: a DSN password, a URL's userinfo, a webhook ?token=. An Authorization payload after a doubled space is registered trimmed. Found by the pre-release adversarial review. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VX7dtHJ4q7GJ1YDYeXLmjW Signed-off-by: Feng Ruohang --- cmd/admin-trace.go | 70 ++++++++-- cmd/client-s3-trace-redact.go | 68 ++++++++-- cmd/redaction-prose_test.go | 239 ++++++++++++++++++++++++++++++++++ cmd/secret-registry.go | 57 +++++++- 4 files changed, 405 insertions(+), 29 deletions(-) create mode 100644 cmd/redaction-prose_test.go diff --git a/cmd/admin-trace.go b/cmd/admin-trace.go index f48dd83e5c..2f8cc7fb9a 100644 --- a/cmd/admin-trace.go +++ b/cmd/admin-trace.go @@ -700,21 +700,29 @@ func shortTrace(ti madmin.ServiceTraceInfo) shortTraceMsg { s := shortTraceMsg{} t := ti.Trace + // The server supplies every text field verbatim for other clients' + // requests; the short rendering must withhold credentials exactly like + // the verbose one. Work on redacted copies - the event is shared. + var eventSecrets []string + if t.HTTP != nil { + eventSecrets = traceEventSecrets(t.HTTP.ReqInfo.Headers, t.HTTP.RespInfo.Headers) + } + s.trcType = t.TraceType s.Type = t.TraceType.String() s.FuncName = t.FuncName s.Time = t.Time s.Path = t.Path - s.Error = t.Error + s.Error = redactTraceText(t.Error, eventSecrets) s.Host = t.NodeName s.Duration = t.Duration - s.StatusMsg = t.Message - s.Extra = t.Custom + s.StatusMsg = redactTraceText(t.Message, eventSecrets) + s.Extra = redactTraceCustom(t.Custom, eventSecrets) s.Size = t.Bytes switch t.TraceType { case madmin.TraceS3, madmin.TraceInternal: - s.Query = t.HTTP.ReqInfo.RawQuery + s.Query = redactTraceQuery(t.HTTP.ReqInfo.RawQuery, eventSecrets) s.StatusCode = t.HTTP.RespInfo.StatusCode s.StatusMsg = http.StatusText(t.HTTP.RespInfo.StatusCode) s.Client = t.HTTP.ReqInfo.Client @@ -819,6 +827,10 @@ func colorizedNodeName(nodeName string) string { } func (t traceMessage) JSON() string { + var eventSecrets []string + if t.Trace.HTTP != nil { + eventSecrets = traceEventSecrets(t.Trace.HTTP.ReqInfo.Headers, t.Trace.HTTP.RespInfo.Headers) + } trc := verboseTrace{ trcType: t.Trace.TraceType, Type: t.Trace.TraceType.String(), @@ -827,10 +839,10 @@ func (t traceMessage) JSON() string { Time: t.Trace.Time, Duration: t.Trace.Duration, Path: t.Trace.Path, - Error: t.Trace.Error, + Error: redactTraceText(t.Trace.Error, eventSecrets), HealResult: t.Trace.HealResult, - Message: t.Trace.Message, - Extra: t.Trace.Custom, + Message: redactTraceText(t.Trace.Message, eventSecrets), + Extra: redactTraceCustom(t.Trace.Custom, eventSecrets), } if t.Trace.HTTP != nil { @@ -850,14 +862,13 @@ func (t traceMessage) JSON() string { for k, v := range redactHeaderMap(rs.Headers) { rspHdrs[k] = strings.Join(v, " ") } - eventSecrets := traceEventSecrets(rq.Headers, rs.Headers) trc.RequestInfo = &requestInfo{ Time: rq.Time, Proto: rq.Proto, Method: rq.Method, Path: rq.Path, - RawQuery: redactTraceText(rq.RawQuery, eventSecrets), + RawQuery: redactTraceQuery(rq.RawQuery, eventSecrets), Body: redactTraceText(string(rq.Body), eventSecrets), Headers: rqHdrs, } @@ -889,13 +900,23 @@ func (t traceMessage) String() string { var nodeNameStr string b := &strings.Builder{} + // Render a redacted copy: the server supplies the error, message and + // annotations verbatim for other clients' requests, and the event is + // shared with the JSON path. trc := t.Trace + var eventSecrets []string + if trc.HTTP != nil { + eventSecrets = traceEventSecrets(trc.HTTP.ReqInfo.Headers, trc.HTTP.RespInfo.Headers) + } + trc.Error = redactTraceText(trc.Error, eventSecrets) + trc.Message = redactTraceText(trc.Message, eventSecrets) + trc.Custom = redactTraceCustom(trc.Custom, eventSecrets) if trc.NodeName != "" { nodeNameStr = fmt.Sprintf("%s ", colorizedNodeName(trc.NodeName)) } extra := "" - if len(t.Trace.Custom) > 0 { - for k, v := range t.Trace.Custom { + if len(trc.Custom) > 0 { + for k, v := range trc.Custom { extra = fmt.Sprintf("%s %s=%s", extra, k, v) } extra = console.Colorize("Extra", extra) @@ -927,12 +948,11 @@ func (t traceMessage) String() string { // is shared with the JSON path and must not be mutated. reqHeaders := redactHeaderMap(ri.Headers) respHeaders := redactHeaderMap(rs.Headers) - eventSecrets := traceEventSecrets(ri.Headers, rs.Headers) fmt.Fprintf(b, "%s%s", nodeNameStr, console.Colorize("Request", fmt.Sprintf("[REQUEST %s] ", trc.FuncName))) fmt.Fprintf(b, "[%s] %s\n", ri.Time.Local().Format(traceTimeFormat), console.Colorize("Host", fmt.Sprintf("[Client IP: %s]", ri.Client))) fmt.Fprintf(b, "%s%s", nodeNameStr, console.Colorize("Method", fmt.Sprintf("%s %s", ri.Method, ri.Path))) if ri.RawQuery != "" { - fmt.Fprintf(b, "?%s", redactTraceText(ri.RawQuery, eventSecrets)) + fmt.Fprintf(b, "?%s", redactTraceQuery(ri.RawQuery, eventSecrets)) } fmt.Fprint(b, "\n") fmt.Fprintf(b, "%s%s", nodeNameStr, console.Colorize("Method", fmt.Sprintf("Proto: %s\n", ri.Proto))) @@ -1075,3 +1095,27 @@ func redactTraceText(text string, eventSecrets []string) string { text = redactSecretValues(text, eventSecrets) return scrubKnownSecrets(text) } + +// redactTraceQuery redacts a raw query string. The query shape keys on the +// "?" or "&" in front of a parameter name, so the "?" the trace strips is put +// back for the scan; a credential in the first parameter is then caught too. +func redactTraceQuery(rawQuery string, eventSecrets []string) string { + if rawQuery == "" { + return "" + } + redacted := redactTraceText("?"+rawQuery, eventSecrets) + return strings.TrimPrefix(redacted, "?") +} + +// redactTraceCustom returns a redacted copy of an event's custom annotations; +// the event is shared with the other rendering and is left untouched. +func redactTraceCustom(custom map[string]string, eventSecrets []string) map[string]string { + if len(custom) == 0 { + return custom + } + redacted := make(map[string]string, len(custom)) + for key, value := range custom { + redacted[key] = redactTraceText(value, eventSecrets) + } + return redacted +} diff --git a/cmd/client-s3-trace-redact.go b/cmd/client-s3-trace-redact.go index df8b8b0d8c..78fc49c99c 100644 --- a/cmd/client-s3-trace-redact.go +++ b/cmd/client-s3-trace-redact.go @@ -51,24 +51,52 @@ const redactedMarker = "**REDACTED**" var traceAuthSchemeRegexp = regexp.MustCompile(`^[A-Za-z0-9!#$%&'*+.^_` + "`" + `|~-]+`) // Credential shapes in free text. Each replacement keeps at most the token -// that identifies the shape and withholds the rest. +// that identifies the shape and withholds the rest. A shape that could also +// match ordinary prose - "Basic auth is disabled", "AWS S3 compatible", +// "the token has expired" - is anchored on something only a credential +// carries, so the error messages this client composes keep their meaning. var traceTextShapes = []struct { pattern *regexp.Regexp replacement string + // accept, when set, must approve the captured payload (group 2) before + // the match is replaced. + accept func(payload string) bool }{ - // A whole Signature v4 value echoed into text. - {regexp.MustCompile(`\bAWS4-HMAC-SHA256\b[^\n"'<>]*`), "AWS4-HMAC-SHA256 " + redactedMarker}, + // A whole Signature v4 value echoed into text: the scheme followed, on the + // same line, by its Credential field. + {pattern: regexp.MustCompile(`\bAWS4-HMAC-SHA256\b[^\n"'<>]*?\bCredential=[^\n"'<>]*`), replacement: "AWS4-HMAC-SHA256 " + redactedMarker}, // Its fields on their own. - {regexp.MustCompile(`\bCredential=[^,\s"'<>]+`), "Credential=" + redactedMarker}, - {regexp.MustCompile(`\bSignedHeaders=[^,\s"'<>]+`), "SignedHeaders=" + redactedMarker}, - {regexp.MustCompile(`\bSignature=[^,\s"'<>&]+`), "Signature=" + redactedMarker}, - // Signature v2: "AWS :". - {regexp.MustCompile(`\bAWS [^\s"'<>]+`), "AWS " + redactedMarker}, - // Scheme-prefixed tokens. - {regexp.MustCompile(`\b((?i:Bearer|Basic|Digest|Negotiate|NTLM|Token)) [^\s"'<>,]+`), "$1 " + redactedMarker}, + {pattern: regexp.MustCompile(`\bCredential=[^,\s"'<>]+`), replacement: "Credential=" + redactedMarker}, + {pattern: regexp.MustCompile(`\bSignedHeaders=[^,\s"'<>]+`), replacement: "SignedHeaders=" + redactedMarker}, + {pattern: regexp.MustCompile(`\bSignature=[^,\s"'<>&]+`), replacement: "Signature=" + redactedMarker}, + // Signature v2: "AWS :". The colon separates it + // from prose such as "AWS S3 compatible". + {pattern: regexp.MustCompile(`\bAWS [^\s:"'<>]+:[^\s"'<>]+`), replacement: "AWS " + redactedMarker}, + // Scheme-prefixed tokens, when the payload looks like a token rather than + // the next word of a sentence. + {pattern: regexp.MustCompile(`\b((?i:Bearer|Basic|Digest|Negotiate|NTLM|Token)) ([^\s"'<>,]+)`), replacement: "$1 " + redactedMarker, accept: looksLikeCredentialPayload}, // Credential-bearing query parameters wherever a URL appears, matched by // name fragment so a parameter this client never sends is still caught. - {regexp.MustCompile(`([?&][^=&\s"'<>]*(?i:token|signature|credential|secret|password|api[-_]?key|auth|sig|key)[^=&\s"'<>]*=)[^&\s"'<>]+`), "${1}" + redactedMarker}, + {pattern: regexp.MustCompile(`([?&][^=&\s"'<>]*(?i:token|signature|credential|secret|password|api[-_]?key|auth|sig|key)[^=&\s"'<>]*=)[^&\s"'<>]+`), replacement: "${1}" + redactedMarker}, +} + +// looksLikeCredentialPayload tells a token after a scheme word from the next +// word of a sentence. Tokens are long and mix character classes; a sentence +// continues with a short word or a lowercase one ("Basic auth is disabled", +// "token has expired", "Negotiate authentication"). +func looksLikeCredentialPayload(payload string) bool { + if len(payload) < 8 { + return false + } + for i, r := range payload { + switch { + case r >= '0' && r <= '9', strings.ContainsRune("+/=._~-", r): + return true + case r >= 'A' && r <= 'Z' && i > 0: + return true + } + } + return false } // isSecretHeaderName reports whether a header's value is credential material. @@ -225,7 +253,18 @@ func redactURLString(raw string) string { // endpoint echoed back with different spacing or framing. func scrubCredentialText(text string) string { for _, shape := range traceTextShapes { - text = shape.pattern.ReplaceAllString(text, shape.replacement) + if shape.accept == nil { + text = shape.pattern.ReplaceAllString(text, shape.replacement) + continue + } + pattern, replacement, accept := shape.pattern, shape.replacement, shape.accept + text = pattern.ReplaceAllStringFunc(text, func(match string) string { + groups := pattern.FindStringSubmatch(match) + if len(groups) < 3 || !accept(groups[2]) { + return match + } + return pattern.ReplaceAllString(match, replacement) + }) } return text } @@ -260,7 +299,10 @@ func authorizationSecretValues(value string) []string { secrets = append(secrets, key, signature) } } - if scheme, payload, ok := strings.Cut(value, " "); ok && payload != "" { + // A doubled space after the scheme ("Bearer token") must not leave the + // payload registered with a leading space it is never echoed with. + if scheme, payload, ok := strings.Cut(value, " "); ok && strings.TrimSpace(payload) != "" { + payload = strings.TrimSpace(payload) secrets = append(secrets, payload) if strings.EqualFold(scheme, "Basic") { if decoded, err := base64.StdEncoding.DecodeString(payload); err == nil { diff --git a/cmd/redaction-prose_test.go b/cmd/redaction-prose_test.go new file mode 100644 index 0000000000..b61708ec09 --- /dev/null +++ b/cmd/redaction-prose_test.go @@ -0,0 +1,239 @@ +// Copyright (c) 2026 PGSTY +// +// This file is part of the Silo object storage client. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package cmd + +import ( + "net/http" + "strings" + "testing" + "time" + + "github.com/minio/madmin-go/v3" +) + +// The credential shapes are swept over every error message, so a shape that +// also matches prose would mangle messages this client composes itself. +func TestScrubCredentialTextKeepsProse(t *testing.T) { + resetSecretRegistryForTest() + t.Cleanup(resetSecretRegistryForTest) + for _, text := range []string{ + "Invalid configuration for AWS S3 compatible remote tier", + "cannot be combined with AWS role authentication", + "access token not found in response", + "The provided token has expired", + "Content-MD5 digest mismatch for part 3", + "Basic auth is disabled", + "AWS4-HMAC-SHA256 is required for this region", + "Negotiate authentication is not supported", + "Token expired", + "TOKEN EXPIRED", + "NTLM is deprecated", + } { + if got := scrubSecretsFromOutput(text); got != text { + t.Errorf("scrubSecretsFromOutput(%q) = %q, want it unchanged", text, got) + } + } +} + +// Tightening the shapes must not let an echoed credential through. +func TestScrubCredentialTextStillRedactsCredentialShapes(t *testing.T) { + for text, want := range map[string]string{ + "got Bearer eyJhbGciOiJIUzI1NiJ9.payload.signature back": "got Bearer " + redactedMarker + " back", + "Basic dXNlcjpwYXNz": "Basic " + redactedMarker, + "Proxy-Authorization: Basic dXNlcjpodW50ZXIy": "Proxy-Authorization: Basic " + redactedMarker, + "AWS AKIAIOSFODNN7EXAMPLE:frJIUN8DYpKDtOLCwo//yllqDzg=": "AWS " + redactedMarker, + "AWS a/b:sig0123456789": "AWS " + redactedMarker, + "Token 0123456789abcdef": "Token " + redactedMarker, + "Negotiate YIIFmQYGKwYBBQUCoIIFjTCCBYmgJDAi": "Negotiate " + redactedMarker, + "AWS4-HMAC-SHA256 Credential=k/20260831/us-east-1/s3/aws4_request, SignedHeaders=host, Signature=ab12": "AWS4-HMAC-SHA256 " + redactedMarker, + "AWS4-HMAC-SHA256 SECRETVALUE0123 Credential=k/20260831/us-east-1/s3/aws4_request": "AWS4-HMAC-SHA256 " + redactedMarker, + "scope Credential=k/20260831/us-east-1/s3/aws4_request only": "scope Credential=" + redactedMarker + " only", + "http://h/x?X-Amz-Security-Token=PROBE0123&y=1": "http://h/x?X-Amz-Security-Token=" + redactedMarker + "&y=1", + } { + if got := scrubCredentialText(text); got != want { + t.Errorf("scrubCredentialText(%q) = %q, want %q", text, got, want) + } + } +} + +func TestLooksLikeCredentialPayload(t *testing.T) { + for payload, want := range map[string]bool{ + "auth": false, + "authentication": false, + "Authentication": false, + "expired": false, + "mismatch": false, + "dXNlcjpwYXNz": true, + "0123456789abcdef": true, + "eyJhbGciOiJIUzI1NiJ9.": true, + "some-long-secret": true, + "ABCDEFGHIJKLMNOP": true, + } { + if got := looksLikeCredentialPayload(payload); got != want { + t.Errorf("looksLikeCredentialPayload(%q) = %v, want %v", payload, got, want) + } + } +} + +// A trace's raw query has no leading "?", so a credential in the first +// parameter needs the anchor put back before the query shape can see it. +func TestRedactTraceQueryRedactsFirstParameter(t *testing.T) { + for raw, want := range map[string]string{ + "": "", + "X-Amz-Security-Token=PROBESESSIONTOKEN0123&X-Amz-Signature=SIG0123&versionId=keep": "X-Amz-Security-Token=" + redactedMarker + "&X-Amz-Signature=" + redactedMarker + "&versionId=keep", + "token=abc0123&x=1": "token=" + redactedMarker + "&x=1", + "AWSAccessKeyId=AKIA0123": "AWSAccessKeyId=" + redactedMarker, + "versionId=keep&list-type=2": "versionId=keep&list-type=2", + } { + if got := redactTraceQuery(raw, nil); got != want { + t.Errorf("redactTraceQuery(%q) = %q, want %q", raw, got, want) + } + } +} + +func shortTraceProbeEvent(sentinel string) madmin.ServiceTraceInfo { + return madmin.ServiceTraceInfo{ + Trace: madmin.TraceInfo{ + TraceType: madmin.TraceS3, + NodeName: "node1", + FuncName: "s3.GetObject", + Time: time.Unix(100, 0).UTC(), + Path: "/bucket/object", + Error: "denied for Bearer " + sentinel, + Message: "token=" + sentinel, + Custom: map[string]string{"note": "Authorization: Bearer " + sentinel}, + HTTP: &madmin.TraceHTTPStats{ + ReqInfo: madmin.TraceRequestInfo{ + Time: time.Unix(100, 0).UTC(), + Proto: "HTTP/1.1", + Method: http.MethodGet, + Path: "/bucket/object", + RawQuery: "X-Amz-Security-Token=" + sentinel + "&X-Amz-Signature=" + sentinel + "&versionId=keep-version", + Headers: map[string][]string{"X-Amz-Security-Token": {sentinel}, "User-Agent": {"keep-user-agent"}}, + Client: "10.0.0.1", + }, + RespInfo: madmin.TraceResponseInfo{ + Time: time.Unix(101, 0).UTC(), + StatusCode: http.StatusForbidden, + }, + }, + }, + } +} + +// The default (non-verbose) `admin trace` rendering prints the request's +// query string, error and annotations; it must withhold credentials exactly +// like the verbose rendering, and must not modify the shared event. +func TestShortTraceRedactsQueryErrorAndCustom(t *testing.T) { + const sentinel = "SHORTTRACESENTINEL0123456789" + event := shortTraceProbeEvent(sentinel) + short := shortTrace(event) + for name, out := range map[string]string{"text": short.String(), "json": short.JSON()} { + if strings.Contains(out, sentinel) { + t.Errorf("short trace %s output leaks the sentinel:\n%s", name, out) + } + if !strings.Contains(out, "versionId=keep-version") { + t.Errorf("short trace %s output lost the harmless query parameter:\n%s", name, out) + } + } + if short.Query != "X-Amz-Security-Token="+redactedMarker+"&X-Amz-Signature="+redactedMarker+"&versionId=keep-version" { + t.Errorf("short trace query = %q", short.Query) + } + if !strings.Contains(event.Trace.HTTP.ReqInfo.RawQuery, sentinel) || !strings.Contains(event.Trace.Custom["note"], sentinel) || !strings.Contains(event.Trace.Error, sentinel) { + t.Errorf("the shared event was modified: %+v", event.Trace) + } +} + +// The verbose rendering must catch a credential in the first query parameter +// and in the event's error, message and custom annotations. +func TestVerboseTraceRedactsLeadingQueryTokenAndAnnotations(t *testing.T) { + const sentinel = "VERBOSETRACESENTINEL0123456789" + msg := traceMessage{Status: "success", ServiceTraceInfo: shortTraceProbeEvent(sentinel)} + for name, out := range map[string]string{"text": msg.String(), "json": msg.JSON()} { + if strings.Contains(out, sentinel) { + t.Errorf("verbose trace %s output leaks the sentinel:\n%s", name, out) + } + if !strings.Contains(out, "versionId=keep-version") || !strings.Contains(out, "keep-user-agent") { + t.Errorf("verbose trace %s output lost harmless content:\n%s", name, out) + } + } + if !strings.Contains(msg.Trace.HTTP.ReqInfo.RawQuery, sentinel) || !strings.Contains(msg.Trace.Custom["note"], sentinel) { + t.Errorf("the shared event was modified: %+v", msg.Trace) + } +} + +// "auth_token=off" and "client_secret=true" carry no secret; registering +// them would redact every later "off" and "true". +func TestRegisterSecretIgnoresPlaceholders(t *testing.T) { + resetSecretRegistryForTest() + t.Cleanup(resetSecretRegistryForTest) + registerKeyValueSecrets([]string{"auth_token=off", "client_secret=true", `token="none"`, "password=Enabled"}) + registerSecret("default", "OFF", "unset") + const text = "turn it off; is it true? none by default; OFF and unset stay; Enabled too" + if got := scrubKnownSecrets(text); got != text { + t.Errorf("placeholders were registered as secrets: %q", got) + } +} + +// admin config set values embed credentials the key name does not announce. +func TestRegisterKeyValueSecretsCoversEmbeddedCredentials(t *testing.T) { + resetSecretRegistryForTest() + t.Cleanup(resetSecretRegistryForTest) + redacted := registerKeyValueSecrets([]string{ + "notify_postgres:1", + "connection_string=host=db user=u password=pgSecret0123 dbname=x", + "notify_amqp:1", + "url=amqp://user:amqpSecret0123@host:5672", + "endpoint=https://h/hook?token=hookToken0123", + "dsn_string=user:mysqlSecret0123@tcp(db:3306)/db", + "queue_dir=/var/queue", + }) + joined := strings.Join(redacted, " ") + for _, secret := range []string{"pgSecret0123", "amqpSecret0123", "hookToken0123"} { + if strings.Contains(joined, secret) { + t.Errorf("redacted arguments still carry %q: %s", secret, joined) + } + if got := scrubKnownSecrets("[" + secret + "]"); got != "["+redactedMarker+"]" { + t.Errorf("%q was not registered: %q", secret, got) + } + } + for _, keep := range []string{"notify_postgres:1", "host=db user=u password=" + redactedMarker + " dbname=x", "amqp://user:" + redactedMarker + "@host:5672", "https://h/hook?token=" + redactedMarker, "queue_dir=/var/queue"} { + if !strings.Contains(joined, keep) { + t.Errorf("redacted arguments lost %q: %s", keep, joined) + } + } + // A DSN whose password sits in a form neither rule recognizes is at least + // still reported as given, without breaking anything else. + if !strings.Contains(joined, "dsn_string=user:mysqlSecret0123@tcp(db:3306)/db") { + t.Errorf("unrecognized DSN form was altered: %s", joined) + } +} + +// A doubled space after the scheme must not register the payload with a +// leading space it is never echoed with. +func TestAuthorizationSecretValuesTrimPayloadSpaces(t *testing.T) { + found := false + for _, value := range authorizationSecretValues("Bearer probeTokenValue0123") { + if value == "probeTokenValue0123" { + found = true + } + } + if !found { + t.Errorf("payload after a doubled space was not registered: %v", authorizationSecretValues("Bearer probeTokenValue0123")) + } +} diff --git a/cmd/secret-registry.go b/cmd/secret-registry.go index 41ac062793..e5b004b19a 100644 --- a/cmd/secret-registry.go +++ b/cmd/secret-registry.go @@ -21,6 +21,8 @@ import ( "bytes" "encoding/base64" "encoding/json" + "net/url" + "regexp" "sort" "strings" "sync" @@ -56,13 +58,29 @@ var ( secretRegistry []string ) +// secretPlaceholders are values that turn up under secret-named keys without +// being secrets - "auth_token=off", "client_secret=true". Registering them +// would turn every later "off" or "true" in an error message into a marker. +var secretPlaceholders = map[string]struct{}{ + "on": {}, "off": {}, "true": {}, "false": {}, "yes": {}, "no": {}, + "none": {}, "null": {}, "nil": {}, "auto": {}, "default": {}, "empty": {}, + "enable": {}, "enabled": {}, "disable": {}, "disabled": {}, + "required": {}, "optional": {}, "unset": {}, +} + +func isSecretPlaceholder(value string) bool { + _, ok := secretPlaceholders[strings.ToLower(value)] + return ok +} + // registerSecret records credential material so scrubKnownSecrets can remove -// it from any later output. Empty and very short values are ignored. +// it from any later output. Empty, very short and placeholder values are +// ignored. func registerSecret(values ...string) { secretRegistryMu.Lock() defer secretRegistryMu.Unlock() for _, value := range values { - if len(value) < secretRegistryMinLen || value == redactedMarker { + if len(value) < secretRegistryMinLen || value == redactedMarker || isSecretPlaceholder(value) { continue } duplicate := false @@ -119,8 +137,30 @@ func isSecretKeyValueName(key string) bool { return false } +// embeddedSecretRegexp finds a credential assignment inside a composite +// value whose own key names nothing secret: a DSN ("host=db user=u +// password=s"), a connection string, a webhook endpoint with "?token=s". +var embeddedSecretRegexp = regexp.MustCompile(`(?i)(?:^|[\s;,&?])(?:password|passwd|pwd|secret|token|api[_-]?key|access[_-]?key|secret[_-]?key|client[_-]?secret|private[_-]?key)=([^\s;,&]+)`) + +// embeddedSecrets returns the credential material a value carries inside it: +// the password of a URL's userinfo and the value of every password=/token= +// style assignment. +func embeddedSecrets(value string) []string { + var secrets []string + if parsed, err := url.Parse(value); err == nil && parsed.User != nil { + if password, ok := parsed.User.Password(); ok { + secrets = append(secrets, password) + } + } + for _, match := range embeddedSecretRegexp.FindAllStringSubmatch(value, -1) { + secrets = append(secrets, strings.Trim(match[1], `"'`)) + } + return secrets +} + // registerKeyValueSecrets registers the value of every key=value argument -// whose key names a secret, and returns the arguments with those values +// whose key names a secret, and the credentials embedded in the other values +// (a DSN, a URL with userinfo), and returns the arguments with those values // replaced, for use in an error message that would otherwise echo them. func registerKeyValueSecrets(args []string) []string { redacted := make([]string, 0, len(args)) @@ -131,6 +171,17 @@ func registerKeyValueSecrets(args []string) []string { redacted = append(redacted, key+"="+redactedMarker) continue } + if embedded := embeddedSecrets(value); len(embedded) > 0 { + registerSecret(embedded...) + masked := arg + for _, secret := range embedded { + if len(secret) >= secretRegistryMinLen && !isSecretPlaceholder(secret) { + masked = strings.ReplaceAll(masked, secret, redactedMarker) + } + } + redacted = append(redacted, masked) + continue + } redacted = append(redacted, arg) } return redacted From 59989f8a0ddf81d9a20329bd411ddf2bed7bbb6c Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Mon, 31 Aug 2026 19:30:06 +0800 Subject: [PATCH 2/7] fix: let svcacct set clear a policy again, report bad global flags once admin user svcacct set --policy refused an empty document, and an empty document is the only way to clear a service account's inline policy and return it to the inherited one: the server treats {"Statement":[]} as a reset. The shape is still validated strictly; emptiness is allowed on set. A malformed --custom-header, --resolve or --limit-* value was printed twice, and the first copy went to stdout: the CLI library echoes a Before error to its writer before the process reports it. The parse failure is now reported through fatalIf - once, on stderr, or as a JSON error document under --json - at both the app and the command level. checksum verify scrubbed the server's error text but not a transport read or close error before writing it into a record; both go through the scrubber now. The functional test that claimed explicit --quiet silences checksum verify could not fail, because the harness swallows stdout; it now captures the command's own stdout. The Windows CI step runs under bash so a failed go build fails the step, and a comment in the release gate no longer claims exclude_pull_requests filters runs. Found by the pre-release adversarial review. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VX7dtHJ4q7GJ1YDYeXLmjW Signed-off-by: Feng Ruohang --- .github/workflows/go.yml | 5 ++++- buildscripts/check-release-commit.sh | 7 ++++--- cmd/admin-user-svcacct-set.go | 8 ++++---- cmd/checksum-verify.go | 4 ++-- cmd/globals.go | 14 +++++++++++++- functional-tests.sh | 12 +++++++----- 6 files changed, 34 insertions(+), 16 deletions(-) diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index da967d6dfd..ea2a760fdb 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -41,8 +41,11 @@ jobs: - name: Build on ${{ matrix.os }} if: matrix.os == 'windows-latest' + # bash, not pwsh: pwsh only propagates the last command's exit code, + # and %GOPATH% is a cmd.exe form that pwsh leaves unexpanded. + shell: bash run: | - go build --ldflags="-s -w" -o %GOPATH%\bin\mc.exe + go build --ldflags="-s -w" -o "$(go env GOPATH)/bin/mc.exe" go test -v -race --timeout 30m ./... - name: Build on ${{ matrix.os }} if: matrix.os == 'macos-latest' diff --git a/buildscripts/check-release-commit.sh b/buildscripts/check-release-commit.sh index d5fe53c6af..9015288f34 100755 --- a/buildscripts/check-release-commit.sh +++ b/buildscripts/check-release-commit.sh @@ -57,9 +57,10 @@ if [ -z "${fixture}" ]; then error_file="$(mktemp)" trap 'rm -f "${error_file}"' EXIT - # exclude_pull_requests: a pull_request run checks out GitHub's synthetic - # merge ref, so a green run proves the merge result was good, not that this - # commit was ever built on its own. + # A pull_request run checks out GitHub's synthetic merge ref, so a green + # run proves the merge result was good, not that this commit was ever built + # on its own. exclude_pull_requests only trims the pull_requests field of + # each run; the event/head_branch filter below is what drops those runs. if ! runs_json="$( gh api --paginate \ "repos/${repository}/actions/runs?head_sha=${release_commit}&exclude_pull_requests=true&per_page=100" \ diff --git a/cmd/admin-user-svcacct-set.go b/cmd/admin-user-svcacct-set.go index c260675eca..189946f17e 100644 --- a/cmd/admin-user-svcacct-set.go +++ b/cmd/admin-user-svcacct-set.go @@ -113,11 +113,11 @@ func mainAdminUserSvcAcctSet(ctx *cli.Context) error { buf, e = os.ReadFile(policyPath) fatalIf(probe.NewError(e), "Unable to open the policy document.") - p, e := parsePolicyForWrite(buf) + // An empty document is the one way to clear an inline policy and + // return the account to its inherited one: the server treats + // {"Statement":[]} as a reset. Validate the shape, allow emptiness. + _, e = parsePolicyForWrite(buf) fatalIf(probe.NewError(e), "Unable to parse the policy document.") - if p.IsEmpty() { - fatalIf(errInvalidArgument(), "empty policies are not allowed") - } } var expiryTime time.Time diff --git a/cmd/checksum-verify.go b/cmd/checksum-verify.go index 430aecc372..8720c30dee 100644 --- a/cmd/checksum-verify.go +++ b/cmd/checksum-verify.go @@ -752,12 +752,12 @@ func verifyChecksumCandidate(ctx context.Context, backend checksumVerifyBackend, result.BytesRead = read if readErr != nil { result.Result = checksumResultUnknownReadError - result.ErrorMessage = readErr.Error() + result.ErrorMessage = scrubSecretsFromOutput(readErr.Error()) return result } if closeErr != nil { result.Result = checksumResultUnknownReadError - result.ErrorMessage = closeErr.Error() + result.ErrorMessage = scrubSecretsFromOutput(closeErr.Error()) return result } if read != info.Size { diff --git a/cmd/globals.go b/cmd/globals.go index fd43c0f80c..bb6a5b83b4 100644 --- a/cmd/globals.go +++ b/cmd/globals.go @@ -35,6 +35,7 @@ import ( "github.com/dustin/go-humanize" "github.com/minio/cli" "github.com/minio/madmin-go/v3" + "github.com/minio/mc/pkg/probe" "github.com/muesli/termenv" "github.com/pgsty/silo-pkg/v3/console" "golang.org/x/net/http/httpguts" @@ -122,8 +123,19 @@ func parsePagerDisableFlag(args []string) { } } -// Set global states. NOTE: It is deliberately kept monolithic to ensure we dont miss out any flags. +// setGlobalsFromContext applies the global flags and stops the command on a +// malformed value. Reporting through fatalIf prints the error once, on stderr +// or as a JSON document; returning it would have the CLI library echo it to +// stdout first and the process print it again. func setGlobalsFromContext(ctx *cli.Context) error { + if e := applyGlobalsFromContext(ctx); e != nil { + fatalIf(probe.NewError(e), "Unable to apply global flags.") + } + return nil +} + +// Set global states. NOTE: It is deliberately kept monolithic to ensure we dont miss out any flags. +func applyGlobalsFromContext(ctx *cli.Context) error { quiet := ctx.Bool("quiet") || ctx.GlobalBool("quiet") debug := ctx.Bool("debug") || ctx.GlobalBool("debug") json := ctx.Bool("json") || ctx.GlobalBool("json") diff --git a/functional-tests.sh b/functional-tests.sh index 784cb944cc..dc7e4a51ca 100755 --- a/functional-tests.sh +++ b/functional-tests.sh @@ -1057,11 +1057,13 @@ function test_checksum_verify() { # ...and explicit quiet must still silence stdout while --report keeps working. report="${WORK_DIR}/checksum-report-$RANDOM.jsonl" - quiet_out=$(mc_cmd checksum verify --recursive --fail-on none --report "${report}" \ - "${SERVER_ALIAS}/${BUCKET_NAME}/${prefix}/") - assert_success "$start_time" "${FUNCNAME[0]}" show_on_failure $? "checksum verify failed under --quiet" - diff -bB <(echo "") <(echo "$quiet_out") >/dev/null 2>&1 - assert_success "$start_time" "${FUNCNAME[0]}" show_on_failure $? "explicit --quiet must silence checksum verify stdout" + # mc_cmd swallows stdout, so the command line is built here as well: the + # assertion must see the command's own stdout, and nothing else. + quiet_out=$("${MC_CMD[@]}" checksum verify --recursive --fail-on none --report "${report}" \ + "${SERVER_ALIAS}/${BUCKET_NAME}/${prefix}/" 2>"${report}.stderr") + assert_success "$start_time" "${FUNCNAME[0]}" show_on_failure $? "checksum verify failed under --quiet: $(cat "${report}.stderr")" + [ -z "$quiet_out" ] + assert_success "$start_time" "${FUNCNAME[0]}" show_on_failure $? "explicit --quiet must silence checksum verify stdout, got: ${quiet_out}" diff -bB <(echo "1") <(jq -s -r '[.[] | select(.type == "summary")] | length' "${report}") >/dev/null 2>&1 assert_success "$start_time" "${FUNCNAME[0]}" show_on_failure $? "--report must contain exactly one summary under --quiet" From d38412a3213ca2bf9784660fce2292e8e053dd63 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Mon, 31 Aug 2026 19:35:46 +0800 Subject: [PATCH 3/7] fix: classify an SSE-C refusal with a key as a read error, keep policy and size messages clear checksum verify labelled the server's refusal of SSE-C over plain HTTP as UNKNOWN_SSEC_KEY_MISSING although a key had been supplied; with a key the complaint is about the request, so it is a read error carrying the server's message. --max-size rejects a bad value with the value and the expected form instead of strconv's text, and its help says that 0 means no limit. The strict policy parser answers an empty document with "EOF"; the write paths say "policy input cannot be empty" again, as the permissive parser did. docs record that an endpoint must report x-amz-checksum-type, and the prose test pins the server's "security token included in the request" message, which the old shapes mangled. Found by the pre-release adversarial review. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VX7dtHJ4q7GJ1YDYeXLmjW Signed-off-by: Feng Ruohang --- cmd/checksum-verify.go | 22 ++++++++++++---------- cmd/checksum-verify_test.go | 21 +++++++++++++++++---- cmd/policy-validation.go | 3 +++ cmd/redaction-prose_test.go | 1 + docs/checksum-verify.md | 5 ++++- 5 files changed, 37 insertions(+), 15 deletions(-) diff --git a/cmd/checksum-verify.go b/cmd/checksum-verify.go index 8720c30dee..c9ec00477e 100644 --- a/cmd/checksum-verify.go +++ b/cmd/checksum-verify.go @@ -101,7 +101,7 @@ var checksumVerifyFlags = []cli.Flag{ }, cli.StringFlag{ Name: "max-size", - Usage: "skip objects larger than this size (e.g. 10GiB)", + Usage: "skip objects larger than this size (e.g. 10GiB); 0 or empty means no limit", }, cli.StringFlag{ Name: "manifest", @@ -400,7 +400,7 @@ func parseChecksumVerifyMaximumSize(value string) (int64, error) { } size, err := humanize.ParseBytes(value) if err != nil { - return 0, err + return 0, fmt.Errorf("%q is not a size; use a value such as 10GiB, or 0 for no limit", value) } if size > uint64(^uint64(0)>>1) { return 0, fmt.Errorf("maximum size is too large") @@ -596,11 +596,13 @@ func checksumVerifyResultFor(candidate checksumVerifyCandidate) checksumVerifyRe } } -func checksumVerifyErrorResult(candidate checksumVerifyCandidate, err error) checksumVerifyResult { - return applyChecksumVerifyError(checksumVerifyResultFor(candidate), err) +func checksumVerifyErrorResult(candidate checksumVerifyCandidate, err error, sseSupplied bool) checksumVerifyResult { + return applyChecksumVerifyError(checksumVerifyResultFor(candidate), err, sseSupplied) } -func applyChecksumVerifyError(result checksumVerifyResult, err error) checksumVerifyResult { +// sseSupplied tells an SSE-C rejection with a key from one without: with a +// key the server's complaint is about the request, not a missing key. +func applyChecksumVerifyError(result checksumVerifyResult, err error, sseSupplied bool) checksumVerifyResult { response := minio.ToErrorResponse(err) result.ErrorCode = response.Code // Server-controlled text, printed and written to --report: an endpoint @@ -618,10 +620,10 @@ func applyChecksumVerifyError(result checksumVerifyResult, err error) checksumVe result.Result = checksumResultUnknownObjectChanged case strings.Contains(code, "kms") || strings.Contains(message, "kms"): result.Result = checksumResultUnknownKMSError - case strings.Contains(code, "sse") || + case !sseSupplied && (strings.Contains(code, "sse") || strings.Contains(message, "server side encryption") || strings.Contains(message, "customer key") || - strings.Contains(message, "sse-c"): + strings.Contains(message, "sse-c")): result.Result = checksumResultUnknownSSECKeyMissing default: result.Result = checksumResultUnknownReadError @@ -675,7 +677,7 @@ func verifyChecksumCandidate(ctx context.Context, backend checksumVerifyBackend, sse := getSSE(resource, opts.Encryption[candidate.Alias]) info, err := backend.statObjectForChecksumVerify(ctx, candidate.Bucket, candidate.Key, candidate.VersionID, sse) if err != nil { - return checksumVerifyErrorResult(candidate, err) + return checksumVerifyErrorResult(candidate, err, sse != nil) } result.Size = info.Size result.ETag = info.ETag @@ -745,7 +747,7 @@ func verifyChecksumCandidate(ctx context.Context, backend checksumVerifyBackend, } reader, err := backend.getObjectForChecksumVerify(ctx, candidate.Bucket, candidate.Key, readVersionID, ifMatch, sse) if err != nil { - return applyChecksumVerifyError(result, err) + return applyChecksumVerifyError(result, err, sse != nil) } read, readErr := io.Copy(io.MultiWriter(writers...), reader) closeErr := reader.Close() @@ -769,7 +771,7 @@ func verifyChecksumCandidate(ctx context.Context, backend checksumVerifyBackend, if mutableVersion { after, statErr := backend.statObjectForChecksumVerify(ctx, candidate.Bucket, candidate.Key, readVersionID, sse) if statErr != nil { - return applyChecksumVerifyError(result, statErr) + return applyChecksumVerifyError(result, statErr, sse != nil) } if checksumVerifyObjectChanged(info, after) { result.Result = checksumResultUnknownObjectChanged diff --git a/cmd/checksum-verify_test.go b/cmd/checksum-verify_test.go index c35270e594..b801294ccd 100644 --- a/cmd/checksum-verify_test.go +++ b/cmd/checksum-verify_test.go @@ -529,9 +529,10 @@ func TestChecksumVerifySummaryFailOn(t *testing.T) { func TestApplyChecksumVerifyError(t *testing.T) { base := checksumVerifyResult{SchemaVersion: 1, Type: "object", Bucket: "archive", Key: "object", Size: 42} tests := []struct { - name string - err error - want string + name string + err error + sseSupplied bool + want string }{ { name: "access denied", @@ -567,10 +568,22 @@ func TestApplyChecksumVerifyError(t *testing.T) { }, want: checksumResultUnknownSSECKeyMissing, }, + { + // A key was supplied; the server refused the request itself + // (SSE-C over plain HTTP). That is not a missing key. + name: "SSE-C request refused although a key was supplied", + err: minio.ErrorResponse{ + Code: "InvalidRequest", + StatusCode: http.StatusBadRequest, + Message: "Requests specifying Server Side Encryption with Customer provided keys must be made over a secure connection.", + }, + sseSupplied: true, + want: checksumResultUnknownReadError, + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got := applyChecksumVerifyError(base, tc.err) + got := applyChecksumVerifyError(base, tc.err, tc.sseSupplied) if got.Result != tc.want { t.Fatalf("result %q, want %q", got.Result, tc.want) } diff --git a/cmd/policy-validation.go b/cmd/policy-validation.go index 8738e3fe94..d87a09fa40 100644 --- a/cmd/policy-validation.go +++ b/cmd/policy-validation.go @@ -33,6 +33,9 @@ var _ func(policy.Resource) bool = policy.Resource.IsBareARN // parsePolicyForWrite applies the strict validation required for new and // updated policy documents while read paths remain backward-compatible. func parsePolicyForWrite(policyBytes []byte) (*policy.Policy, error) { + if len(bytes.TrimSpace(policyBytes)) == 0 { + return nil, errors.New("policy input cannot be empty") + } return policy.ParseConfigStrict(bytes.NewReader(policyBytes)) } diff --git a/cmd/redaction-prose_test.go b/cmd/redaction-prose_test.go index b61708ec09..254e1b82b9 100644 --- a/cmd/redaction-prose_test.go +++ b/cmd/redaction-prose_test.go @@ -36,6 +36,7 @@ func TestScrubCredentialTextKeepsProse(t *testing.T) { "cannot be combined with AWS role authentication", "access token not found in response", "The provided token has expired", + "The security token included in the request is invalid", "Content-MD5 digest mismatch for part 3", "Basic auth is disabled", "AWS4-HMAC-SHA256 is required for this region", diff --git a/docs/checksum-verify.md b/docs/checksum-verify.md index 4a5e04054c..58dfeab297 100644 --- a/docs/checksum-verify.md +++ b/docs/checksum-verify.md @@ -16,7 +16,10 @@ certainty, inspect `xl.meta`, repair metadata, or write object data. The command is intentionally not under `admin`: version one uses only S3 data plane operations and should work with SILO, inherited MinIO data, and other -compatible endpoints when the caller has sufficient object permissions. +compatible endpoints when the caller has sufficient object permissions. The +endpoint must report the checksum type (`x-amz-checksum-type`) alongside the +checksum; on one that does not, every checksummed object is classified +`UNKNOWN_CHECKSUM_TYPE` rather than guessed at. ## Historical problem From b291b539b2cc21bc7dd4ea0dec431ba4642e00bf Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Mon, 31 Aug 2026 19:44:48 +0800 Subject: [PATCH 4/7] fix: register a URL password as written, keep listing parameters visible in traces A password with an "@" or ":" travels percent-encoded in a URL, so an admin config set value such as url=amqp://user:p%40ss@host registered only the decoded form and a failed command still echoed the encoded one. The raw userinfo password is registered and masked as well; a quoted DSN payload (password='p@ss word') is recognized; a placeholder equal to its own key (password=password, the documentation's example) is not a secret. max-keys, key-marker and continuation-token carry the fragments the query shape keys on, and hiding them blanked every ListObjects line of the default trace. Those names are never secrets and stay visible in trace output and in --debug URLs alike. The SigV2 shape now needs a base64 signature after the colon, so "AWS https://s3.amazonaws.com" and "AWS us-east-1:GetObject" stay readable, and scheme words match in the spelling a header uses, so "digest SHA256:..." is prose. The dead error check after setGlobalsFromContext in registerBefore is gone. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VX7dtHJ4q7GJ1YDYeXLmjW Signed-off-by: Feng Ruohang --- cmd/client-s3-trace-redact.go | 39 +++++++++++++++++++++++++---------- cmd/main.go | 9 +++----- cmd/redaction-prose_test.go | 18 +++++++++++++--- cmd/secret-registry.go | 38 ++++++++++++++++++++++++++++++---- 4 files changed, 80 insertions(+), 24 deletions(-) diff --git a/cmd/client-s3-trace-redact.go b/cmd/client-s3-trace-redact.go index 78fc49c99c..389ce617fc 100644 --- a/cmd/client-s3-trace-redact.go +++ b/cmd/client-s3-trace-redact.go @@ -58,9 +58,9 @@ var traceAuthSchemeRegexp = regexp.MustCompile(`^[A-Za-z0-9!#$%&'*+.^_` + "`" + var traceTextShapes = []struct { pattern *regexp.Regexp replacement string - // accept, when set, must approve the captured payload (group 2) before - // the match is replaced. - accept func(payload string) bool + // accept, when set, must approve the submatch groups before the match is + // replaced. + accept func(groups []string) bool }{ // A whole Signature v4 value echoed into text: the scheme followed, on the // same line, by its Credential field. @@ -69,15 +69,29 @@ var traceTextShapes = []struct { {pattern: regexp.MustCompile(`\bCredential=[^,\s"'<>]+`), replacement: "Credential=" + redactedMarker}, {pattern: regexp.MustCompile(`\bSignedHeaders=[^,\s"'<>]+`), replacement: "SignedHeaders=" + redactedMarker}, {pattern: regexp.MustCompile(`\bSignature=[^,\s"'<>&]+`), replacement: "Signature=" + redactedMarker}, - // Signature v2: "AWS :". The colon separates it - // from prose such as "AWS S3 compatible". - {pattern: regexp.MustCompile(`\bAWS [^\s:"'<>]+:[^\s"'<>]+`), replacement: "AWS " + redactedMarker}, - // Scheme-prefixed tokens, when the payload looks like a token rather than - // the next word of a sentence. - {pattern: regexp.MustCompile(`\b((?i:Bearer|Basic|Digest|Negotiate|NTLM|Token)) ([^\s"'<>,]+)`), replacement: "$1 " + redactedMarker, accept: looksLikeCredentialPayload}, + // Signature v2: "AWS :". The colon and + // the base64 signature separate it from prose such as "AWS S3 compatible" + // or "AWS https://s3.amazonaws.com". + {pattern: regexp.MustCompile(`\bAWS [^\s:"'<>]+:[A-Za-z0-9+/]{20,}={0,2}`), replacement: "AWS " + redactedMarker}, + // Scheme-prefixed tokens, in the canonical spelling a header uses, when + // the payload looks like a token rather than the next word of a sentence. + {pattern: regexp.MustCompile(`\b(Bearer|Basic|Digest|Negotiate|NTLM|Token) ([^\s"'<>,]+)`), replacement: "$1 " + redactedMarker, accept: func(groups []string) bool { return looksLikeCredentialPayload(groups[2]) }}, // Credential-bearing query parameters wherever a URL appears, matched by // name fragment so a parameter this client never sends is still caught. - {pattern: regexp.MustCompile(`([?&][^=&\s"'<>]*(?i:token|signature|credential|secret|password|api[-_]?key|auth|sig|key)[^=&\s"'<>]*=)[^&\s"'<>]+`), replacement: "${1}" + redactedMarker}, + {pattern: regexp.MustCompile(`([?&]([^=&\s"'<>]*(?i:token|signature|credential|secret|password|api[-_]?key|auth|sig|key)[^=&\s"'<>]*)=)[^&\s"'<>]+`), replacement: "${1}" + redactedMarker, accept: func(groups []string) bool { return !isListingQueryParam(groups[2]) }}, +} + +// listingQueryParams are S3 parameters whose names carry a secret-looking +// fragment ("key", "token") but never a secret: hiding them would blank +// every ListObjects trace line. +var listingQueryParams = map[string]struct{}{ + "max-keys": {}, "key-marker": {}, "continuation-token": {}, + "next-key-marker": {}, "next-continuation-token": {}, "start-after": {}, +} + +func isListingQueryParam(name string) bool { + _, ok := listingQueryParams[strings.ToLower(name)] + return ok } // looksLikeCredentialPayload tells a token after a scheme word from the next @@ -137,6 +151,9 @@ func isCustomHeaderName(name string) bool { // material, for presigned URLs of either signature version and for anything a // proxy in front of the endpoint might expect. func isSecretQueryParam(name string) bool { + if isListingQueryParam(name) { + return false + } lower := strings.ToLower(name) for _, fragment := range []string{"token", "signature", "credential", "secret", "password", "api-key", "apikey", "api_key", "auth", "sig", "awsaccesskeyid", "key"} { if strings.Contains(lower, fragment) { @@ -260,7 +277,7 @@ func scrubCredentialText(text string) string { pattern, replacement, accept := shape.pattern, shape.replacement, shape.accept text = pattern.ReplaceAllStringFunc(text, func(match string) string { groups := pattern.FindStringSubmatch(match) - if len(groups) < 3 || !accept(groups[2]) { + if len(groups) < 3 || !accept(groups) { return match } return pattern.ReplaceAllString(match, replacement) diff --git a/cmd/main.go b/cmd/main.go index fd5818651e..1aa1815878 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -364,12 +364,9 @@ func registerBefore(ctx *cli.Context) error { setMcConfigDir(ctx.GlobalString("config-dir")) } - // Set global flags. Propagate a parse failure: dropping it here meant a - // malformed app-level --custom-header, --resolve or --limit-* value was - // silently ignored and the command ran without it. - if e := setGlobalsFromContext(ctx); e != nil { - return e - } + // Set global flags. A malformed app-level --custom-header, --resolve or + // --limit-* value stops the command here; it used to be silently ignored. + setGlobalsFromContext(ctx) // Migrate any old version of config / state files to newer format. migrate() diff --git a/cmd/redaction-prose_test.go b/cmd/redaction-prose_test.go index 254e1b82b9..3dddcf944c 100644 --- a/cmd/redaction-prose_test.go +++ b/cmd/redaction-prose_test.go @@ -44,6 +44,10 @@ func TestScrubCredentialTextKeepsProse(t *testing.T) { "Token expired", "TOKEN EXPIRED", "NTLM is deprecated", + "Invalid endpoint for AWS https://s3.amazonaws.com", + "AWS us-east-1:GetObject denied", + "Content digest SHA256:0123abcd mismatch for part 3", + "?list-type=2&max-keys=1000&prefix=foo&continuation-token=abc0123&key-marker=obj", } { if got := scrubSecretsFromOutput(text); got != text { t.Errorf("scrubSecretsFromOutput(%q) = %q, want it unchanged", text, got) @@ -58,7 +62,7 @@ func TestScrubCredentialTextStillRedactsCredentialShapes(t *testing.T) { "Basic dXNlcjpwYXNz": "Basic " + redactedMarker, "Proxy-Authorization: Basic dXNlcjpodW50ZXIy": "Proxy-Authorization: Basic " + redactedMarker, "AWS AKIAIOSFODNN7EXAMPLE:frJIUN8DYpKDtOLCwo//yllqDzg=": "AWS " + redactedMarker, - "AWS a/b:sig0123456789": "AWS " + redactedMarker, + "AWS a/b:frJIUN8DYpKDtOLCwo//yllqDzg=": "AWS " + redactedMarker, "Token 0123456789abcdef": "Token " + redactedMarker, "Negotiate YIIFmQYGKwYBBQUCoIIFjTCCBYmgJDAi": "Negotiate " + redactedMarker, "AWS4-HMAC-SHA256 Credential=k/20260831/us-east-1/s3/aws4_request, SignedHeaders=host, Signature=ab12": "AWS4-HMAC-SHA256 " + redactedMarker, @@ -100,6 +104,7 @@ func TestRedactTraceQueryRedactsFirstParameter(t *testing.T) { "token=abc0123&x=1": "token=" + redactedMarker + "&x=1", "AWSAccessKeyId=AKIA0123": "AWSAccessKeyId=" + redactedMarker, "versionId=keep&list-type=2": "versionId=keep&list-type=2", + "max-keys=1000&key-marker=obj&continuation-token=abc0123&x-amz-security-token=tok0123": "max-keys=1000&key-marker=obj&continuation-token=abc0123&x-amz-security-token=" + redactedMarker, } { if got := redactTraceQuery(raw, nil); got != want { t.Errorf("redactTraceQuery(%q) = %q, want %q", raw, got, want) @@ -203,9 +208,12 @@ func TestRegisterKeyValueSecretsCoversEmbeddedCredentials(t *testing.T) { "endpoint=https://h/hook?token=hookToken0123", "dsn_string=user:mysqlSecret0123@tcp(db:3306)/db", "queue_dir=/var/queue", + "broker=amqps://svc:p%40ss%3Aw0rd@broker.example:5671/vhost", + "connection_string=host=db password='sp ace0123' sslmode=disable", + "connection_string=host=db user=u password=password sslmode=disable", }) joined := strings.Join(redacted, " ") - for _, secret := range []string{"pgSecret0123", "amqpSecret0123", "hookToken0123"} { + for _, secret := range []string{"pgSecret0123", "amqpSecret0123", "hookToken0123", "p%40ss%3Aw0rd", "p@ss:w0rd", "sp ace0123"} { if strings.Contains(joined, secret) { t.Errorf("redacted arguments still carry %q: %s", secret, joined) } @@ -213,7 +221,7 @@ func TestRegisterKeyValueSecretsCoversEmbeddedCredentials(t *testing.T) { t.Errorf("%q was not registered: %q", secret, got) } } - for _, keep := range []string{"notify_postgres:1", "host=db user=u password=" + redactedMarker + " dbname=x", "amqp://user:" + redactedMarker + "@host:5672", "https://h/hook?token=" + redactedMarker, "queue_dir=/var/queue"} { + for _, keep := range []string{"notify_postgres:1", "host=db user=u password=" + redactedMarker + " dbname=x", "amqp://user:" + redactedMarker + "@host:5672", "https://h/hook?token=" + redactedMarker, "queue_dir=/var/queue", "amqps://svc:" + redactedMarker + "@broker.example:5671/vhost", "password='" + redactedMarker + "'", "password=password sslmode=disable"} { if !strings.Contains(joined, keep) { t.Errorf("redacted arguments lost %q: %s", keep, joined) } @@ -223,6 +231,10 @@ func TestRegisterKeyValueSecretsCoversEmbeddedCredentials(t *testing.T) { if !strings.Contains(joined, "dsn_string=user:mysqlSecret0123@tcp(db:3306)/db") { t.Errorf("unrecognized DSN form was altered: %s", joined) } + // The documentation's own placeholder must not poison later output. + if got := scrubKnownSecrets("invalid password for user u"); got != "invalid password for user u" { + t.Errorf("the literal word password was registered: %q", got) + } } // A doubled space after the scheme must not register the payload with a diff --git a/cmd/secret-registry.go b/cmd/secret-registry.go index e5b004b19a..406c99b3e4 100644 --- a/cmd/secret-registry.go +++ b/cmd/secret-registry.go @@ -140,24 +140,54 @@ func isSecretKeyValueName(key string) bool { // embeddedSecretRegexp finds a credential assignment inside a composite // value whose own key names nothing secret: a DSN ("host=db user=u // password=s"), a connection string, a webhook endpoint with "?token=s". -var embeddedSecretRegexp = regexp.MustCompile(`(?i)(?:^|[\s;,&?])(?:password|passwd|pwd|secret|token|api[_-]?key|access[_-]?key|secret[_-]?key|client[_-]?secret|private[_-]?key)=([^\s;,&]+)`) +// The payload may be quoted ("password='p@ss word'"). +var embeddedSecretRegexp = regexp.MustCompile(`(?i)(?:^|[\s;,&?])(password|passwd|pwd|secret|token|api[_-]?key|access[_-]?key|secret[_-]?key|client[_-]?secret|private[_-]?key)=("[^"]*"|'[^']*'|[^\s;,&]+)`) // embeddedSecrets returns the credential material a value carries inside it: -// the password of a URL's userinfo and the value of every password=/token= -// style assignment. +// the password of a URL's userinfo - decoded, and exactly as written, since +// a password with "@" or ":" travels percent-encoded - and the value of every +// password=/token= style assignment. A placeholder equal to its own key +// ("password=password", the documentation's example) is not a secret. func embeddedSecrets(value string) []string { var secrets []string if parsed, err := url.Parse(value); err == nil && parsed.User != nil { if password, ok := parsed.User.Password(); ok { secrets = append(secrets, password) + if raw := rawURLPassword(value); raw != "" { + secrets = append(secrets, raw) + } } } for _, match := range embeddedSecretRegexp.FindAllStringSubmatch(value, -1) { - secrets = append(secrets, strings.Trim(match[1], `"'`)) + if secret := strings.Trim(match[2], `"'`); !strings.EqualFold(secret, match[1]) { + secrets = append(secrets, secret) + } } return secrets } +// rawURLPassword returns the password of a URL's userinfo as it is written, +// without decoding: "amqp://user:p%40ss@host" yields "p%40ss". +func rawURLPassword(value string) string { + _, rest, ok := strings.Cut(value, "://") + if !ok { + return "" + } + end := strings.IndexAny(rest, "/?#") + if end < 0 { + end = len(rest) + } + at := strings.LastIndex(rest[:end], "@") + if at < 0 { + return "" + } + _, password, ok := strings.Cut(rest[:at], ":") + if !ok { + return "" + } + return password +} + // registerKeyValueSecrets registers the value of every key=value argument // whose key names a secret, and the credentials embedded in the other values // (a DSN, a URL with userinfo), and returns the arguments with those values From 9dfca8fcc1c7221463163f053f8c802ebb54189f Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Mon, 31 Aug 2026 19:47:21 +0800 Subject: [PATCH 5/7] test: drive the SSE-C refusal classification through the verify path The unit table exercised applyChecksumVerifyError directly; this case runs verifyChecksumCandidate with and without a matching --enc-c prefix against a backend that refuses SSE-C, so the sseSupplied wiring itself is covered. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VX7dtHJ4q7GJ1YDYeXLmjW Signed-off-by: Feng Ruohang --- cmd/checksum-verify_test.go | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/cmd/checksum-verify_test.go b/cmd/checksum-verify_test.go index b801294ccd..1a0e5af70d 100644 --- a/cmd/checksum-verify_test.go +++ b/cmd/checksum-verify_test.go @@ -1243,3 +1243,39 @@ func TestChecksumVerifyCLIFailOnExitStatus(t *testing.T) { }) } } + +// An SSE-C refusal is a missing key only when no key was sent for the +// object. With a matching --enc-c prefix the server's complaint is about the +// request itself - SSE-C over plain HTTP - and the result is a read error +// that carries its message. +func TestVerifyChecksumCandidateSSECRefusalDependsOnKey(t *testing.T) { + refusal := minio.ErrorResponse{ + Code: "InvalidRequest", + StatusCode: http.StatusBadRequest, + Message: "Requests specifying Server Side Encryption with Customer provided keys must be made over a secure connection.", + } + key, err := encrypt.NewSSEC([]byte("0123456789abcdef0123456789abcdef")) + if err != nil { + t.Fatal(err) + } + candidate := checksumVerifyCandidate{Alias: "play", Bucket: "archive", Key: "object"} + for name, tc := range map[string]struct { + encryption map[string][]prefixSSEPair + want string + }{ + "no key sent": {map[string][]prefixSSEPair{}, checksumResultUnknownSSECKeyMissing}, + "key for another prefix": {map[string][]prefixSSEPair{"play": {{Prefix: "play/other/", SSE: key}}}, checksumResultUnknownSSECKeyMissing}, + "key sent": {map[string][]prefixSSEPair{"play": {{Prefix: "play/archive/", SSE: key}}}, checksumResultUnknownReadError}, + } { + t.Run(name, func(t *testing.T) { + backend := &checksumVerifyFakeBackend{statErr: refusal} + result := verifyChecksumCandidate(context.Background(), backend, candidate, checksumVerifyOptions{Encryption: tc.encryption}) + if result.Result != tc.want { + t.Fatalf("result %q, want %q: %+v", result.Result, tc.want, result) + } + if !strings.Contains(result.ErrorMessage, "secure connection") { + t.Fatalf("server message was dropped: %+v", result) + } + }) + } +} From 76b2e891ba9dc62f3e487dda01537ed2d675d861 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Mon, 31 Aug 2026 19:49:31 +0800 Subject: [PATCH 6/7] fix: keep an unquoted DSN password whole and match auth schemes case-insensitively libpq allows ";", "," and "&" in an unquoted connection-string value, so the DSN rule now runs to the next whitespace; the URL-query rule still stops at "&". Scheme names are case-insensitive on the wire and a proxy may echo "bearer ", so Bearer, Basic, Negotiate and NTLM match in any spelling again; Digest and Token, which are also English words, match only as a header spells them. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VX7dtHJ4q7GJ1YDYeXLmjW Signed-off-by: Feng Ruohang --- cmd/client-s3-trace-redact.go | 9 ++++++--- cmd/redaction-prose_test.go | 7 +++++-- cmd/secret-registry.go | 22 ++++++++++++++-------- 3 files changed, 25 insertions(+), 13 deletions(-) diff --git a/cmd/client-s3-trace-redact.go b/cmd/client-s3-trace-redact.go index 389ce617fc..f8a8111c93 100644 --- a/cmd/client-s3-trace-redact.go +++ b/cmd/client-s3-trace-redact.go @@ -73,9 +73,12 @@ var traceTextShapes = []struct { // the base64 signature separate it from prose such as "AWS S3 compatible" // or "AWS https://s3.amazonaws.com". {pattern: regexp.MustCompile(`\bAWS [^\s:"'<>]+:[A-Za-z0-9+/]{20,}={0,2}`), replacement: "AWS " + redactedMarker}, - // Scheme-prefixed tokens, in the canonical spelling a header uses, when - // the payload looks like a token rather than the next word of a sentence. - {pattern: regexp.MustCompile(`\b(Bearer|Basic|Digest|Negotiate|NTLM|Token) ([^\s"'<>,]+)`), replacement: "$1 " + redactedMarker, accept: func(groups []string) bool { return looksLikeCredentialPayload(groups[2]) }}, + // Scheme-prefixed tokens, when the payload looks like a token rather than + // the next word of a sentence. Scheme names are case-insensitive on the + // wire, so a proxy may echo "bearer "; "Digest" and "Token" are also + // English words ("digest mismatch", "token has expired") and match only in + // the spelling a header uses. + {pattern: regexp.MustCompile(`\b((?i:Bearer|Basic|Negotiate|NTLM)|Digest|Token) ([^\s"'<>,]+)`), replacement: "$1 " + redactedMarker, accept: func(groups []string) bool { return looksLikeCredentialPayload(groups[2]) }}, // Credential-bearing query parameters wherever a URL appears, matched by // name fragment so a parameter this client never sends is still caught. {pattern: regexp.MustCompile(`([?&]([^=&\s"'<>]*(?i:token|signature|credential|secret|password|api[-_]?key|auth|sig|key)[^=&\s"'<>]*)=)[^&\s"'<>]+`), replacement: "${1}" + redactedMarker, accept: func(groups []string) bool { return !isListingQueryParam(groups[2]) }}, diff --git a/cmd/redaction-prose_test.go b/cmd/redaction-prose_test.go index 3dddcf944c..7df7f8342d 100644 --- a/cmd/redaction-prose_test.go +++ b/cmd/redaction-prose_test.go @@ -65,6 +65,8 @@ func TestScrubCredentialTextStillRedactsCredentialShapes(t *testing.T) { "AWS a/b:frJIUN8DYpKDtOLCwo//yllqDzg=": "AWS " + redactedMarker, "Token 0123456789abcdef": "Token " + redactedMarker, "Negotiate YIIFmQYGKwYBBQUCoIIFjTCCBYmgJDAi": "Negotiate " + redactedMarker, + "invalid bearer eyJhbGciOiJIUzI1NiJ9.payload.sig": "invalid bearer " + redactedMarker, + "BASIC dXNlcjpwYXNz": "BASIC " + redactedMarker, "AWS4-HMAC-SHA256 Credential=k/20260831/us-east-1/s3/aws4_request, SignedHeaders=host, Signature=ab12": "AWS4-HMAC-SHA256 " + redactedMarker, "AWS4-HMAC-SHA256 SECRETVALUE0123 Credential=k/20260831/us-east-1/s3/aws4_request": "AWS4-HMAC-SHA256 " + redactedMarker, "scope Credential=k/20260831/us-east-1/s3/aws4_request only": "scope Credential=" + redactedMarker + " only", @@ -211,9 +213,10 @@ func TestRegisterKeyValueSecretsCoversEmbeddedCredentials(t *testing.T) { "broker=amqps://svc:p%40ss%3Aw0rd@broker.example:5671/vhost", "connection_string=host=db password='sp ace0123' sslmode=disable", "connection_string=host=db user=u password=password sslmode=disable", + "connection_string=host=db password=a;b,c&d0123 dbname=x", }) joined := strings.Join(redacted, " ") - for _, secret := range []string{"pgSecret0123", "amqpSecret0123", "hookToken0123", "p%40ss%3Aw0rd", "p@ss:w0rd", "sp ace0123"} { + for _, secret := range []string{"pgSecret0123", "amqpSecret0123", "hookToken0123", "p%40ss%3Aw0rd", "p@ss:w0rd", "sp ace0123", "a;b,c&d0123"} { if strings.Contains(joined, secret) { t.Errorf("redacted arguments still carry %q: %s", secret, joined) } @@ -221,7 +224,7 @@ func TestRegisterKeyValueSecretsCoversEmbeddedCredentials(t *testing.T) { t.Errorf("%q was not registered: %q", secret, got) } } - for _, keep := range []string{"notify_postgres:1", "host=db user=u password=" + redactedMarker + " dbname=x", "amqp://user:" + redactedMarker + "@host:5672", "https://h/hook?token=" + redactedMarker, "queue_dir=/var/queue", "amqps://svc:" + redactedMarker + "@broker.example:5671/vhost", "password='" + redactedMarker + "'", "password=password sslmode=disable"} { + for _, keep := range []string{"notify_postgres:1", "host=db user=u password=" + redactedMarker + " dbname=x", "amqp://user:" + redactedMarker + "@host:5672", "https://h/hook?token=" + redactedMarker, "queue_dir=/var/queue", "amqps://svc:" + redactedMarker + "@broker.example:5671/vhost", "password='" + redactedMarker + "'", "password=password sslmode=disable", "host=db password=" + redactedMarker + " dbname=x"} { if !strings.Contains(joined, keep) { t.Errorf("redacted arguments lost %q: %s", keep, joined) } diff --git a/cmd/secret-registry.go b/cmd/secret-registry.go index 406c99b3e4..159bff10bf 100644 --- a/cmd/secret-registry.go +++ b/cmd/secret-registry.go @@ -137,11 +137,15 @@ func isSecretKeyValueName(key string) bool { return false } -// embeddedSecretRegexp finds a credential assignment inside a composite -// value whose own key names nothing secret: a DSN ("host=db user=u -// password=s"), a connection string, a webhook endpoint with "?token=s". -// The payload may be quoted ("password='p@ss word'"). -var embeddedSecretRegexp = regexp.MustCompile(`(?i)(?:^|[\s;,&?])(password|passwd|pwd|secret|token|api[_-]?key|access[_-]?key|secret[_-]?key|client[_-]?secret|private[_-]?key)=("[^"]*"|'[^']*'|[^\s;,&]+)`) +// embeddedSecretRegexps find a credential assignment inside a composite +// value whose own key names nothing secret. In a DSN or connection string +// ("host=db user=u password=s") the payload runs to the next whitespace - +// libpq allows ";", "," and "&" in an unquoted value - or is quoted +// ("password='p@ss word'"); in a URL query ("?token=s&x=1") it stops at "&". +var embeddedSecretRegexps = []*regexp.Regexp{ + regexp.MustCompile(`(?i)(?:^|[\s;,])(password|passwd|pwd|secret|token|api[_-]?key|access[_-]?key|secret[_-]?key|client[_-]?secret|private[_-]?key)=("[^"]*"|'[^']*'|[^\s]+)`), + regexp.MustCompile(`(?i)[?&](password|passwd|pwd|secret|token|api[_-]?key|access[_-]?key|secret[_-]?key|client[_-]?secret|private[_-]?key)=([^\s&]+)`), +} // embeddedSecrets returns the credential material a value carries inside it: // the password of a URL's userinfo - decoded, and exactly as written, since @@ -158,9 +162,11 @@ func embeddedSecrets(value string) []string { } } } - for _, match := range embeddedSecretRegexp.FindAllStringSubmatch(value, -1) { - if secret := strings.Trim(match[2], `"'`); !strings.EqualFold(secret, match[1]) { - secrets = append(secrets, secret) + for _, pattern := range embeddedSecretRegexps { + for _, match := range pattern.FindAllStringSubmatch(value, -1) { + if secret := strings.Trim(match[2], `"'`); !strings.EqualFold(secret, match[1]) { + secrets = append(secrets, secret) + } } } return secrets From 328efee1b06480d450c8935dbf46794462f82cf8 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Mon, 31 Aug 2026 19:51:21 +0800 Subject: [PATCH 7/7] fix: register the password before the first separator of an unquoted DSN payload too An ODBC-style connection string has no whitespace, so the DSN rule captures the whole tail after Password=; a later echo carries only the password, so the part before the first ";" or "," is registered as well. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VX7dtHJ4q7GJ1YDYeXLmjW Signed-off-by: Feng Ruohang --- cmd/redaction-prose_test.go | 3 ++- cmd/secret-registry.go | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/cmd/redaction-prose_test.go b/cmd/redaction-prose_test.go index 7df7f8342d..a42ea3987c 100644 --- a/cmd/redaction-prose_test.go +++ b/cmd/redaction-prose_test.go @@ -214,9 +214,10 @@ func TestRegisterKeyValueSecretsCoversEmbeddedCredentials(t *testing.T) { "connection_string=host=db password='sp ace0123' sslmode=disable", "connection_string=host=db user=u password=password sslmode=disable", "connection_string=host=db password=a;b,c&d0123 dbname=x", + "conn=Server=x;Password=Str0ngP4ss0123;Database=z", }) joined := strings.Join(redacted, " ") - for _, secret := range []string{"pgSecret0123", "amqpSecret0123", "hookToken0123", "p%40ss%3Aw0rd", "p@ss:w0rd", "sp ace0123", "a;b,c&d0123"} { + for _, secret := range []string{"pgSecret0123", "amqpSecret0123", "hookToken0123", "p%40ss%3Aw0rd", "p@ss:w0rd", "sp ace0123", "a;b,c&d0123", "Str0ngP4ss0123"} { if strings.Contains(joined, secret) { t.Errorf("redacted arguments still carry %q: %s", secret, joined) } diff --git a/cmd/secret-registry.go b/cmd/secret-registry.go index 159bff10bf..dd17fc6b40 100644 --- a/cmd/secret-registry.go +++ b/cmd/secret-registry.go @@ -164,8 +164,18 @@ func embeddedSecrets(value string) []string { } for _, pattern := range embeddedSecretRegexps { for _, match := range pattern.FindAllStringSubmatch(value, -1) { - if secret := strings.Trim(match[2], `"'`); !strings.EqualFold(secret, match[1]) { - secrets = append(secrets, secret) + secret := strings.Trim(match[2], `"'`) + if strings.EqualFold(secret, match[1]) { + continue + } + secrets = append(secrets, secret) + // An ODBC-style string ("Password=x;Database=z") has no whitespace, + // so the whole tail was captured; the part before the first + // separator is what a later echo would carry. + if unquoted := match[2] == secret; unquoted { + if i := strings.IndexAny(secret, ";,"); i > 0 { + secrets = append(secrets, secret[:i]) + } } } }