-
Notifications
You must be signed in to change notification settings - Fork 4
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
Conversation
cc @shalupov |
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
i.e. whether this needs to be validated at this point or 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 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. |
Right yeah I haven't had time to validate whether my putting an intermediate in then it would be trusted without the root. 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 |
Interesting test will be to check whether ROOT -> INTERMEDIATE1 -> INTERMEDIATE2 will work |
I decided to merge this PR to have less stacked PRs around |
Fixes #9
TODO:
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