Copilot commented on code in PR #66077:
URL: https://github.com/apache/airflow/pull/66077#discussion_r3368119817
##########
task-sdk/src/airflow/sdk/configuration.py:
##########
@@ -161,6 +161,35 @@ def __init__(
if default_config is not None:
self._update_defaults_from_string(default_config)
+ def mask_secrets(self) -> None:
+ """Mask sensitive config values in the secrets masker."""
+ from airflow.sdk._shared.configuration.exceptions import
AirflowConfigException
+ from airflow.sdk._shared.configuration.parser import
_build_kwarg_env_prefix, _collect_kwarg_env_vars
+ from airflow.sdk._shared.secrets_masker import mask_secret
+
+ for section, key in self.sensitive_config_values:
+ try:
+ with self.suppress_future_warnings():
+ value = self.get(section, key, suppress_warnings=True)
+ except AirflowConfigException:
+ log.debug(
+ "Could not retrieve value from section %s, for key %s.
Skipping redaction of this conf.",
+ section,
+ key,
+ )
+ continue
+ mask_secret(value)
+
+ # Mask per-key backend kwarg env vars
(AIRFLOW__SECRETS__BACKEND_KWARG__* etc.).
+ # These are not in sensitive_config_values but may contain sensitive
values.
+ for _section, _kwargs_key in [
+ ("secrets", "backend_kwargs"),
+ ("workers", "secrets_backend_kwargs"),
+ ]:
+ _prefix = _build_kwarg_env_prefix(_section, _kwargs_key)
+ for _value in _collect_kwarg_env_vars(_prefix).values():
+ mask_secret(_value)
Review Comment:
`mask_secrets()` currently only registers values with the Task SDK secrets
masker (`airflow.sdk._shared.secrets_masker`). In the reported failure mode
(user’s `airflow_local_settings` overwriting `airflow.settings.conf` with SDK
`conf`), `settings.initialize()` still configures *both* the core and SDK
maskers, and core logging continues to use the core masker. If secrets are not
also registered with `airflow._shared.secrets_masker`, sensitive config values
can leak into core logs even though `conf.mask_secrets()` was called.
##########
task-sdk/tests/task_sdk/test_configuration.py:
##########
@@ -184,3 +184,25 @@ def test_expands_env_var_in_airflow_home_fallback(self):
env = {"AIRFLOW_HOME": "$CUSTOM_DIR/airflow", "CUSTOM_DIR":
"/resolved"}
with mock.patch.dict("os.environ", env, clear=True):
assert get_airflow_config() == "/resolved/airflow/airflow.cfg"
+
+
+class TestAirflowSDKConfigParser:
+ @mock.patch("airflow.sdk._shared.secrets_masker.mask_secret")
+ def test_mask_secrets(self, mock_mask_secret):
+ from airflow.sdk.configuration import AirflowSDKConfigParser
+
+ parser = AirflowSDKConfigParser()
+
+ # Set a sensitive value
+ parser.sensitive_config_values.add(("test_section", "secret_key"))
+ if not parser.has_section("test_section"):
+ parser.add_section("test_section")
+ parser.set("test_section", "secret_key", "super_secret_value")
+
+ with mock.patch.dict(
+ "os.environ", {"AIRFLOW__SECRETS__BACKEND_KWARG__SOME_KEY":
"secret_kwarg_value"}
+ ):
+ parser.mask_secrets()
+
+ mock_mask_secret.assert_any_call("super_secret_value")
+ mock_mask_secret.assert_any_call("secret_kwarg_value")
Review Comment:
The new test only asserts that the SDK secrets masker is called. If
`AirflowSDKConfigParser.mask_secrets()` is meant to mirror the core
implementation and prevent secrets from leaking in *core* log redaction too,
this test should also verify that `airflow._shared.secrets_masker.mask_secret`
is invoked when available.
--
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]
