potiuk commented on code in PR #71711:
URL: https://github.com/apache/airflow/pull/71711#discussion_r4076988119
##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -516,26 +567,96 @@ def _get_spark_binary_path(self) -> list[str]:
def _mask_cmd(self, connection_cmd: str | list[str]) -> str:
# Mask any password related fields in application args with key value
pair
# where key contains password (case insensitive), e.g.
HivePassword='abc'
- connection_cmd_masked = re.sub(
- r"("
- r"\S*?" # Match all non-whitespace characters before...
- r"(?:secret|password)" # ...literally a "secret" or "password"
- # word (not capturing them).
- r"\S*?" # All non-whitespace characters before either...
- r"(?:=|\s+)" # ...an equal sign or whitespace characters
- # (not capturing them).
- r"(['\"]?)" # An optional single or double quote.
- r")" # This is the end of the first capturing group.
- r"(?:(?!\2\s).)*" # All characters between optional quotes
- # (matched above); if the value is quoted,
- # it may contain whitespace.
- r"(\2)", # Optional matching quote.
- r"\1******\3",
- " ".join(connection_cmd),
- flags=re.I,
- )
-
- return connection_cmd_masked
+ #
+ # Tokenise on whitespace so each token is examined once. For key=value
+ # tokens the unanchored _SENSITIVE_KV_RE handles multiple sensitive
keys
+ # inside a single token (e.g. ``Config(secret="x",password=y)``).
+ # Space-separated key/value pairs and quoted values that span multiple
+ # tokens are handled by manual lookahead. The overall scan is O(n).
+ if isinstance(connection_cmd, str):
+ connection_cmd = [connection_cmd]
+ cmd = " ".join(connection_cmd)
+ # re.split with a capturing group keeps the delimiters in the result
list,
+ # so we can reassemble the string without altering whitespace.
+ parts = re.split(r"(\s+)", cmd)
+ i = 0
+ while i < len(parts):
+ part = parts[i]
+ if part and not part.isspace() and
_SENSITIVE_KEYWORD_RE.search(part):
+ if "=" in part:
+ # key=value in the same token.
+ open_info = _find_open_quote_key(part)
+ if open_info is not None:
+ # The value's opening quote is not closed within this
token,
+ # so the quoted value spans multiple
whitespace-delimited
+ # tokens. Consume them until the closing quote, but
stop
+ # at newlines so an unterminated quote doesn't swallow
+ # subsequent log lines.
+ key_start, eq_idx, quote = open_info
+ before_key = part[:key_start]
+ key_eq = part[key_start : eq_idx + 1]
+
+ # Mask any earlier key=value pairs in the prefix.
+ if before_key and
_SENSITIVE_KEYWORD_RE.search(before_key):
+ before_key =
_SENSITIVE_KV_RE.sub(_mask_sensitive_kv, before_key)
+
+ found_close = False
+ j = i + 1
+ while j < len(parts):
+ if parts[j].isspace() and "\n" in parts[j]:
+ break
+ if not parts[j].isspace() and
parts[j].endswith(quote):
Review Comment:
**Blocking:** `endswith(quote)` only recognises a closing quote that is the
last character of the token, so `"x",`, `'x')` and `'x'}` never close the
value. The loop then runs to the newline and falls back to masking only the
first token, leaving the rest of the secret in the clear:
```
IN : Config(password="my pass word", user=x)
main: Config(password="******", user=x)
PR : Config(password=****** pass word", user=x)
IN : {'password': 'a b'}
main: {'password': '******'}
PR : {'password': ****** b'}
```
The second case goes through the space-separated branch at line 645, which
has the same check. Please pin both payloads in `test_masks_passwords`.
##########
providers/apache/spark/src/airflow/providers/apache/spark/hooks/spark_submit.py:
##########
@@ -55,6 +55,57 @@
_K8S_WAIT_APP_COMPLETION_CONF = "spark.kubernetes.submission.waitAppCompletion"
+# Quick check for whether a token might contain a sensitive key.
+_SENSITIVE_KEYWORD_RE = re.compile(r"secret|password", re.IGNORECASE)
+
+# Per-token pattern for key=value forms where the value is fully contained in
+# the token. Intentionally *unanchored* so that it finds multiple sensitive
+# keys inside a single whitespace-delimited token (e.g.
+# ``Config(secret="x",password=y)``). Running it only on tokens that pass
+# the cheap ``_SENSITIVE_KEYWORD_RE`` check keeps the overall scan O(n).
+_SENSITIVE_KV_RE = re.compile(
+ r"(\S*?(?:secret|password)\S*?=)"
Review Comment:
**Major:** because this is unanchored and `\S*?` can cross `=`, a token with
an `=` ahead of many keywords is still super-linear:
```
"a=" + "secret" * n n=100: 33 ms n=200: 244 ms n=400: 1.9 s n=800:
15 s
```
`main` is 4.5 s at n=400 on the same payload, so this is a constant-factor
gain here, not the O(n) that the comments at lines 65 and 575 claim. Keeping
`=` out of the key (e.g. `[^\s=]*?(?:secret|password)[^\s=]*?=`) keeps each
attempt bounded. A timing test on this payload would stop it regressing.
--
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]
