Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v5]

2025-04-05 Thread Sean Mullan
On Mon, 27 Jan 2025 22:43:32 GMT, Tim Jacomb 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-r

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v5]

2025-04-02 Thread Sean Mullan
On Tue, 1 Apr 2025 16:13:55 GMT, Tim Jacomb wrote: > > I am dubious that this is the right thing to do. There is a distinct > > difference between a certificate that is trusted and one that requires > > additional validation to determine if it is trusted. Blindly trusting > > self-signed certi

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v5]

2025-04-02 Thread Tim Jacomb
On Tue, 1 Apr 2025 19:23:27 GMT, Sean Mullan wrote: > We need to be really careful here. With this fix we are deciding at runtime > that these intermediate certificates should be treated as > `KeyStore.TrustedCertificateEntry` objects just because they validated ok, > and without any interacti

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v5]

2025-04-01 Thread Sean Mullan
On Mon, 27 Jan 2025 22:43:32 GMT, Tim Jacomb 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-r

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v5]

2025-04-01 Thread Bernd
On Mon, 27 Jan 2025 22:43:32 GMT, Tim Jacomb 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-r

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v5]

2025-04-01 Thread Alexey Bakhtin
On Tue, 1 Apr 2025 17:29:57 GMT, Sean Mullan wrote: > > > I am dubious that this is the right thing to do. There is a distinct > > > difference between a certificate that is trusted and one that requires > > > additional validation to determine if it is trusted. Blindly trusting > > > self-sig

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v5]

2025-04-01 Thread Tim Jacomb
On Tue, 1 Apr 2025 15:25:45 GMT, Sean Mullan wrote: > I am dubious that this is the right thing to do. There is a distinct > difference between a certificate that is trusted and one that requires > additional validation to determine if it is trusted. Blindly trusting > self-signed certificates

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v3]

2025-03-21 Thread Sean Mullan
On Mon, 27 Jan 2025 13:06:57 GMT, Sean Mullan wrote: >> Tim Jacomb has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Revert unneeded change > > This change is significant and should be reviewed by at least 2 Reviewers. > Hi @seanjmullan c

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v3]

2025-03-17 Thread Tim Jacomb
On Mon, 27 Jan 2025 13:06:57 GMT, Sean Mullan wrote: >> Tim Jacomb has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Revert unneeded change > > This change is significant and should be reviewed by at least 2 Reviewers. Hi @seanjmullan can

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v5]

2025-02-06 Thread Tim Jacomb
On Mon, 27 Jan 2025 22:43:32 GMT, Tim Jacomb 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-r

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v5]

2025-01-27 Thread Alexey Bakhtin
On Mon, 27 Jan 2025 22:43:32 GMT, Tim Jacomb 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-r

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v5]

2025-01-27 Thread Tim Jacomb
> ## 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 wit

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v4]

2025-01-27 Thread Alexey Bakhtin
On Mon, 27 Jan 2025 21:34:08 GMT, Tim Jacomb 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-r

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v4]

2025-01-27 Thread Tim Jacomb
> ## 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 wit

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v3]

2025-01-27 Thread Alexey Bakhtin
On Sun, 26 Jan 2025 22:58:04 GMT, Tim Jacomb wrote: >> test/jdk/java/security/KeyStore/CheckMacOSKeyChainIntermediateCATrust.java >> line 166: >> >>> 164: private static void assertThat(boolean expected, String message, >>> List certificates) { >>> 165: if (!expected) { >>> 166:

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v3]

2025-01-27 Thread Alexey Bakhtin
On Sat, 25 Jan 2025 01:12:01 GMT, Alexey Bakhtin wrote: >> Tim Jacomb has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Revert unneeded change > > test/jdk/java/security/KeyStore/CheckMacOSKeyChainIntermediateCATrust.java > line 48: > >>

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v3]

2025-01-27 Thread Alexey Bakhtin
On Sun, 26 Jan 2025 23:04:29 GMT, Tim Jacomb 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-r

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v3]

