RFR: 8359956: Support algorithm constraints and certificate checks in SunX509 key manager [v14]

[Sean Mullan]

On Tue, 29 Jul 2025 15:11:45 GMT, Artur Barashev <abarashev at openjdk.org> 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