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 test/jdk/java/security/KeyStore/CheckMacOSKeyChainIntermediateCATrust.java line 48: > 46: * @summary Check whether loading of certificates from macOS Keychain > correctly > 47: * loads intermediate CA certificates > 48: * @run junit/manual CheckMacOSKeyChainIntermediateCATrust I think you can add negative tests also. E.g. add Root CA without trust settings test/jdk/java/security/KeyStore/CheckMacOSKeyChainIntermediateCATrust.java line 123: > 121: System.out.println("Command line: " + params); > 122: try { > 123: int exitStatus = > ProcessTools.executeProcess(params.toArray(new String[0])).getExitValue(); shouldHaveExitValue(0) can be used test/jdk/java/security/KeyStore/CheckMacOSKeyChainIntermediateCATrust.java line 166: > 164: private static void assertThat(boolean expected, String message, > List<X509Certificate> certificates) { > 165: if (!expected) { > 166: throw new AssertionError(message + ", subjects: " + > getSubjects(certificates)); I do not like printing all KeyChain certificates on the failure. It could be sensitive information. If you do not collect all certificates, the test could be simplified - without Stream API ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/22911#discussion_r1929425287 PR Review Comment: https://git.openjdk.org/jdk/pull/22911#discussion_r1929426230 PR Review Comment: https://git.openjdk.org/jdk/pull/22911#discussion_r1929424938
