jboss-container-images / openjdk

Source To Image (S2I) image for Red Hat OpenShift providing OpenJDK
Apache License 2.0
53 stars 58 forks source link

[OPENJDK-2833] Possible fix for OpenJDK image should scrub passwords from logs #466

Closed ammachado closed 2 months ago

ammachado commented 3 months ago

https://issues.redhat.com/browse/OPENJDK-2833


This PR masks password provided via Java parameters on a command line. This is a possible fix for https://issues.redhat.com/browse/CSB-3783.

Thanks for submitting your Pull Request!

Please make sure your PR meets the following requirements:

jmtd commented 3 months ago

If users are specifying sensitive values in environment variables, then detecting that in logs (even perfectly which I contend is impossible) and masking it there is only part of the problem. It will still be present in the environment wrt to the java process, and could end up written out in either logs or other outputs depending on situational context; that can be mitigated against in the run scripts (see e.g. https://github.com/jboss-container-images/openjdk/commit/4a2ae60747ff6ff3d1f70ae8cfc013c52063693f for a technique) but it will also be permanently enshrined in the container metadata, and I don't know of any way of addressing that.

Worse, masking it in logs might lead the user to believe it is properly protected in the other contexts when it isn't.

I believe OpenShift has a secrets system to manage injecting sensitive data into containers without these drawbacks. It's probably better that users rely on that than bare environment variables. We might need to extend the images to support bridging that functionality to e.g. javax.net.ssl.trustStorePassword. That needs some working through.

jmtd commented 2 months ago

@jerboaa I've made an adjustment and added a Behave test; please TAL!

jmtd commented 2 months ago

OK. trustStoreSecret=sensitive_string won't work, but alright.

That's true. It could be extended to that without much trouble. I haven't surveyed what the most common likely sensitive parameters will be named. I'll leave this as a follow-on.