Skip to content

Windows: Support intermediate certificates #8

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

Closed

Conversation

timja
Copy link
Contributor

@timja timja commented Feb 22, 2025

Fixes #9

TODO:

  • Run tests on Windows

I haven't run this yet on Windows but its the same code I wrote for node.js nodejs/node#57164 which I have tested as working. - Ran on Windows its working.

Also see Chromium's implementation: https://github.com/chromium/chromium/blob/98f89988c9774d0e138a0724aa64c46187203a77/net/cert/internal/trust_store_win.cc#L220-L222

@timja timja marked this pull request as draft February 22, 2025 10:37
@timja timja marked this pull request as ready for review February 22, 2025 16:50
@timja
Copy link
Contributor Author

timja commented Feb 22, 2025

cc @shalupov

@shalupov shalupov self-assigned this Feb 24, 2025
@shalupov shalupov self-requested a review February 24, 2025 21:23
@shalupov
Copy link
Collaborator

Why you don't check that intermediate CA is signed with any root CA like in mac OS?

@timja
Copy link
Contributor Author

timja commented Feb 24, 2025

Why you don't check that intermediate CA is signed with any root CA like in mac OS?

Its based on the chromium implementation for Windows, https://github.com/chromium/chromium/blob/98f89988c9774d0e138a0724aa64c46187203a77/net/cert/internal/trust_store_win.cc#L220-L222. How they do the macOS implementation for intermediates is not clear (although it does work).

I've been thinking about whether its necessary or not to do that on macOS, its based on https://developer.apple.com/documentation/security/sectrustsettingscopytrustsettings(_:_:_:)?language=objc

No trust-settings array means “this certificate must be verifiable using a known trusted certificate”.

i.e. whether this needs to be validated at this point or whether it should be validated later by path building.
Using the OS verification also means that you know that the trust settings are fine for it but that can be checked as well.

@shalupov
Copy link
Collaborator

shalupov commented Feb 25, 2025

whether it should be validated later by path building.

In this design, we can't validate certificates later because we return all certificates as a single trusted root list. While Chromium may correctly inject intermediate certificates into the validating trust chain, we simplify it by passing everything as a single list to TrustManagerFactory as one key store.

This approach could probably be improved, but I haven't found a better way yet--and so far, it has been good enough.

According to the javadoc for SecurityFrameworkUtil, it would be better to implement X509TrustManager using the native API. However, this would require a larger effort and is best handled at the JDK level.

That's why I prefer to validate that all intermediate certificates are signed by a trusted root--no completely untrusted certificates are injected into TrustManagerFactory.

@timja
Copy link
Contributor Author

timja commented Feb 25, 2025

Right yeah I haven't had time to validate whether my putting an intermediate in then it would be trusted without the root.
e.g. in node.js it was fine as they validated the chain separately.

Do you happen to know if there's a similar OS API to macOS I can use here? (I can validate against all certificates but may need to check a bunch).

@timja timja mentioned this pull request Feb 25, 2025
@timja
Copy link
Contributor Author

timja commented Feb 25, 2025

Do you happen to know if there's a similar OS API to macOS I can use here? (I can validate against all certificates but may need to check a bunch).

I think a combination of

@shalupov
Copy link
Collaborator

@shalupov
Copy link
Collaborator

Interesting test will be to check whether ROOT -> INTERMEDIATE1 -> INTERMEDIATE2 will work

@shalupov
Copy link
Collaborator

I decided to merge this PR to have less stacked PRs around

@shalupov
Copy link
Collaborator

merged manually cc7cba2

added intermediate certificates check: 8819503

@shalupov shalupov closed this Feb 26, 2025
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.

Windows: intermediate certificates are missing
2 participants