Fix/redact credential logs

CHANGELOG.md

@@ -15,6 +15,17 @@ 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.
+- 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
 
 - Added `--collectdatabase`/`S3GC_COLLECTDATABASE` to qualify the auxiliary

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
@@ -557,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
@@ -607,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 = "<redacted>"
         if backslash:
             print(" \\")
         print(f" S3GC_{key.upper()}={value}", end="")
@@ -672,7 +676,11 @@ def graceful_exit():
 consoleHandler.setFormatter(logFormatter)
 logger.addHandler(consoleHandler)
 
-logger.debug(f"Parameters: {args}")
+diagnostic_args = vars(args).copy()
+for _secret_argument in SENSITIVE_ARGUMENTS:
+    if diagnostic_args.get(_secret_argument):
+        diagnostic_args[_secret_argument] = "<redacted>"
+logger.debug("Parameters: %s", Namespace(**diagnostic_args))
 
 tname = ""
 dbname = None
@@ -925,7 +933,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
 

tests/test_s3gc.py

@@ -259,6 +259,79 @@ 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_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}=<redacted>" 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")],