Use a merged JVM+OS trust store as default SSLContext #1176
Conversation
| handleSplash(); | ||
|
|
||
| try { | ||
| new KeyStoreUtil().setUpSslContext(getOS()); |
There was a problem hiding this comment.
Should we not make it just static? If not
| new KeyStoreUtil().setUpSslContext(getOS()); | |
| new KeyStoreUtil(getOS()).setUpSslContext(); |
There was a problem hiding this comment.
The tests need to subclass, so I'll use the constructor approach.
|
Great to see progress on this. Please include copyright/license headers in the new *.java files. |
| // the location of the boot plugin we are going to use | ||
| handleSplash(); | ||
|
|
||
| try { |
There was a problem hiding this comment.
I think one would at least want a system property to disable this behavior without the need to configure an own keystore.
There was a problem hiding this comment.
Yes, I agree there should be an easy opt-out option.
There was a problem hiding this comment.
Makes sense. Any proposal for a property name? I see a bunch in org.eclipse.equinox.launcher.Main that just use eclipse. as prefix, so maybe eclipse.load.os.trust.store.by.default
There was a problem hiding this comment.
I think one would at least want a system property to disable this behavior without the need to configure an own keystore.
So, with the next update, an existing installatioin would silently start trusting more certs? 🤔
There was a problem hiding this comment.
So, with the next update, an existing installatioin would silently start trusting more certs?
I'm not sure I would word things that way. It will find more root certificates. During a past update, it went from finding them only in the JRE to finding them only in the windows root. That change was silent and one hoped it would find more roots that way, but apparent the windows root can be quite empty initially. With this change it will find them in both paces.
If you don't trust the OS's root certificates and/or the JRE's root certificates, you probably should not connect the internet at all.
|
Test execution on Windows and Mac now fails with No idea why. |
|
org.eclipse.osgi.internal.loader.EquinoxClassLoader@6622a690[junit-platform-engine:1.14.0(id=215)] |
|
There are two of these: 
So probably adding this to constrain the latter will help: 
Or maybe you have to constrain both, which wouldn't hurt. 😢 |
That works, but why is this necessary? Who's to blame here for having a too wide version range? |
|
The blame seemed to be in multiple places. E.g., the jdt junit 5 runtime had no upper bounds. A great many places had/have no bounds. I've not done an analysis of the junit/jupiter bundle dependencies themselves but one might hope those would prevent this. In any case, I don't fully understand the whole process of how these things are actually loaded, but it's a bit of a nightmare. And of course no PR is complete without some random infrastructure problems:
|
"KeychainStore" on macOS CI machine is likely empty.
| String p11KeyStore = "PKCS11"; | ||
| String none = "NONE"; |
There was a problem hiding this comment.
🤔 These are effectively constants, better to make them final, at least, or convert to real constants?
| equinoxConfig.setFrameworkArgs(frameworkArgs); | ||
| equinoxConfig.setAppArgs(appArgs); | ||
|
|
||
| try { |
There was a problem hiding this comment.
This is the wrong place to put SSL context setup if we want it to be universally available when the framework starts.
Having SSL context management built into the framework is not the proper solution. If it is determined SSL context must be setup and managed very early then it should be placed in a system bundle fragment. That is similar to our fragment org.eclipse.osgi.compatibility.state that the platform ships
That fragment is specific to Equinox with a host org.eclipse.osgi;bundle-version="3.12.0" but here you don't have any dependencies on Equinox specifically, so you can simply have a Fragment-Host value be system.bundle so it can be attached to any OSGi framework implementation and initialized as early as possible. Then you would specify a ExtensionBundle-Activator with your BundleActivator implementation in the fragment and that can make these calls to initialize the KeyStoreUtil.
With this fragment installed into any OSGi R8 framework implementation then the SSL context management would be initialized as early as possible when the framework implementation initializes. That is regardless of what launcher is being used to initialize and start the framework and that can be done with any framework implementation.
Also, there is no reason such a fragment must exist in Equinox. This could be completely managed by the Platform project itself (which is what I would prefer).
There was a problem hiding this comment.
so it can be attached to any OSGi framework implementation and initialized as early as possible
...
Then you would specify a ExtensionBundle-Activator with your BundleActivator implementation in the fragment and that can make these calls to initialize the KeyStoreUtil.
Just as a reference from the R8 spec it is described here https://docs.osgi.org/specification/osgi.core/8.0.0/framework.module.html#framework.module.extensionActivator
Also, there is no reason such a fragment must exist in Equinox. This could be completely managed by the Platform project itself (which is what I would prefer).
I see some benefits having it in Equinox as it is general purpose and not bound to any platform specific APIs.
There was a problem hiding this comment.
I see some benefits having it in Equinox as it is general purpose and not bound to any platform specific APIs.
Platform can certainly contain general purpose things, all our artifacts get published to maven central with the same group ID, so it makes no difference to the users what sub-project it is maintained at. My point is that Equinox doesn't have to be the place to maintain this and Equinox itself will not make any use of it internally so I would prefer it be placed somewhere else where the maintainers are using it.
There was a problem hiding this comment.
Yes I agree this is independent but with that argument we could have an almost empty repository except the org.eclipse.osgi :-)
So even if Equinox per se not is making use of this now it sounds like something useful in the context of OSGi (what is the mission statement of Equinox) and is at the same time also equally independent from platform (what mostly is connected to the RCP 3/4 platform and IDE) but I would simply lean to what works best.
Maybe in exchange we can move some other bundles to the platform instead :-)
There was a problem hiding this comment.
TBH I don't have that strong of a preference. I just want it somewhere that is easy to maintain by the authors that are going to maintain it.
My overall preference would be to move everything out of Equinox except things directly related to implementing the OSGi standard. That ship has sailed, but I still would rather have non-standard utility type APIs in some other Eclipse project.
There was a problem hiding this comment.
My overall preference would be to move everything out of Equinox except things directly related to implementing the OSGi standard.
That sounds reasonable
That ship has sailed
I think it is not hopeless, just create an issue for any particular bundle you like to get rid of an I'll try to take care to move it somewhere else, it will always take some time but we already have moved code for other reasons as well.
|
Moving to |
Merge JVM and OS trust stores in case opt-in system property eclipse.platform.mergeTrust is set and trust is not otherwise customized via the javax.net.ssl.trustStore[...] system properties. Resolves: https://bugs.eclipse.org/bugs/show_bug.cgi?id=567504 #1690 Obsoletes (to-be-reverted): eclipse-platform/eclipse.platform.releng.aggregator#929 eclipse-packaging/packages#224 Replaces: eclipse-equinox/equinox#1176 See also: eclipse-simrel/.github#38
Merge JVM and OS trust stores in case opt-in system property eclipse.platform.mergeTrust is set and trust is not otherwise customized via the javax.net.ssl.trustStore[...] system properties. Resolves: https://bugs.eclipse.org/bugs/show_bug.cgi?id=567504 #1690 Obsoletes (to-be-reverted): eclipse-platform/eclipse.platform.releng.aggregator#929 eclipse-packaging/packages#224 Replaces: eclipse-equinox/equinox#1176 See also: eclipse-simrel/.github#38
5 participants
Resolves:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=567504
eclipse-platform/eclipse.platform#1690
Obsoletes:
eclipse-platform/eclipse.platform.releng.aggregator#929
eclipse-packaging/packages#224
See also:
eclipse-simrel/.github#38