You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "amoghrajesh (via GitHub)" <gi...@apache.org> on 2026/08/04 09:55:55 UTC
[PR] Only refuse team scoped like secret ids when multi_team is on [airflow]
amoghrajesh opened a new pull request, #71078:
URL: https://github.com/apache/airflow/pull/71078
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
Related to (#70878), (#70876), (#70899), (#70869), (#70877), (#70736)
## Summary
- Azure Key Vault, AWS Secrets Manager, AWS SSM, GCP Secret Manager, and Yandex Lockbox all refuse to look up any id that contains the team-namespace separator, to avoid ambiguity with a team-scoped secret name.
- This runs even when multi-team mode is off, which is the default for almost every deployment. Without multi-team mode, no team-scoped secret can exist, so there's no ambiguity to protect against - it just silently breaks ordinary ids
- On Azure specifically, an id is compared against its underscore-to-dash normalized form, so an ordinary
connection id containing a double underscore (e.g. aws__prod) was silently refused as if it were a missing secret: https://github.com/apache/airflow/blob/main/providers/microsoft/azure/src/airflow/providers/microsoft/azure/secrets/key_vault.py#L230-L251
Trying to gate this by only run the check when `[core] multi_team` is enabled, same as Airflow already does at the model layer.
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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
Re: [PR] Only refuse team scoped like secret ids when multi_team is on [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #71078:
URL: https://github.com/apache/airflow/pull/71078#discussion_r3713597888
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -248,7 +248,12 @@ def _names_a_team_namespace(self, secret_id: str) -> bool:
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.
+
+ Only checked in multi-team mode: ``team_name`` is never non-``None`` otherwise, so no
+ team scoped secret can exist to collide with.
"""
+ if not conf.getboolean("core", "multi_team", fallback=False):
Review Comment:
Thanks, here you go: https://github.com/apache/airflow/pull/71106
##########
providers/microsoft/azure/tests/unit/microsoft/azure/secrets/test_key_vault.py:
##########
@@ -215,6 +224,17 @@ def test_refusing_an_ambiguous_id_is_logged(self, mock_client, caplog):
assert sum(refused_id in r.getMessage() for r in refusals) == 1
mock_client.get_secret.assert_not_called()
+ @mock.patch(f"{KEY_VAULT_MODULE}.AzureKeyVaultBackend.client")
+ def test_ambiguous_id_resolves_when_multi_team_is_disabled(self, mock_client):
+ """No team scoped secret can exist without multi-team mode, so there is no ambiguity
+ to refuse -- an ordinary id containing the separator must resolve normally."""
+ mock_client.get_secret.return_value = mock.Mock(value="world")
+ backend = AzureKeyVaultBackend()
+
+ assert backend.get_conn_value("prod--my_db") == "world"
Review Comment:
Handled in https://github.com/apache/airflow/pull/71106
##########
providers/amazon/tests/unit/amazon/aws/secrets/test_systems_manager.py:
##########
@@ -116,6 +116,7 @@ def test_get_conn_value_with_team_name(self):
returned_uri = ssm_backend.get_conn_value(conn_id="test_postgres", team_name="my_team")
assert returned_uri == "postgresql://airflow:airflow@host:5432/airflow"
+ @conf_vars({("core", "multi_team"): "True"})
Review Comment:
Handled in https://github.com/apache/airflow/pull/71106
--
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
Re: [PR] Only refuse team scoped like secret ids when multi_team is on [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #71078:
URL: https://github.com/apache/airflow/pull/71078#issuecomment-5177516967
Here's the companion PR for env backend: https://github.com/apache/airflow/pull/71079
--
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
Re: [PR] Only refuse team scoped like secret ids when multi_team is on [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #71078:
URL: https://github.com/apache/airflow/pull/71078#issuecomment-5177496955
@potiuk this touches providers only, so no need to backport.
--
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
Re: [PR] Only refuse team scoped like secret ids when multi_team is on [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #71078:
URL: https://github.com/apache/airflow/pull/71078#discussion_r3713408479
##########
providers/google/src/airflow/providers/google/cloud/secrets/secret_manager.py:
##########
@@ -248,7 +248,12 @@ def _names_a_team_namespace(self, secret_id: str) -> bool:
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.
+
+ Only checked in multi-team mode: ``team_name`` is never non-``None`` otherwise, so no
+ team scoped secret can exist to collide with.
"""
+ if not conf.getboolean("core", "multi_team", fallback=False):
Review Comment:
The Google backend docs still say the opposite. `providers/google/docs/secrets-backends/google-cloud-secret-manager-backend.rst:236` warns that the refusal "applies whether or not you use teams" and that an id already containing `--` "stops resolving after upgrading and you must rename it". That rename advice is wrong after this change, and the new failure mode (an id with `--` resolves fine until someone enables `multi_team`, then silently stops) isn't documented anywhere. Can this PR update that section?
##########
providers/microsoft/azure/src/airflow/providers/microsoft/azure/secrets/key_vault.py:
##########
@@ -247,7 +248,12 @@ def _names_a_team_namespace(self, secret_id: str) -> bool:
The id is normalised first because :meth:`build_path` maps ``_`` onto the separator
everywhere in this backend, so ``b__c`` reaches Key Vault as ``b--c`` and would
otherwise manufacture the team separator from an id that does not visibly contain it.
+
+ Only checked in multi-team mode: ``team_name`` is never non-``None`` otherwise, so no
+ team scoped secret can exist to collide with.
"""
+ if not conf.getboolean("core", "multi_team", fallback=False):
Review Comment:
`get_config` runs through this guard too (line 193), but config lookups are never team-scoped: `_get_secret(self.config_prefix, key)` passes no `team_name`, so a config key has no team-scoped name to collide with in either mode. The key here is the operator-chosen path from `<option>_secret`, and this backend maps `_` onto the separator, so a path named after the env var (`airflow__database__sql_alchemy_conn` becomes `airflow--database--sql-alchemy-conn`) is still refused whenever `multi_team` is on. Google and Yandex never guarded `get_config`, and the Google docs state config lookups are unaffected, so dropping it from `get_config` here and in the two AWS backends would make all five agree. Happy for that to be a follow-up if you want to keep this PR narrow.
##########
providers/microsoft/azure/tests/unit/microsoft/azure/secrets/test_key_vault.py:
##########
@@ -215,6 +224,17 @@ def test_refusing_an_ambiguous_id_is_logged(self, mock_client, caplog):
assert sum(refused_id in r.getMessage() for r in refusals) == 1
mock_client.get_secret.assert_not_called()
+ @mock.patch(f"{KEY_VAULT_MODULE}.AzureKeyVaultBackend.client")
+ def test_ambiguous_id_resolves_when_multi_team_is_disabled(self, mock_client):
+ """No team scoped secret can exist without multi-team mode, so there is no ambiguity
+ to refuse -- an ordinary id containing the separator must resolve normally."""
+ mock_client.get_secret.return_value = mock.Mock(value="world")
+ backend = AzureKeyVaultBackend()
+
+ assert backend.get_conn_value("prod--my_db") == "world"
Review Comment:
`get_secret` returns `world` for any name here, so these pass regardless of the name the backend computes, which is the part the `prod__hello` case in the description turns on. The tests above assert the resolved name (lines 45, 88, 111). Worth at least `mock_client.get_secret.assert_any_call(name="airflow-variables-prod--hello")` for the `prod__hello` lookup.
##########
providers/amazon/tests/unit/amazon/aws/secrets/test_systems_manager.py:
##########
@@ -116,6 +116,7 @@ def test_get_conn_value_with_team_name(self):
returned_uri = ssm_backend.get_conn_value(conn_id="test_postgres", team_name="my_team")
assert returned_uri == "postgresql://airflow:airflow@host:5432/airflow"
+ @conf_vars({("core", "multi_team"): "True"})
Review Comment:
The other four test files in this PR define `multi_team_enabled = conf_vars({("core", "multi_team"): "True"})` at module level. Worth doing the same here rather than repeating the literal on six tests.
--
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
Re: [PR] Only refuse team scoped like secret ids when multi_team is on [airflow]
Posted by "vatsrahul1001 (via GitHub)" <gi...@apache.org>.
vatsrahul1001 merged PR #71078:
URL: https://github.com/apache/airflow/pull/71078
--
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