Skip to content

Apply the team scope in the Google Secret Manager backend - #70869

Merged
potiuk merged 9 commits into
apache:mainfrom
potiuk:scope-google-secret-manager-lookups-to-their-team
Aug 1, 2026
Merged

Apply the team scope in the Google Secret Manager backend#70869
potiuk merged 9 commits into
apache:mainfrom
potiuk:scope-google-secret-manager-lookups-to-their-team

Conversation

@potiuk

@potiuk potiuk commented Jul 31, 2026

Copy link
Copy Markdown
Member

CloudSecretManagerBackend.get_conn_value and get_variable both accept a
team_name, but the helper they delegate to has no such parameter:

def get_conn_value(self, conn_id: str, team_name: str | None = None) -> str | None:
    ...
    return self._get_secret(self.connections_prefix, conn_id)   # team_name dropped

def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:

The argument was discarded at the call boundary, so the backend built the team
agnostic secret name in every case and a team scoped lookup resolved whatever
that name pointed at. No team scoping was applied at all.

Approach

The team scope is threaded through and the team scoped name is resolved first,
using the same <team><TEAM_SEP><secret id> convention the other secrets
backends use (TEAM_SEP = "--", matching the Azure Key Vault backend).

  1. team scoped lookup first — safe by construction, since it can only ever
    build the caller's own namespace;
  2. after it misses, an id that already spells out a team scoped name is
    refused, because the team agnostic name would otherwise land inside some
    team's namespace;
  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 team a with id b--c from team a--b with id c. 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.

get_config is unchanged in behaviour for ordinary configuration keys; it shares
_get_secret, so a config id that spells out a team scoped name is now refused
too, which matches how the Azure backend already behaves.

Behaviour change

  • A team scoped lookup now resolves the team scoped secret name, where before it
    silently resolved the team agnostic one. Deployments that ran multi-team mode
    against this backend and relied on the old behaviour must store secrets under
    the team scoped name.
  • An id that spells out a team scoped name is no longer resolvable through the
    team agnostic name from any scope.
  • Single-team and non-team deployments are unaffected.

Test plan

  • providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py
    36 pass (30 existing, 6 new)
  • New TestCloudSecretManagerBackendTeamScope: resolved for its own team;
    not resolved for another team; not resolved without a team scope; team
    agnostic secrets still resolve for any scope; Variables scoped identically
  • test_team_whose_name_extends_the_callers_is_not_readable — a team named
    team_a--prod is not readable by a caller in team_a, and still reaches its
    own secret with the bare id
  • 5 of the 6 new tests fail against the unmodified backend; the sixth is a
    no-regression guard for team agnostic lookups
  • 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

`get_conn_value` and `get_variable` both accept a `team_name`, but the helper
they delegate to, `_get_secret`, has no such parameter, so the argument was
dropped at the call boundary. The backend built the team agnostic secret name in
every case, and a team scoped lookup resolved whatever the team agnostic name
pointed at.

Thread the team through and resolve the team scoped name first, using the same
`<team><TEAM_SEP><secret id>` convention the other backends use. The team scoped
name is safe by construction: it can only ever build the caller's own namespace.

After that lookup misses, an id that already spells out a team scoped name is
refused rather than resolved, because the team agnostic name would otherwise
land inside some team's namespace.

The id is never parsed to work out which team it names. A team name may itself
contain the separator, so nothing distinguishes team `a` with id `b--c` from
team `a--b` with id `c`. Comparing the id against the prefix the caller's own
team builds looks equivalent and is not -- a caller in team `a` would match
`a--b`'s namespace on the prefix and 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
@potiuk
potiuk requested a review from shahar1 as a code owner July 31, 2026 19:22
@boring-cyborg boring-cyborg Bot added area:providers area:secrets provider:google Google (including GCP) related issues labels Jul 31, 2026

@bugraoz93 bugraoz93 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Non blocker comment to be referenced in the future

