potiuk commented on code in PR #66575:
URL: https://github.com/apache/airflow/pull/66575#discussion_r3264957811
##########
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:
Good catch — confirmed and fixed. Introduced
`AirflowSecretsBackendAccessDenied(PermissionError)` in
`airflow.sdk.exceptions` and threaded `except
AirflowSecretsBackendAccessDenied: raise` ahead of the catch-all `except
Exception:` in:
- `task-sdk/src/airflow/sdk/execution_time/context.py` — `_get_connection`,
`_async_get_connection`, `_get_variable`
- `airflow-core/src/airflow/models/connection.py` —
`get_connection_from_secrets`
- `airflow-core/src/airflow/models/variable.py` — `get_variable_from_secrets`
Went with a distinct subclass (rather than special-casing the bare
`PermissionError`) so an incidental `OSError`-family `PermissionError` from
inside an unrelated backend isn't mis-treated as an authoritative authz deny.
Inheriting from `PermissionError` keeps any existing `except PermissionError:`
caller working.
Also added `TestDispatcherRefusesFallbackOnDeny` that pins the end-to-end
behaviour — see reply to the test-coverage comment below.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
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:
Agreed — PR body updated. The default chain `[Env, ExecutionAPI]` has
nothing after `ExecutionAPISecretsBackend` for the dispatcher to fall through
to; the fall-through-leak scenario requires a non-default reordering or a
custom backend explicitly placed after `ExecutionAPISecretsBackend`. Fix is
still warranted because (a) some users do reorder, and (b) silently downgrading
an authoritative deny to "next backend, please" is wrong on principle.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
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):
Review Comment:
Added `TestDispatcherRefusesFallbackOnDeny` with three end-to-end tests
covering `_get_connection`, `_get_variable`, and `_async_get_connection`. Each
one inserts a spy backend AFTER `ExecutionAPISecretsBackend` in the chain and
asserts both:
1. `AirflowSecretsBackendAccessDenied` reaches the caller.
2. The downstream spy backend is **never** called — `assert_not_called()`.
These would have failed on the previous revision precisely because the outer
`except Exception:` swallowed the deny.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
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:
Hoisted `ErrorType` / `ErrorResponse` / `ConnectionResult` /
`VariableResult` to the module top — applied to the older tests in the file as
well.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
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:
Done — all hoisted to module top.
---
Drafted-by: Claude Code (Opus 4.7); 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]
