kaxil commented on code in PR #66575:
URL: https://github.com/apache/airflow/pull/66575#discussion_r3262697507
##########
task-sdk/tests/task_sdk/execution_time/test_secrets.py:
##########
@@ -120,6 +120,70 @@ def test_get_conn_value_not_implemented(self):
with pytest.raises(NotImplementedError, match="Use get_connection
instead"):
backend.get_conn_value("test_conn")
+ def test_get_connection_raises_on_permission_denied(self,
mock_supervisor_comms):
+ """An explicit deny from the Execution API must raise, not fall
through.
+
+ Returning None on a 401/403 would let the secrets-backend dispatcher
+ fall through to a less-restrictive backend (e.g.
EnvironmentVariablesBackend).
+ """
+ from airflow.sdk.exceptions import ErrorType
+ from airflow.sdk.execution_time.comms import ErrorResponse
Review Comment:
AI is clearly reimporting it on every test. Let's not do that.
##########
task-sdk/src/airflow/sdk/execution_time/secrets/execution_api.py:
##########
@@ -43,6 +43,27 @@ def get_conn_value(self, conn_id: str, team_name: str | None
= None) -> str | No
"""
raise NotImplementedError("Use get_connection instead")
+ def _raise_if_authz_denied(self, msg, *, resource: str, key: str) -> None:
+ """
+ Raise PermissionError on an explicit deny response from the Execution
API.
+
+ Returning None on a 401/403 would let the secrets-backend dispatcher
+ fall through to a less-restrictive backend (e.g.
EnvironmentVariablesBackend
+ which performs no authorization checks). The Execution API explicitly
+ denied this request — we must not silently route around that decision.
+ Other ErrorResponse types (NOT_FOUND, transient API_SERVER_ERROR,
+ GENERIC_ERROR) keep the existing fallthrough behaviour so the
+ not-found-here path remains usable.
+ """
+ from airflow.sdk.exceptions import ErrorType
+ from airflow.sdk.execution_time.comms import ErrorResponse
+
+ if isinstance(msg, ErrorResponse) and msg.error ==
ErrorType.PERMISSION_DENIED:
Review Comment:
Raising `PermissionError` here doesn't actually close the gap because the
outer dispatcher loops in `airflow/sdk/execution_time/context.py` swallow it.
`_get_connection` (lines 164-176), `_async_get_connection` (lines 203-224), and
`_get_variable` (lines 252-269) all do `except Exception:` and continue to the
next backend. `PermissionError` is a subclass of `Exception` (via `OSError`),
so it's caught and the dispatcher falls through silently. I verified this with
a small reproduction: with the patched backend raising `PermissionError`, a
follow-up `EnvironmentVariablesBackend.get_connection` in the same loop still
returns the secret. `models/connection.py:552-563` has the same pattern.
To make this actually deny, the dispatcher loops need to special-case
`PermissionError` (or an explicit `AirflowAuthError`-shaped subclass) and
re-raise instead of falling through.
##########
task-sdk/src/airflow/sdk/execution_time/secrets/execution_api.py:
##########
@@ -43,6 +43,27 @@ def get_conn_value(self, conn_id: str, team_name: str | None
= None) -> str | No
"""
raise NotImplementedError("Use get_connection instead")
+ def _raise_if_authz_denied(self, msg, *, resource: str, key: str) -> None:
Review Comment:
Worth tightening the threat model in the PR description: the default worker
search path defined in
`task-sdk/src/airflow/sdk/execution_time/secrets/__init__.py` is
`[EnvironmentVariablesBackend, ExecutionAPISecretsBackend]`, with
`ExecutionAPISecretsBackend` last. With a custom backend listed via `[secrets]
backend`, that custom backend is prepended (`configuration.py:254`), making the
order `[Custom, Env, ExecutionAPI]`. So in the default and common
configurations, there is no later backend for the dispatcher to fall through to
after `ExecutionAPISecretsBackend` denies; the fall-through-leak scenario
requires either a non-default reordering or a custom backend explicitly placed
after `ExecutionAPISecretsBackend`.
Not a reason to drop the fix, but the description currently overstates the
gap in the default config.
--
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]
