From f713f07f042fc3dd83669be33b63a3c1dccf6921 Mon Sep 17 00:00:00 2001 From: Diego Nieto Date: Sun, 25 Oct 2026 15:00:11 +0100 Subject: [PATCH 1/2] fix: redact credentials from log records Problem: ClickHouse passwords and S3 credentials were included in raw log records, so short values or handlers without the console formatter's masking could expose them.\n\nAcceptance criteria:\n- ClickHouse connection log records contain no password.\n- Debug parameter records redact the ClickHouse password, S3 access key, secret key, and session token before logging.\n- Regression tests cover short credentials and both log paths. --- CHANGELOG.md | 8 ++++++++ s3gc.py | 9 +++++++-- tests/test_s3gc.py | 48 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f41523a..5ccf21d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,14 @@ expose. ## [Unreleased] +### Fixed + +- Removed ClickHouse and S3 credentials from diagnostic log records. The + ClickHouse connection message no longer prints its password, and the debug + parameter dump redacts passwords, access keys, and session tokens before + constructing the log record. This protects short credentials and handlers + that do not use the console formatter's masking. + ### Added - Added `--collectdatabase`/`S3GC_COLLECTDATABASE` to qualify the auxiliary diff --git a/s3gc.py b/s3gc.py index 9f2a917..c00b6f8 100644 --- a/s3gc.py +++ b/s3gc.py @@ -14,6 +14,7 @@ It is possible to split these stages or do everything at one go. """ +from argparse import Namespace import os import sys from io import StringIO @@ -672,7 +673,11 @@ def graceful_exit(): consoleHandler.setFormatter(logFormatter) logger.addHandler(consoleHandler) -logger.debug(f"Parameters: {args}") +diagnostic_args = vars(args).copy() +for _secret_argument in ("chpass", "s3accesskey", "s3secretkey", "s3sessiontoken"): + if diagnostic_args.get(_secret_argument): + diagnostic_args[_secret_argument] = "" +logger.debug("Parameters: %s", Namespace(**diagnostic_args)) tname = "" dbname = None @@ -925,7 +930,7 @@ class UserVisibleError(RuntimeError): def connect_to_ch(): logger.info( - f"Connecting to ClickHouse, host={args.chhost}, port={args.chport}, username={args.chuser}, password={args.chpass}, s3path={args.s3path}, bucket={args.s3bucket}, s3path={args.s3path}" + f"Connecting to ClickHouse, host={args.chhost}, port={args.chport}, username={args.chuser}, s3path={args.s3path}, bucket={args.s3bucket}" ) global ch_client, ch_writer diff --git a/tests/test_s3gc.py b/tests/test_s3gc.py index f06141d..e884811 100644 --- a/tests/test_s3gc.py +++ b/tests/test_s3gc.py @@ -259,6 +259,54 @@ def get_client(**kwargs): assert namespace["ch_client"] is not namespace["ch_writer"] +def test_connect_to_ch_does_not_put_password_in_log_record( + s3gc_module, args_factory, monkeypatch, caplog +): + namespace = s3gc_module["connect_to_ch"].__globals__ + secret = "p7" + monkeypatch.setitem( + namespace, + "args", + args_factory( + chhost="clickhouse.example", + chport=8123, + chuser="s3gc", + chpass=secret, + chtimeout=60, + ), + ) + monkeypatch.setattr(namespace["clickhouse_connect"], "get_client", lambda **_kwargs: object()) + + with caplog.at_level("INFO", logger=namespace["logger"].name): + s3gc_module["connect_to_ch"]() + + assert secret not in caplog.text + assert "Connecting to ClickHouse" in caplog.text + + +def test_debug_parameter_log_redacts_all_credential_values(): + secrets = ("p7", "aK2", "sK3", "tK4") + code = """ +import runpy +import sys +script = sys.argv[1] +sys.argv = [script, "--debug", "--chpass", "p7", "--s3-access-key", "aK2", + "--s3-secret-key", "sK3", "--s3-session-token", "tK4"] +runpy.run_path(script, run_name="s3gc_test") +""" + result = subprocess.run( + [sys.executable, "-c", code, str(ROOT / "s3gc.py")], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0, result.stderr + assert "Parameters:" in result.stdout + for secret in secrets: + assert secret not in result.stdout + + def test_delete_entrypoint_requires_confirmation(): result = subprocess.run( ["sh", str(ROOT / "docker/kubernetes-entrypoint.sh")], From b870ffb7f78f58a78574ecef110e85f64f5a50ea Mon Sep 17 00:00:00 2001 From: Diego Nieto Date: Sun, 25 Oct 2026 15:03:01 +0100 Subject: [PATCH 2/2] fix: redact credentials from list options Problem: --listoptions printed parsed ClickHouse and S3 credentials to stdout, where they could enter terminal history or captured job output.\n\nAcceptance criteria:\n- Hide the ClickHouse password, S3 access key, secret key, and session token in listoptions output.\n- Preserve non-secret configuration output.\n- Add an offline regression test covering short credential values. --- CHANGELOG.md | 3 +++ s3gc.py | 5 ++++- tests/test_s3gc.py | 25 +++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5ccf21d..c35a900 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ expose. parameter dump redacts passwords, access keys, and session tokens before constructing the log record. This protects short credentials and handlers that do not use the console formatter's masking. +- Redacted ClickHouse and S3 credentials from the `--listoptions` environment + output while preserving its non-secret settings, so the configuration helper + cannot print secrets into terminal history or captured job output. ### Added diff --git a/s3gc.py b/s3gc.py index c00b6f8..625d93c 100644 --- a/s3gc.py +++ b/s3gc.py @@ -558,6 +558,7 @@ def coerce_bool(value): # print(out) args = parser.parse_args() +SENSITIVE_ARGUMENTS = ("chpass", "s3accesskey", "s3secretkey", "s3sessiontoken") # Every flag declared with action="store_true" arrives from the environment as a # raw string, and every non-empty string is truthy. Normalise all boolean @@ -608,6 +609,8 @@ def coerce_bool(value): # if (key in ['listoptions', 'verbose', 'debug', 'silent', 'keepdata', 'collectonly', 'usecollected']): if key in ["listoptions"]: continue + if key.lower() in SENSITIVE_ARGUMENTS and value: + value = "" if backslash: print(" \\") print(f" S3GC_{key.upper()}={value}", end="") @@ -674,7 +677,7 @@ def graceful_exit(): logger.addHandler(consoleHandler) diagnostic_args = vars(args).copy() -for _secret_argument in ("chpass", "s3accesskey", "s3secretkey", "s3sessiontoken"): +for _secret_argument in SENSITIVE_ARGUMENTS: if diagnostic_args.get(_secret_argument): diagnostic_args[_secret_argument] = "" logger.debug("Parameters: %s", Namespace(**diagnostic_args)) diff --git a/tests/test_s3gc.py b/tests/test_s3gc.py index e884811..391a88a 100644 --- a/tests/test_s3gc.py +++ b/tests/test_s3gc.py @@ -307,6 +307,31 @@ def test_debug_parameter_log_redacts_all_credential_values(): assert secret not in result.stdout +def test_listoptions_redacts_all_credential_values(): + secrets = { + "S3GC_CHPASS": "p7", + "S3GC_S3ACCESSKEY": "aK2", + "S3GC_S3SECRETKEY": "sK3", + "S3GC_S3SESSIONTOKEN": "tK4", + } + env = {key: value for key, value in os.environ.items() if not key.startswith("S3GC_")} + env.update(secrets) + + result = subprocess.run( + [sys.executable, str(ROOT / "s3gc.py"), "--listoptions"], + capture_output=True, + text=True, + check=False, + env=env, + ) + + assert result.returncode == 0, result.stderr + assert "ENV " in result.stdout + for name, secret in secrets.items(): + assert f"{name}=" in result.stdout + assert secret not in result.stdout + + def test_delete_entrypoint_requires_confirmation(): result = subprocess.run( ["sh", str(ROOT / "docker/kubernetes-entrypoint.sh")],