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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
