Re: [PR] Refuse the team agnostic fall-through for a team scoped AWS secret name [airflow]

["shahar1 (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org