PPI-1616 deprecate CLI flags for secrets, accept env vars

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## [Unreleased]
 
+- PPI-1616 Deprecated passing `--client-secret`/`--client-id`/`--auth-password`/`--auth-user` on the
+  command line (they leak via shell history and the process list); added `PIWIK_CLIENT_SECRET`,
+  `PIWIK_CLIENT_ID`, `PIWIK_AUTH_PASSWORD`, and `PIWIK_AUTH_USER` environment variable equivalents,
+  which take precedence when the corresponding flag isn't set.
 - PPNA-10000 Redact sensitive values from HTTP/auth debug logging
 - PPPSYS-56851 Require Python 3.10 or newer (3.9 and older are end-of-life); CI tests 3.10 through 3.14
 - PPPSYS-56851 Migrated packaging and CI from Poetry to uv (PEP 621, setuptools, committed `uv.lock`); PyPI releases use `uv publish`.

README.md

@@ -27,7 +27,11 @@ Lint and format with [Ruff](https://docs.astral.sh/ruff/) (same style as the Piw
 1. Download this git repository `git clone git@github.com:PiwikPRO/log-analytics.git`. The script uses only python standard library, so no external packages are required. Alternatively you can download our PyPi package - `pip install piwik-pro-log-analytics`.
 2. Generate Client ID and Client Secret for communication with Piwik PRO API - docs on how to do this can be found on [developers.piwik.pro](https://developers.piwik.pro/en/latest/data_collection/other_integrations/web_log_analytics.html)
 3. You are now ready to import your web server's access logs into Piwik PRO:
-  * `piwik_pro_log_analytics/import_logs.py --client-id <client-id> --client-secret <client-secret> --url=<my-organization>.piwik.pro /path/to/access.log`
+  * Set `PIWIK_CLIENT_ID` and `PIWIK_CLIENT_SECRET` in a secure way
+  * `piwik_pro_log_analytics/import_logs.py --url=<my-organization>.piwik.pro /path/to/access.log`
+  * The `--client-id`/`--client-secret` flags are also accepted, but are deprecated: passing them on
+    the command line exposes them in your shell history and to other local users via the process
+    list. Prefer the environment variables shown above.
   * If you installed log analytics via `pip`, instead of `piwik_pro_log_analytics/import_logs.py` use `piwik_pro_log_analytics`
   * If the code fails, saying, that your log format doesn't contain hostname - you must decide what App you'd like to track to. You can find App ID in Piwik PRO UI> Administration> Sites & apps>. After that, use `--idsite <app-id>` flag to tell the importer which App you'd like to track to.
 ![How to find App ID](docs/app-id.png "How to find App ID")

piwik_pro_log_analytics/import_logs.py

@@ -761,13 +761,19 @@ def _create_parser(self):
         parser.add_argument(
             "--auth-user",
             dest="auth_user",
-            help="Basic auth user",
+            default=None,
+            help="Basic auth user. Can also be set via the PIWIK_AUTH_USER environment variable.",
         )
         # Basic auth password
         parser.add_argument(
             "--auth-password",
             dest="auth_password",
-            help="Basic auth password",
+            default=None,
+            help=(
+                "Basic auth password. Passing it on the command line is deprecated - it exposes the"
+                " password in your shell history and to other users via the process list. Set the"
+                " PIWIK_AUTH_PASSWORD environment variable instead."
+            ),
         )
         parser.add_argument(
             "--debug",
@@ -860,12 +866,21 @@ def _create_parser(self):
         parser.add_argument(
             "--client-id",
             dest="client_id",
-            help="Client ID used when OAuth authentication is needed",
+            default=None,
+            help=(
+                "Client ID used when OAuth authentication is needed. Can also be set via the"
+                " PIWIK_CLIENT_ID environment variable."
+            ),
         )
         parser.add_argument(
             "--client-secret",
             dest="client_secret",
-            help="Client secret used when OAuth authentication is needed",
+            default=None,
+            help=(
+                "Client secret used when OAuth authentication is needed. Passing it on the command"
+                " line is deprecated - it exposes the secret in your shell history and to other"
+                " users via the process list. Set the PIWIK_CLIENT_SECRET environment variable instead."
+            ),
         )
 
         parser.add_argument(
@@ -1358,6 +1373,28 @@ def _parse_args(self, option_parser, argv=None):
             level=logging.DEBUG if self.options.debug >= 1 else logging.INFO,
         )
 
+        if self.options.client_secret is None:
+            self.options.client_secret = os.environ.get("PIWIK_CLIENT_SECRET")
+        else:
+            logging.warning(
+                "DeprecationWarning: passing --client-secret on the command line is deprecated and may be"
+                " removed in a future release, since it can leak the secret via shell history or the process"
+                " list. Set the PIWIK_CLIENT_SECRET environment variable instead."
+            )
+        if self.options.client_id is None:
+            self.options.client_id = os.environ.get("PIWIK_CLIENT_ID")
+
+        if self.options.auth_password is None:
+            self.options.auth_password = os.environ.get("PIWIK_AUTH_PASSWORD")
+        else:
+            logging.warning(
+                "DeprecationWarning: passing --auth-password on the command line is deprecated and may be"
+                " removed in a future release, since it can leak the password via shell history or the process"
+                " list. Set the PIWIK_AUTH_PASSWORD environment variable instead."
+            )
+        if self.options.auth_user is None:
+            self.options.auth_user = os.environ.get("PIWIK_AUTH_USER")
+
         self.options.excluded_useragents = set([s.lower() for s in self.options.excluded_useragents])
 
         self._parse_paths()
@@ -1486,9 +1523,7 @@ def _get_token_auth(self):
 
             return api_result
         else:
-            fatal_error(
-                "OAuth authentication failed. Make sure that --client-id and --client-secret options are provided."
-            )
+            fatal_error("OAuth authentication failed. Make sure that PIWIK_CLIENT_ID and PIWIK_CLIENT_SECRET are set.")
 
     def get_resolver(self):
         if self.options.site_id:

tests/test_main.py

@@ -1324,6 +1324,106 @@ def test_glob_filenames():
     ]
 
 
+def test_client_credentials_fall_back_to_environment_variables():
+    argv = ["--url=http://localhost", "logs/common.log"]
+
+    with patch.dict(os.environ, {"PIWIK_CLIENT_ID": "env-id", "PIWIK_CLIENT_SECRET": "env-secret"}):
+        config = import_logs.Configuration(argv)
+
+    assert config.options.client_id == "env-id"
+    assert config.options.client_secret == "env-secret"
+
+
+def test_client_credentials_cli_args_take_precedence_over_environment():
+    argv = [
+        "--url=http://localhost",
+        "--client-id=cli-id",
+        "--client-secret=cli-secret",
+        "logs/common.log",
+    ]
+
+    with patch.dict(os.environ, {"PIWIK_CLIENT_ID": "env-id", "PIWIK_CLIENT_SECRET": "env-secret"}):
+        config = import_logs.Configuration(argv)
+
+    assert config.options.client_id == "cli-id"
+    assert config.options.client_secret == "cli-secret"
+
+
+def test_client_secret_via_cli_arg_logs_a_warning(caplog):
+    argv = ["--url=http://localhost", "--client-secret=cli-secret", "logs/common.log"]
+
+    with patch.dict(os.environ, {}, clear=False):
+        os.environ.pop("PIWIK_CLIENT_SECRET", None)
+        with caplog.at_level(logging.WARNING):
+            import_logs.Configuration(argv)
+
+    assert any(
+        "PIWIK_CLIENT_SECRET" in record.message and "deprecated" in record.message.lower() for record in caplog.records
+    )
+
+
+def test_client_credentials_absent_without_args_or_environment():
+    argv = ["--url=http://localhost", "logs/common.log"]
+
+    with patch.dict(os.environ, {}, clear=False):
+        os.environ.pop("PIWIK_CLIENT_ID", None)
+        os.environ.pop("PIWIK_CLIENT_SECRET", None)
+        config = import_logs.Configuration(argv)
+
+    assert config.options.client_id is None
+    assert config.options.client_secret is None
+
+
+def test_basic_auth_credentials_fall_back_to_environment_variables():
+    argv = ["--url=http://localhost", "logs/common.log"]
+
+    with patch.dict(os.environ, {"PIWIK_AUTH_USER": "env-user", "PIWIK_AUTH_PASSWORD": "env-password"}):
+        config = import_logs.Configuration(argv)
+
+    assert config.options.auth_user == "env-user"
+    assert config.options.auth_password == "env-password"
+
+
+def test_basic_auth_credentials_cli_args_take_precedence_over_environment():
+    argv = [
+        "--url=http://localhost",
+        "--auth-user=cli-user",
+        "--auth-password=cli-password",
+        "logs/common.log",
+    ]
+
+    with patch.dict(os.environ, {"PIWIK_AUTH_USER": "env-user", "PIWIK_AUTH_PASSWORD": "env-password"}):
+        config = import_logs.Configuration(argv)
+
+    assert config.options.auth_user == "cli-user"
+    assert config.options.auth_password == "cli-password"
+
+
+def test_auth_password_via_cli_arg_logs_a_warning(caplog):
+    argv = ["--url=http://localhost", "--auth-password=cli-password", "logs/common.log"]
+
+    with patch.dict(os.environ, {}, clear=False):
+        os.environ.pop("PIWIK_AUTH_PASSWORD", None)
+        with caplog.at_level(logging.WARNING):
+            import_logs.Configuration(argv)
+
+    assert any(
+        "PIWIK_AUTH_PASSWORD" in record.message and "deprecated" in record.message.lower() for record in caplog.records
+    )
+
+
+def test_basic_auth_credentials_absent_without_args_or_environment():
+    argv = ["--url=http://localhost", "logs/common.log"]
+
+    with patch.dict(os.environ, {}, clear=False):
+        os.environ.pop("PIWIK_AUTH_USER", None)
+        os.environ.pop("PIWIK_AUTH_PASSWORD", None)
+        config = import_logs.Configuration(argv)
+
+    assert config.options.auth_user is None
+    assert config.options.auth_password is None
+
+
 # UrlHelper tests
 def test_urlhelper_convert_array_args():
     def _test(input, expected):