This is an automated email from the ASF dual-hosted git repository.
potiuk pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/main by this push:
new 0b3aaccfa01 Apply the team scope in the Google Secret Manager backend
(#70869)
0b3aaccfa01 is described below
commit 0b3aaccfa015c69c51c5a68c6469fdfc0d68c16d
Author: Jarek Potiuk <[email protected]>
AuthorDate: Sat Aug 1 04:07:00 2026 +0200
Apply the team scope in the Google Secret Manager backend (#70869)
* Apply the team scope in the Google Secret Manager backend
`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
* Apply the team scope in the Google Secret Manager backend
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.
* Log why an ambiguous secret id is refused in the Google backend
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.
* Use American spelling in the Google secrets backend docs
The docs spellcheck rejects 'normalise'.
* Build the team scope test backend once per test
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.
* Delete ub2_69231
* Delete ub2_69666
* Delete ub2_69885
* Delete ub2_69908
---
providers/google/docs/changelog.rst | 14 ++
.../google-cloud-secret-manager-backend.rst | 42 +++++
.../google/cloud/secrets/secret_manager.py | 85 ++++++++-
.../google/cloud/secrets/test_secret_manager.py | 193 ++++++++++++++++++++-
4 files changed, 328 insertions(+), 6 deletions(-)
diff --git a/providers/google/docs/changelog.rst
b/providers/google/docs/changelog.rst
index 6ec953e93dc..450e13becb0 100644
--- a/providers/google/docs/changelog.rst
+++ b/providers/google/docs/changelog.rst
@@ -27,6 +27,20 @@
Changelog
---------
+.. note::
+ ``CloudSecretManagerBackend`` now applies the team scope it is given.
Previously
+ ``get_conn_value`` and ``get_variable`` accepted a ``team_name`` and
dropped it, so every
+ lookup resolved the team-agnostic secret name regardless of the team.
+
+ In multi-team deployments a team-scoped secret must now be stored under
+ ``{prefix}{sep}{team_name}--{secret_id}``; the team-agnostic name is still
used as a
+ fallback when no team-scoped secret exists. Secrets that were relied on to
resolve for a
+ specific team have to be re-stored under the team-scoped name.
+
+ Connection ids and variable keys containing ``--`` are no longer resolved
in either mode,
+ because a team name may itself contain ``--`` and the resulting name would
be ambiguous.
+ See :doc:`/secrets-backends/google-cloud-secret-manager-backend` for the
full convention.
+
22.3.0
......
diff --git
a/providers/google/docs/secrets-backends/google-cloud-secret-manager-backend.rst
b/providers/google/docs/secrets-backends/google-cloud-secret-manager-backend.rst
index cce93a1ff8c..7a4328ad3c2 100644
---
a/providers/google/docs/secrets-backends/google-cloud-secret-manager-backend.rst
+++
b/providers/google/docs/secrets-backends/google-cloud-secret-manager-backend.rst
@@ -196,6 +196,48 @@ To check the variables is correctly read from the backend
secret, you can use ``
$ airflow variables get first-variable
secret_content
+Multi-team lookup
+=================
+
+In multi-team mode this backend looks for a team-scoped secret first and falls
back to the
+team-agnostic name when no team-scoped secret exists.
+
+For connections:
+
+* Team-scoped: ``{connections_prefix}{sep}{team_name}--{conn_id}``
+* Team-agnostic fallback: ``{connections_prefix}{sep}{conn_id}``
+
+For variables:
+
+* Team-scoped: ``{variables_prefix}{sep}{team_name}--{key}``
+* Team-agnostic fallback: ``{variables_prefix}{sep}{key}``
+
+The team name and the secret id are separated by ``--``, while ``sep``
(default ``-``) separates
+the prefix from what follows. So with
``connections_prefix="airflow-connections"``,
+``team_name="team_a"`` and ``conn_id="smtp_default"``, the backend looks up
+``airflow-connections-team_a--smtp_default`` before falling back to
+``airflow-connections-smtp_default``.
+
+.. note::
+
+ Unlike the Azure Key Vault backend, this backend does **not** normalize
underscores to the
+ separator. The secret id keeps the exact form you request it under, in the
team-scoped name
+ as well as the team-agnostic one.
+
+Because a team name may itself contain ``--``, an id that contains ``--`` is
ambiguous — team
+``team_a`` with conn_id ``prod--db`` and team ``team_a--prod`` with conn_id
``db`` would name the
+same secret. Such an id is therefore never resolved, for team-scoped and
team-agnostic lookups
+alike, and the backend never parses an id to infer which team it belongs to. A
warning is logged
+whenever an id is refused for this reason, so the resulting ``None`` is not
mistaken for a missing
+secret. Avoid ``--`` in connection ids and variable keys.
+
+.. warning::
+
+ This refusal applies whether or not you use teams. If you already store a
connection or
+ variable whose id contains ``--``, it stops resolving after upgrading and
you must rename it.
+
+``get_config`` is not team-scoped and is unaffected by any of the above.
+
Clean up
========
diff --git
a/providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py
b/providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py
index 2b505499124..8966e1372ae 100644
---
a/providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py
+++
b/providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py
@@ -37,6 +37,10 @@ log = logging.getLogger(__name__)
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 = "--"
+
class CloudSecretManagerBackend(BaseSecretsBackend, LoggingMixin):
"""
@@ -166,7 +170,11 @@ class CloudSecretManagerBackend(BaseSecretsBackend,
LoggingMixin):
if self.connections_prefix is None:
return None
- return self._get_secret(self.connections_prefix, conn_id)
+ if self._names_a_team_namespace(conn_id):
+ self._log_refusal("connection", conn_id)
+ return None
+
+ return self._get_secret(self.connections_prefix, conn_id, team_name)
def get_variable(self, key: str, team_name: str | None = None) -> str |
None:
"""
@@ -179,7 +187,11 @@ class CloudSecretManagerBackend(BaseSecretsBackend,
LoggingMixin):
if self.variables_prefix is None:
return None
- return self._get_secret(self.variables_prefix, key)
+ if self._names_a_team_namespace(key):
+ self._log_refusal("variable", key)
+ return None
+
+ return self._get_secret(self.variables_prefix, key, team_name)
def get_config(self, key: str) -> str | None:
"""
@@ -193,12 +205,75 @@ class CloudSecretManagerBackend(BaseSecretsBackend,
LoggingMixin):
return self._get_secret(self.config_prefix, key)
- def _get_secret(self, path_prefix: str, secret_id: str) -> str | None:
+ def _log_refusal(self, kind: str, secret_id: str) -> None:
+ self.log.warning(
+ "%s id %r contains %r, which separates the team name from the
secret id in a team "
+ "scoped secret name. Such an id is ambiguous and is not looked up.
Returning None.",
+ kind.capitalize(),
+ secret_id,
+ TEAM_SEP,
+ )
+
+ 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.
+
+ The secret id is used verbatim. Normalizing it (``_`` -> ``sep``, as
the Azure Key
+ Vault backend does) would let a plain id manufacture ``TEAM_SEP``:
team ``a`` asking
+ for ``b__c`` would build the same name as team ``a--b`` asking for
``c``. It would
+ also contradict this backend's naming, which keeps underscores
everywhere else --
+ unlike Azure, this backend does not override :meth:`build_path`.
+ """
+ team_prefix = self.build_path(path_prefix, team_name, self.sep)
+ return f"{team_prefix}{TEAM_SEP}{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 itself contains the team separator makes the built name
ambiguous: team ``a``
+ with id ``b--c`` and team ``a--b`` with id ``c`` produce the same
string. Such an id
+ is refused for *every* lookup -- team scoped as well as team agnostic
-- because the
+ ambiguity exists in both directions and the caller's own namespace is
not a safe
+ harbor for it.
+
+ The id is never parsed to work out *which* team it names, because it
cannot be:
+ nothing in the string distinguishes the two readings above. 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.
+
+ The raw id is matched. Routing it through :meth:`build_path` first, as
the Azure
+ backend does, is wrong here: the inherited implementation prepends a
separator to an
+ empty prefix (``'' -> '-smtp_default'``) and normalizes nothing, so
the guard would
+ both mis-anchor and miss ids whose separator only appears after
normalization.
+ """
+ return TEAM_SEP in 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)
"""
- secret_id = self.build_path(path_prefix, secret_id, self.sep)
- return self.client.get_secret(secret_id=secret_id,
project_id=self.project_id)
+ # ``self.client`` builds a new ``_SecretManagerClient`` -- and with it
a real gRPC
+ # client -- on every access, so it is bound once for both lookups
below.
+ client = self.client
+
+ # The team scoped name is tried first and is safe by construction: it
can only ever
+ # build the caller's own namespace. Ids that would make that name
ambiguous are
+ # refused by the callers before reaching here.
+ if team_name:
+ team_secret = client.get_secret(
+ secret_id=self._build_team_secret_name(path_prefix, team_name,
secret_id),
+ project_id=self.project_id,
+ )
+ if team_secret is not None:
+ return team_secret
+
+ return client.get_secret(
+ secret_id=self.build_path(path_prefix, secret_id, self.sep),
project_id=self.project_id
+ )
diff --git
a/providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py
b/providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py
index 7d2d381cf7c..090f46b150e 100644
--- a/providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py
+++ b/providers/google/tests/unit/google/cloud/secrets/test_secret_manager.py
@@ -26,7 +26,7 @@ from google.cloud.secretmanager_v1.types import
AccessSecretVersionResponse
from airflow.models import Connection
from airflow.providers.common.compat.sdk import AirflowException
-from airflow.providers.google.cloud.secrets.secret_manager import
CloudSecretManagerBackend
+from airflow.providers.google.cloud.secrets.secret_manager import TEAM_SEP,
CloudSecretManagerBackend
CREDENTIALS = "test-creds"
KEY_FILE = "test-file.json"
@@ -34,6 +34,7 @@ PROJECT_ID = "test-project-id"
OVERRIDDEN_PROJECT_ID = "overridden-test-project-id"
CONNECTIONS_PREFIX = "test-connections"
VARIABLES_PREFIX = "test-variables"
+CONFIG_PREFIX = "test-config"
SEP = "-"
CONN_ID = "test-postgres"
CONN_URI = "postgresql://airflow:airflow@host:5432/airflow"
@@ -272,3 +273,193 @@ class TestCloudSecretManagerBackend:
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):
+ """
+ Build a backend over an in-memory secret store keyed by full secret
name.
+
+ The store is returned rather than taken as an argument so tests can
populate it after
+ building the backend -- the lookup closes over it by reference. Names
are derived from
+ the backend, so it has to exist first.
+ """
+ store: dict[str, str] = {}
+ client = mock.MagicMock()
+ client.get_secret.side_effect = lambda secret_id, project_id,
**kwargs: store.get(secret_id)
+ mock_client_callable.return_value = client
+ backend = CloudSecretManagerBackend(
+ connections_prefix=CONNECTIONS_PREFIX,
+ variables_prefix=VARIABLES_PREFIX,
+ config_prefix=CONFIG_PREFIX,
+ project_id=PROJECT_ID,
+ )
+ return backend, store
+
+ @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, store = self._backend(mock_client)
+ store[self._team_name(backend, self.TEAM, CONN_ID)] = CONN_URI
+
+ 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, store = self._backend(mock_client)
+ encoded = f"{self.TEAM}--{CONN_ID}"
+ store[self._team_name(backend, self.TEAM, CONN_ID)] = CONN_URI
+
+ 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, store = self._backend(mock_client)
+ encoded = f"{self.TEAM}--{CONN_ID}"
+ store[self._team_name(backend, self.TEAM, CONN_ID)] = CONN_URI
+
+ 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):
+ """A prefix match on the caller's namespace is not proof of
ownership."""
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend, store = self._backend(mock_client)
+ store[self._team_name(backend, self.EXTENDING_TEAM, CONN_ID)] =
CONN_URI
+ encoded = f"{self.EXTENDING_TEAM}--{CONN_ID}"
+
+ assert backend.get_conn_value(conn_id=encoded, team_name=self.TEAM) is
None
+ # and the owning team still reaches it normally, with the bare id
+ assert backend.get_conn_value(conn_id=CONN_ID,
team_name=self.EXTENDING_TEAM) == CONN_URI
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_team_agnostic_secret_is_resolved_for_any_team_scope(self,
mock_client, mock_get_creds):
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend, store = self._backend(mock_client)
+ store[backend.build_path(CONNECTIONS_PREFIX, CONN_ID, SEP)] = CONN_URI
+
+ assert backend.get_conn_value(conn_id=CONN_ID) == CONN_URI
+ 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_variable_is_not_resolved_for_another_team(self,
mock_client, mock_get_creds):
+ """Variables share the lookup, so they must be scoped identically."""
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend, store = self._backend(mock_client)
+ encoded = f"{self.TEAM}--{VAR_KEY}"
+ store[self._team_name(backend, self.TEAM, VAR_KEY, VARIABLES_PREFIX)]
= VAR_VALUE
+
+ assert backend.get_variable(key=VAR_KEY, team_name=self.TEAM) ==
VAR_VALUE
+ assert backend.get_variable(key=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_secret_name_is_built_literally(self, mock_client,
mock_get_creds):
+ """
+ Pin the literal name rather than deriving it from the method under
test.
+
+ Every other test in this class keys its store with
``_build_team_secret_name``, so it
+ agrees with the implementation by construction and cannot see a naming
defect.
+ """
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend, _ = self._backend(mock_client)
+
+ assert (
+ backend._build_team_secret_name(CONNECTIONS_PREFIX, "team_a",
"test_postgres")
+ == f"{CONNECTIONS_PREFIX}{SEP}team_a{TEAM_SEP}test_postgres"
+ )
+ # Underscores survive in the secret id, matching the team agnostic
name this backend
+ # documents -- unlike Azure, nothing here normalises ``_`` to the
separator.
+ assert (
+ backend._build_team_secret_name(CONNECTIONS_PREFIX, "team_a",
"smtp_default")
+ == f"{CONNECTIONS_PREFIX}{SEP}team_a{TEAM_SEP}smtp_default"
+ )
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_underscores_cannot_manufacture_a_team_namespace(self,
mock_client, mock_get_creds):
+ """
+ An id containing ``__`` must not resolve into a team whose name ends
with that segment.
+
+ Normalising ``_`` to the separator when building the team scoped name
(as the Azure
+ backend does) made team ``team_a`` asking for ``prod__conn`` build
exactly the name
+ team ``team_a--prod`` stores ``conn`` under.
+ """
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend, store = self._backend(mock_client)
+ victim = backend._build_team_secret_name(CONNECTIONS_PREFIX,
self.EXTENDING_TEAM, CONN_ID)
+ store[victim] = CONN_URI
+
+ assert backend.get_conn_value(conn_id=f"prod__{CONN_ID}",
team_name=self.TEAM) is None
+ # the two names must not coincide in the first place
+ assert backend._build_team_secret_name(CONNECTIONS_PREFIX, self.TEAM,
f"prod__{CONN_ID}") != victim
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_ambiguous_id_is_refused_even_for_its_own_team(self, mock_client,
mock_get_creds):
+ """
+ An id containing the separator is ambiguous in both directions, so it
is never resolved.
+
+ ``team_a`` with id ``b--c`` and ``team_a--b`` with id ``c`` name the
same secret, so
+ honouring the id for its own team would still hand one team the
other's secret.
+ """
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend, store = self._backend(mock_client)
+ ambiguous = f"prod{TEAM_SEP}{CONN_ID}"
+ store[backend._build_team_secret_name(CONNECTIONS_PREFIX, self.TEAM,
ambiguous)] = CONN_URI
+
+ assert backend.get_conn_value(conn_id=ambiguous, team_name=self.TEAM)
is None
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_refusing_an_ambiguous_id_is_logged(self, mock_client,
mock_get_creds, caplog):
+ """A silent ``None`` is indistinguishable from a missing secret, so
the refusal is logged."""
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend, _ = self._backend(mock_client)
+ ambiguous = f"prod{TEAM_SEP}{CONN_ID}"
+
+ assert backend.get_conn_value(conn_id=ambiguous) is None
+ assert backend.get_variable(key=ambiguous) is None
+
+ refusals = [r for r in caplog.records if "is ambiguous and is not
looked up" in r.getMessage()]
+ assert len(refusals) == 2
+ assert all(r.levelname == "WARNING" for r in refusals)
+ assert all(ambiguous in r.getMessage() for r in refusals)
+
+ @mock.patch(MODULE_NAME + ".get_credentials_and_project_id")
+ @mock.patch(MODULE_NAME + "._SecretManagerClient")
+ def test_config_lookup_is_not_team_scoped(self, mock_client,
mock_get_creds):
+ """
+ ``get_config`` has no team scope, so the namespace guard must not
apply to it.
+
+ Refusing config keys containing the separator would be stricter than
the Azure backend
+ this convention comes from, and there is no team namespace for a
config key to invade.
+ """
+ mock_get_creds.return_value = CREDENTIALS, PROJECT_ID
+ backend, store = self._backend(mock_client)
+ key = f"some{TEAM_SEP}option"
+ store[backend.build_path(CONFIG_PREFIX, key, SEP)] = "config-value"
+
+ assert backend.get_config(key) == "config-value"
