[v3-3-test] Require existing-connection read access when testing an existing connection - #71428
Merged
Merged
Conversation
…ection (apache#67620) * Require existing-connection read access when testing an existing connection 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 * Treat unreadable existing connection as missing in /connections/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 * Gate /connections/test secrets lookup on read authorization 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 * Preserve team_name through /connections/test secrets lookup 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 * Fall back to body team_name for secrets-only connections in /connections/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 (cherry picked from commit 779efc6)
pierrejeambrun
requested review from
bugraoz93,
choo121600,
ephraimbuddy,
henry3260,
jason810496,
rawwar and
shubhamraj-git
as code owners
August 11, 2026 11:48
Contributor
|
Hi maintainer, this PR was merged without a milestone set.
|
vatsrahul1001
pushed a commit
that referenced
this pull request
Aug 13, 2026
…ection (#67620) (#71428) * Require existing-connection read access when testing an existing connection 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 * Treat unreadable existing connection as missing in /connections/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 * Gate /connections/test secrets lookup on read authorization 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 * Preserve team_name through /connections/test secrets lookup 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 * Fall back to body team_name for secrets-only connections in /connections/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 (cherry picked from commit 779efc6) Co-authored-by: Jarek Potiuk <jarek@potiuk.com>
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
2 participants
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.
Backport of #67620 to
v3-3-test.Cherry-picked from 779efc6.
#67620 merged to
mainon 2026-06-25, about a day afterv3-3-testwas branched (2026-06-24), and it never carried abackport-to-v3-3-testlabel — so despite its 3.3.0 milestone it missed the branch and shipped absent from 3.3.0. This restores the access-control fix on the release branch:POST /connections/testnow authorizes GET (read) access on an existingconnection_idbefore its secrets are merged into the test object, gating the secrets-backend lookup itself (so an unauthorized caller can't enumerate ids, generate audit-backend access-log entries, or impose backend load).Conflict resolution.
connections.pyconflicted with #70010 (the already-backported "skip stored credentials when a connection test overrides host or port", #69957), which touches the same block. Resolved so the endpoint keeps both protections: the read-access gate runs first (around loading the existing connection), and the credential-skip guard runs after, insideif existing_conn is not None:. The resolvedtest_connectionis byte-for-byte identical tomain's (which carries both fixes).test_connections.pymerged cleanly.All
TestConnectiontests pass locally onv3-3-test, covering both fixes (the read-access / secrets-lookup gate and the target-override credential guard).Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 4.8) following the guidelines