On Fri, 3 Jan 2025 11:28:01 GMT, Tim Jacomb <d...@openjdk.org> wrote:
> ## The change > > Without this change intermediate certificates that don't have explicit trust > settings are ignored not added to the truststore. > > > > ## Reproducer > > See https://github.com/timja/openjdk-intermediate-ca-reproducer > > Without this change the reproducer fails, and with this change it succeeds. > > ## Example failing architecture > > Root CA -> Intermediate 1 -> Intermediate 2 -> Leaf > > Where: > * All certs are in admin domain kSecTrustSettingsDomainAdmin > * Root CA is marked as always trust > * Intermediate 1 and 2 are Unspecified > > Previously Root CA would be found but intermediate 1 and 2 would be skipped > when verifying trust settings. > > ## Background reading > > ### Rust > see also Rust Lib that is used throughout Rust ecosystem for this: > https://github.com/rustls/rustls-native-certs/blob/efe7b1d77bf6080851486535664d1dc7ef0dea68/src/macos.rs#L39-L58 > > e.g. in Deno `https://github.com/denoland/deno/pull/11491` where I've > verified it is correctly implemented and works in my setup > > ## Python > > I also looked at the Python implementation for inspiration as well (which > also works on my system): > https://github.com/sethmlarson/truststore/blob/main/src/truststore/_macos.py I'm afraid but it looks like this implementation violates the https://developer.apple.com/documentation/security/sectrustsettingscopytrustsettings(_:_:_:) specification. It always creates an empty input trust and fills it with the provided non-null trustSettings. So, the certificate always has at least empty trustSettings. However, according to this specification, two different scenarios should be considered: 1. Empty trustSettings means that the certificate is trusted if it is self-signed. 2. Null trustSettings means that the certificate must be verified before adding to the trust store. The current implementation adds Root certificate to the TrustStore because it has "Always Trust" settings. Implementation does not add the intermediate certificate to the TrustStore because it has "System Defaults" settings ( SecTrustSettingsCopyTrustSettings returns null trustSettings), and the current implementation only adds certificates with trust settings. I think, in this particular case, we need two iterations to add certificates into the trust store. The first iteration will add certificates with non-null trust settings, and the second iteration should verify and add certificates with null trust settings. Thank you for this patch. It looks correct now (see my comment about subjCerts above) Is it possible to add jtreg test for this scenario? Also, You'll need a jbs issue to submit this PR src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 480: > 478: CFRelease(policy); > 479: } > 480: if (secTrust) { I think subjCerts should be released here too ------------- PR Comment: https://git.openjdk.org/jdk/pull/22911#issuecomment-2569957562 PR Comment: https://git.openjdk.org/jdk/pull/22911#issuecomment-2573747523 PR Review Comment: https://git.openjdk.org/jdk/pull/22911#discussion_r1904455215
