Re: [PR] Redact Connection extra fields that providers declare as secrets [airflow]

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

kaxil commented on code in PR #73797:
URL: https://github.com/apache/airflow/pull/73797#discussion_r4262244842


##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -54,24 +54,33 @@ def to_dict(self) -> dict[str, Any]: ...
         "access_token",
         "api_key",
         "apikey",
+        "app_key",
         "auth_header",
         "authorization",
         "bearer",
+        "certificate",
         "connection_string",
+        "credential",

Review Comment:
   These are substring matches with no scoping by connection type, so some catch fields that aren't secrets. `kube_config` matches `kube_config_path` (a plain string in the kubernetes provider.yaml), and `credential` matches google's `credential_config_file`. When a key matches, `add_mask` adds the value to the log patterns, so a kubeconfig path turns into `***` in task logs. And users can't take a name back out through `sensitive_var_conn_names`.
   
   I'd narrow these to exact field names. Using the `format: password` that provider.yaml already declares per field would get there too, and would also catch apprise `config`.



##########
airflow-core/tests/unit/always/test_providers_manager.py:
##########
@@ -263,6 +263,42 @@ def test_connection_form_widgets(self, yaml_ui_metadata_counts):
             # widgets loaded from YAML metadata only
             assert len(connections_form_widgets) == yaml_widgets
 
+    def test_sensitive_connection_fields_are_redactable(self):
+        """
+        Every connection field a provider declares as sensitive must be in the redaction allowlist.
+
+        Providers already state which fields are secret; this asserts ``DEFAULT_SENSITIVE_FIELDS``
+        never drifts apart from them.
+        """
+        pytest.importorskip("flask_appbuilder", reason="connection form widgets need flask_appbuilder")
+
+        from airflow._shared.secrets_masker import DEFAULT_SENSITIVE_FIELDS
+        from airflow._shared.secrets_masker.secrets_masker import SecretsMasker
+
+        # Check the shipped defaults rather than the process-wide masker, whose
+        # sensitive_variables_fields is only populated once settings have been configured.
+        masker = SecretsMasker()
+        masker.sensitive_variables_fields = list(DEFAULT_SENSITIVE_FIELDS)
+
+        # "config" is too generic a field name to add to the allowlist; apprise's
+        # "config" field is handled separately.

Review Comment:
   I couldn't find where apprise `config` is handled. `should_hide_value_for_key("config")` is False, so the connections API returns it unredacted inside `extra`, and the apprise docs put a webhook URL in it. Could the comment say this is a known unredacted secret, so the exemption doesn't read as already covered?



##########
airflow-core/tests/unit/always/test_providers_manager.py:
##########
@@ -263,6 +263,42 @@ def test_connection_form_widgets(self, yaml_ui_metadata_counts):
             # widgets loaded from YAML metadata only
             assert len(connections_form_widgets) == yaml_widgets
 
+    def test_sensitive_connection_fields_are_redactable(self):

Review Comment:
   `connection_form_widgets` goes through `_import_info_from_all_hooks()`, and the comment on the test above says whichever test hits that first can exceed the default 60s. Should this one get `@pytest.mark.execution_timeout(120)` as well, since xdist ordering can make it first on its worker?



##########
airflow-core/src/airflow/providers_manager.py:
##########
@@ -270,6 +270,21 @@ class ConnectionFormWidgetInfo(NamedTuple):
     is_sensitive: bool
 
 
+def _is_sensitive_form_field(field: Any) -> bool:

Review Comment:
   I don't think this changes anything. `field` is an unbound field, so `field.field_class.widget` is the class-level default widget, not the `widget=` kwarg the provider passed. For any `PasswordField` that is `PasswordInput` with `input_type == "password"`, so the asb `credential` was already `is_sensitive=True` before this PR. The `__mro__` branch only matters for a subclass, and subclasses are rejected earlier by the `widget.field_class not in allowed_field_classes` check. The docstring is off too: `StringField(widget=PasswordInput())` still comes out non-sensitive. Could we keep the original one-line expression, and drop the "regardless of the widget" sentence from the description?



##########
airflow-core/tests/unit/always/test_providers_manager.py:
##########
@@ -263,6 +263,42 @@ def test_connection_form_widgets(self, yaml_ui_metadata_counts):
             # widgets loaded from YAML metadata only
             assert len(connections_form_widgets) == yaml_widgets
 
+    def test_sensitive_connection_fields_are_redactable(self):
+        """
+        Every connection field a provider declares as sensitive must be in the redaction allowlist.
+
+        Providers already state which fields are secret; this asserts ``DEFAULT_SENSITIVE_FIELDS``
+        never drifts apart from them.
+        """
+        pytest.importorskip("flask_appbuilder", reason="connection form widgets need flask_appbuilder")
+
+        from airflow._shared.secrets_masker import DEFAULT_SENSITIVE_FIELDS

Review Comment:
   Nit: both are exported from `airflow._shared.secrets_masker`, so these can be one import at the top of the file.



-- 
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