RehanAhmad25 commented on issue #72377:
URL: https://github.com/apache/airflow/issues/72377#issuecomment-5504179025
Hi @eitan-shalev .
Took a look at the current source before weighing in here. Scope is a bit
wider than my first pass suggested, but still tractable.
`Param.schema` already accepts arbitrary JSON Schema keywords via `**kwargs`
(see `task-sdk/src/airflow/sdk/definitions/param.py`), so `format="password"`
works today with zero changes to the `Param` class. The "Trigger DAG w/ config"
form is also already covered: `FieldPassword.tsx` renders a masked input with a
show/hide toggle whenever `schema.format === "password"`, wired through
`FieldSelector.tsx`. A new `SecretParam` class would mostly duplicate what this
schema-based approach already gives us.
The real gap is redaction downstream of the trigger form: it's masked while
typing, but nothing masks it once stored.
Proposed scope for a first PR:
- Redact `conf` in the API/UI: a helper that checks each top-level `conf`
key against its declared `Param`'s schema and replaces the value with `***`
when `format == "password"`. Applied to `DAGRunResponse` /
`DAGRunCollectionResponse` so both the DAG Run Details page and the REST API
are covered.
- Register these values with `SecretsMasker` at resolve time, so they're
also redacted in task logs if a user does something like `print(dag_run.conf)`.
Doing the UI masking without this would create a false sense of security.
- Document `format="password"` in `core-concepts/params.rst`, since it
already works but isn't mentioned there.
Open questions before I start:
1. Rendered Templates: if a task templates this value (`{{
dag_run.conf['api_token'] }}`), it'll currently show up in plaintext in that
task instance's Rendered Template tab. That's a separate rendering path from
`DAGRunResponse` and I haven't dug into whether there's an existing hook to
mask it there too. Worth doing in this PR, or a known follow-up?
2. Storage at rest: `dag_run.conf` is stored as plaintext JSON in the
metadata DB today. Is masking in the API/UI/logs sufficient for v1, or is there
appetite for Fernet-encrypting these specific values the way
Connections/Variables are handled? I'd treat that as a separate, larger
discussion rather than bundle it into this fix, but wanted to flag it
explicitly rather than assume.
One known edge case I'm aware of and not planning to solve in v1: a `Param`
can itself declare a nested object schema with its own `format` on a
sub-property, which the top-level-key check above won't catch. Flagging it
rather than building full recursive masking before the top-level approach is
even confirmed.
Happy to pick this up if the scope above sounds right. Let me know your take
on the two open questions.
--
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]
