You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2026/09/07 16:22:07 UTC
[PR] Mask CLI secrets in the audit log by parsed argument, not argv position [airflow]
potiuk opened a new pull request, #72650:
URL: https://github.com/apache/airflow/pull/72650
`_build_metrics` records `sys.argv` into the audit log (`Log.extra`) and masked
the `variables` value **by position**, assuming the key and value were the final
two arguments.
Any supported trailing option displaces them. `airflow variables set KEY VALUE
--description TEXT` ends in `--description TEXT`, so the value was written to the
audit log in clear. `--description` is the instance, not the class — the same is
true of `--serialize-json` and of any option added later.
### The change
* **Take the key and value from the parsed `Namespace`** and redact that value
wherever it appears in the recorded command. Position no longer matters.
* **`--conn-extra` is now masked.** It was not handled at all, and it is
precisely the field connection secrets live in.
* **`--conn-json` and `--conn-extra` are walked recursively.** `--conn-json`
masked only top-level keys, so a secret one level down was recorded in clear.
* **Both argparse spellings are handled.** `--conn-json`, `--conn-extra` and
`--conn-uri` were matched as standalone tokens only, so the equally valid
`--conn-json=VALUE` form skipped redaction entirely. `--conn-uri` had that gap
before this change too.
A value that cannot be parsed as JSON is redacted whole: these options carry
connection material, and a value that cannot be inspected is not safe to record.
### For operators
This prevents *new* disclosures only. **Existing `Log.extra` rows and backups
retain values already written** and should be redacted or expired, and any
credentials exposed that way rotated.
### Tests
Seven new cases: the value displaced by `--description` and by `--serialize-json`,
a non-sensitive key that must still be recorded as given, nested `--conn-extra`,
nested `--conn-json`, and the `=` spelling for `--conn-json`, `--conn-extra` and
`--conn-uri`. All seven fail against unpatched sources; the existing cases are
unchanged and still pass.
### Not addressed here
`airflow dags trigger --conf` and `airflow backfill create --dag-run-conf` also
accept JSON that reaches the audit log unmasked. That is a real and separate
pre-existing exposure with a different blast radius, and folding it in would
widen this change beyond the defect it was scoped to. Worth its own issue.
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
Generated-by: Claude Code following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mask CLI secrets in the audit log by parsed argument, not argv position [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #72650:
URL: https://github.com/apache/airflow/pull/72650#issuecomment-6277537810
### Backport successfully created: v3-4-test
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>✅</td>
<td>v3-4-test</td>
<td><a href="https://github.com/apache/airflow/pull/75144"><img src="https://img.shields.io/badge/PR-75144-blue" alt="PR Link"></a></td>
</tr>
</table>
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mask CLI secrets in the audit log by parsed argument, not argv position [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #72650:
URL: https://github.com/apache/airflow/pull/72650#issuecomment-6207505281
@amoghrajesh @kaxil Could you take a look at this when you have a moment? It's targeted at 3.4.0 and needs a review to make the cut on 2026-10-19. CI is green.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mask CLI secrets in the audit log by parsed argument, not argv position [airflow]
Posted by "vatsrahul1001 (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mask CLI secrets in the audit log by parsed argument, not argv position [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #72650:
URL: https://github.com/apache/airflow/pull/72650
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Mask CLI secrets in the audit log by parsed argument, not argv position [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #72650:
URL: https://github.com/apache/airflow/pull/72650#discussion_r4315509543
##########
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:
Done in ce5d9f1d9f. The masking now lives in `_mask_full_command`, and `_build_metrics` calls it inside a `try`. If it raises, the audit log records `airflow ********` (fully masked) and the command still runs. The warning logs only the exception type, since the message could quote the secret. A new test makes the masking raise and checks that the command still runs and the secret is not recorded.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
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:
Added in ce5d9f1d9f. `test_short_option_table_matches_the_parsers` walks the real CLI parser, finds every command with a short option that maps to one of the masked destinations, and compares their flag/value letters with `_SHORT_OPTION_ARITY`. A stale entry, or a new command with such an option, now fails the test. Provider groups (`users`, `celery`) are compared only when the provider is installed.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org