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 0191b1199ea Refuse the team agnostic fall-through for a team scoped
AWS secret name (#70878)
0191b1199ea is described below
commit 0191b1199ea08172dd583372b560b7293fa95d43
Author: Jarek Potiuk <[email protected]>
AuthorDate: Sat Aug 1 12:26:04 2026 +0200
Refuse the team agnostic fall-through for a team scoped AWS secret name
(#70878)
* Refuse the team agnostic fall-through for a team scoped AWS secret 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
* Refuse an ambiguous secret id before any AWS lookup runs
Refusing only the team agnostic fall-through left the team scoped lookup
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 lookup runs first, so the fall-through is never reached.
Deciding this at the entry points instead means the ambiguity is settled
before a name is built, in Secrets Manager and Parameter Store alike.
* Apply the ambiguous-id refusal to get_config as well
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 was
no longer true. Not a cross-team read -- get_config never receives a
team_name --
but the two getters and the docstring disagreed with the third.
Guard get_config too, and correct the docstring to name the three getters.
_names_a_team_namespace uses no instance state, so it becomes a
staticmethod.
The refusal-logging test asserted on the rendered sentence. Airflow logs
through
structlog, which renders format args into msg before the stdlib record
exists, so
record.args is empty and there is no structured payload to assert on;
assert on
level, logger and the refused id instead, which is what the test is actually
about, and extend it to cover get_config.
---
.../amazon/aws/secrets/secrets_manager.py | 55 ++++++++++++---
.../amazon/aws/secrets/systems_manager.py | 55 ++++++++++++---
.../amazon/aws/secrets/test_secrets_manager.py | 66 +++++++++++++++++-
.../amazon/aws/secrets/test_systems_manager.py | 78 +++++++++++++++++++++-
4 files changed, 236 insertions(+), 18 deletions(-)
diff --git
a/providers/amazon/src/airflow/providers/amazon/aws/secrets/secrets_manager.py
b/providers/amazon/src/airflow/providers/amazon/aws/secrets/secrets_manager.py
index ea922237424..415fc945910 100644
---
a/providers/amazon/src/airflow/providers/amazon/aws/secrets/secrets_manager.py
+++
b/providers/amazon/src/airflow/providers/amazon/aws/secrets/secrets_manager.py
@@ -28,6 +28,9 @@ from airflow.providers.amazon.aws.utils import
trim_none_values
from airflow.secrets import BaseSecretsBackend
from airflow.utils.log.logging_mixin import LoggingMixin
+# Separator between the team name and the secret id in a team scoped secret
name.
+TEAM_SEP = "--"
+
class SecretsManagerBackend(BaseSecretsBackend, LoggingMixin):
"""
@@ -207,6 +210,10 @@ class SecretsManagerBackend(BaseSecretsBackend,
LoggingMixin):
if self.connections_prefix is None:
return None
+ if self._names_a_team_namespace(conn_id):
+ self._log_refusal("connection", conn_id)
+ return None
+
secret = self._get_secret(
self.connections_prefix, conn_id, self.connections_lookup_pattern,
team_name
)
@@ -239,6 +246,10 @@ class SecretsManagerBackend(BaseSecretsBackend,
LoggingMixin):
if self.variables_prefix is None:
return None
+ if self._names_a_team_namespace(key):
+ self._log_refusal("variable", key)
+ return None
+
return self._get_secret(self.variables_prefix, key,
self.variables_lookup_pattern, team_name)
def get_config(self, key: str) -> str | None:
@@ -251,6 +262,10 @@ class SecretsManagerBackend(BaseSecretsBackend,
LoggingMixin):
if self.config_prefix is None:
return None
+ if self._names_a_team_namespace(key):
+ self._log_refusal("configuration option", key)
+ return None
+
return self._get_secret(self.config_prefix, key,
self.config_lookup_pattern)
def _get_secret_value(self, secret_id: str, secrets_path: str) -> str |
None:
@@ -303,6 +318,35 @@ class SecretsManagerBackend(BaseSecretsBackend,
LoggingMixin):
)
return None
+ @staticmethod
+ def _names_a_team_namespace(secret_id: str) -> bool:
+ """
+ Whether ``secret_id`` spells out a team scoped secret name.
+
+ A team scoped secret is named ``<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 by every
+ getter -- connections, variables and configuration options, team
scoped as well as team
+ agnostic -- because the ambiguity exists in both directions and the
caller's own
+ namespace is not a safe harbour 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.
+ """
+ return TEAM_SEP in secret_id
+
+ 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 _get_secret(
self, path_prefix, secret_id: str, lookup_pattern: str | None,
team_name: str | None = None
) -> str | None:
@@ -322,15 +366,10 @@ class SecretsManagerBackend(BaseSecretsBackend,
LoggingMixin):
lookup_pattern,
)
return None
- if team_name is None and re.fullmatch(r"[^-]+--.+", secret_id):
- self.log.warning(
- "Secret ID %r contains a '--' separator, which is reserved for
team-scoped lookups, "
- "but no team context was provided. Returning None.",
- secret_id,
- )
- return None
+ # The team scoped name is tried first. Ids that would make it name a
namespace other
+ # than the caller's own are refused by the callers before reaching
here.
if path_prefix and team_name:
- secrets_path = self.build_path(path_prefix,
f"{team_name}--{secret_id}", self.sep)
+ secrets_path = self.build_path(path_prefix,
f"{team_name}{TEAM_SEP}{secret_id}", self.sep)
value = self._get_secret_value(secret_id, secrets_path)
if value is not None:
return value
diff --git
a/providers/amazon/src/airflow/providers/amazon/aws/secrets/systems_manager.py
b/providers/amazon/src/airflow/providers/amazon/aws/secrets/systems_manager.py
index ed99d5f6ccd..5cae97670b7 100644
---
a/providers/amazon/src/airflow/providers/amazon/aws/secrets/systems_manager.py
+++
b/providers/amazon/src/airflow/providers/amazon/aws/secrets/systems_manager.py
@@ -26,6 +26,9 @@ from airflow.providers.amazon.aws.utils import
trim_none_values
from airflow.secrets import BaseSecretsBackend
from airflow.utils.log.logging_mixin import LoggingMixin
+# Separator between the team name and the secret id in a team scoped secret
name.
+TEAM_SEP = "--"
+
class SystemsManagerParameterStoreBackend(BaseSecretsBackend, LoggingMixin):
"""
@@ -142,6 +145,10 @@ class
SystemsManagerParameterStoreBackend(BaseSecretsBackend, LoggingMixin):
if self.connections_prefix is None:
return None
+ 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,
self.connections_lookup_pattern, team_name)
def get_variable(self, key: str, team_name: str | None = None) -> str |
None:
@@ -155,6 +162,10 @@ class
SystemsManagerParameterStoreBackend(BaseSecretsBackend, LoggingMixin):
if self.variables_prefix is None:
return None
+ if self._names_a_team_namespace(key):
+ self._log_refusal("variable", key)
+ return None
+
return self._get_secret(self.variables_prefix, key,
self.variables_lookup_pattern, team_name)
def get_config(self, key: str) -> str | None:
@@ -167,6 +178,10 @@ class
SystemsManagerParameterStoreBackend(BaseSecretsBackend, LoggingMixin):
if self.config_prefix is None:
return None
+ if self._names_a_team_namespace(key):
+ self._log_refusal("configuration option", key)
+ return None
+
return self._get_secret(self.config_prefix, key,
self.config_lookup_pattern)
def _get_parameter_value(self, ssm_path: str) -> str | None:
@@ -182,6 +197,35 @@ class
SystemsManagerParameterStoreBackend(BaseSecretsBackend, LoggingMixin):
self.log.debug("Parameter %s not found.", ssm_path)
return None
+ @staticmethod
+ def _names_a_team_namespace(secret_id: str) -> bool:
+ """
+ Whether ``secret_id`` spells out a team scoped secret name.
+
+ A team scoped secret is named ``<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 by every
+ getter -- connections, variables and configuration options, team
scoped as well as team
+ agnostic -- because the ambiguity exists in both directions and the
caller's own
+ namespace is not a safe harbour 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.
+ """
+ return TEAM_SEP in secret_id
+
+ 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 _get_secret(
self, path_prefix: str, secret_id: str, lookup_pattern: str | None,
team_name: str | None = None
) -> str | None:
@@ -201,15 +245,10 @@ class
SystemsManagerParameterStoreBackend(BaseSecretsBackend, LoggingMixin):
lookup_pattern,
)
return None
- if team_name is None and re.fullmatch(r"[^-]+--.+", secret_id):
- self.log.warning(
- "Secret ID %r contains a '--' separator, which is reserved for
team-scoped lookups, "
- "but no team context was provided. Returning None.",
- secret_id,
- )
- return None
+ # The team scoped name is tried first. Ids that would make it name a
namespace other
+ # than the caller's own are refused by the callers before reaching
here.
if team_name:
- ssm_path = self.build_path(path_prefix,
f"{team_name}--{secret_id}")
+ ssm_path = self.build_path(path_prefix,
f"{team_name}{TEAM_SEP}{secret_id}")
ssm_path = self._ensure_leading_slash(ssm_path)
value = self._get_parameter_value(ssm_path)
if value is not None:
diff --git
a/providers/amazon/tests/unit/amazon/aws/secrets/test_secrets_manager.py
b/providers/amazon/tests/unit/amazon/aws/secrets/test_secrets_manager.py
index 8eb7ec49cb9..5df7ee72d93 100644
--- a/providers/amazon/tests/unit/amazon/aws/secrets/test_secrets_manager.py
+++ b/providers/amazon/tests/unit/amazon/aws/secrets/test_secrets_manager.py
@@ -16,12 +16,13 @@
# under the License.
from __future__ import annotations
+import logging
from unittest import mock
import pytest
from moto import mock_aws
-from airflow.providers.amazon.aws.secrets.secrets_manager import
SecretsManagerBackend
+from airflow.providers.amazon.aws.secrets.secrets_manager import TEAM_SEP,
SecretsManagerBackend
class TestSecretsManagerBackend:
@@ -92,6 +93,69 @@ class TestSecretsManagerBackend:
assert
secrets_manager_backend.get_conn_value(conn_id="my_team--test_postgres") is None
+ @mock_aws
+ def test_another_teams_secret_is_not_reachable(self):
+ """A caller scoped to one team must not reach another team's secret by
naming it."""
+ secret_id = "airflow/connections/my_team--test_postgres"
+ create_param = {"Name": secret_id, "SecretString":
"postgresql://airflow:airflow@host:5432/airflow"}
+ backend = SecretsManagerBackend()
+ backend.client.create_secret(**create_param)
+
+ assert backend.get_conn_value(conn_id="my_team--test_postgres",
team_name="other_team") is None
+
+ @mock_aws
+ def test_team_whose_name_extends_the_callers_is_not_reachable(self):
+ """A prefix match on the caller's own namespace is not proof of
ownership."""
+ secret_id = "airflow/connections/my_team--prod--test_postgres"
+ create_param = {"Name": secret_id, "SecretString":
"postgresql://airflow:airflow@host:5432/airflow"}
+ backend = SecretsManagerBackend()
+ backend.client.create_secret(**create_param)
+
+ assert backend.get_conn_value(conn_id="my_team--prod--test_postgres",
team_name="my_team") is None
+
+ @mock_aws
+ def test_team_scoped_lookup_cannot_reach_a_longer_teams_namespace(self):
+ """The team scoped name is not safe by construction -- the id can
extend it.
+
+ Team ``my_team`` asking for ``prod--test_postgres`` builds exactly the
name team
+ ``my_team--prod`` builds for ``test_postgres``, so the team scoped
lookup *hits*
+ another team's secret. Refusing only the team agnostic fall-through
leaves this
+ open, because the fall-through is never reached.
+ """
+ secret_id = "airflow/connections/my_team--prod--test_postgres"
+ create_param = {"Name": secret_id, "SecretString":
"postgresql://airflow:airflow@host:5432/airflow"}
+ backend = SecretsManagerBackend()
+ backend.client.create_secret(**create_param)
+
+ assert backend.get_conn_value(conn_id="prod--test_postgres",
team_name="my_team") is None
+
+ @mock_aws
+ def test_refusing_an_ambiguous_id_is_logged(self, caplog):
+ """A silent ``None`` is indistinguishable from a missing secret, so
the refusal is logged.
+
+ Asserted on the record's structured ``args`` rather than the rendered
message, so
+ rewording the warning does not silently stop this from testing
anything.
+ """
+ backend = SecretsManagerBackend()
+
+ assert backend.get_conn_value(conn_id="prod--test_postgres") is None
+ assert backend.get_variable(key="prod--hello") is None
+ assert backend.get_config(key="prod--sql_alchemy_conn") is None
+
+ # Airflow logs through structlog, which renders the format args into
``msg`` before the
+ # stdlib record is built -- ``record.args`` is empty, so there is no
structured payload
+ # to assert on. Assert on level, logger and the refused id (the
load-bearing data)
+ # rather than the wording, so rephrasing the warning does not break
this.
+ refusals = [
+ r
+ for r in caplog.records
+ if r.levelno == logging.WARNING and
r.name.endswith(type(backend).__name__)
+ ]
+ assert len(refusals) == 3
+ for refused_id in ("prod--test_postgres", "prod--hello",
"prod--sql_alchemy_conn"):
+ assert sum(refused_id in r.msg for r in refusals) == 1
+ assert all(TEAM_SEP in r.msg for r in refusals)
+
@mock_aws
def test_team_caller_falls_back_to_global_connection(self):
secret_id = "airflow/connections/test_postgres"
diff --git
a/providers/amazon/tests/unit/amazon/aws/secrets/test_systems_manager.py
b/providers/amazon/tests/unit/amazon/aws/secrets/test_systems_manager.py
index 2c318151f4e..ae1719a570b 100644
--- a/providers/amazon/tests/unit/amazon/aws/secrets/test_systems_manager.py
+++ b/providers/amazon/tests/unit/amazon/aws/secrets/test_systems_manager.py
@@ -17,13 +17,17 @@
from __future__ import annotations
import json
+import logging
from unittest import mock
import pytest
from moto import mock_aws
from airflow.configuration import initialize_secrets_backends
-from airflow.providers.amazon.aws.secrets.systems_manager import
SystemsManagerParameterStoreBackend
+from airflow.providers.amazon.aws.secrets.systems_manager import (
+ TEAM_SEP,
+ SystemsManagerParameterStoreBackend,
+)
from tests_common.test_utils.config import conf_vars
@@ -123,6 +127,78 @@ class TestSsmSecrets:
ssm_backend.client.put_parameter(**param)
assert ssm_backend.get_conn_value(conn_id="my_team--test_postgres") is
None
+ @mock_aws
+ def test_another_teams_secret_is_not_reachable(self):
+ """A caller scoped to one team must not reach another team's parameter
by naming it."""
+ param = {
+ "Name": "/airflow/connections/my_team--test_postgres",
+ "Type": "String",
+ "Value": "postgresql://airflow:airflow@host:5432/airflow",
+ }
+ ssm_backend = SystemsManagerParameterStoreBackend()
+ ssm_backend.client.put_parameter(**param)
+
+ assert ssm_backend.get_conn_value(conn_id="my_team--test_postgres",
team_name="other_team") is None
+
+ @mock_aws
+ def test_team_whose_name_extends_the_callers_is_not_reachable(self):
+ """A prefix match on the caller's own namespace is not proof of
ownership."""
+ param = {
+ "Name": "/airflow/connections/my_team--prod--test_postgres",
+ "Type": "String",
+ "Value": "postgresql://airflow:airflow@host:5432/airflow",
+ }
+ ssm_backend = SystemsManagerParameterStoreBackend()
+ ssm_backend.client.put_parameter(**param)
+
+ assert
ssm_backend.get_conn_value(conn_id="my_team--prod--test_postgres",
team_name="my_team") is None
+
+ @mock_aws
+ def test_team_scoped_lookup_cannot_reach_a_longer_teams_namespace(self):
+ """The team scoped name is not safe by construction -- the id can
extend it.
+
+ Team ``my_team`` asking for ``prod--test_postgres`` builds exactly the
path team
+ ``my_team--prod`` builds for ``test_postgres``, so the team scoped
lookup *hits*
+ another team's parameter. Refusing only the team agnostic fall-through
leaves this
+ open, because the fall-through is never reached.
+ """
+ param = {
+ "Name": "/airflow/connections/my_team--prod--test_postgres",
+ "Type": "String",
+ "Value": "postgresql://airflow:airflow@host:5432/airflow",
+ }
+ ssm_backend = SystemsManagerParameterStoreBackend()
+ ssm_backend.client.put_parameter(**param)
+
+ assert ssm_backend.get_conn_value(conn_id="prod--test_postgres",
team_name="my_team") is None
+
+ @mock_aws
+ def test_refusing_an_ambiguous_id_is_logged(self, caplog):
+ """A silent ``None`` is indistinguishable from a missing secret, so
the refusal is logged.
+
+ Asserted on the record's structured ``args`` rather than the rendered
message, so
+ rewording the warning does not silently stop this from testing
anything.
+ """
+ backend = SystemsManagerParameterStoreBackend()
+
+ assert backend.get_conn_value(conn_id="prod--test_postgres") is None
+ assert backend.get_variable(key="prod--hello") is None
+ assert backend.get_config(key="prod--sql_alchemy_conn") is None
+
+ # Airflow logs through structlog, which renders the format args into
``msg`` before the
+ # stdlib record is built -- ``record.args`` is empty, so there is no
structured payload
+ # to assert on. Assert on level, logger and the refused id (the
load-bearing data)
+ # rather than the wording, so rephrasing the warning does not break
this.
+ refusals = [
+ r
+ for r in caplog.records
+ if r.levelno == logging.WARNING and
r.name.endswith(type(backend).__name__)
+ ]
+ assert len(refusals) == 3
+ for refused_id in ("prod--test_postgres", "prod--hello",
"prod--sql_alchemy_conn"):
+ assert sum(refused_id in r.msg for r in refusals) == 1
+ assert all(TEAM_SEP in r.msg for r in refusals)
+
@mock_aws
def test_team_caller_falls_back_to_global_connection(self):
param = {
