Allow root certificates to be configured at run time of native image #2831
Conversation
|
Not sure why the checkstyle job is failing when I've explicitly disabled checkstyle on tha specific block of code: |
…an alternate location for root certificates at native image run time
|
Never mind, I think my latest push should solve that job failure. |
|
So the final checkstyle error that's remaining is: I would like to understand why |
| TrustStoreManagerSupport(Set<X509Certificate> trustedCerts, KeyStore trustedKeyStore) { | ||
| this.trustedCerts = trustedCerts; | ||
| this.trustedKeyStore = trustedKeyStore; | ||
| } | ||
|
|
||
| static boolean useEmbeddedCerts() { | ||
| if (truststoreSysPropSet != null) { |
There was a problem hiding this comment.
I don't think you can do this that easily:
- The
trustStoresystem property can be changed (set and unset) at any time at run time. You need to always reflect the current correct value. - Separating the check "is system property set" from the actual usage of the property in the JDK code has the problem of race conditions: if the system property is changed in between, the wrong value is used.
So I don't think you can have a proper implementation without deep integration intoTrustStoreManagerandTrustStoreDescriptorof the JDK.
|
|
||
| @Alias | ||
| static Target_sun_security_ssl_TrustStoreManager_TrustStoreDescriptor createInstance() { | ||
| return null; |
There was a problem hiding this comment.
Please always add comments for substituted methods what the original did vs. what the new version is doing. I cannot judge right now if returning null here (and in the methods below) is correct or not.
|
|
||
| // we do not need these paths (corresponding to the build time environment) to be carried over | ||
| // to the run time | ||
| @Alias @RecomputeFieldValue(kind = RecomputeFieldValue.Kind.Reset) private static String defaultStorePath = null; |
There was a problem hiding this comment.
I hope that with the proper integration of the "default store", these fields can actually be marked as @Delete rather than @RecomputeFieldValue. That would assure us that the JDK code for the default store is really properly replaced with the "store from the image heap".
|
@jaikiran @christianwimmer what's the status of this PR? |
|
Hello @matthyx, I've been swamped at work for the past several weeks and I am not sure when I can get back to discussing these suggestions from @christianwimmer.
Please do go ahead. I will be watching this PR and the repo and can help in small parts if/when needed, if time permits. |
|
@matthyx As I commented already on the PR, I do not think this PR contains a valid implementation. A proper solution is for sure a good bit of work. |
The commit in this PR introduces the "option#2" support that has been noted and requested in #1999.
More specifically, this commit now allows users to (optionally) specify a path to root certificates using the (standard)
javax.net.ssl.trustStoresystem property at run time, while launching the native application.Details about this change:
javax.net.ssl.trustStoresystem property. If it finds one, then instead of using the embedded certs, it goes ahead and uses the (existing) JRE infrastructure to parse and use the root certificates pointed to by value of that system property.sun.security.util.UntrustedCertificatesat build time. That class internally loads alib/security/blacklisted.certsfile, which is relative to the Java installation used at build time of the image. This file contains the blacklisted certificates and is used to validate the "trusted" certificate. The commit in this PR (intentionally) continues to allow this logic to stay and thus continues to use the build time blacklist file for decision making at run time. IMO, this should be fine because unlike new certificates getting added to a trust store, a blacklisted certificate rarely (is it ever?) is considered to be good again. So carrying over this information to the run time, IMO, is a good thing. However, there isn't a way to add new certificates to this blacklist at runtime and I don't think that's something that should be in the scope of this specific change.TrustStoreManagerFeaturewhich I don't fully understand:I have given a short glance at that
org.jcp.xml.dsig.internal.dom.XMLDSigRIto see if the change in this commit impacts in any way, that specific class and whether anything should be done to handle it. With my limited knowledge of that class, I haven't been able to decide if anything is needed for that one. So please do let me know if something needs to be done there.Finally, I've done some manual testing with this change. I used the following Java class to build a native image from:
Pretty trivial code which connects to an endpoint using HTTPS. With these change, I built the native image:
and then run the following tests:
(for each of these tests, I included
-Djavax.net.debug=trustmanager,certpathso that necessary log messages are printed to verify the trust management)-Djavax.net.ssl.trustStore, this should internally use the embedded certs and the program should successfully complete:(absence of logs from the "trustmanager" layer and a successfull completion is an indication that embedded root certs were used for trust management)
-Djavax.net.ssl.trustStore, this should be same as not passing that system property and should use the embedded certs:-Djavax.net.ssl.trustStore, this should use the root certs pointed to by the path that's passed to this property:-Djavax.net.ssl.trustStore, this should fail the program during SSL communication:-Djavax.net.ssl.trustStore, this should fail the program during SSL communication:/cc @christianwimmer