Refuse the team agnostic fall-through for a team scoped AWS secret name - #70878
Conversation
The team scoped lookup is tried first and then, on a miss, the lookup falls through to the team agnostic name. A guard was meant to stop a caller naming another team's secret, but its first condition is `team_name is None`, so it does nothing whenever a team scope is supplied -- exactly when it matters. A caller authorised for one team could therefore supply an id that spells out another team's namespace and resolve that team's secret. Refuse the fall-through instead. The team scoped lookup still runs first and is safe by construction, since it can only ever build the caller's own namespace. After it misses, an id that spells out any team namespace is refused rather than resolved through the team agnostic name. The id is never parsed to work out which team it names, because it cannot be: a team name may itself contain the separator, so nothing distinguishes one team's namespace from another whose name extends it. Comparing the id against the prefix the caller's own team builds looks equivalent and is not -- a caller in team `a` matches `a--b`'s namespace on the prefix and would read its secrets. Generated-by: Claude Opus 5 (1M context) following the guidelines at https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions
boring-cyborg
Bot
added
area:providers
area:secrets
provider:amazon
Refusing only the team agnostic fall-through left the team scoped lookup open to the same ambiguity it was meant to close: a caller in team 'a' asking for 'b--c' builds exactly the name team 'a--b' builds for 'c', and that lookup runs first, so the fall-through is never reached. Deciding this at the entry points instead means the ambiguity is settled before a name is built, in Secrets Manager and Parameter Store alike.
shahar1
left a comment
shahar1
left a comment
There was a problem hiding this comment.
Approach looks right and the tests are what convinced me; a few observations inline and below, none blocking — though two of them look in-scope for this PR rather than follow-ups.
The old guard's first condition was team_name is None, so it did nothing exactly when a team scope was supplied. Moving the refusal ahead of both lookups rather than only the team agnostic fall-through is the correct shape, and test_team_scoped_lookup_cannot_reach_a_longer_teams_namespace is the test that earns it: team my_team asking for prod--test_postgres builds exactly the name team my_team--prod builds for test_postgres, so the team scoped probe hits another team's secret before any fall-through is reached. Guarding only the fall-through would have left that open.
1. The same flaw is in core's EnvironmentVariablesBackend, and isn't fixed here
The guard quoted in the PR description —
return team_name is None and bool(re.fullmatch(..., secret_id))— is airflow-core/src/airflow/secrets/environment_variables.py:66, not the Amazon code this PR patches (the Amazon one has no bool(...) wrapper). That backend has the identical fall-through:
- team scoped name:
AIRFLOW_CONN_+_<TEAM>___+<CONN_ID> - team agnostic name:
AIRFLOW_CONN_<CONN_ID>
A caller scoped to teama requesting conn_id _teamb___myconn misses AIRFLOW_CONN__TEAMA____TEAMB___MYCONN, then falls through to AIRFLOW_CONN__TEAMB___MYCONN — team b's connection. The collision variant applies too: teama asking for prod___myconn builds the same variable name that team teama___prod builds for myconn.
Fixing the two Amazon backends while the code the description quotes stays broken leaves the sibling caller open. Fine as a separate PR, but it seems worth doing together, or at least tracking.
2. The provider docs still describe the old, conditional refusal
providers/amazon/docs/secrets-backends/aws-secrets-manager.rst:176providers/amazon/docs/secrets-backends/aws-ssm-parameter-store.rst:152
Both still carry:
Connection ids and variable keys matching
<team>--<name>are reserved for team-scoped lookups. A request without team context for a key matching this pattern will returnNoneto prevent cross-team access.
After this PR the refusal is unconditional in every scope, including the owning team — which is exactly the behaviour change the description calls out. Worth updating in the same PR, ideally with a line telling a deployment that already has ids containing -- what to do about them.
3. The description's rationale contradicts its own tests
Approach bullet 1 says the team scoped lookup is "safe by construction, since it can only ever build the caller's own namespace", while the docstring of test_team_scoped_lookup_cannot_reach_a_longer_teams_namespace says "The team scoped name is not safe by construction -- the id can extend it." The code follows the test — the refusal correctly runs before both lookups — so it is the rationale that is stale, and it will land in the commit message as written.
Nit — the helper is duplicated verbatim
TEAM_SEP, the 15-line _names_a_team_namespace docstring and _log_refusal are now copy-pasted identically into both modules. The two backends already duplicate the _get_secret shape so this follows the local grain, but a shared helper would keep the rationale in one place — it is the kind of comment that drifts once only one copy gets edited.
Not requesting anyone, just flagging who has context on this code: justinpakzad authored the original guard (#65703) and its logging follow-up (#66059), and vincbeck has been across the Amazon secrets backends and multi-team.
This review was drafted by an AI-assisted tool and
confirmed by an Airflow maintainer. The findings
below are observations, not blockers; an Airflow
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.More on how Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
The check used to live in _get_secret, which all three getters share. Moving it up into get_conn_value and get_variable left get_config reaching _get_secret unguarded, so the helper's claim that such an id is refused for every lookup was no longer true. Not a cross-team read -- get_config never receives a team_name -- but the two getters and the docstring disagreed with the third. Guard get_config too, and correct the docstring to name the three getters. _names_a_team_namespace uses no instance state, so it becomes a staticmethod. The refusal-logging test asserted on the rendered sentence. Airflow logs through structlog, which renders format args into msg before the stdlib record exists, so record.args is empty and there is no structured payload to assert on; assert on level, logger and the refused id instead, which is what the test is actually about, and extend it to cover get_config.
|
Thanks @shahar1 — all four addressed, and the
On the duplicated docstring and One thing you should know: the same |
Follow-up to apache#70876. The check used to live in _get_secret, which all three getters share. Moving it up into get_conn_value and get_variable left get_config reaching _get_secret unguarded, so the helper's claim that such an id is refused for every lookup stopped being true. Not a cross-team read -- get_config never receives a team_name -- but two getters and the docstring disagreed with the third. The same gap was found in review of the Amazon sibling (apache#70878) and fixed there; this is the Azure half, which had already merged by then. _names_a_team_namespace stays an instance method here, unlike Amazon's: it builds the candidate path with self.build_path and self.sep. The refusal-logging assertion uses getMessage() rather than msg, since how a record carries its payload differs between the Airflow version this runs on and the ones the provider compat tests use.
The refusal-logging assertions in the Amazon secrets tests read record.msg directly. How a record carries its payload depends on the Airflow version: on main, structlog renders the format args into msg before the stdlib record exists, so args is empty and msg holds the final text; on the versions the provider compat tests run against, plain stdlib logging leaves msg as the format string with the values in args. Reading msg therefore passes on main and fails under Compat 2.11.1 and 3.0.6, which is where these currently break: assert 0 == 1 where 0 = sum(refused_id in r.msg for r in refusals) getMessage() renders in both shapes. The assertion still targets the refused id rather than the wording, so rephrasing the warning stays free. Introduced in #70878, where the assertions were moved off getMessage and verified only against main.
Follow-up to apache#70876. The check used to live in _get_secret, which all three getters share. Moving it up into get_conn_value and get_variable left get_config reaching _get_secret unguarded, so the helper's claim that such an id is refused for every lookup stopped being true. Not a cross-team read -- get_config never receives a team_name -- but two getters and the docstring disagreed with the third. The same gap was found in review of the Amazon sibling (apache#70878) and fixed there; this is the Azure half, which had already merged by then. _names_a_team_namespace stays an instance method here, unlike Amazon's: it builds the candidate path with self.build_path and self.sep. The refusal-logging assertion uses getMessage() rather than msg, since how a record carries its payload differs between the Airflow version this runs on and the ones the provider compat tests use.
…#70899) Follow-up to #70876. The check used to live in _get_secret, which all three getters share. Moving it up into get_conn_value and get_variable left get_config reaching _get_secret unguarded, so the helper's claim that such an id is refused for every lookup stopped being true. Not a cross-team read -- get_config never receives a team_name -- but two getters and the docstring disagreed with the third. The same gap was found in review of the Amazon sibling (#70878) and fixed there; this is the Azure half, which had already merged by then. _names_a_team_namespace stays an instance method here, unlike Amazon's: it builds the candidate path with self.build_path and self.sep. The refusal-logging assertion uses getMessage() rather than msg, since how a record carries its payload differs between the Airflow version this runs on and the ones the provider compat tests use.
2 participants
The team scoped lookup runs first and, on a miss, falls through to the team
agnostic name. A guard was meant to stop a caller naming another team's secret:
Its first condition is
team_name is None, so it does nothing whenever a teamscope is supplied — exactly when it matters. A caller authorised for one team
could supply an id spelling out another team's namespace; the team scoped probe
missed, and the team agnostic lookup resolved the other team's secret.
Approach
ever build the caller's own namespace;
than resolved through the team agnostic name;
The id is never parsed to work out which team it names, because it cannot be:
a team name may itself contain the separator, so nothing in the string
distinguishes one team's namespace from another whose name extends it. Comparing
the id against the prefix the caller's own team builds looks equivalent and is
not — a caller in team
amatchesa--b's namespace on the prefix and would readits secrets. Only the caller's own namespace is ever constructed, never parsed.
Behaviour change
An id that spells out a team scoped name is no longer resolvable through the team
agnostic name from any scope, including the team that owns it — that spelling is
what made a prefix match look like ownership. Callers reach their own team's
secrets with the bare id plus their team scope, which is unaffected, as are
single-team and non-team deployments.
Test plan
naming it, and a team whose name extends the caller's (
teama--prodvsteama) is not reachable eitherresolvable in the fixture, so a backend that falls through returns its value
ruff check/ruff formatcleanWas generative AI tooling used to co-author this PR?
Generated-by: Claude Opus 5 (1M context) following the guidelines at
https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions