TSFenwick commented on PR #15485: URL: https://github.com/apache/druid/pull/15485#issuecomment-1845812813
> We can extend the masking logic in [getMaskedCommand](https://github.com/apache/druid/blob/master/indexing-service/src/main/java/org/apache/druid/indexing/overlord/ForkingTaskRunner.java#L696) to also account for parsing value strings, i.e., splits[1] (as with sasl.jaas.config). To keep it simple, we can consider only values of type Map for now. I like this idea, but i think it becomes more complicated when needing to consider generic things, like how far down a map should we go. Also the less handwritten parsing code i write the better for everyone :) The other question is defining the values to be masked in core druid code isn't probably the best. Using the dynamic config provider means it can mask any and all values stored in the configProvider's map. I think the solution i have implemented in this PR suffices enough and moves the needle forward in protecting secrets. Also this change follows the practices Druid uses elsewhere to secure maps of values like in the Kafka consumer https://github.com/apache/druid/blob/master/extensions-core/kafka-indexing-service/src/main/java/org/apache/druid/indexing/kafka/KafkaRecordSupplier.java#L263 -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
