do not print potentially sensitive properties

[pjfanning]

#85

[mdedetrich]

Ill have a look into this, I think it makes sense to mirror what is done upstream and there might be a handy function that does this.

[pjfanning]

I've added a shared function. If we find an existing function, that's fine but I didn't find one.

[pjfanning]

@mdedetrich the CVE is about logging properties. The properties are quite unlikely to be needed by people reading the logs. They can examine the configs to find them. If we really need to log properties, then maybe that can be looked at separately (there are a few ways to do this - maybe debug level logging).

[mdedetrich]

@pjfanning I am looking into this now, the main thing I want to investigate is that we redact the sensitive information in the same way that Kafka does so its not confusing

[mdedetrich]

I completely forgot about this, will try and have a look at the weekend

[pjfanning]

@mdedetrich would you have time to review this? My view is that toString is not core functionality. It is sometimes useful for debugging but we can still err on the side of being very cautious about what toString returns. Users who need the Kafka client properties have other ways to get them.

[mdedetrich]

Ill look at it today, didn't have time this weekend.

[mdedetrich]

@pjfanning So I just had a look at this and it appears that you are masking every single field in the .properties which doesn't appear to be correct, only sensitive fields should be masked which appears to already be done (see https://github.com/apache/incubator-pekko-connectors-kafka/pull/84/files#diff-2501d013cd63390d0b3cd227432636a1fc4b700c25f37b87cbccfa759f897e8dL630).

Am I missing something here?

[mdedetrich]

@pjfanning I managed to find a much better solution to this which defers to Kafka's own sensitive field/sanitization tooling so its always in sync with kafka, making PR now

[pjfanning]

@mdedetrich see akka/alpakka-kafka#1592 and the issue with sasl.jaas.config property

• we should not take risks
• only approved properties should be printed
• for me, why worry about even approved properties - why fluster about a toString? - better safe than sorry - I don't want us to get a CVE over some toString

[mdedetrich]

@mdedetrich see akka/alpakka-kafka#1592 and the issue with sasl.jaas.config property

Yes I know, Apache Kafka has written its own internal Config type which also gives to types for each key in the config. One of those types is org.apache.kafka.common.config.ConfigDef.Type.PASSWORD which is what Kafka uses to do its own masking. Have a look at https://kafka.apache.org/documentation/#consumerconfigs for more info, writing a PR now

[mdedetrich]

@pjfanning So I have made a PR at #100 that properly solves the CVE.

Note that my version uses the already existing inbuilt mechanism that upstream Kafka uses to filter out sensitive fields, which means its 100% correct when it comes to filtering out the correct properties and hence properly resolving the CVE

for me, why worry about even approved properties - why fluster about a toString? - better safe than sorry - I don't want us to get a CVE over some toString

Filtering out every single value from .properties makes the entire .toString almost pointless because its not even printing out the most useful properties that one needs.