shahar1 commented on code in PR #70878:
URL: https://github.com/apache/airflow/pull/70878#discussion_r3694826106
##########
providers/amazon/src/airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -239,6 +246,10 @@ def get_variable(self, key: str, team_name: str | None =
None) -> str | None:
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:
Review Comment:
`get_config` silently loses the guard. It was previously covered because the
check lived in `_get_secret`, which all three getters share; now that the check
has moved up into `get_conn_value` and `get_variable`, `get_config` reaches
`_get_secret` unguarded, so `get_config("a--b")` resolves `airflow/config/a--b`
where it used to return `None` with a warning.
There is no cross-team read here — `get_config` takes no `team_name`, so a
team scoped name is never built for it — and arguably config keys should never
have been caught by the old guard in the first place. But as it stands the new
`_names_a_team_namespace` docstring asserts such an id is *"refused for
**every** lookup"*, which is no longer true, and the loosening isn't mentioned
in the PR's "Behaviour change" section.
Either add the check to `get_config` too, or keep the exemption and say so
in both the docstring and the PR description.
##########
providers/amazon/src/airflow/providers/amazon/aws/secrets/secrets_manager.py:
##########
@@ -303,6 +314,33 @@ def _get_secret_value(self, secret_id: str, secrets_path:
str) -> str | None:
)
return None
+ def _names_a_team_namespace(self, secret_id: str) -> bool:
Review Comment:
Two small things on this helper:
1. The docstring says such an id is refused for *"**every** lookup -- team
scoped as well as team agnostic"*. That is true for connections and variables,
but `get_config` calls `_get_secret` without going through this check, so
config lookups are exempt (see the comment on `get_config`). Worth reconciling
the claim with the behaviour in whichever direction you pick.
2. It doesn't use `self` — `@staticmethod` fits, and matches core's
equivalent `EnvironmentVariablesBackend._is_team_specific_accessed_as_global`,
which is already a `@staticmethod`.
##########
providers/amazon/tests/unit/amazon/aws/secrets/test_secrets_manager.py:
##########
@@ -92,6 +92,54 @@ def
test_global_caller_cannot_access_team_scoped_connection(self):
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."""
+ backend = SecretsManagerBackend()
+
+ assert backend.get_conn_value(conn_id="prod--test_postgres") is None
+ assert backend.get_variable(key="prod--hello") is None
+
+ refusals = [r for r in caplog.records if "is ambiguous and is not
looked up" in r.getMessage()]
Review Comment:
This matches on the rendered log message, which the project's testing
standard steers away from:
> Do not assert on raw log text (`caplog.text` for example), these are
legacy string matching APIs planned for removal. Structured assertions via
`caplog` (which resolves to `cap_structlog` under structlog) are fine and
preferred: `"event name" in caplog` or `{"event": ..., "field": ...} in caplog`.
Beyond the standard, the coupling is what makes it worth changing: the
assertion pins the exact sentence *"is ambiguous and is not looked up"*, so any
rewording of `_log_refusal` breaks the test for a reason that has nothing to do
with the behaviour under test. A structured assertion on the event plus the
level would survive that.
Testing that the refusal is logged at all is a good call, though — a silent
`None` really is indistinguishable from a missing secret.
##########
providers/amazon/src/airflow/providers/amazon/aws/secrets/systems_manager.py:
##########
@@ -155,6 +162,10 @@ def get_variable(self, key: str, team_name: str | None =
None) -> str | None:
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:
Review Comment:
Same as in `secrets_manager.py`: `get_config` still calls `_get_secret`
directly, so it no longer gets the check that used to live there. Not a
cross-team read (no `team_name` is ever passed here), but it means the "refused
for every lookup" claim in `_names_a_team_namespace` doesn't hold, and the
loosening isn't in the PR's "Behaviour change" section.
##########
providers/amazon/src/airflow/providers/amazon/aws/secrets/systems_manager.py:
##########
@@ -182,6 +193,33 @@ def _get_parameter_value(self, ssm_path: str) -> str |
None:
self.log.debug("Parameter %s not found.", ssm_path)
return None
+ def _names_a_team_namespace(self, secret_id: str) -> bool:
Review Comment:
`@staticmethod` here too — no `self` use, and it matches core's
`EnvironmentVariablesBackend._is_team_specific_accessed_as_global`.
Also, this docstring and `_log_refusal` below are byte-identical to the
copies in `secrets_manager.py`. Not a blocker given the two backends already
mirror each other, but the rationale is long enough that one copy will
eventually get updated and the other won't.
--
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]
