Require existing-connection read access when testing an existing connection#67620
Open
potiuk wants to merge 5 commits into
Open
Require existing-connection read access when testing an existing connection#67620potiuk wants to merge 5 commits into
potiuk wants to merge 5 commits into
Conversation
…ection The `POST /api/v2/connections/test` route was authorizing the caller only as a connection POST operation (i.e. "can create a connection"). When the request body referenced an existing `connection_id`, the route then loaded that connection from the configured secrets backend and merged its hidden fields (`login`, `password`, parts of `extra`) into the test object. The route did not check whether the caller was authorized to read that existing connection — so a caller authorized to create connections but not to read a given connection could effectively borrow that connection's secrets. This change adds a `GET` authorization check on the existing connection before its secrets are merged into the test object. A caller authorized to create connections but not to read the existing `connection_id` now gets a 403. Reference: airflow-s/airflow-s#444 Generated-by: Claude Opus 4.7 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
potiuk
requested review from
bugraoz93,
choo121600,
ephraimbuddy,
jason810496,
pierrejeambrun,
rawwar and
shubhamraj-git
as code owners
potiuk
added
the
backport-to-v3-2-test
Codex's adversarial review of the previous commit on this branch pointed out that returning 403 only when the existing connection is found AND unreadable creates an existence oracle: callers with route-level POST permission but no read permission could distinguish a protected connection_id (which returns 403) from a non-existent id (which falls through to the body-only test path).
This commit removes the oracle by collapsing both cases — "connection not found" and "connection found but caller lacks read access" — into the same body-only test path. The hidden-field borrow is gated on `(existing_conn is not None and auth_manager.is_authorized_connection("GET", ...))` and the response shape is identical for any unreadable connection_id, regardless of whether it exists in the secrets backend.
The regression test is updated to assert that the response to an unreadable existing connection_id is indistinguishable (status code + response body keys) from the response to a non-existent connection_id, under the same submitted body fields.
Generated-by: Claude Opus 4.7 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
A second Codex adversarial-review pass on this branch pointed out that the previous follow-up commit still performed the secrets-backend lookup (`Connection.get_connection_from_secrets`) before the GET authorization check. Although the response shape was normalized so the caller could not distinguish unreadable-existing from missing, the lookup itself still ran for arbitrary connection ids — meaning an unauthorized caller could still: trigger backend queries against every configured secrets backend, generate access-log entries in audited backends, and impose backend load by submitting arbitrary ids. This commit moves the GET authorization boundary *ahead of* the secrets-backend lookup. Team scope is resolved via `Connection.get_team_name`, which is a metadata-only DB lookup against the `connection` table and does not touch any secrets backend. Only when the caller is authorized to read the requested `connection_id` does the route call `Connection.get_connection_from_secrets`. Unauthorized callers fall through to the body-only test path without any secrets-backend interaction. A new regression test spies on `Connection.get_connection_from_secrets` with `wraps=` and asserts it is never called when the caller lacks GET access — locking in the gate at the test level. Generated-by: Claude Opus 4.7 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
A third Codex adversarial-review pass on this branch flagged that the route resolves and authorizes against `team_name` from `Connection.get_team_name`, but then calls `Connection.get_connection_from_secrets(connection_id)` without forwarding that team. `Connection.get_connection_from_secrets` accepts a `team_name` kwarg and forwards it to team-aware secrets backends (Vault, Akeyless, …); when omitted, those backends fall back to global secrets. In multi-team deployments this could either silently fail to merge hidden fields for an existing team-owned connection, or — worse — merge a global secret with the same `conn_id` after the route had only authorized access to the team-owned scope. Both are cross-scope secret confusion paths. This commit threads `team_name` through to the secrets lookup, keeping the read scope consistent end-to-end. A new regression test enables `[core] multi_team=true`, creates a team-owned `TEST_CONN_ID`, and asserts the spy on `get_connection_from_secrets` is called with the correct `team_name` kwarg — locking in the team-scope propagation at the test level. Generated-by: Claude Opus 4.7 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
…ons/test A fourth Codex adversarial-review pass on this branch flagged that for connections that live *only* in a team-aware secrets backend (Vault, Kubernetes, Akeyless, …) and have no metadata-DB row, `Connection.get_team_name(connection_id)` returns None. The route was then authorizing GET access and calling `get_connection_from_secrets(..., team_name=None)`, dropping the caller's already-validated body `team_name`. Team-aware backends only consult team-scoped paths when `team_name is not None` and otherwise fall back to global lookups — so a team-scoped existing connection in a secrets backend would be authorized and looked up as global, defeating the multi-team isolation in deployments that don't keep connections in the DB. This commit falls back to `test_body.team_name` whenever the DB metadata lookup returns None. The body's `team_name` is already validated by `ConnectionBody.validate_team_name` (which rejects a non-None value unless `[core] multi_team` is enabled), so a non-None body value here is always already gated by that validator. A new regression test enables `[core] multi_team=true`, posts to `/connections/test` with a `team_name` body field, does *not* create a metadata-DB row, and asserts the secrets-backend spy is called with the body's `team_name`. This locks in the body-fallback path at the test level so future refactors can't silently drop team scope for secrets-only connections. Generated-by: Claude Opus 4.7 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
POST /api/v2/connections/testroute was authorizing the caller only as a connection POST operation (i.e. "can create a connection"). When the request body referenced an existingconnection_id, the route then loaded that connection from the configured secrets backend and merged its hidden fields (login,password, parts ofextra) into the test object. The route did not check whether the caller was authorized to read that existing connection — so a caller authorized to create connections but not to read a given connection could effectively borrow that connection's secrets.This change adds a
GETauthorization check on the existing connection before its secrets are merged into the test object. A caller authorized to create connections but not to read the existingconnection_idnow gets a 403.Reference: airflow-s/airflow-s#444
Test plan
can_create-but-not-can_getcase (returns 403).test_connections.py::TestConnectionstill pass.prek run --from-ref main --stage pre-commitclean on touched files.prek run --from-ref main --stage manualclean on touched files.Was generative AI tooling used to co-author this PR?
Generated-by: Claude Opus 4.7 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions