diff --git a/CHANGELOG.md b/CHANGELOG.md index b7ea01d..e46bbca 100644 --- a/CHANGELOG.md +++ b/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`. diff --git a/README.md b/README.md index 80edd42..608e88a 100644 --- a/README.md +++ b/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-secret --url=.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=.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 ` 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") diff --git a/piwik_pro_log_analytics/import_logs.py b/piwik_pro_log_analytics/import_logs.py index e92857a..23c2de9 100755 --- a/piwik_pro_log_analytics/import_logs.py +++ b/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: diff --git a/tests/test_main.py b/tests/test_main.py index f21e014..f185cac 100644 --- a/tests/test_main.py +++ b/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):