Skip to content

Bug 567504 - Use system trust store on Windows instead of cacerts #929

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

sratz
Copy link
Member

@sratz sratz commented Mar 3, 2023

Use the Windows operating system trust store instead of the cacerts bundled with the JVM.

https://bugs.eclipse.org/bugs/show_bug.cgi?id=567504

@sratz sratz requested review from merks and jonahgraham March 3, 2023 10:49
@merks
Copy link
Contributor

merks commented Mar 3, 2023

Do we generally expect that the Windows trust store will be more up-to-date? I.e., is there a possibility that a recently published Termurin-based JRE would have root certificates not in the Windows trust store?

@sratz
Copy link
Member Author

sratz commented Mar 3, 2023

Do we generally expect that the Windows trust store will be more up-to-date? I.e., is there a possibility that a recently published Termurin-based JRE would have root certificates not in the Windows trust store?

That's certainly a possibility but unlikely that this should cause any trouble. Microsoft regularly updates the trusted roots:
https://learn.microsoft.com/en-us/security/trusted-root/release-notes

I guess it comes down to what expectations of users we want to fulfill. The operating system trust store is the one users are used to (e.g. in browsers) and what they probably expect to be used in arbitrary 3rd party applications. It may also contains corporate specific certificates.

Our experience is that it is quite unintuitive that the EPP packages come with their own cacerts ('Why does it work in my browser but not in Eclipse?')

@merks
Copy link
Contributor

merks commented Mar 3, 2023

This issue of certificates used by a corporate firewall is quite a common problem and with an embedded JRE this is even harder for users to fix, especially for the installer where the whole thing is packaged as a *.exe...

@merks
Copy link
Contributor

merks commented Apr 16, 2023

@sratz

Note that I tried these options in the installer, but the result just hangs trying to access the internet. I have not had time to track down what might be causing that. Maybe the installer is special because it extracts itself into the temp folder. Using the options with a debug launch works fine...

@sratz
Copy link
Member Author

sratz commented May 3, 2023

@sratz

Note that I tried these options in the installer, but the result just hangs trying to access the internet. I have not had time to track down what might be causing that. Maybe the installer is special because it extracts itself into the temp folder. Using the options with a debug launch works fine...

I went trough the JDK sources and apparently the correct properties to set actually are

-Djavax.net.ssl.trustStoreType=Windows-ROOT -Djavax.net.ssl.trustStore=NONE

instead of

-Djavax.net.ssl.trustStoreType=Windows-ROOT -Djavax.net.ssl.trustStore=NUL

Can you see if using NONE helps in the Installer?

@merks
Copy link
Contributor

merks commented May 7, 2023

Yes, the installer works with this in the product:

      <vmArgsWin>-Djavax.net.ssl.trustStoreType=Windows-ROOT
-Djavax.net.ssl.trustStore=NONE
      </vmArgsWin>

@akurtakov
Copy link
Member

Is there anything still planned here?

@merks
Copy link
Contributor

merks commented Aug 9, 2023

I've not changed this for the installer because I'm kind of scared to do that because this isn't something I can really test...

@akurtakov
Copy link
Member

So should this one be closed if nothing will be done?

@merks
Copy link
Contributor

merks commented Nov 28, 2023

I do get the feeling this is a correct and good thing. I’ve seen more than once this solves problems inside firewalls.

@BeckerWdf
Copy link
Contributor

@sratz: Should we merge this?

@sratz sratz force-pushed the windows-truststore branch from 0e47035 to 583c1db Compare December 4, 2023 10:26
@sratz
Copy link
Member Author

sratz commented Dec 4, 2023

@sratz: Should we merge this?

I updated the PR to use the correct NONE instead of NUL.

Technically this works, but

  • This is only the platform product. The impact would probably be bigger if that was done in the EPPs' INI files.
  • This only improves the situation on Windows.

@akurtakov
Copy link
Member

@niraj-modi Would you please review this one as it's Windows specific?

@akurtakov
Copy link
Member

What is the status here?

@merks
Copy link
Contributor

merks commented Oct 11, 2024

I've been experimenting with this today and hence I can definitely confirm that it really works. Previously I was able to access some sites only by adding the certificates to the Java installation's cacerts. But today updated some JDKs today then of course my workarounds stopped working. So this time I installed the certificates using crtmgr which helps make firefox happy and making the installer happy only after using these two options The installed IDE (Eclipse SDK) works well with these options too.

There was mention of this also being easy to do on macos, I believe by using -Djavax.net.ssl.trustStoreType=KeychainStore:

https://stackoverflow.com/questions/14280578/how-to-set-up-java-vm-to-use-the-root-certificates-truststore-handled-by-mac-o

I think we should do the Windows this for EPP as well. And it would be great if someone knew if this worked well on macos too...

@merks merks force-pushed the windows-truststore branch from 583c1db to fba40a9 Compare October 11, 2024 11:37
Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm that the IDE functions better with these options than it does without.

merks added a commit to eclipse-oomph/oomph that referenced this pull request Oct 11, 2024
- -Djavax.net.ssl.trustStoreType=Windows-ROOT
- -Djavax.net.ssl.trustStore=NONE

eclipse-platform/eclipse.platform.releng.aggregator#929
merks added a commit to merks/packages that referenced this pull request Oct 11, 2024
- -Djavax.net.ssl.trustStoreType=Windows-ROOT
- -Djavax.net.ssl.trustStore=NONE

eclipse-platform/eclipse.platform.releng.aggregator#929
@merks
Copy link
Contributor

merks commented Oct 11, 2024

/request-license-review

Copy link
Contributor

/request-license-review

License review requests:

After all reviews have concluded, re-run the license-vetting check from the Github Actions web-interface to update its status.