@shahar1 shahar1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for picking this up — the underlying bug is real and worth fixing: get_conn_value / get_variable accepted team_name and dropped it at the _get_secret boundary, so this backend applied no team scoping at all. The lookup-order design is also the right shape: try the team-scoped name first, refuse the team-agnostic fallback for an id that spells out a namespace, and never parse the id to infer a team. The reasoning in the description about why a prefix match is not proof of ownership is correct.

The problem is in the naming half. It was transplanted from the Azure Key Vault backend, which overrides build_path, into the Google backend, which does not — and that one difference reopens the cross-namespace read this PR sets out to close.

The cross-team read is still reachable

_build_team_secret_name normalises _sep in the secret id, which can manufacture TEAM_SEP out of a plain conn_id. Running the PR's two helpers verbatim against the real BaseSecretsBackend.build_path:

victim   (team 'team_a--prod', id 'conn')      -> test-connections-team_a--prod--conn
attacker (team 'team_a',       id 'prod__conn') -> test-connections-team_a--prod--conn
cross-team read reachable: True
guard refuses 'prod__conn': False

A caller in team_a requesting conn_id prod__conn resolves the secret that team team_a--prod stores under conn_id conn. _names_a_team_namespace does not catch it because it does not normalise (see the inline note on that line) — it only sees a literal --.

This is the same failure class the description says it closes; test_team_whose_name_extends_the_callers_is_not_readable covers only the literal--- route.

Documentation

The Azure PR that established this convention (#65692) added a 20-line "Multi-team lookup" section to its backend docs, and the amazon, yandex, and cncf backends document theirs too. providers/google/docs/secrets-backends/google-cloud-secret-manager-backend.rst is untouched here. Given the naming inconsistency flagged inline, docs matter more for this backend than they did for Azure — a user cannot derive the team-scoped name from the documented team-agnostic one.

Changelog

providers/AGENTS.md:

When a provider change needs a user-visible note (typically a breaking change or important behavior change that warrants explanation), update the provider's docs/changelog.rst directly in the same PR

The description's own "Behaviour change" section is that trigger — multi-team deployments running this backend today have to re-store their secrets under the new names.

get_config

get_config is unchanged in behaviour for ordinary configuration keys; it shares _get_secret, so a config id that spells out a team scoped name is now refused too, which matches how the Azure backend already behaves.

Azure's get_config calls _get_secret(self.config_prefix, key) and never invokes _is_team_specific_accessed_as_global, so it does not refuse such an id. The change here is stricter than the stated reference, and there's no test covering it.

CI

The provider test jobs haven't reported yet on this head — the CI image build is still pending and no Providers job appears in the rollup. Worth waiting for that before merge regardless of the above.


Suggested additional reviewers for the multi-team secrets convention (named, not requested — nobody is being notified): vincbeck, most recent author on this file and on the multi-team secrets work; PrithviBadiga, who authored the Azure backend this mirrors.


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.


Drafted-by: Claude Code (Opus 5, 1M context); reviewed by @shahar1 before posting

Comment thread providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py Outdated
Comment thread providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py Outdated
Comment thread providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py Outdated
Comment thread providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py Outdated
potiuk added 8 commits August 1, 2026 00:52
Address review: the underscore normalisation copied from the Azure Key Vault
backend could manufacture the team separator out of a plain secret id, so a
caller in one team resolved another team's secret. That backend overrides
build_path and normalises the whole name; this one inherits the base
implementation and normalises nothing, so the transplanted naming reopened the
cross-namespace read it was meant to close.
A refused id returned None with no trace, which is indistinguishable from
a secret that simply does not exist. The refusal also applies to
deployments that use no teams at all, so an operator upgrading with a
'--' in a connection id needs to be told why the lookup stopped working.
The docs spellcheck rejects 'normalise'.
The store is closed over by reference, so it can be filled after the
backend exists rather than discarding a throwaway backend built only to
derive secret names.
@potiuk
potiuk merged commit 0b3aacc into apache:main Aug 1, 2026
6 checks passed
@potiuk
potiuk deleted the scope-google-secret-manager-lookups-to-their-team branch August 1, 2026 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers area:secrets provider:google Google (including GCP) related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants