shahar1 commented on code in PR #75651:
URL: https://github.com/apache/airflow/pull/75651#discussion_r4494238758
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -572,6 +572,14 @@ def _adaptations(self, secret: str) -> Generator[str,
None, None]:
"""Yield the secret along with any adaptations to the secret that
should be masked."""
yield secret
+ # Formatters such as ``pprint`` (used by ``str()`` of Kubernetes
models) split a multi-line
+ # string across several literals, so the secret never appears
contiguously in the output.
+ lines = secret.splitlines()
+ if len(lines) > 1:
Review Comment:
A secret with only a trailing newline still stays readable, and covering it
takes a small change. `"token\n".splitlines()` returns one item, so `len(lines)
> 1` skips it. I ran this logic with `add_mask` on a token ending in `\n`, then
`redact(pprint.pformat({"v": token}))`: the token came through in clear. A
token read from a file often ends in a newline, so this is the same leak you
are fixing. The description lists it as a limit, but it does not have to be one:
```python
lines = secret.splitlines()
if lines != [secret]:
yield from map(str.strip, lines)
```
With this version the `pformat` output becomes `{'v': '***\n'}`, `\r\n`
works the same way, and `" error "` still registers only as itself.
`add_mask` already drops empty and too-short values, so the `if line.strip()`
guard is no longer needed. A `"token\n"` test would cover the new case.
##########
shared/secrets_masker/src/airflow_shared/secrets_masker/secrets_masker.py:
##########
@@ -572,6 +572,14 @@ def _adaptations(self, secret: str) -> Generator[str,
None, None]:
"""Yield the secret along with any adaptations to the secret that
should be masked."""
yield secret
+ # Formatters such as ``pprint`` (used by ``str()`` of Kubernetes
models) split a multi-line
+ # string across several literals, so the secret never appears
contiguously in the output.
+ lines = secret.splitlines()
Review Comment:
Is it intended that this also splits a pretty-printed connection `extra`?
`_mask_connection_secrets` in
`task-sdk/src/airflow/sdk/execution_time/context.py` calls
`mask_secret(conn.extra)` with no name, so the sensitive-name check does not
apply. The UI connection form formats extra with `JSON.stringify(parsed,
undefined, 2)`, and `post_connection` stores that string as is. Each line of
that JSON then becomes a global pattern.
I ran this logic on `{"region_name": "us-east-1", "timeout": 30, "verify":
true, "tags": ["prod", "error"]}` formatted with `indent=2`:
- `kwargs {"region_name": "us-east-1", "timeout": 30}` becomes `kwargs {***
"timeout": 30}`
- `"error" occurred` becomes `*** occurred`
`extra` is inside the masking contract in `security_model.rst`, and the
pprint form of a raw `extra` benefits from this, so it may be an acceptable
trade. The limits in the description cover common lines in a secret, but not
this case, where the masked text is ordinary configuration. Please add it to
the limits, or exclude this path if you think the noise outweighs the benefit.
A maintainer should weigh in on which.
--
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]
