oscerd opened a new pull request, #1814:
URL: https://github.com/apache/camel-kafka-connector/pull/1814
Fixes #1797.
## What
`CamelKafkaConnectMain.Builder.build()` logs every initial property at INFO,
and `filterSensitive()`
decided what to mask from the property **key** alone:
```java
if (SensitiveUtils.containsSensitive((String) entry.getKey())) {
return entry.getKey() + "=xxxxxxx";
}
return entry.getKey() + "=" + entry.getValue();
```
That misses the values that actually carry the credentials.
`TaskHelper.buildUrl` folds every
`camel.sink.endpoint.*` / `camel.source.endpoint.*` option into one composed
URI and stores it under
`camel.kamelet.ckcSink.toUrl` / `camel.kamelet.ckcSource.fromUrl` — keys
containing no token
`SensitiveUtils` recognises. So an option correctly declared
`ConfigDef.Type.PASSWORD`, and dutifully
masked under its own key, was reproduced verbatim inside the composed URI on
the same line. The same
applies to credentials embedded in `camel.sink.url` / `camel.source.url`.
Here is the actual log line, taken from the new test running against the
unpatched code:
```
Setting initial properties in Camel context: [[camel.kamelet.ckcSink.toUrl=
aws2-kms://label?accessKey=AKIAEXAMPLEKEY&secretKey=sUp3rS3cr3tAccessValue®ion=eu-west-1,
...]]
```
## Fix
Run `String` values through `URISupport.sanitizeUri`, which masks userinfo
and the query parameters
`SensitiveUtils` matches. I chose this over blanket-masking the `toUrl` /
`fromUrl` keys so the
endpoint stays readable — knowing a task is pointed at
`aws2-kms://label?...®ion=eu-west-1` is
exactly what you want from that line when diagnosing a startup problem; only
the secret goes.
## Tests
`StartupPropertiesLoggingTest` attaches a log4j2 appender to the
`CamelKafkaConnectMain` logger and
asserts on the real emitted line. Five cases: the composed sink URI, the
composed source URI,
credentials as userinfo in `camel.sink.url`, key-based masking still
applying, and ordinary
configuration still being visible.
Three of the five fail against the unpatched code — the message quoted above
is that failure output.
The other two (`testKeyBasedMaskingStillApplies`,
`testNonSensitiveValuesAreStillLogged`) pass either
way by design: they guard the existing behaviour and guard against
over-masking, rather than
demonstrating the bug.
## Verification
- `core`: full suite passes (117 tests, +5).
- Full reactor build from the repository root (`./mvnw clean install
-DskipTests`): BUILD SUCCESS.
- Checkstyle: the new file is clean. `core` currently reports one violation
in `DataFormatTest` from
`225a5b7ab3`, which is unrelated to this PR and is fixed by #1813.
## Note on CI
This PR will show red until #1813 merges, for the reason above — PR CI
builds the merge commit
against `main`, and `main` is currently failing checkstyle. Nothing in this
branch causes it.
--
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]
