vatsrahul1001 commented on code in PR #74840:
URL: https://github.com/apache/airflow/pull/74840#discussion_r4293859359
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -588,9 +595,13 @@ def should_hide_value_for_key(self, name):
Name might be a Variable name, or key in conn.extra_dejson, for
example.
"""
if isinstance(name, str) and self.hide_sensitive_var_conn_fields:
- name = name.strip().lower()
- normalized = re.sub(r"\W+", "_", name)
- return any(s in normalized for s in
self.sensitive_variables_fields)
+ name = name.strip()
+ normalized = _NON_WORD_RE.sub("_", name.lower())
+ split = _CAMEL_ACRONYM_RE.sub(r"\1_\2",
_CAMEL_LOWER_UPPER_RE.sub(r"\1_\2", name))
Review Comment:
Nit: since this is a substring match with no word-end anchor, I think any
camelCase key that merely *starts with* a multi-word sensitive name now matches
too. Running the default fields over a few keys, `serviceAccountName`,
`accessKeyPath`, `authHeaderName`, `keyfileDictPath` and `webhookUrlTemplate`
all flip from visible to `***`.
`serviceAccountName` is the one I'd think about most, since it's all over
k8s pod specs / KubernetesPodOperator / executor config and would now be masked
in rendered templates, the UI and logs. `service_account_name` is already
masked in snake_case today, so this is arguably consistent, but it's a visible
change. Is that intended? If so a line in the newsfragment would help; if not,
an end anchor would be needed.
Also optional: this runs per dict key and is roughly 3x slower than before
in a quick micro-benchmark (about 0.8us -> 2.5us per call). Negligible in
absolute terms, so just mentioning it in case a small cache or precomputed
regex is easy.
##########
shared/secrets_masker/tests/secrets_masker/test_secrets_masker.py:
##########
@@ -855,6 +856,30 @@ class TestShouldHideValueForKey:
("auth.example.com/token", True),
("spark.executor.memory", False),
("spark.driver.cores", False),
+ # camelCase / PascalCase keys matching multi-word default fields.
+ ("privateKey", True),
+ ("PrivateKey", True),
+ ("privateKEY", True),
+ ("sshPrivateKey", True),
+ ("connectionString", True),
+ ("webhookUrl", True),
+ ("webhookURL", True),
+ ("slackWebhookUrl", True),
+ ("accessKey", True),
+ ("accessKeyId", True),
+ ("authHeader", True),
+ ("keyfileDict", True),
+ ("serviceAccount", True),
+ ("serviceKey", True),
+ ("apiKey", True),
+ ("APIKey", True),
+ ("accessToken", True),
+ ("proxyPassword", True),
+ # camelCase keys that do not contain a sensitive field stay
visible.
Review Comment:
Nit: the negatives here are `primaryKey`, `connectionTimeout`, `serviceName`
and `webhookName`, but nothing pins the prefix case (e.g.
`serviceAccountName`), so whichever way it's meant to behave it isn't
documented by a test. It might also be worth adding a digit boundary case like
`oauth2Token`, and a `redact`-level test on a nested dict with a camelCase key,
since everything here goes through `should_hide_value_for_key` directly.
##########
airflow-core/newsfragments/74840.bugfix.rst:
##########
@@ -0,0 +1 @@
+Redact values of camelCase and PascalCase keys (for example ``privateKey``,
``connectionString`` or ``webhookUrl``) that match multi-word sensitive field
names such as ``private_key``, ``connection_string`` or ``webhook_url``.
Review Comment:
Nit: this is labelled `bugfix`, but it also means more keys get hidden than
before (including some ordinary-looking ones, see my comment on
`secrets_masker.py`). Worth saying that explicitly, or checking whether the
project wants a different label for this kind of change.
--
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]
