Fix secrets not masked in Rendered Templates view with KubernetesPodOperator - #68975
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
…perator When var.json is used in a Jinja template (e.g. to pass secrets as env vars to a KubernetesPodOperator), _get_variable deserialised the JSON before masking. The isinstance(var_val, str) guard evaluated to False for the resulting dict, so mask_secret was never called. The secret was stored unredacted in the rendered_task_instance_fields table and shown as plain text in the Rendered Templates view. Both the SecretCache hit path and the secrets-backend path receive the same fix.
df8e81f to
b23955a
Compare
e95a347 to
b23955a
Compare
SameerMesiah97
left a comment
There was a problem hiding this comment.
Just 2 missing test cases. CI needs to be triggered. Looks good otherwise.
|
It's an interesting one - I am not sure if we want to merge this one. This gives false sense of security - you know you are not supposed to pass secrets in K8S Pod Operator Env Vars ?
I will psss that to the security team for consideration - but I think this one - especially that it addresses something we explicitly told is a bad practice is not a good idea |
|
@potiuk totally agree passing secrets via pod env vars isn't safe practice, and this doesn't change that guidance at all, k8s level visibility stays the same either way. The reason I'd still love to see this merged: this masking already works fine in Airflow 2. In AF3 it quietly stopped working for var.json values in some cases, so this is really just restoring existing behavior, not adding anything new. I worry that teams who tested and trusted this on AF2 might not realize it's silently different on AF3. One more bit of context in case it's useful: I'm working on an AF2 to AF3 migration with a lot of existing DAGs, some of which already use this pattern (not by choice, just the reality of a large existing codebase). Losing masking silently on upgrade is a real regression for that kind of migration, not just a hypothetical, but happy to defer to whatever the security team decides either way. |
Addressed two bugs in _mask_and_deserialize_variable flagged in review: - String scalars (e.g. var.json storing '"s3cr3t"'): only the quoted raw was registered as a mask pattern, leaving the unquoted value that lands in rendered_task_instance_fields unmasked. Fix: call mask_secret(val, key) for str values so both forms are registered. - List variables: mask_secret(val) without a name is unconditional mode, causing all elements of non-sensitive lists to be redacted from logs. Fix: restrict the dict-walk call to dict values only.
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Regression fix, restores Airflow 2 behavior, adds nothing new.
When var.json is used in a Jinja template (e.g. to pass secrets as env vars to a KubernetesPodOperator), _get_variable deserialised the JSON before masking. The isinstance(var_val, str) guard evaluated to False for the resulting dict, so mask_secret was never called. The secret was stored unredacted in the rendered_task_instance_fields table and shown as plain text in the Rendered Templates view.
Both the SecretCache hit path and the secrets-backend path receive the same fix.
Before the fix:
After the fix:
DAG to reproduce it:
Fixes the issue #69021
Was generative AI tooling used to co-author this PR?