Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
potiuk closed pull request #67503: Warn when supervisor-side mask_secret IPC send fails URL: https://github.com/apache/airflow/pull/67503 -- 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]
Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
potiuk commented on PR #67503: URL: https://github.com/apache/airflow/pull/67503#issuecomment-4857648536 No further response on the review, and since @ashb isn't worried about the underlying scenario, this is at most defense-in-depth — and the warning would itself log through the unmasked path in exactly the case it fires — so we can drop it. Closing. --- Drafted-by: Claude Code (Opus 4.8); 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
potiuk commented on PR #67503: URL: https://github.com/apache/airflow/pull/67503#issuecomment-4763503949 Possibly still worth merging it now @ashb ? -- 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]
Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
potiuk commented on PR #67503: URL: https://github.com/apache/airflow/pull/67503#issuecomment-4700343874 Thanks @ashb — good catch on the leak risk especially. Addressed all three and rebased onto main: 1. **The leak:** you're right. A `MaskSecret` build failure can raise an error whose text reprs the value, and on this path the local `mask_logs` processor is already off — so `exc_info=True` could have guaranteed the very leak the warning reports on. It now records only `error_type=type(e).__name__` — no `exc_info`, no exception message, no value. The new test pins this down: the unserializable value's `repr` embeds a canary string and the test asserts that canary never appears in the emitted warning. 2. **"context" nit:** reworded to "No supervisor to notify (not running under a supervisor process)" so it doesn't clash with the SDK's `context`. 3. **On whether `MaskSecret` can realistically fail to serialize** — you may well be right that it shouldn't in normal use (callers pass `JsonValue`). I've kept a forced-failure test only as defense-in-depth: if a future caller ever does pass a bad value, the warning must still not leak it. The tests now exist mainly to prove that no-leak property, with the broken-socket case as the other defensible path. Since the warning is now provably leak-safe and only fires on a genuine registration failure, hopefully this addresses the concern — happy to drop it entirely if you'd still rather not carry it. -- 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]
Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
ashb commented on code in PR #67503:
URL: https://github.com/apache/airflow/pull/67503#discussion_r3373321935
##
task-sdk/src/airflow/sdk/log.py:
##
@@ -271,19 +271,37 @@ def mask_secret(secret: JsonValue, name: str | None =
None) -> None:
they're masked in both the task subprocess AND supervisor's log output.
Works safely in both sync and async contexts.
"""
-from contextlib import suppress
-
from airflow.sdk._shared.secrets_masker import _secrets_masker
_secrets_masker().add_mask(secret, name)
-with suppress(Exception):
-# Try to tell supervisor (only if in task execution context)
-from airflow.sdk.execution_time import task_runner
-from airflow.sdk.execution_time.comms import MaskSecret
+# Mirror the mask to the supervisor so it also masks the value in any logs
+# forwarded through it. When this fails we *must* emit a warning rather
+# than silently swallowing the failure: when ``sending_to_supervisor=True``
+# the local task process skips its own ``mask_logs`` processor (it relies
+# on the supervisor doing the masking instead), so a silent IPC failure
+# would leave the secret unmasked in supervisor-level logs.
+from airflow.sdk.execution_time import task_runner
+
+comms = getattr(task_runner, "SUPERVISOR_COMMS", None)
+if comms is None:
+# Not in a task-execution context — there is no supervisor to notify.
Review Comment:
Nit: don't use the word "context" here, as we already have a context meaning
in the SDK that this is dangerously close to overlapping with the thing Kaxil
mentioned which was client or in process die etc.)
cc @kaxil
##
task-sdk/src/airflow/sdk/log.py:
##
@@ -249,19 +249,37 @@ def mask_secret(secret: JsonValue, name: str | None =
None) -> None:
they're masked in both the task subprocess AND supervisor's log output.
Works safely in both sync and async contexts.
"""
-from contextlib import suppress
-
from airflow.sdk._shared.secrets_masker import _secrets_masker
_secrets_masker().add_mask(secret, name)
-with suppress(Exception):
-# Try to tell supervisor (only if in task execution context)
-from airflow.sdk.execution_time import task_runner
-from airflow.sdk.execution_time.comms import MaskSecret
+# Mirror the mask to the supervisor so it also masks the value in any logs
+# forwarded through it. When this fails we *must* emit a warning rather
+# than silently swallowing the failure: when ``sending_to_supervisor=True``
+# the local task process skips its own ``mask_logs`` processor (it relies
+# on the supervisor doing the masking instead), so a silent IPC failure
+# would leave the secret unmasked in supervisor-level logs.
+from airflow.sdk.execution_time import task_runner
+
+comms = getattr(task_runner, "SUPERVISOR_COMMS", None)
+if comms is None:
+# Not in a task-execution context — there is no supervisor to notify.
+return
+
+from airflow.sdk.execution_time.comms import MaskSecret
-if comms := getattr(task_runner, "SUPERVISOR_COMMS", None):
-comms.send(MaskSecret(value=secret, name=name))
+try:
+comms.send(MaskSecret(value=secret, name=name))
+except Exception:
+# Dedicated logger so operators can grep for this signal — without a
+# warning they'd have no way to find out the supervisor never received
+# the registration.
+structlog.get_logger("airflow.logging.mask_secret").warning(
+"supervisor_mask_secret_failed",
+secret_name=name,
+note="Could not register secret with supervisor; secret may not be
masked in supervisor-level logs.",
+exc_info=True,
Review Comment:
An interesting thought. Is there any chance that this exception being
included essentially _guarantees_ the sensitive data does get logged? Can you
force this case to happen and show what gets recorded to the task logs please?
--
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]
Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
potiuk commented on PR #67503: URL: https://github.com/apache/airflow/pull/67503#issuecomment-4683271250 @ashb — gentle nudge on this one. It's now rebased on `main` (the `test_log.py` conflict from the #66571 revert is resolved) and green. The open question from my last comment is still the decision point: `mask_secret` is public API and routinely receives `Any` values, and `add_mask` deliberately masks non-`str`/`dict` iterables (`set`/`tuple`/`frozenset`/generators) element-wise — but `MaskSecret.value: JsonValue` rejects exactly those at construction, so today they're masked locally yet silently never registered with the supervisor. I'd rather not gate a redaction control on mypy, since users aren't required to run it. Two ways forward, your call: 1. **Warning-only** (this PR as-is) — surface the silent failure. 2. **Coerce + warn** — `list()` the `set`/`tuple`/`frozenset` before building the message so they actually propagate and get masked supervisor-side, leaving the warning as a genuine last-resort guard. I think this is the stronger fix and I'm happy to push it. Which would you prefer? --- Drafted-by: Claude Code (Opus 4.8); 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
potiuk commented on PR #67503: URL: https://github.com/apache/airflow/pull/67503#issuecomment-4633488155 To lay out where this stands and a way to make it stronger. **Current approach.** The PR replaces the old `with suppress(Exception)` around the supervisor IPC with a `try/except` that logs a dedicated `supervisor_mask_secret_failed` warning. The motivation is that this is a *security* path, and the failure is silent today: `add_mask` is deliberately built to mask non-`str`/`dict` iterables element-wise (its `elif isinstance(secret, Iterable)` branch — `set`/`tuple`/`frozenset`/generators), but `MaskSecret.value: JsonValue` rejects exactly those at construction. So `mask_secret()` masks locally yet never registers with the supervisor, and under `sending_to_supervisor=True` (where the task drops its own `mask_logs` processor) the secret surfaces unmasked in supervisor-side logs. A small test pins this down: same input fails on `main` (warning swallowed) and passes here (warning fires), with local masking and `comms.send`-not-reached both holding — i.e. a real, supervisor-alive silent leak. **On the "the types already prevent this" point.** You're right that `JsonValue` + mypy reject a literal `set`/`tuple`, and I agree a statically-`JsonValue` value can't fail this. But I'm not comfortable basing a security control on mypy at all: there's no guarantee or expectation that our users run mypy — it isn't part of the contract for Dag/provider authors, and until the mypy plugin we're currently preparing actually ships, running mypy against Airflow code is practically discouraged. On top of that, `mask_secret` is public API and the values that actually reach it are routinely `Any` (anything pulled out of a bare `dict`, a secrets backend, etc.), which mypy waves straight through even if the author does run it. For redaction, "the annotation says it can't happen" isn't the same as "it can't happen", and the cost of being wrong is a leaked secret — so I'd rather have a runtime signal than rely on the annotation. **Proposal to make it stronger.** Rather than just *reporting* the gap, we can mostly *close* it by coercing the non-JSON iterables to a `list` before building the message, so they actually propagate and get masked supervisor-side: ```python ipc_value = list(secret) if isinstance(secret, (set, frozenset, tuple)) else secret comms.send(MaskSecret(value=ipc_value, name=name)) ``` That makes the two paths agree for the common case (a `set`/`tuple`/`frozenset` of secrets) instead of papering over the difference. The warning then drops to a genuine last-resort guard for the residue coercion can't save — an exhausted generator (already consumed by the earlier `add_mask`) or a truly non-serializable object — which is a much more defensible role for it. I'd also document the `JsonValue` constraint on `MaskSecret` so the boundary isn't implicit. Happy to push the coercion + a test asserting the supervisor now receives the masked list, if you're on board — or keep it warning-only if you'd prefer the smaller change. Which do you want? --- Drafted-by: Claude Code (Opus 4.8); 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
ashb commented on PR #67503: URL: https://github.com/apache/airflow/pull/67503#issuecomment-4630009615 > the MaskSecret message can fail to build/serialize before the socket is touched No, I don't think it can. -- 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]
Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
potiuk commented on PR #67503: URL: https://github.com/apache/airflow/pull/67503#issuecomment-4627244474 Fair point that in a full supervisor crash the warning can't be delivered — stdout/stderr are the supervisor's sockets, as you say. But `comms.send()` raising isn't *only* the crash case: the `MaskSecret` message can fail to build/serialize **before** the socket is touched. `MaskSecret.value` is `JsonValue`, and `send()` runs `_make_frame(msg).as_bytes()` (pydantic `model_dump` + msgspec encode, plus OTel trace injection) before `socket.sendall`. A non-serializable value (or an encode hiccup) raises while the supervisor is alive and reachable — so the warning is both deliverable and the only signal that registration was lost. And that's the case that actually bites: under `sending_to_supervisor=True` the task drops its own `mask_logs` processor (`log.py`), so a silently-swallowed failure leaves the secret unmasked in supervisor-level logs. The old `with suppress(Exception)` hid exactly that. You're right that the original test (a generic `RuntimeError` mock) read like the unrealistic crash case — I've reworked it: it now feeds a non-serializable value so the `MaskSecret` build genuinely fails and asserts `comms.send` is never reached (a real, supervisor-alive failure), plus a separate `BrokenPipeError` case for the socket branch. Mind taking another look? --- Drafted-by: Claude Code (Opus 4.8); 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
ashb commented on code in PR #67503:
URL: https://github.com/apache/airflow/pull/67503#discussion_r3353423112
##
task-sdk/tests/task_sdk/test_log.py:
##
@@ -118,3 +118,59 @@ def test_silent_on_success(self, tmp_path):
assert captured == []
handler.upload.assert_called_once_with(relative.as_posix(), ti)
+
+
+class TestMaskSecretSupervisorIPC:
+"""When ``mask_secret`` cannot register a secret with the supervisor it
must surface a warning.
+
+The local task drops its own ``mask_logs`` processor when forwarding logs
to the supervisor
+(see ``sending_to_supervisor=True`` branches in ``log.py``); a silent IPC
failure would leave
+the secret unmasked in supervisor-level logs.
+"""
+
+def test_warns_when_supervisor_send_fails(self):
+from airflow.sdk.execution_time import task_runner
+
+comms = mock.MagicMock()
+comms.send.side_effect = RuntimeError("supervisor IPC down")
Review Comment:
This is not a realistic case. The only way this can happen is if the entire
parent process has crashed, at which point there is no whether to send logs to
either, because stdout and stderr of the task process are connected to sockets
that the supervisor is reading from
--
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]
Re: [PR] Warn when supervisor-side mask_secret IPC send fails [airflow]
potiuk commented on PR #67503: URL: https://github.com/apache/airflow/pull/67503#issuecomment-4617452115 Gentle nudge — this and a few related task-SDK / execution-API hardening PRs are green and have been waiting on review (9 days): #67503, #67504, #67505, #67506, #67628. They're small, self-contained fixes. @ashb @kaxil @amoghrajesh — would appreciate a look when you have a moment. --- Drafted-by: Claude Code (Opus 4.8); 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
