Use a merged JVM+OS trust store as default SSLContext

[sratz]

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

[github-actions]

Test Results

 1 953 files  + 6   1 953 suites  +6   1h 22m 36s ⏱️ - 2m 7s
 4 744 tests + 9   4 720 ✅ + 9   24 💤 ±0  0 ❌ ±0 
14 232 runs  +27  14 050 ✅ +26  182 💤 +1  0 ❌ ±0 

Results for commit abf093d. ± Comparison against base commit 144d934.

♻️ This comment has been updated with latest results.

[laeubi]

@sratz you need suppress the illegal access here, the testfailure look suspicious...

[sratz]

@sratz you need suppress the illegal access here, the testfailure look suspicious...

The problem is

eclipse.platform/runtime/tests/org.eclipse.core.tests.runtime/src/org/eclipse/core/tests/internal/runtime/PlatformURLSessionTest.java

Lines 50 to 52 in 879c3bd

	@RegisterExtension
	SessionTestExtension extension = SessionTestExtension.forPlugin(PI_RUNTIME_TESTS)
	.withCustomization(SessionTestExtension.createCustomConfiguration().setCascaded().setReadOnly()).create();

which uses SessionTestExtension with CustomSessionConfigurationImpl

eclipse.platform/runtime/tests/org.eclipse.core.tests.harness/src/org/eclipse/core/tests/harness/session/customization/CustomSessionConfigurationImpl.java

Line 41 in 879c3bd

public class CustomSessionConfigurationImpl implements CustomSessionConfiguration {

I have added org.Mockito as a dependency, which now cannot be resolved with the default of set bundles configured by CustomSessionConfigurationImpl.

Where should I add those additional bundles? customize just PlatformURLSessionTest or adjust the default in CustomSessionConfigurationImpl?

[laeubi]

Mockito might be used in more tests in the future so I think you can decide what works best for you here.

[akurtakov]

I have just read eclipse-jdt/eclipse.jdt.core#4587 which somehow made me more cautious about mocking frameworks.

[HeikoKlare]

Where should I add those additional bundles? customize just PlatformURLSessionTest or adjust the default in CustomSessionConfigurationImpl?

If we can assume that every consumer of the session test framework has Mockito available, it would probably make sense to add it to the default bundles. Note that they are used in other projects as well (such as Equinox), but I guess all of them will have Mockito available.

[sratz]

I have just read eclipse-jdt/eclipse.jdt.core#4587 which somehow made me more cautious about mocking frameworks.

True, mocking concrete classes (especially final ones) / static mocks / etc. can be dangerous and are often result of a bad design. Rules of good design are typically:

• Limit mocking to interfaces
• Limit mocking to first-order / direct dependencies of the class-under-test.

In this case, X509Certificate is abstract with a bunch of abstract methods. Writing CollectionTrustManagerTest without mocks would be quite the overhead - especially since the tests does not care about the implementation at all.

[sratz]

I added the needed bundles to just the one test class needing them.

Alternatively, use custom stubs instead of mocks and the need for Mockito:

With vs. without mocks:
e4ca190...c72d4b3

I have no real preference, but the latter would avoid the dependency.

[laeubi]

@sratz I would say use what works best for you, do you plan to finish this for inclusion in M3?

[sratz]

@sratz I would say use what works best for you,

Then I'd say let's go with the latest state of the PR (explicit stubs) which avoids the hassle with new dependencies.

do you plan to finish this for inclusion in M3?

This is ready for review now.

It would be great to get it into 2025-12 - but of course if considered a too impactful of a change for M3 we can aim for 2026-03 M1.

[laeubi]

I would suggest using an flag to explicitly enable the feature what would make it save to be merged now.

If it proves to be bullet-proof we might enable it default later on.

[sratz]

As suggested, switched to an opt-in boolean system property eclipse.platform.mergeTrust instead, hence, the feature is disabled by default.

@merks @HeikoKlare @iloveeclipse @akurtakov:

• Any opinions on whether to get this into 2025-12 M3 or wait for 2026-03 M1?
• Any opinions on the property name?

[iloveeclipse]

• Any opinions on whether to get this into 2025-12 M3 or wait for 2026-03 M1?

I would prefer to wait for M1.

[merks]

Personally I'd rather see this move forward. Given it's opt-in, it seems safe. Note that I can test this in an Eclipse Installer as soon as it's available in an I-build, i.e., well before Friday.

[akurtakov]

As it's opt-in and has been one of the most often reported problems it's good to have it in this cycle IMO too.

[merks]

@sratz

I'd push the "update branch" button to rebase, but maybe that would be disruptive?

[laeubi]

+1 for have it in M3

[merks]

It looks property guarded.

[sratz]

OK, rebased + squashed locally to fix the 'committer' metadata and cleaned up some comments

[laeubi]

@sratz you have approval from two committers now an an implicit one from @akurtakov so I think you should merge this as soon as you feel comfortable with it.

[HeikoKlare]

+1 for M3 also from my side.

I cannot review the actual implementation, but it's properly implemented as full opt-out with no side effects other than instantiating the KeyStoreUtil class and evaluating one system property, so it should be safe to do.