RehanAhmad25 commented on issue #72377: URL: https://github.com/apache/airflow/issues/72377#issuecomment-5506756841
Thanks @eitan-shalev for the fast turnaround. Went and actually traced point 1 in the docs page as well as in the source, didn't want to propose something I hadn't verified. Here's what I found, changes the plan a bit and is more precise than my first pass. **1. Rendered Templates** You're right, and the real mechanism is even better than what I described. The redaction that actually matters isn't server-side, it happens in the worker: `_serialize_rendered_fields()` in `task-sdk/src/airflow/sdk/execution_time/task_runner.py` calls `redact()` from `airflow.sdk._shared.secrets_masker` on every template field before it gets sent to the API server via the `SetRenderedFields` message. There's actually a comment on that function confirming this is intentional: "This ensures that the secrets those are registered via mask_secret() on workers / dag processor are properly masked on the UI." I double checked the import path before trusting this, since `mask_secret()` needs to be writing to the same masker instance that `redact()` reads from. `airflow.sdk.log.mask_secret` (the public function from the masking docs) imports from that exact same `airflow.sdk._shared.secrets_masker` module, so they share the same cached singleton. This matters because the masker is scoped per import path: `airflow._shared.secrets_masker` (used by the airflow-core `RenderedTaskInstanceFields` model) and `airflow.sdk._shared.secrets_masker` (used by task-sdk) are actually two separate singletons even within the same process, it says so right in that module's docstring. So we need to call `mask_secret()` from the task-sdk side, not the airflow-core side, or it just won't take effect. Concretely: `process_params()` in `task-sdk/src/airflow/sdk/definitions/param.py`, right after `params.validate()` (this is what feeds `context["params"]` in `execution_time/context.py`), for each key whose declared `Param` has `schema.format == "password"`. That runs in the worker before `_serialize_rendered_fields()` runs, so the registration is actually in place by the time it's needed. Worth flagging while I'm in here: this only gets us Task Logs and Rendered Templates. If someone XComs `dag_run.conf['api_token']`, it'd still show up in plaintext in the XCom UI, since neither `models/xcom.py` nor the XCom API routes run anything through `redact()`. Not proposing to fix that in this PR, it's the same shape of gap as `conf` in the DAG Run API and would need its own redaction pass, just wanted to be upfront about the boundary of what this actually covers. **2. Storage at rest** Agreed, I'll leave Fernet encryption out of this PR and just note it as a follow-up discussion in the PR description rather than deciding it myself. **3. Nested values** Agreed, limiting `format="password"` to string-typed params sidesteps the whole object-secret question for v1. Updated plan for the PR: - `mask_secret()` call in `process_params()` for string params with `format == "password"`, covers Task Logs and Rendered Templates. - Redaction helper for `DAGRunResponse` / `DAGRunCollectionResponse.conf` at the API layer, covers the DAG Run Details page and the REST API responses, since those aren't part of the SecretsMasker's redact-on-serialize path. - Docs for `format="password"` in `core-concepts/params.rst`, spelling out what's covered (logs, rendered templates, DAG Run Details/API) and what isn't (XCom display, DB encryption at rest), so nobody assumes broader coverage than what's actually there. Starting on this, will open a PR once tests are passing locally. -- 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]
