Skip to content

fix: mask sensitive values in env override logging#1441

Open
mvanhorn wants to merge 9 commits into
Altinity:masterfrom
mvanhorn:fix/1429-mask-sensitive-env-overrides
Open

fix: mask sensitive values in env override logging#1441
mvanhorn wants to merge 9 commits into
Altinity:masterfrom
mvanhorn:fix/1429-mask-sensitive-env-overrides

Conversation

@mvanhorn

Copy link
Copy Markdown
Contributor

Summary

When environment variables are overridden via --env, the override is logged at info level as override NAME=VALUE with the value in cleartext. For secret-bearing variables (passwords, access keys, storage credentials, encryption keys) that writes the raw secret into the log, where it can persist in log files, aggregators, or CI output. This masks the value in that log line when the variable name looks sensitive, printing [MASKED] instead, while leaving non-sensitive overrides readable.

Why this matters

Issue #1429 asks for the env-override logging to stop leaking secrets. The override log is otherwise useful for debugging which variables were applied, so the fix keeps the line but redacts only the value, and only when the variable name matches a sensitive token. The token list covers the documented credential and encryption-key variables across backends (passwords, secrets, access/secret keys, AZBLOB_ACCOUNT_KEY / SSE keys, S3_SSE_CUSTOMER_KEY, GCS_ENCRYPTION_KEY, tokens, SAS, connection strings), so common Azure/GCS/S3 SSE configurations no longer expose credentials in logs.

Changes

pkg/config/config.go gains a maskSensitiveEnvValue(name, value) helper backed by a sensitiveEnvNameTokens list and uses it only at the override NAME=VALUE info-log call site. The override behavior itself is unchanged; only the logged representation of the value changes for sensitive names.

Testing

Added tests in pkg/config/config_test.go asserting that sensitive overrides (including the Azure/GCS/SSE key variables) log [MASKED] and never leak the raw value, that a non-sensitive variable still logs its value, and that the env override is applied regardless. The log-capture test pins LOG_LEVEL=info so it is independent of the surrounding process's log level. go test ./pkg/config passes (including under LOG_LEVEL=error); go vet and gofmt are clean.

Closes #1429

@Slach Slach self-requested a review July 1, 2026 03:00
Slach and others added 4 commits July 1, 2026 17:19
The env-override masking added for OverrideEnvVars only covered the
config.go log line. When an action is submitted via POST /backup/actions
with the command string carrying --env=NAME=VALUE, the raw command was
logged, stored in the async status list (exposed via GET /backup/actions
and system.backup_actions), and echoed back in error/ack responses with
the secret in cleartext.

Add config.MaskEnvOverrideCommand to redact only the value of sensitive
--env/--environment-override tokens (=, space, and quoted forms) while
preserving the rest of the command, and apply it to row.Command in the
actions() handler after args are parsed from the raw string, so all
downstream logging/status/response uses inherit the masked form while
execution still applies the real values. Also mask the value on the
"can't override" warn branch in OverrideEnvVars, which the original fix
missed.

Refs Altinity#1429
When clickhouse fails to become healthy on an Azure-backed disk, the
azurite log dump (added in 350fcde) emitted ~13m of debug-level output
as one unprefixed blob, burying the actual "--- FAIL: TestX" line under
thousands of idle GC lines and making the failing test hard to spot in
interleaved parallel CI output.

Thread testName into DumpContainerLogsSince so every dumped line is
prefixed with the owning test, and filter azurite's idle
maintenance chatter (GC mark-sweep loops, per-extent skip lines, account
re-init) so the request/error lines that explain the timeout stay
visible. --debug is left on; the fix is noise reduction, not less detail.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
queryWithNoError dumped clickhouse container logs with an empty testName
on query failure, so in interleaved parallel CI output the dump could not
be tied to the owning test. Pass t through and feed t.Name() into
DumpContainerLogsSince (same fix as the azurite dump in 3a48b39), and add
t.Helper() so failures point at the caller.

Swept all call sites (r, -> t, r,) and threaded t through the handful of
helpers that call it but didn't receive it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--env overrides show values of potentially sensitive variables

2 participants