Fix team name authorization bypass in edgeworker#64556
Fix team name authorization bypass in edgeworker#64556gopidesupavan wants to merge 2 commits intoapache:mainfrom
Conversation
boring-cyborg
bot
added
area:providers
provider:edge
|
@jscheffl found by codex security.. dont have much depth on edge worker.. probably please can you decide is it valid or not. at first instance for me it looks valid. |
|
from this commit: codex flagged. gopidesupavan@ee6d8dd |
gopidesupavan
force-pushed
the
fix-team_name-authorization-bypass-in-edgeworker
branch
from
8fc48f0 to
9daa2f0
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a multi-team authorization bypass in the Edge Worker “fetch job” API by no longer trusting the client-provided team_name and instead selecting jobs based on the persisted worker record.
Changes:
- Load the
EdgeWorkerModelfrom the DB injobs.fetch()and return 404 when the worker is unknown. - Filter queued jobs by
worker.team_name(when set) instead ofbody.team_name. - Update unit tests to create worker records and assert
body.team_nameis ignored for job selection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| providers/edge3/src/airflow/providers/edge3/worker_api/routes/jobs.py | Fetch now resolves the worker from DB, returns 404 if missing, and uses persisted worker.team_name for filtering. |
| providers/edge3/tests/unit/edge3/worker_api/routes/test_jobs.py | Tests now create EdgeWorkerModel rows and validate fetch behavior with persisted team settings. |
| if worker.team_name is not None: | ||
| query = query.where(EdgeJobModel.team_name == worker.team_name) |
There was a problem hiding this comment.
The team isolation now relies on worker.team_name, but that value is still writable by the worker via the worker registration/heartbeat endpoints (e.g. worker_api/routes/worker.py assigns worker.team_name = body.team_name). A client holding the shared JWT secret can therefore change its persisted team and fetch jobs for another team, so the authorization bypass is not fully addressed.
Consider making team_name server-controlled/immutable (e.g. reject changes when a worker already exists, or pre-provision workers with a fixed team and disallow team_name in the request body), and add validation tests for this behavior.
| worker = session.scalar(select(EdgeWorkerModel).where(EdgeWorkerModel.worker_name == worker_name)) | ||
| if not worker: | ||
| raise HTTPException(status.HTTP_404_NOT_FOUND, "Worker not found") | ||
|
|
There was a problem hiding this comment.
fetch() now raises an HTTP 404 when the worker is unknown, but the route’s documented responses= only includes 400/403. Please update the OpenAPI exception docs for this endpoint to include 404 as well, so clients can see this behavior in the API schema.
| worker = session.scalar(select(EdgeWorkerModel).where(EdgeWorkerModel.worker_name == worker_name)) | ||
| if not worker: | ||
| raise HTTPException(status.HTTP_404_NOT_FOUND, "Worker not found") | ||
|
|
There was a problem hiding this comment.
New behavior: fetch() raises HTTPException(404) when the worker name is not found. The unit tests currently only cover the happy path; please add a regression test asserting the 404 for an unknown worker.
| @@ -136,6 +138,11 @@ def test_state(self, mock_stats_incr, session: Session): | |||
|
|
|||
| def test_fetch_filters_by_team_name(self, session: Session): | |||
There was a problem hiding this comment.
The test name test_fetch_filters_by_team_name is now a bit misleading since the filter is based on the persisted worker’s team (and explicitly ignores body.team_name). Renaming it to something like test_fetch_filters_by_worker_team_name would make the intent clearer and reduce confusion when reading failures.
| def test_fetch_filters_by_team_name(self, session: Session): | |
| def test_fetch_filters_by_worker_team_name(self, session: Session): |
|
Thanks for the fix, currently a bit "under water" and yes, this is a limitation we know and had documented: https://github.com/boschglobal/airflow/blob/main/providers/edge3/docs/edge_executor.rst?plain=1#L112 Assuming that one administrator has access to both sides anyway it is not a multi tenacy - at least for the moment. |
jscheffl
left a comment
jscheffl
left a comment
There was a problem hiding this comment.
I think it is not harming to merge the fix but as also CoPilot suggeststs all model is based on shared secret key. Everybody getting hold of the shared secret key can imitate any worker. So this hardening in the PR is only helping minimal.
why:
The fetch API trusted client-supplied team_name, so any worker with the shared token could impersonate another team and fetch cross-team jobs. This broke multi-team isolation and created an authorization bypass.
what:
The fetch route now resolves the worker from the database, returns 404 for unknown workers, and uses persisted worker.team_name for job selection instead of body.team_name.
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.