On Fri, 1 Mar 2024 13:59:31 GMT, Matthew Donovan <[email protected]> wrote:
>> For the PKIX KeyManager and PKCS12 Keystore, when the TLS server sends the
>> ServerHello message and ultimately calls the
>> X509KeyManagerImpl.chooseEngineServerAlias() method, it retrieves the
>> private key from the keystore, decrypts it, and caches both the key and its
>> certificate. This caching currently occurs only during a single handshake.
>> Since decryption can be time-consuming, a modification has been implemented
>> to cache the keystore entries at initialization time. This way, it won't be
>> necessary to retrieve and decrypt the keys for multiple handshakes, which
>> could lead to performance drawbacks.
>>
>> A change was made to also update/refresh the cached entry as the
>> certificates in the PKCS12 keystore may change, for scenarios like when the
>> certificate expires and a new one is added under a different alias, and the
>> certificate chain returned by the PKCS12 keystore is not the same as the one
>> in the cache. While attempting to handle when to refresh a cached entry to
>> accommodate keystore changes, we would like to know if you agree that this
>> improvement is worth the risk. We would also like to know if you have a
>> preference for other options:
>>
>> 1. Accept that PKIX+PKCS12 is slow.
>> 2. Add a configuration option (system property, maybe) to decide the level
>> of caching (1 - same as the existing one, 2 - same caching as in
>> SunX509KeyManagerImpl, 3 - the new caching introduced in this change).
>>
>> Additionally, the benchmark test SSLHandshake.java is modified to include a
>> @Param annotation, allowing it to pass different KeyManagerFactory values
>> (SunX509 and PKIX) to the benchmark method.
>>
>> Running modified SSLHandshake.java test prior to the change that caches the
>> PKCS12 keystore entries for PKIX:
>> Benchmark (keyMgr) (resume) (tlsVersion) Mode Cnt
>> Score Error Units
>> SSLHandshake.doHandshake SunX509 true TLSv1.2 thrpt 15
>> 9346.292 ? 379.023 ops/s
>> SSLHandshake.doHandshake SunX509 true TLS thrpt 15
>> 940.175 ? 21.215 ops/s
>> SSLHandshake.doHandshake SunX509 false TLSv1.2 thrpt 15
>> 594.418 ? 23.374 ops/s
>> SSLHandshake.doHandshake SunX509 false TLS thrpt 15
>> 534.030 ? 16.709 ops/s
>> SSLHandshake.doHandshake PKIX true TLSv1.2 thrpt 15
>> 9359.086 ? 246.257 ops/s
>> SSLHandshake.doHandshake PKIX true TLS thrpt 15
>> 933.835 ? 81.388 ops/s
>> SSLHandshake.doHandshake PKIX false TLSv1.2 thrpt 15 ...
>
> src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 194:
>
>> 192: }
>> 193: } catch (Exception e) {
>> 194: // ignore
>
> Should this exception be logged?
Yes, log is added. Thanks!
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1510655807