Workflow run (with attached summary files):
https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/actions/runs/11292041017

@sratz
Copy link
Member Author

sratz commented Oct 12, 2024

There was mention of this also being easy to do on macos, I believe by using -Djavax.net.ssl.trustStoreType=KeychainStore:

https://stackoverflow.com/questions/14280578/how-to-set-up-java-vm-to-use-the-root-certificates-truststore-handled-by-mac-o

I think we should do the Windows this for EPP as well. And it would be great if someone knew if this worked well on macos too...

This looks promising! We will do some more extensive tests on macOS next week and update the PR.

And I agree, this should go into all products (platform + EPP) in a coordinated manner.

@merks
Copy link
Contributor

merks commented Oct 13, 2024

FYI, I spent an embarrassing amount of time tracking down why these options made the Tycho-built installers fail. It was simply because the module jdk.crypto.mscapi was not part of the minimal JustJ JRE which is now fixed:

eclipse-justj/justj@2d6c63d

Because this module is specific to Windows, it's very well hidden in scheme of things, so I thought worth mentioning here.

@sratz
Copy link
Member Author

sratz commented Oct 14, 2024

So the situation on macOS is more complicated.

The correct system properties would be

-Djavax.net.ssl.trustStoreProvider=Apple 
-Djavax.net.ssl.trustStoreType=KeychainStore
-Djavax.net.ssl.trustStore=NONE

however this does NOT have the desired effect:

macOS distinguishes between a 'System' keystore and a 'System Roots' keystore:
image
image

The 'System' keystore contains those corporate certificates that we are interested in, but all the other commonly/publicly trusted CAs are in the 'System Roots' store.

With the system properties above, we would only get access to the 'System' store and this would essentially break all existing standard scenarios, so is not an option.

What we would need is a union of both keystores, which is currently not possible (neither is accessing the 'System Roots' nor is telling the JVM to use a union of multiple stores).

@merks
Copy link
Contributor

merks commented Oct 14, 2024

The union is something that the JDK would need to support. It seems odd that it's so poorly support...

In any case, do you plan to amend this PR to update sdk.product? M2 is rapidly approaching...

Use the Windows operating system trust store instead of the cacerts
bundled with the JVM.

https://bugs.eclipse.org/bugs/show_bug.cgi?id=567504
@sratz sratz force-pushed the windows-truststore branch from fba40a9 to 8a2fa30 Compare October 14, 2024 14:42
@sratz
Copy link
Member Author

sratz commented Oct 14, 2024

The union is something that the JDK would need to support. It seems odd that it's so poorly support...

A union can be achieved in code via a custom SSLContext: javax.net.ssl.SSLContext.init(KeyManager[], TrustManager[], SecureRandom).

We are using this already for our product-specific HTTPS calls, but as for setting this as the default system-wide SSLContext we could be too late in the game unless done at a very early stage after JVM startup.

It would be better if the JVM provided more flexible configuration so this is not needed.

In any case, do you plan to amend this PR to update sdk.product? M2 is rapidly approaching...

Done.

@sratz
Copy link
Member Author

sratz commented Oct 14, 2024

/request-license-review

Copy link
Contributor

/request-license-review

License review requests:

After all reviews have concluded, re-run the license-vetting check from the Github Actions web-interface to update its status.

Workflow run (with attached summary files):
https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/actions/runs/11329998432

@merks
Copy link
Contributor

merks commented Oct 14, 2024

Ignore the license thing. I'm so frustrated with that problem. 😱

eclipse-orbit/orbit-simrel#43
https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/16675

@scottslewis
Copy link

The union is something that the JDK would need to support. It seems odd that it's so poorly support...

A union can be achieved in code via a custom SSLContext: javax.net.ssl.SSLContext.init(KeyManager[], TrustManager[], SecureRandom).

If there are still two ECF filetransfer providers included with Eclipse (apache httpclient 5 and java jre httpclient), and the fact that this issue remains unaddresssed as per

https://gitlab.eclipse.org/eclipse-wg/ide-wg/ide-wg-dev-funded-efforts/ide-wg-dev-funded-program-planning-council-top-issues/-/issues/37

Currently, these two providers handle SSLContext creation/init usage differently and so will respond differently to any additional/earlier customization of SSLContext (e.g. calling SSLContext.init in some other place in Eclipse).

We are using this already for our product-specific HTTPS calls, but as for setting this as the default system-wide SSLContext we could be too late in the game unless done at a very early stage after JVM startup.

Right. With all previous filetransfer providers SSLContext customization has been done for Eclipse install/update in the org.eclipse.ecf.ssl fragment. The java jre httpclient currently does not.

@JavaJoeS
Copy link

My attempts to add some PKI into eclipse-platform did not seem to be well received. I was directed to try to utilize marketplace for a launching platform for PKI. I did just that. However, in my travels I have found a lot of different implementations of SSLContext throughout the eclipse package. Most of these implementations trounce on any SSLContext.getDefault() that may have been set up to a certain specification, i.e, SSLContext,init(keyMgr, trustMgr, securerandom) with valid parameters.

@merks
Copy link
Contributor

merks commented Oct 15, 2024

@sratz

I assume this PR is ready to merge.

I believe this comment is @jonahgraham's approval:

eclipse-packaging/packages#224 (review)

Copy link
Contributor

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the change to sdk.product too. +1

@merks merks merged commit b3ad094 into eclipse-platform:master Oct 15, 2024
3 of 4 checks passed
merks added a commit to eclipse-packaging/packages that referenced this pull request Oct 15, 2024
- -Djavax.net.ssl.trustStoreType=Windows-ROOT
- -Djavax.net.ssl.trustStore=NONE

eclipse-platform/eclipse.platform.releng.aggregator#929
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants