Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v13]
On Wed, 23 Jul 2025 21:20:49 GMT, Artur Barashev wrote: >> SunX509 key manager should support the same certificate checks that are >> supported by PKIX key manager. >> >> Effectively there should be only 2 differences between 2 key managers: >> - PKIX supports multiple key stores through KeyStore.Builder interface while >> SunX509 supports only a single keystore. >> - SunX509 caches its whole key store on initialization thus improving >> performance. This means that subsequent modifications of the KeyStore have >> no effect on SunX509 KM, unlike PKIX . >> >> **SUNX509 KeyManager performance before the change** >> Benchmark(resume) (tlsVersion) Mode >> Cnt Score Error Units >> SSLHandshake.doHandshake true TLSv1.2 thrpt 15 19758.012 ± >> 758.237 ops/s >> SSLHandshake.doHandshake true TLS thrpt 15 1861.695 ± >> 14.681 ops/s >> SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1186.962** >> ± 12.085 ops/s >> SSLHandshake.doHandshake false TLS thrpt 15 **1056.288** >> ± 7.197 ops/s >> >> **SUNX509 KeyManager performance after the change** >> Benchmark (resume) (tlsVersion) Mode Cnt Score >> Error Units >> SSLHandshake.doHandshake true TLSv1.2 thrpt 15 20954.399 ± >> 260.817 ops/s >> SSLHandshake.doHandshake true TLS thrpt 15 1813.401 ± >> 13.917 ops/s >> SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1158.190** >> ± 6.023 ops/s >> SSLHandshake.doHandshake false TLS thrpt 15 **1012.988** >> ± 10.943 ops/s > > Artur Barashev has updated the pull request incrementally with one additional > commit since the last revision: > > Update system property name src/java.base/share/conf/security/java.security line 29: > 27: # an unspecified error when initializing the java.security.Security class. > 28: # Properties in this file are typically parsed only once. If any of the > 29: # Properties in this file are typically parsed only once. If any of the Duplicate line added, is this an accidental change? test/jdk/sun/security/ssl/SignatureScheme/MD5NotAllowedInTLS13CertificateSignature.java line 1: > 1: /* Needs copyright update. test/jdk/sun/security/ssl/X509TrustManagerImpl/PKIXExtendedTM.java line 36: > 34: * @run main/othervm PKIXExtendedTM 1 > 35: * @run main/othervm PKIXExtendedTM 2 > 36: * @run main/othervm PKIXExtendedTM 3 Why did you remove these lines/test cases? - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240064384 PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240068846 PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240090370
RFR: 8360408: [TEST] Use @requires tag instead of exiting based on "os.name" property value for sun/net/www/protocol/file/FileURLTest.java
Refactor remove the test logic for Windows OS and replace with jtreg check for os - Commit messages: - 8360408: [TEST] Use @requires tag instead of exiting based on "os.name" property value for sun/net/www/protocol/file/FileURLTest.java Changes: https://git.openjdk.org/jdk/pull/26537/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=26537&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8360408 Stats: 6 lines in 1 file changed: 1 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/26537.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/26537/head:pull/26537 PR: https://git.openjdk.org/jdk/pull/26537
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
> SunX509 key manager should support the same certificate checks that are > supported by PKIX key manager. > > Effectively there should be only 2 differences between 2 key managers: > - PKIX supports multiple key stores through KeyStore.Builder interface while > SunX509 supports only a single keystore. > - SunX509 caches its whole key store on initialization thus improving > performance. This means that subsequent modifications of the KeyStore have no > effect on SunX509 KM, unlike PKIX . > > **SUNX509 KeyManager performance before the change** > Benchmark(resume) (tlsVersion) Mode > Cnt Score Error Units > SSLHandshake.doHandshake true TLSv1.2 thrpt 15 19758.012 ± > 758.237 ops/s > SSLHandshake.doHandshake true TLS thrpt 15 1861.695 ± > 14.681 ops/s > SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1186.962** ± > 12.085 ops/s > SSLHandshake.doHandshake false TLS thrpt 15 **1056.288** ± > 7.197 ops/s > > **SUNX509 KeyManager performance after the change** > Benchmark (resume) (tlsVersion) Mode Cnt Score > Error Units > SSLHandshake.doHandshake true TLSv1.2 thrpt 15 20954.399 ± > 260.817 ops/s > SSLHandshake.doHandshake true TLS thrpt 15 1813.401 ± > 13.917 ops/s > SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1158.190** ± > 6.023 ops/s > SSLHandshake.doHandshake false TLS thrpt 15 **1012.988** ± > 10.943 ops/s Artur Barashev has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.org/jdk/pull/25016/files - new: https://git.openjdk.org/jdk/pull/25016/files/eb972544..df1a1fac Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25016&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25016&range=12-13 Stats: 5 lines in 3 files changed: 3 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/25016.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25016/head:pull/25016 PR: https://git.openjdk.org/jdk/pull/25016
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 15:11:45 GMT, Artur Barashev wrote: >> SunX509 key manager should support the same certificate checks that are >> supported by PKIX key manager. >> >> Effectively there should be only 2 differences between 2 key managers: >> - PKIX supports multiple key stores through KeyStore.Builder interface while >> SunX509 supports only a single keystore. >> - SunX509 caches its whole key store on initialization thus improving >> performance. This means that subsequent modifications of the KeyStore have >> no effect on SunX509 KM, unlike PKIX . >> >> **SUNX509 KeyManager performance before the change** >> Benchmark(resume) (tlsVersion) Mode >> Cnt Score Error Units >> SSLHandshake.doHandshake true TLSv1.2 thrpt 15 19758.012 ± >> 758.237 ops/s >> SSLHandshake.doHandshake true TLS thrpt 15 1861.695 ± >> 14.681 ops/s >> SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1186.962** >> ± 12.085 ops/s >> SSLHandshake.doHandshake false TLS thrpt 15 **1056.288** >> ± 7.197 ops/s >> >> **SUNX509 KeyManager performance after the change** >> Benchmark (resume) (tlsVersion) Mode Cnt Score >> Error Units >> SSLHandshake.doHandshake true TLSv1.2 thrpt 15 20954.399 ± >> 260.817 ops/s >> SSLHandshake.doHandshake true TLS thrpt 15 1813.401 ± >> 13.917 ops/s >> SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1158.190** >> ± 6.023 ops/s >> SSLHandshake.doHandshake false TLS thrpt 15 **1012.988** >> ± 10.943 ops/s > > Artur Barashev has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments test/jdk/sun/security/ssl/X509KeyManager/PeerConstraintsCheck.java line 74: > 72: * This class tests against the peer supported certificate signatures > sent in > 73: * "signature_algorithms_cert" extension. > 74: */ Can you add a sentence or two about why the test should fail when the SunX509 KM is used and the certCheck property is enabled or the PKIX KM is used? I think it is because the server's cert is signed with SHA256withECDSA and the "signature_algorithms_cert" extension is set to SHA384withECDSA, so it must be signed with SHA384withECDSA, is that right? - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240586473
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 18:05:01 GMT, Sean Mullan wrote: >> Artur Barashev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments > > test/jdk/sun/security/ssl/X509KeyManager/PeerConstraintsCheck.java line 74: > >> 72: * This class tests against the peer supported certificate signatures >> sent in >> 73: * "signature_algorithms_cert" extension. >> 74: */ > > Can you add a sentence or two about why the test should fail when the SunX509 > KM is used and the certCheck property is enabled or the PKIX KM is used? I > think it is because the server's cert is signed with SHA256withECDSA and the > "signature_algorithms_cert" extension is set to SHA384withECDSA, so it must > be signed with SHA384withECDSA, is that right? Yes, correct: `jdk.tls.client.SignatureSchemes` and `jdk.tls.server.SignatureSchemes` system properties set signature schemes for both "signature_algorithms" and "signature_algorithms_cert" extensions. Then we fail because server's certificate is signed with `SHA256withECDSA`. I'll update the comment to highlight that detail. - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240621241
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 15:11:45 GMT, Artur Barashev wrote:
>> SunX509 key manager should support the same certificate checks that are
>> supported by PKIX key manager.
>>
>> Effectively there should be only 2 differences between 2 key managers:
>> - PKIX supports multiple key stores through KeyStore.Builder interface while
>> SunX509 supports only a single keystore.
>> - SunX509 caches its whole key store on initialization thus improving
>> performance. This means that subsequent modifications of the KeyStore have
>> no effect on SunX509 KM, unlike PKIX .
>>
>> **SUNX509 KeyManager performance before the change**
>> Benchmark(resume) (tlsVersion) Mode
>> Cnt Score Error Units
>> SSLHandshake.doHandshake true TLSv1.2 thrpt 15 19758.012 ±
>> 758.237 ops/s
>> SSLHandshake.doHandshake true TLS thrpt 15 1861.695 ±
>> 14.681 ops/s
>> SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1186.962**
>> ± 12.085 ops/s
>> SSLHandshake.doHandshake false TLS thrpt 15 **1056.288**
>> ± 7.197 ops/s
>>
>> **SUNX509 KeyManager performance after the change**
>> Benchmark (resume) (tlsVersion) Mode Cnt Score
>> Error Units
>> SSLHandshake.doHandshake true TLSv1.2 thrpt 15 20954.399 ±
>> 260.817 ops/s
>> SSLHandshake.doHandshake true TLS thrpt 15 1813.401 ±
>> 13.917 ops/s
>> SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1158.190**
>> ± 6.023 ops/s
>> SSLHandshake.doHandshake false TLS thrpt 15 **1012.988**
>> ± 10.943 ops/s
>
> Artur Barashev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Address review comments
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 88:
> 86: uidCounter = new AtomicLong();
> 87: entryCacheMap = Collections.synchronizedMap
> 88: (new SizedMap<>());
You can remove `SizedMap` on lines 94-101. Did you see any reason why a
`LinkedHashMap` was used? (I cannot, but this code has not changed in many
releases, so we should be sure its ok).
test/jdk/sun/security/ssl/X509KeyManager/CertChecking.java line 110:
> 108: private static final boolean[] NO_DG_USAGE =
> 109: new boolean[]{false, true, true, true, true, true};
> 110: private static final boolean[] NO_DG_NO_KE_USAGE =
Nit: how about NO_DS instead of NO_DG?
test/jdk/sun/security/ssl/X509KeyManager/CertChecking.java line 128:
> 126: // --- Usage and expired test cases --
> 127:
> 128: // Both should fail with no usages at all
Clarify what you mean by "Both should fail"? This test doesn't do a TLS
handshake. Maybe what you want to comment on is the order when checking is
enabled (i.e. cert with bad usage is always preferred last).
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240708490
PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240672544
PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240675270
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v15]
> SunX509 key manager should support the same certificate checks that are > supported by PKIX key manager. > > Effectively there should be only 2 differences between 2 key managers: > - PKIX supports multiple key stores through KeyStore.Builder interface while > SunX509 supports only a single keystore. > - SunX509 caches its whole key store on initialization thus improving > performance. This means that subsequent modifications of the KeyStore have no > effect on SunX509 KM, unlike PKIX . > > **SUNX509 KeyManager performance before the change** > Benchmark(resume) (tlsVersion) Mode > Cnt Score Error Units > SSLHandshake.doHandshake true TLSv1.2 thrpt 15 19758.012 ± > 758.237 ops/s > SSLHandshake.doHandshake true TLS thrpt 15 1861.695 ± > 14.681 ops/s > SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1186.962** ± > 12.085 ops/s > SSLHandshake.doHandshake false TLS thrpt 15 **1056.288** ± > 7.197 ops/s > > **SUNX509 KeyManager performance after the change** > Benchmark (resume) (tlsVersion) Mode Cnt Score > Error Units > SSLHandshake.doHandshake true TLSv1.2 thrpt 15 20954.399 ± > 260.817 ops/s > SSLHandshake.doHandshake true TLS thrpt 15 1813.401 ± > 13.917 ops/s > SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1158.190** ± > 6.023 ops/s > SSLHandshake.doHandshake false TLS thrpt 15 **1012.988** ± > 10.943 ops/s Artur Barashev has updated the pull request incrementally with one additional commit since the last revision: Review fixes - Changes: - all: https://git.openjdk.org/jdk/pull/25016/files - new: https://git.openjdk.org/jdk/pull/25016/files/df1a1fac..c8c4008a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=25016&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=25016&range=13-14 Stats: 44 lines in 4 files changed: 22 ins; 2 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/25016.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/25016/head:pull/25016 PR: https://git.openjdk.org/jdk/pull/25016
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 19:24:53 GMT, Sean Mullan wrote: >> Sounds good, changing it to "Both client and server should fail". >> `usageTestCase` method takes 2 boolean values to indicate whether to check >> for server and client failure. > > But I am still confused by what you mean by fail? Typically that means > catching an Exception and checking that it is expected. It means incorrect alias returned, or incorrect order of aliases. Yes, probably `fail` is a bad word choice here, I'll rephrase it. - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240780122
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 18:46:03 GMT, Sean Mullan wrote: >> Artur Barashev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments > > test/jdk/sun/security/ssl/X509KeyManager/CertChecking.java line 128: > >> 126: // --- Usage and expired test cases -- >> 127: >> 128: // Both should fail with no usages at all > > Clarify what you mean by "Both should fail"? This test doesn't do a TLS > handshake. Maybe what you want to comment on is the order when checking is > enabled (i.e. cert with bad usage is always preferred last). Sounds good, changing it to "Both client and server should fail". `usageTestCase` method takes 2 boolean values to indicate whether to check for server and client failure. - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240741713
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 15:11:45 GMT, Artur Barashev wrote: >> SunX509 key manager should support the same certificate checks that are >> supported by PKIX key manager. >> >> Effectively there should be only 2 differences between 2 key managers: >> - PKIX supports multiple key stores through KeyStore.Builder interface while >> SunX509 supports only a single keystore. >> - SunX509 caches its whole key store on initialization thus improving >> performance. This means that subsequent modifications of the KeyStore have >> no effect on SunX509 KM, unlike PKIX . >> >> **SUNX509 KeyManager performance before the change** >> Benchmark(resume) (tlsVersion) Mode >> Cnt Score Error Units >> SSLHandshake.doHandshake true TLSv1.2 thrpt 15 19758.012 ± >> 758.237 ops/s >> SSLHandshake.doHandshake true TLS thrpt 15 1861.695 ± >> 14.681 ops/s >> SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1186.962** >> ± 12.085 ops/s >> SSLHandshake.doHandshake false TLS thrpt 15 **1056.288** >> ± 7.197 ops/s >> >> **SUNX509 KeyManager performance after the change** >> Benchmark (resume) (tlsVersion) Mode Cnt Score >> Error Units >> SSLHandshake.doHandshake true TLSv1.2 thrpt 15 20954.399 ± >> 260.817 ops/s >> SSLHandshake.doHandshake true TLS thrpt 15 1813.401 ± >> 13.917 ops/s >> SSLHandshake.doHandshake false TLSv1.2 thrpt 15 **1158.190** >> ± 6.023 ops/s >> SSLHandshake.doHandshake false TLS thrpt 15 **1012.988** >> ± 10.943 ops/s > > Artur Barashev has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments src/java.base/share/classes/sun/security/ssl/X509KeyManagerCertChecking.java line 58: > 56: * Layer that adds algorithm constraints and certificate checking to a key > 57: * manager. > 58: */ Can you add some more comments about the algorithm it uses for selecting certificates (when certChecking is enabled)? In other words, what the preference order is for selecting certs, and which certificates are not chosen due to disabled algs, or other reasons. You can probably copy some/most of this from the comments in `X509KeyManagerImpl`. - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240798471
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 18:20:54 GMT, Artur Barashev wrote: >> test/jdk/sun/security/ssl/X509KeyManager/PeerConstraintsCheck.java line 74: >> >>> 72: * This class tests against the peer supported certificate signatures >>> sent in >>> 73: * "signature_algorithms_cert" extension. >>> 74: */ >> >> Can you add a sentence or two about why the test should fail when the >> SunX509 KM is used and the certCheck property is enabled or the PKIX KM is >> used? I think it is because the server's cert is signed with SHA256withECDSA >> and the "signature_algorithms_cert" extension is set to SHA384withECDSA, so >> it must be signed with SHA384withECDSA, is that right? > > Yes, correct: `jdk.tls.client.SignatureSchemes` and > `jdk.tls.server.SignatureSchemes` system properties set signature schemes for > both "signature_algorithms" and "signature_algorithms_cert" extensions. Then > we fail because server's certificate is signed with `SHA256withECDSA`. I'll > update the comment to highlight that detail. BTW, by using the above system properties we also bypass this check in the process: https://github.com/openjdk/jdk/blob/ea754316fd6d691a701dfb4bc921ea8c92dc5dd4/src/java.base/share/classes/sun/security/ssl/CertificateMessage.java#L1011 - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240642639
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 19:03:26 GMT, Sean Mullan wrote: >> Artur Barashev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments > > src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 88: > >> 86: uidCounter = new AtomicLong(); >> 87: entryCacheMap = Collections.synchronizedMap >> 88: (new SizedMap<>()); > > You can remove `SizedMap` on lines 94-101. Did you see any reason why a > `LinkedHashMap` was used? (I cannot, but this code has not changed in many > releases, so we should be sure its ok). Looks like `LinkedHashMap` was used so we can remove the eldest entry, the `SizedMap` is limited to the size of 10. Actually, I'll restore the original assignment of `entryCacheMap` to preserve the original cache design. I should have paid more attention to this change. - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240765575
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v13]
On Tue, 29 Jul 2025 14:40:09 GMT, Sean Mullan wrote: >> Artur Barashev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update system property name > > test/jdk/sun/security/ssl/X509TrustManagerImpl/PKIXExtendedTM.java line 36: > >> 34: * @run main/othervm PKIXExtendedTM 1 >> 35: * @run main/othervm PKIXExtendedTM 2 >> 36: * @run main/othervm PKIXExtendedTM 3 > > Why did you remove these lines/test cases? Good catch, thanks! Probably removed them for testing and forgot to restore. - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240130988
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v13]
On Tue, 29 Jul 2025 14:47:29 GMT, Artur Barashev wrote: >> test/jdk/sun/security/ssl/SignatureScheme/MD5NotAllowedInTLS13CertificateSignature.java >> line 1: >> >>> 1: /* >> >> Needs copyright update. > > The year is set to 2025 already, maybe something else needs to be changed? > Please elaborate. Wrong file, I meant `test/jdk/sun/security/mscapi/ShortRSAKeyWithinTLS.java` - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240125935
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v13]
On Tue, 29 Jul 2025 14:31:19 GMT, Sean Mullan wrote: >> Artur Barashev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update system property name > > src/java.base/share/conf/security/java.security line 29: > >> 27: # an unspecified error when initializing the java.security.Security >> class. >> 28: # Properties in this file are typically parsed only once. If any of the >> 29: # Properties in this file are typically parsed only once. If any of the > > Duplicate line added, is this an accidental change? Yes, looks like it was pasted by accident, we shouldn't make any changes to this file. Thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240143909
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v13]
On Tue, 29 Jul 2025 14:52:46 GMT, Sean Mullan wrote: >> The year is set to 2025 already, maybe something else needs to be changed? >> Please elaborate. > > Wrong file, I meant `test/jdk/sun/security/mscapi/ShortRSAKeyWithinTLS.java` Done, thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240138237
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v13]
On Tue, 29 Jul 2025 14:32:48 GMT, Sean Mullan wrote: >> Artur Barashev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update system property name > > test/jdk/sun/security/ssl/SignatureScheme/MD5NotAllowedInTLS13CertificateSignature.java > line 1: > >> 1: /* > > Needs copyright update. The year is set to 2025 already, maybe something else needs to be changed? Please elaborate. - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240111694
Re: RFR: 8325766: Review seclibs tests for cert expiry [v6]
> This PR updates the CertificateBuilder with a new method that creates a new > instance with common fields (subject name, public key, serial number, > validity, and key uses) filled-in. One test, IPIdentities.java, is updated to > show how the method can be used to create various certificates. I attached > screenshots that compare the old hard-coded certificates (left) with the new > generated certificates. > >  >  >  Matthew Donovan has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - changed keystore to PKCS12 and remove key initialization - added new method, setOneHourValidity(), and removed it from the static method. - Merge branch 'master' into certbuilder - fixed redundant setNotAfter() calls. One of them should have been setNotBefore - Merge branch 'master' into certbuilder - expanded wildcard imports - Merge branch 'master' into certbuilder - Merge branch 'master' into certbuilder - reversed order of DN strings when making certificates. - Merge branch 'master' into certbuilder - ... and 5 more: https://git.openjdk.org/jdk/compare/011de4c8...963625db - Changes: https://git.openjdk.org/jdk/pull/23700/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=23700&range=05 Stats: 789 lines in 3 files changed: 162 ins; 582 del; 45 mod Patch: https://git.openjdk.org/jdk/pull/23700.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/23700/head:pull/23700 PR: https://git.openjdk.org/jdk/pull/23700
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 18:45:27 GMT, Sean Mullan wrote:
>> Artur Barashev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Address review comments
>
> test/jdk/sun/security/ssl/X509KeyManager/CertChecking.java line 110:
>
>> 108: private static final boolean[] NO_DG_USAGE =
>> 109: new boolean[]{false, true, true, true, true, true};
>> 110: private static final boolean[] NO_DG_NO_KE_USAGE =
>
> Nit: how about NO_DS instead of NO_DG?
Good catch, I'll rename it. Not sure why I used `DG` and not `DS`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240722307
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 19:19:15 GMT, Artur Barashev wrote: >> test/jdk/sun/security/ssl/X509KeyManager/CertChecking.java line 128: >> >>> 126: // --- Usage and expired test cases -- >>> 127: >>> 128: // Both should fail with no usages at all >> >> Clarify what you mean by "Both should fail"? This test doesn't do a TLS >> handshake. Maybe what you want to comment on is the order when checking is >> enabled (i.e. cert with bad usage is always preferred last). > > Sounds good, changing it to "Both client and server should fail". > `usageTestCase` method takes 2 boolean values to indicate whether to check > for server and client failure. But I am still confused by what you mean by fail? Typically that means catching an Exception and checking that it is expected. - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240752548
Re: RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]
On Tue, 29 Jul 2025 19:50:16 GMT, Sean Mullan wrote: >> Artur Barashev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Address review comments > > src/java.base/share/classes/sun/security/ssl/X509KeyManagerCertChecking.java > line 58: > >> 56: * Layer that adds algorithm constraints and certificate checking to a >> key >> 57: * manager. >> 58: */ > > Can you add some more comments about the algorithm it uses for selecting > certificates (when certChecking is enabled)? In other words, what the > preference order is for selecting certs, and which certificates are not > chosen due to disabled algs, or other reasons. You can probably copy > some/most of this from the comments in `X509KeyManagerImpl`. Done, thanks! - PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2240958831
