Skip to content

Refuse the team agnostic fall-through for a team scoped Key Vault secret name - #70876

Merged
potiuk merged 2 commits into
apache:mainfrom
potiuk:scope-azure-key-vault-lookups-to-their-team
Aug 1, 2026
Merged

Refuse the team agnostic fall-through for a team scoped Key Vault secret name#70876
potiuk merged 2 commits into
apache:mainfrom
potiuk:scope-azure-key-vault-lookups-to-their-team

Conversation

@potiuk

@potiuk potiuk commented Jul 31, 2026

Copy link
Copy Markdown
Member

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:

return team_name is None and bool(re.fullmatch(..., secret_id))

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 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

  1. the team scoped lookup runs first — safe by construction, since it can only
    ever build the caller's own namespace;
  2. after it misses, an id that spells out any team namespace is refused rather
    than resolved through the team agnostic name;
  3. otherwise the team agnostic lookup proceeds as before.

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 a matches a--b's namespace on the prefix and would read
its 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

  • New tests: a caller scoped to one team cannot reach another team's secret by
    naming it, and a team whose name extends the caller's (teama--prod vs
    teama) is not reachable either
  • Both new tests fail against the unmodified backend — the target secret is
    resolvable in the fixture, so a backend that falls through returns its value
  • Existing team-scope tests unchanged and passing
  • ruff check / ruff format clean
Was generative AI tooling used to co-author this PR?
  • Yes — Claude Opus 5 (1M context)

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

…ret name

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
Refusing only the team agnostic fall-through left the team scoped probe
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 probe runs first, so the fall-through is never reached. Because the
backend maps underscores onto the separator, 'b__c' reaches the vault the
same way without visibly containing it.

Deciding this at the entry points instead means the ambiguity is settled
before a name is built, and the refusal is logged so the resulting None is
not read as a missing secret.
@potiuk
potiuk merged commit 41a3be7 into apache:main Aug 1, 2026
83 checks passed
@potiuk
potiuk deleted the scope-azure-key-vault-lookups-to-their-team branch August 1, 2026 09:56
potiuk added a commit to potiuk/airflow that referenced this pull request Aug 1, 2026
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.
potiuk added a commit to potiuk/airflow that referenced this pull request Aug 1, 2026
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.
Miretpl pushed a commit that referenced this pull request Aug 1, 2026
…#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants