vatsrahul1001 commented on code in PR #72650:
URL: https://github.com/apache/airflow/pull/72650#discussion_r4306172279
##########
airflow-core/src/airflow/utils/cli.py:
##########
@@ -137,6 +137,297 @@ def wrapper(*args, **kwargs):
return action_logging
+MASKED_VALUE = "*" * 8
+
+# Options whose value is a secret on its own, keyed by the destination
argparse stores the
+# parsed value under. Masking is driven by what is present in the parsed
namespace, so ``-p``
+# is only treated as a password on commands where it parses into ``password``
(``api-server -p``
+# parses into ``port`` and is left alone).
+_SECRET_OPTIONS: dict[str, tuple[str, ...]] = {
+ "password": ("-p", "--password"),
+ "conn_password": ("--conn-password",),
+ "basic_auth": ("-A", "--basic-auth"),
+}
+# Options whose value is JSON that may hold secrets under sensitive keys.
+_JSON_OPTIONS: dict[str, tuple[str, ...]] = {
+ "conn_json": ("--conn-json",),
+ "conn_extra": ("--conn-extra",),
+ "conf": ("-c", "--conf"),
+ "dag_run_conf": ("--dag-run-conf",),
+ "task_params": ("-t", "--task-params"),
+ "env_vars": ("--env-vars",),
+}
+# Options whose value is a URI that may hold secrets in its credentials or
query.
+_URI_OPTIONS: dict[str, tuple[str, ...]] = {
+ "conn_uri": ("--conn-uri",),
+ "broker_api": ("-a", "--broker-api"),
+}
+# Short options of the commands that take one of the short options above, as
+# ``(boolean flags, options taking a value)``, so a cluster such as
``-vpVALUE`` is walked the
+# way argparse reads it. Keep in sync with the command definitions in
``cli_config`` and the
+# FAB and Celery providers. Commands missing here are masked conservatively.
+_SHORT_OPTION_ARITY: dict[tuple[str, str], tuple[str, str]] = {
Review Comment:
Optional: this table is hand-synced with the command definitions, and drift
would silently change what gets masked. A test that checks the entries against
the real parsers in `cli_config` would catch that.
##########
airflow-core/src/airflow/utils/cli.py:
##########
@@ -154,59 +445,42 @@ def _build_metrics(func_name, namespace):
sub_commands_to_check_for_sensitive_fields = {"users", "connections"}
sub_commands_to_check_for_sensitive_key = {"variables"}
sensitive_fields = {"-p", "--password", "--conn-password"}
+ # The long spellings name a password on every subcommand, including ones
contributed by
+ # providers (``keycloak-auth-manager ... --password``), so they are masked
everywhere. ``-p``
+ # is only a password on the subcommands above; elsewhere it is e.g.
``--port``.
+ always_sensitive_fields = {"--password", "--conn-password"}
full_command = list(sys.argv)
sub_command = full_command[1] if len(full_command) > 1 else None
# For cases when value under sub_commands_to_check_for_sensitive_key have
sensitive info
if sub_command in sub_commands_to_check_for_sensitive_key:
- key = full_command[-2] if len(full_command) > 3 else None
- if key and _secrets_masker().should_hide_value_for_key(key):
- # Mask the sensitive value since key contain sensitive keyword
- full_command[-1] = "*" * 8
- elif sub_command in sub_commands_to_check_for_sensitive_fields:
+ # Identify the value from the parsed command rather than from its
position in
+ # argv. The key and value are not reliably the last two arguments: any
supported
+ # trailing option displaces them (``variables set KEY VALUE
--description TEXT``
+ # ends in ``--description TEXT``), which left the value recorded in
clear.
+ key = getattr(namespace, "key", None)
+ value = getattr(namespace, "value", None)
+ if key and value and _secrets_masker().should_hide_value_for_key(key):
+ full_command = [MASKED_VALUE if arg == value else arg for arg in
full_command]
+ else:
+ fields = (
+ sensitive_fields
+ if sub_command in sub_commands_to_check_for_sensitive_fields
+ else always_sensitive_fields
+ )
for idx, command in enumerate(full_command):
- if command in sensitive_fields:
+ if command in fields:
# For cases when password is passed as "--password xyz" (with
space between key and value)
- full_command[idx + 1] = "*" * 8
+ if idx + 1 < len(full_command):
+ full_command[idx + 1] = MASKED_VALUE
else:
# For cases when password is passed as "--password=xyz" (with
'=' between key and value)
- for sensitive_field in sensitive_fields:
+ for sensitive_field in fields:
if command.startswith(f"{sensitive_field}="):
- full_command[idx] = f"{sensitive_field}={'*' * 8}"
-
- # handle conn-json and conn-uri separately as it requires different
handling
- if "--conn-json" in full_command:
- import json
-
- json_index = full_command.index("--conn-json") + 1
- conn_json = json.loads(full_command[json_index])
- for k in conn_json:
- if k and _secrets_masker().should_hide_value_for_key(k):
- conn_json[k] = "*" * 8
- full_command[json_index] = json.dumps(conn_json)
-
- if "--conn-uri" in full_command:
- from urllib.parse import urlparse, urlunparse
-
- uri_index = full_command.index("--conn-uri") + 1
- conn_uri = full_command[uri_index]
- parsed_uri = urlparse(conn_uri)
- netloc = parsed_uri.netloc
- if parsed_uri.password:
- password = "*" * 8
- netloc = f"{parsed_uri.username}:{password}@{parsed_uri.hostname}"
- if parsed_uri.port:
- netloc += f":{parsed_uri.port}"
-
- full_command[uri_index] = urlunparse(
- (
- parsed_uri.scheme,
- netloc,
- parsed_uri.path,
- parsed_uri.params,
- parsed_uri.query,
- parsed_uri.fragment,
- )
- )
+ full_command[idx] = f"{sensitive_field}={MASKED_VALUE}"
+
+ # Options carrying secrets (on their own, or inside JSON and URIs) are
masked by their
+ # parsed value, whatever spelling was used for them on the command line.
+ full_command = _mask_parsed_arguments(full_command, namespace)
Review Comment:
`_build_metrics` runs before the `try` in the `action_logging` wrapper, so
any unexpected exception from this masking aborts every CLI command. It would
be safer to catch exceptions here and fall back to masking the whole recorded
command, so a masking bug can neither break the command nor leak a secret.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