2025-01-27 Thread Sean Mullan
On Sun, 26 Jan 2025 23:04:29 GMT, Tim Jacomb 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-r

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v3]

2025-01-26 Thread Tim Jacomb
> ## 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 wit

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v2]

2025-01-26 Thread Tim Jacomb
On Sat, 25 Jan 2025 01:10:41 GMT, Alexey Bakhtin wrote: >> Tim Jacomb has updated the pull request with a new target base due to a >> merge or a rebase. The incremental webrev excludes the unrelated changes >> brought in by the merge/rebase. The pull request contains 13 additional >> commits s

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore [v2]

2025-01-26 Thread Tim Jacomb
> ## 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 wit

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Alexey Bakhtin
On Fri, 3 Jan 2025 11:28:01 GMT, Tim Jacomb 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 >

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Tim Jacomb
On Tue, 7 Jan 2025 20:20:46 GMT, Tim Jacomb wrote: >> The test should be marked as `@run junit/manual ` and added to the >> `jdk_security_manual_interactive` part of the TEST.groups > > Any idea how I can run the test after making those changes? > > The test just gets skipped with: > > > CONF

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Alexey Bakhtin
On Fri, 3 Jan 2025 11:28:01 GMT, Tim Jacomb 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 >

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Tim Jacomb
On Sat, 4 Jan 2025 00:19:46 GMT, Alexey Bakhtin wrote: > 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 >

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Tim Jacomb
On Fri, 3 Jan 2025 15:36:33 GMT, Tim Jacomb wrote: >> src/java.base/macosx/native/libosxsecurity/KeystoreImpl.m line 496: >> >>> 494: >>> 495: // Only add certificates with trust settings >>> 496: if (inputTrust == NULL) { >> >> From what I can tell non root certificate

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Alexey Bakhtin
On Tue, 7 Jan 2025 17:01:10 GMT, Tim Jacomb 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-re

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Tim Jacomb
On Fri, 3 Jan 2025 11:28:01 GMT, Tim Jacomb 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 >

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Tim Jacomb
On Tue, 7 Jan 2025 17:21:50 GMT, Alexey Bakhtin wrote: >> test/jdk/java/security/KeyStore/CheckMacOSKeyChainIntermediateCATrust.java >> line 43: >> >>> 41: >>> 42: /* >>> 43: * @test >> >> @alexeybakhtin quick question on how this should be marked as manual. >> >> I see all tests in >> htt

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Tim Jacomb
On Fri, 3 Jan 2025 16:52:51 GMT, Tim Jacomb wrote: >> Interesting for root certificate `SecTrustSettingsCopyTrustSettings` returns: >> >> * -25300 (not found) when trust policy is `Use System Defaults` >> * 0 and a `kSecTrustSettingsResult` value of 3 when set to Never Trust >> * 0 and a `kSecTr

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Tim Jacomb
On Fri, 3 Jan 2025 16:29:57 GMT, Tim Jacomb wrote: >> Ok this isn't working properly 😢 >> >> 1. ⛔ Fails: Marking the certificate as OS default (which for CA certs is >> trust: false) - with an intermediate >> 2. ⛔ Fails: Marking the certificate as OS default without an intermediate >> 3. ⛔ Fa

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Tim Jacomb
On Fri, 3 Jan 2025 11:38:29 GMT, Tim Jacomb 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-re

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Tim Jacomb
On Mon, 6 Jan 2025 20:43:22 GMT, Tim Jacomb wrote: > Is it possible to add jtreg test for this scenario? I've done some research. I _think_ it would only be possible with manual intervention to run it. The certificates could be generated with a script, similar to the existing https://github.co

Re: RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Alexey Bakhtin
On Mon, 6 Jan 2025 21:53:34 GMT, Tim Jacomb wrote: > Does that add value to add a test so someone could run it manually? Yes, I think a manual test is better than nothing. - PR Comment: https://git.openjdk.org/jdk/pull/22911#issuecomment-2574022994

RFR: 8347067: Load certificates without explicit trust settings in KeyChainStore

2025-01-24 Thread Tim Jacomb
## 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 succeed