villebro commented on code in PR #233: URL: https://github.com/apache/superset-kubernetes-operator/pull/233#discussion_r3650587629
########## internal/controller/redact.go: ########## @@ -0,0 +1,69 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you 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 controller + +import "regexp" + +// redactedPlaceholder replaces credential values matched by the redaction +// patterns below. +const redactedPlaceholder = "***" + +var ( + // uriUserinfoRe matches the password component of a URI userinfo section + // (scheme://user:password@host). Task failure output commonly embeds full + // database connection URIs in driver error messages, e.g. + // "connection to postgresql://superset:hunter2@db:5432/superset failed". + // The user component may be empty (redis://:password@host). The password + // match is greedy up to the last @ in the whitespace-delimited token, so a + // malformed password containing a raw @ is masked in full rather than + // leaking its tail after the first @. + uriUserinfoRe = regexp.MustCompile(`([a-zA-Z][a-zA-Z0-9+.-]*://[^/@\s:]*):([^\s]+)@`) + + // authorizationHeaderRe matches HTTP Authorization header values, with or + // without an auth scheme: "Authorization: Bearer eyJ...", "authorization= + // Basic dXNlcg==". The scheme word is kept; only the credential is masked. + authorizationHeaderRe = regexp.MustCompile(`(?i)\b(authorization\s*[=:]\s*(?:(?:bearer|basic|token|digest|negotiate)\s+)?)(\S+)`) + + // bareBearerRe matches a bearer token without an Authorization prefix + // ("bearer eyJhbGci..."). The token must contain at least one digit so + // prose like "bearer authentication failed" is not mangled; real tokens + // (JWTs, base64, hex) virtually always contain digits. + bareBearerRe = regexp.MustCompile(`(?i)\b(bearer\s+)([A-Za-z0-9._~+/=-]*[0-9][A-Za-z0-9._~+/=-]*)`) + + // credentialAssignmentRe matches free-form credential assignments such as + // "password=hunter2", "PGPASSWORD: hunter2", or "secret_key = hunter2". + // The key may carry prefixes/suffixes (DB_PASSWORD, api-key-prod). + credentialAssignmentRe = regexp.MustCompile(`(?i)([A-Za-z0-9_-]*(?:password|passwd|pwd|secret|token|api[_-]?key|credential|passphrase)[A-Za-z0-9_-]*\s*[=:]\s*)(\S+)`) Review Comment: Before hand-rolling regex redaction, could you evaluate the existing Go libraries in this space and note the outcome in the PR description? The two closest are [`masq`](https://github.com/m-mizutani/masq) and [`logfusc`](https://github.com/AngusGMorrison/logfusc). My read is that neither quite fits, but it's worth stating rather than implying: - `logfusc` wraps secrets the program holds as typed values (`Secret[T]`) so they redact on log/marshal. Here the credentials arrive inline in opaque task-container stdout — we never hold them as typed values, so there's nothing to wrap. - `masq` is a `slog` `ReplaceAttr` filter. This redaction targets CRD `status` and Kubernetes Events, not a log sink, and its pattern matching would still need the same regexes supplied via `WithRegex` — so it adds a dependency and a slog impedance mismatch without removing the regexes. If that's the conclusion, a sentence or two in the PR description ("evaluated masq/logfusc; both target secrets held as typed values or slog attributes, whereas we scrub unknown credentials out of opaque subprocess output written to status/Events") would preempt exactly this question from other reviewers. If one of them *does* cover it, even better — we avoid maintaining regexes. Worth also noting the patterns here are inspired-by / reducible-to [gitleaks](https://github.com/gitleaks/gitleaks)' ruleset if you want a reference catalog, though importing gitleaks itself would be far too heavy for scrubbing a 256-char string. ########## docs/reference/releases.md: ########## @@ -30,6 +30,10 @@ This page tracks notable changes in Apache Superset Kubernetes Operator releases - **Helm extra manifests.** The Helm chart now supports `extraManifests` for rendering trusted, release-scoped Kubernetes manifests with Helm `tpl`. Use it for companion resources owned by the operator release, not shared cluster infrastructure such as Gateway API controllers, CRDs, or shared Gateways ([#196](https://github.com/apache/superset-kubernetes-operator/pull/196), [@younsl](https://github.com/younsl)). - **Helm resize policy.** The Helm chart now supports `resizePolicy` on the manager container, controlling whether an in-place pod resize (InPlacePodVerticalScaling) restarts the container ([#200](https://github.com/apache/superset-kubernetes-operator/pull/200), [@younsl](https://github.com/younsl)). +### Security Review Comment: `### Security` is a valid [Keep a Changelog](https://keepachangelog.com) category, so the header is fine — but the canonical ordering places it **last** (Added → Changed → Deprecated → Removed → Fixed → Security). Here it's inserted between `### Added` and `### Changed`. Could you move the section below `### Changed`? (Minor: the released `0.1.0` section already carries a "Task failure messages may include credential fragments" entry under **Known limitations** — that stays as historical record; this Security entry is effectively its mitigation.) ########## internal/controller/redact.go: ########## @@ -0,0 +1,69 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you 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 controller + +import "regexp" + +// redactedPlaceholder replaces credential values matched by the redaction +// patterns below. +const redactedPlaceholder = "***" + +var ( + // uriUserinfoRe matches the password component of a URI userinfo section + // (scheme://user:password@host). Task failure output commonly embeds full + // database connection URIs in driver error messages, e.g. + // "connection to postgresql://superset:hunter2@db:5432/superset failed". + // The user component may be empty (redis://:password@host). The password + // match is greedy up to the last @ in the whitespace-delimited token, so a + // malformed password containing a raw @ is masked in full rather than + // leaking its tail after the first @. + uriUserinfoRe = regexp.MustCompile(`([a-zA-Z][a-zA-Z0-9+.-]*://[^/@\s:]*):([^\s]+)@`) + + // authorizationHeaderRe matches HTTP Authorization header values, with or + // without an auth scheme: "Authorization: Bearer eyJ...", "authorization= + // Basic dXNlcg==". The scheme word is kept; only the credential is masked. + authorizationHeaderRe = regexp.MustCompile(`(?i)\b(authorization\s*[=:]\s*(?:(?:bearer|basic|token|digest|negotiate)\s+)?)(\S+)`) + + // bareBearerRe matches a bearer token without an Authorization prefix + // ("bearer eyJhbGci..."). The token must contain at least one digit so + // prose like "bearer authentication failed" is not mangled; real tokens + // (JWTs, base64, hex) virtually always contain digits. + bareBearerRe = regexp.MustCompile(`(?i)\b(bearer\s+)([A-Za-z0-9._~+/=-]*[0-9][A-Za-z0-9._~+/=-]*)`) + + // credentialAssignmentRe matches free-form credential assignments such as + // "password=hunter2", "PGPASSWORD: hunter2", or "secret_key = hunter2". + // The key may carry prefixes/suffixes (DB_PASSWORD, api-key-prod). + credentialAssignmentRe = regexp.MustCompile(`(?i)([A-Za-z0-9_-]*(?:password|passwd|pwd|secret|token|api[_-]?key|credential|passphrase)[A-Za-z0-9_-]*\s*[=:]\s*)(\S+)`) Review Comment: Before hand-rolling regex redaction, could you evaluate the existing Go libraries in this space and note the outcome in the PR description? The two closest that I found are [`masq`](https://github.com/m-mizutani/masq) and [`logfusc`](https://github.com/AngusGMorrison/logfusc). My read is that neither quite fits, but it's worth stating rather than implying: - `logfusc` wraps secrets the program holds as typed values (`Secret[T]`) so they redact on log/marshal. Here the credentials arrive inline in opaque task-container stdout — we never hold them as typed values, so there's nothing to wrap. - `masq` is a `slog` `ReplaceAttr` filter. This redaction targets CRD `status` and Kubernetes Events, not a log sink, and its pattern matching would still need the same regexes supplied via `WithRegex` — so it adds a dependency and a slog impedance mismatch without removing the regexes. If that's the conclusion, a sentence or two in the PR description ("evaluated masq/logfusc; both target secrets held as typed values or slog attributes, whereas we scrub unknown credentials out of opaque subprocess output written to status/Events") would preempt exactly this question from other reviewers. If one of them *does* cover it, even better — we avoid maintaining regexes. Worth also noting the patterns here are inspired-by / reducible-to [gitleaks](https://github.com/gitleaks/gitleaks)' ruleset if you want a reference catalog, though importing gitleaks itself would be far too heavy for scrubbing a 256-char string. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
