You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2026/07/31 19:22:20 UTC
[PR] Apply the team scope in the Google Secret Manager backend [airflow]
potiuk opened a new pull request, #70869:
URL: https://github.com/apache/airflow/pull/70869
`CloudSecretManagerBackend.get_conn_value` and `get_variable` both accept a
`team_name`, but the helper they delegate to has no such parameter:
```python
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
- [x] `providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py` —
36 pass (30 existing, 6 new)
- [x] 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
- [x] `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
- [x] 5 of the 6 new tests fail against the unmodified backend; the sixth is a
no-regression guard for team agnostic lookups
- [x] `ruff check` / `ruff format` clean
##### Was generative AI tooling used to co-author this PR?
- [X] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply the team scope in the Google Secret Manager backend [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #70869:
URL: https://github.com/apache/airflow/pull/70869#discussion_r3694168634
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -37,6 +38,10 @@
SECRET_ID_PATTERN = r"^[a-zA-Z0-9-_]*$"
+# Separator between the team name and the secret id in a team scoped secret name.
+# Matches the convention the other secrets backends use.
+TEAM_SEP = "--"
Review Comment:
Agreed. There's no shared constant in core to import yet -- Azure and Amazon each define their own `TEAM_SEP`, and Yandex derives it from `sep` -- so this stays local for now. Worth consolidating the moment one lands in core.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -193,12 +198,52 @@ def get_config(self, key: str) -> str | None:
return self._get_secret(self.config_prefix, key)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _build_team_secret_name(self, path_prefix: str, team_name: str, secret_id: str) -> str:
+ """Build a team scoped secret name using a dedicated separator before the secret id."""
+ team_prefix = self.build_path(path_prefix, team_name, self.sep)
+ normalized_secret_id = secret_id.replace("_", self.sep)
Review Comment:
Fixed -- the `.replace()` is gone and the secret id is used verbatim.
Both halves of this were right. I reproduced the collision exactly as you wrote it, and the naming inconsistency was the other side of the same coin. `_build_team_secret_name` now builds `<prefix><sep><team>--<secret_id>` with the id untouched, so the team separator can only ever originate from the team name, and underscores survive in the team-scoped name just as they do in the team-agnostic one.
Two tests cover it: `test_team_secret_name_is_built_literally` pins the literal string rather than deriving it from the method under test, and `test_underscores_cannot_manufacture_a_team_namespace` asserts your `prod__conn` case returns `None`. The latter fails against the revision you reviewed.
For the record, the same normalisation is still live in the Azure backend. #70876 closes it there -- but at the entry point rather than in the builder, since Azure normalises everywhere by design and the team-scoped name has to match how the secret is actually stored.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -193,12 +198,52 @@ def get_config(self, key: str) -> str | None:
return self._get_secret(self.config_prefix, key)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _build_team_secret_name(self, path_prefix: str, team_name: str, secret_id: str) -> str:
+ """Build a team scoped secret name using a dedicated separator before the secret id."""
+ team_prefix = self.build_path(path_prefix, team_name, self.sep)
+ normalized_secret_id = secret_id.replace("_", self.sep)
+ return f"{team_prefix}{TEAM_SEP}{normalized_secret_id}"
+
+ def _names_a_team_namespace(self, secret_id: str) -> bool:
+ """
+ Whether ``secret_id`` spells out a team scoped secret name.
+
+ A team scoped secret is named ``<prefix><sep><team><TEAM_SEP><secret id>``, so an id
+ that already contains the team separator makes the team agnostic lookup resolve a
+ secret inside some team's namespace. That lookup is refused for such an id.
+
+ 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`` would match ``a--b``'s namespace on the prefix and read its secrets. Only
+ the caller's own namespace is ever constructed, never parsed.
+ """
+ normalized_secret_id = self.build_path("", secret_id, self.sep)
Review Comment:
Fixed -- the guard matches the raw id now (`TEAM_SEP in secret_id`) and doesn't call `build_path` at all.
Your read of the inheritance is correct: this backend uses the base implementation, so `build_path('', 'smtp_default')` yields `-smtp_default` -- a stray leading separator and no normalisation, which is why the guard never saw the `prod__conn` form. With the id no longer normalised when the team-scoped name is built, there's nothing left for the guard to normalise either, so matching the raw id is both correct and simpler. I left a note in the docstring on why routing through `build_path` is wrong here, so it doesn't get copied back from Azure later.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py:
##########
@@ -272,3 +272,99 @@ def test_init_succeeds_with_explicit_project_id(self, mock_client_callable, mock
backend = CloudSecretManagerBackend(project_id="explicit-project")
assert backend.project_id == "explicit-project"
+
+
+class TestCloudSecretManagerBackendTeamScope:
+ """A team scoped secret must only be resolvable for the team it is stored for."""
+
+ TEAM = "team_a"
+ OTHER_TEAM = "team_b"
+ # A team name may itself contain the team separator, so one team's namespace can start
+ # with another's. Treating a prefix match as ownership is what this guards against.
+ EXTENDING_TEAM = "team_a--prod"
+
+ @staticmethod
+ def _backend(mock_client_callable, store):
+ """Back the backend with an in-memory secret store keyed by full secret name."""
+ client = mock.MagicMock()
+ client.get_secret.side_effect = lambda secret_id, project_id, **kwargs: store.get(secret_id)
+ mock_client_callable.return_value = client
+ return CloudSecretManagerBackend(
+ connections_prefix=CONNECTIONS_PREFIX, variables_prefix=VARIABLES_PREFIX, project_id=PROJECT_ID
+ )
+
+ @staticmethod
+ def _team_name(backend, team, secret_id, prefix=CONNECTIONS_PREFIX):
Review Comment:
Fixed. `test_team_secret_name_is_built_literally` now asserts the literal name:
```
test-connections-team_a--test_postgres
```
plus a second assertion that `smtp_default` keeps its underscore.
Your negative test went in as `test_underscores_cannot_manufacture_a_team_namespace`. I confirmed it fails against the revision you reviewed and passes now -- and re-confirmed after the later refactor by reintroducing the `.replace()` and watching it fail again, so it isn't passing vacuously.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -193,12 +198,52 @@ def get_config(self, key: str) -> str | None:
return self._get_secret(self.config_prefix, key)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _build_team_secret_name(self, path_prefix: str, team_name: str, secret_id: str) -> str:
+ """Build a team scoped secret name using a dedicated separator before the secret id."""
+ team_prefix = self.build_path(path_prefix, team_name, self.sep)
+ normalized_secret_id = secret_id.replace("_", self.sep)
+ return f"{team_prefix}{TEAM_SEP}{normalized_secret_id}"
+
+ def _names_a_team_namespace(self, secret_id: str) -> bool:
+ """
+ Whether ``secret_id`` spells out a team scoped secret name.
+
+ A team scoped secret is named ``<prefix><sep><team><TEAM_SEP><secret id>``, so an id
+ that already contains the team separator makes the team agnostic lookup resolve a
+ secret inside some team's namespace. That lookup is refused for such an id.
+
+ 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`` would match ``a--b``'s namespace on the prefix and read its secrets. Only
+ the caller's own namespace is ever constructed, never parsed.
+ """
+ normalized_secret_id = self.build_path("", secret_id, self.sep)
+ return bool(re.fullmatch(rf".+{re.escape(TEAM_SEP)}.+", normalized_secret_id))
+
+ def _get_secret(self, path_prefix: str, secret_id: str, team_name: str | None = None) -> str | None:
"""
Get secret value from the SecretManager based on prefix.
:param path_prefix: Prefix for the Path to get Secret
:param secret_id: Secret Key
+ :param team_name: Team the lookup is scoped to (if any)
"""
+ # The team scoped name is tried first and is safe by construction: it can only ever
+ # build the caller's own namespace.
+ if team_name:
+ team_secret = self.client.get_secret(
Review Comment:
Fixed -- `client = self.client` is bound once at the top of `_get_secret` and used for both lookups, with a comment recording why, since the cost isn't visible at the call site.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py:
##########
@@ -272,3 +272,99 @@ def test_init_succeeds_with_explicit_project_id(self, mock_client_callable, mock
backend = CloudSecretManagerBackend(project_id="explicit-project")
assert backend.project_id == "explicit-project"
+
+
+class TestCloudSecretManagerBackendTeamScope:
+ """A team scoped secret must only be resolvable for the team it is stored for."""
+
+ TEAM = "team_a"
+ OTHER_TEAM = "team_b"
+ # A team name may itself contain the team separator, so one team's namespace can start
+ # with another's. Treating a prefix match as ownership is what this guards against.
+ EXTENDING_TEAM = "team_a--prod"
+
+ @staticmethod
+ def _backend(mock_client_callable, store):
+ """Back the backend with an in-memory secret store keyed by full secret name."""
+ client = mock.MagicMock()
+ client.get_secret.side_effect = lambda secret_id, project_id, **kwargs: store.get(secret_id)
+ mock_client_callable.return_value = client
+ return CloudSecretManagerBackend(
+ connections_prefix=CONNECTIONS_PREFIX, variables_prefix=VARIABLES_PREFIX, project_id=PROJECT_ID
+ )
+
+ @staticmethod
+ def _team_name(backend, team, secret_id, prefix=CONNECTIONS_PREFIX):
+ return backend._build_team_secret_name(prefix, team, secret_id)
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_resolved_for_its_own_team(self, mock_client, mock_get_creds):
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
Review Comment:
Fixed, using your second suggestion -- `_backend` returns `(backend, store)` and the store is populated after the backend exists, since the lookup closes over it by reference. The discarded backend is gone from every test in the class.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
##########
providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py:
##########
@@ -272,3 +272,99 @@ def test_init_succeeds_with_explicit_project_id(self, mock_client_callable, mock
backend = CloudSecretManagerBackend(project_id="explicit-project")
assert backend.project_id == "explicit-project"
+
+
+class TestCloudSecretManagerBackendTeamScope:
+ """A team scoped secret must only be resolvable for the team it is stored for."""
+
+ TEAM = "team_a"
+ OTHER_TEAM = "team_b"
+ # A team name may itself contain the team separator, so one team's namespace can start
+ # with another's. Treating a prefix match as ownership is what this guards against.
+ EXTENDING_TEAM = "team_a--prod"
+
+ @staticmethod
+ def _backend(mock_client_callable, store):
+ """Back the backend with an in-memory secret store keyed by full secret name."""
+ client = mock.MagicMock()
+ client.get_secret.side_effect = lambda secret_id, project_id, **kwargs: store.get(secret_id)
+ mock_client_callable.return_value = client
+ return CloudSecretManagerBackend(
+ connections_prefix=CONNECTIONS_PREFIX, variables_prefix=VARIABLES_PREFIX, project_id=PROJECT_ID
+ )
+
+ @staticmethod
+ def _team_name(backend, team, secret_id, prefix=CONNECTIONS_PREFIX):
+ return backend._build_team_secret_name(prefix, team, secret_id)
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_resolved_for_its_own_team(self, mock_client, mock_get_creds):
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
+ store = {self._team_name(backend, self.TEAM, CONN_ID): CONN_URI}
+ backend = self._backend(mock_client, store)
+
+ assert backend.get_conn_value(conn_id=CONN_ID, team_name=self.TEAM) == CONN_URI
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_not_resolved_for_another_team(self, mock_client, mock_get_creds):
+ """The whole point: team B must not reach team A's secret by spelling out its name."""
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
+ encoded = f"{self.TEAM}--{CONN_ID}"
+ store = {self._team_name(backend, self.TEAM, CONN_ID): CONN_URI}
+ backend = self._backend(mock_client, store)
+
+ assert backend.get_conn_value(conn_id=encoded, team_name=self.OTHER_TEAM) is None
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_not_resolved_without_a_team_scope(self, mock_client, mock_get_creds):
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
+ encoded = f"{self.TEAM}--{CONN_ID}"
+ store = {self._team_name(backend, self.TEAM, CONN_ID): CONN_URI}
+ backend = self._backend(mock_client, store)
+
+ assert backend.get_conn_value(conn_id=encoded) is None
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_whose_name_extends_the_callers_is_not_readable(self, mock_client, mock_get_creds):
Review Comment:
Covered, though as a separate test rather than folded into this one -- `test_underscores_cannot_manufacture_a_team_namespace` asserts exactly your line:
```python
assert backend.get_conn_value(conn_id=f"prod__{CONN_ID}", team_name=self.TEAM) is None
```
They stay apart because this test is about a team whose *name* extends the caller's, and that one is about the *id* manufacturing the separator -- two different mechanisms that happen to reach the same namespace, each worth failing independently.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply the team scope in the Google Secret Manager backend [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #70869:
URL: https://github.com/apache/airflow/pull/70869
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply the team scope in the Google Secret Manager backend [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #70869:
URL: https://github.com/apache/airflow/pull/70869#discussion_r3692954510
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -193,12 +198,52 @@ def get_config(self, key: str) -> str | None:
return self._get_secret(self.config_prefix, key)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _build_team_secret_name(self, path_prefix: str, team_name: str, secret_id: str) -> str:
+ """Build a team scoped secret name using a dedicated separator before the secret id."""
+ team_prefix = self.build_path(path_prefix, team_name, self.sep)
+ normalized_secret_id = secret_id.replace("_", self.sep)
Review Comment:
**Cross-team read: this line can manufacture `TEAM_SEP`.**
`_` → `sep` normalisation on the secret id means a plain conn_id containing `__` produces a name inside *another* team's namespace:
```
team 'team_a', id 'prod__conn' -> test-connections-team_a--prod--conn
team 'team_a--prod', id 'conn' -> test-connections-team_a--prod--conn
```
Identical. A caller in `team_a` reads team `team_a--prod`'s secret. `_names_a_team_namespace("prod__conn")` returns `False`, so nothing stops it. That is the exact failure the description says is closed — `test_team_whose_name_extends_the_callers_is_not_readable` only covers the literal-`--` spelling.
**Second issue on the same line:** the normalisation makes team-scoped naming inconsistent with this backend's documented convention. The docs promise `airflow-connections-smtp_default` for conn_id `smtp_default` (underscores preserved), but the team-scoped name becomes:
```
team-agnostic: airflow-connections-smtp_default
team-scoped : airflow-connections-team_a--smtp-default
```
Underscores kept in the prefix and the team name, converted in the secret id. In Azure this is consistent because its `build_path` override normalises the whole name; here nothing else does.
Dropping the `.replace()` and using the raw `secret_id` fixes both — the team separator can then only come from a team name, and the naming matches the rest of the backend:
```suggestion
normalized_secret_id = secret_id
```
(at which point the local is worth inlining or renaming).
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -193,12 +198,52 @@ def get_config(self, key: str) -> str | None:
return self._get_secret(self.config_prefix, key)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _build_team_secret_name(self, path_prefix: str, team_name: str, secret_id: str) -> str:
+ """Build a team scoped secret name using a dedicated separator before the secret id."""
+ team_prefix = self.build_path(path_prefix, team_name, self.sep)
+ normalized_secret_id = secret_id.replace("_", self.sep)
+ return f"{team_prefix}{TEAM_SEP}{normalized_secret_id}"
+
+ def _names_a_team_namespace(self, secret_id: str) -> bool:
+ """
+ Whether ``secret_id`` spells out a team scoped secret name.
+
+ A team scoped secret is named ``<prefix><sep><team><TEAM_SEP><secret id>``, so an id
+ that already contains the team separator makes the team agnostic lookup resolve a
+ secret inside some team's namespace. That lookup is refused for such an id.
+
+ 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`` would match ``a--b``'s namespace on the prefix and read its secrets. Only
+ the caller's own namespace is ever constructed, never parsed.
+ """
+ normalized_secret_id = self.build_path("", secret_id, self.sep)
Review Comment:
This was copied from the Azure backend, which **overrides** `build_path` — special-casing the empty prefix and replacing `_` with `sep`:
```python
# key_vault.py
@staticmethod
def build_path(path_prefix: str, secret_id: str, sep: str = "-") -> str:
path = f"{secret_id}" if path_prefix == "" else f"{path_prefix}{sep}{secret_id}"
return path.replace("_", sep)
```
This backend inherits the base implementation, which does neither:
```python
# shared/secrets_backend/.../base.py
return f"{path_prefix}{sep}{secret_id}"
```
So the call yields a stray leading separator and no normalisation at all:
```
google: build_path('', 'smtp_default') -> '-smtp_default'
azure : build_path('', 'smtp_default') -> 'smtp-default'
```
Two consequences: the leading `-` changes the regex outcome for ids starting with `--`, and — more importantly — the guard never sees the normalised form, which is why it misses the `prod__conn` case flagged above.
Given the suggestion on line 204 (stop normalising the id), the guard can just match the raw id and skip `build_path` entirely.
##########
providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py:
##########
@@ -272,3 +272,99 @@ def test_init_succeeds_with_explicit_project_id(self, mock_client_callable, mock
backend = CloudSecretManagerBackend(project_id="explicit-project")
assert backend.project_id == "explicit-project"
+
+
+class TestCloudSecretManagerBackendTeamScope:
+ """A team scoped secret must only be resolvable for the team it is stored for."""
+
+ TEAM = "team_a"
+ OTHER_TEAM = "team_b"
+ # A team name may itself contain the team separator, so one team's namespace can start
+ # with another's. Treating a prefix match as ownership is what this guards against.
+ EXTENDING_TEAM = "team_a--prod"
+
+ @staticmethod
+ def _backend(mock_client_callable, store):
+ """Back the backend with an in-memory secret store keyed by full secret name."""
+ client = mock.MagicMock()
+ client.get_secret.side_effect = lambda secret_id, project_id, **kwargs: store.get(secret_id)
+ mock_client_callable.return_value = client
+ return CloudSecretManagerBackend(
+ connections_prefix=CONNECTIONS_PREFIX, variables_prefix=VARIABLES_PREFIX, project_id=PROJECT_ID
+ )
+
+ @staticmethod
+ def _team_name(backend, team, secret_id, prefix=CONNECTIONS_PREFIX):
+ return backend._build_team_secret_name(prefix, team, secret_id)
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_resolved_for_its_own_team(self, mock_client, mock_get_creds):
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
Review Comment:
This backend is discarded and rebuilt three lines down — same in all six new tests. It only exists to have an object to call `_build_team_secret_name` / `build_path` on.
`_backend`'s `side_effect` closes over `store` by reference, so one backend plus a dict populated afterwards does the same job:
```python
store = {}
backend = self._backend(mock_client, store)
store[self._team_name(backend, self.TEAM, CONN_ID)] = CONN_URI
```
Or make `_backend` return `(backend, store)` and drop the double construction everywhere.
##########
providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py:
##########
@@ -272,3 +272,99 @@ def test_init_succeeds_with_explicit_project_id(self, mock_client_callable, mock
backend = CloudSecretManagerBackend(project_id="explicit-project")
assert backend.project_id == "explicit-project"
+
+
+class TestCloudSecretManagerBackendTeamScope:
+ """A team scoped secret must only be resolvable for the team it is stored for."""
+
+ TEAM = "team_a"
+ OTHER_TEAM = "team_b"
+ # A team name may itself contain the team separator, so one team's namespace can start
+ # with another's. Treating a prefix match as ownership is what this guards against.
+ EXTENDING_TEAM = "team_a--prod"
+
+ @staticmethod
+ def _backend(mock_client_callable, store):
+ """Back the backend with an in-memory secret store keyed by full secret name."""
+ client = mock.MagicMock()
+ client.get_secret.side_effect = lambda secret_id, project_id, **kwargs: store.get(secret_id)
+ mock_client_callable.return_value = client
+ return CloudSecretManagerBackend(
+ connections_prefix=CONNECTIONS_PREFIX, variables_prefix=VARIABLES_PREFIX, project_id=PROJECT_ID
+ )
+
+ @staticmethod
+ def _team_name(backend, team, secret_id, prefix=CONNECTIONS_PREFIX):
Review Comment:
The expected secret names come from `_build_team_secret_name` — the method under test. The mock store is keyed by whatever the implementation emits, so the tests agree with the implementation by construction and any naming defect is invisible to them. That is why the `prod__conn` collision and the underscore inconsistency both pass here.
Worth having at least one test assert the literal name, e.g.:
```python
assert backend._build_team_secret_name(CONNECTIONS_PREFIX, "team_a", "test_postgres") == (
"test-connections-team_a--test_postgres"
)
```
and one negative test that constructs the attacker's id independently of the implementation:
```python
# team_a must not reach team_a--prod's namespace via underscore normalisation
store = {self._team_name(backend, "team_a--prod", CONN_ID): CONN_URI}
assert backend.get_conn_value(conn_id=f"prod__{CONN_ID}", team_name="team_a") is None
```
That second one fails against the current implementation.
##########
providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py:
##########
@@ -272,3 +272,99 @@ def test_init_succeeds_with_explicit_project_id(self, mock_client_callable, mock
backend = CloudSecretManagerBackend(project_id="explicit-project")
assert backend.project_id == "explicit-project"
+
+
+class TestCloudSecretManagerBackendTeamScope:
+ """A team scoped secret must only be resolvable for the team it is stored for."""
+
+ TEAM = "team_a"
+ OTHER_TEAM = "team_b"
+ # A team name may itself contain the team separator, so one team's namespace can start
+ # with another's. Treating a prefix match as ownership is what this guards against.
+ EXTENDING_TEAM = "team_a--prod"
+
+ @staticmethod
+ def _backend(mock_client_callable, store):
+ """Back the backend with an in-memory secret store keyed by full secret name."""
+ client = mock.MagicMock()
+ client.get_secret.side_effect = lambda secret_id, project_id, **kwargs: store.get(secret_id)
+ mock_client_callable.return_value = client
+ return CloudSecretManagerBackend(
+ connections_prefix=CONNECTIONS_PREFIX, variables_prefix=VARIABLES_PREFIX, project_id=PROJECT_ID
+ )
+
+ @staticmethod
+ def _team_name(backend, team, secret_id, prefix=CONNECTIONS_PREFIX):
+ return backend._build_team_secret_name(prefix, team, secret_id)
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_resolved_for_its_own_team(self, mock_client, mock_get_creds):
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
+ store = {self._team_name(backend, self.TEAM, CONN_ID): CONN_URI}
+ backend = self._backend(mock_client, store)
+
+ assert backend.get_conn_value(conn_id=CONN_ID, team_name=self.TEAM) == CONN_URI
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_not_resolved_for_another_team(self, mock_client, mock_get_creds):
+ """The whole point: team B must not reach team A's secret by spelling out its name."""
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
+ encoded = f"{self.TEAM}--{CONN_ID}"
+ store = {self._team_name(backend, self.TEAM, CONN_ID): CONN_URI}
+ backend = self._backend(mock_client, store)
+
+ assert backend.get_conn_value(conn_id=encoded, team_name=self.OTHER_TEAM) is None
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_scoped_secret_is_not_resolved_without_a_team_scope(self, mock_client, mock_get_creds):
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend = self._backend(mock_client, {})
+ encoded = f"{self.TEAM}--{CONN_ID}"
+ store = {self._team_name(backend, self.TEAM, CONN_ID): CONN_URI}
+ backend = self._backend(mock_client, store)
+
+ assert backend.get_conn_value(conn_id=encoded) is None
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_whose_name_extends_the_callers_is_not_readable(self, mock_client, mock_get_creds):
Review Comment:
This test is the right idea and the most valuable one in the class — but it only covers the case where the caller *spells out* `team_a--prod--...` literally. The same namespace is reachable without any `--` in the id at all, via the underscore normalisation on line 204 of the backend:
```python
assert backend.get_conn_value(conn_id=f"prod__{CONN_ID}", team_name=self.TEAM) is None
```
That currently returns `CONN_URI`. Worth adding here once the naming issue is resolved, since it's the regression guard for it.
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -193,12 +198,52 @@ def get_config(self, key: str) -> str | None:
return self._get_secret(self.config_prefix, key)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _build_team_secret_name(self, path_prefix: str, team_name: str, secret_id: str) -> str:
+ """Build a team scoped secret name using a dedicated separator before the secret id."""
+ team_prefix = self.build_path(path_prefix, team_name, self.sep)
+ normalized_secret_id = secret_id.replace("_", self.sep)
+ return f"{team_prefix}{TEAM_SEP}{normalized_secret_id}"
+
+ def _names_a_team_namespace(self, secret_id: str) -> bool:
+ """
+ Whether ``secret_id`` spells out a team scoped secret name.
+
+ A team scoped secret is named ``<prefix><sep><team><TEAM_SEP><secret id>``, so an id
+ that already contains the team separator makes the team agnostic lookup resolve a
+ secret inside some team's namespace. That lookup is refused for such an id.
+
+ 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`` would match ``a--b``'s namespace on the prefix and read its secrets. Only
+ the caller's own namespace is ever constructed, never parsed.
+ """
+ normalized_secret_id = self.build_path("", secret_id, self.sep)
+ return bool(re.fullmatch(rf".+{re.escape(TEAM_SEP)}.+", normalized_secret_id))
+
+ def _get_secret(self, path_prefix: str, secret_id: str, team_name: str | None = None) -> str | None:
"""
Get secret value from the SecretManager based on prefix.
:param path_prefix: Prefix for the Path to get Secret
:param secret_id: Secret Key
+ :param team_name: Team the lookup is scoped to (if any)
"""
+ # The team scoped name is tried first and is safe by construction: it can only ever
+ # build the caller's own namespace.
+ if team_name:
+ team_secret = self.client.get_secret(
Review Comment:
`self.client` is a property that constructs a new `_SecretManagerClient` on every access, and that object's `client` is a per-instance `cached_property` holding a real gRPC `SecretManagerServiceClient`. A team-scoped miss now builds two of them per lookup, on a path that's hit for every connection and variable resolution.
Bind it once at the top of `_get_secret`:
```python
client = self.client
```
and use `client.get_secret(...)` for both calls.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Apply the team scope in the Google Secret Manager backend [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #70869:
URL: https://github.com/apache/airflow/pull/70869#discussion_r3692923145
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -37,6 +38,10 @@
SECRET_ID_PATTERN = r"^[a-zA-Z0-9-_]*$"
+# Separator between the team name and the secret id in a team scoped secret name.
+# Matches the convention the other secrets backends use.
+TEAM_SEP = "--"
Review Comment:
Good to import this from core in the future not a blocker
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org