On Tue, 26 Mar 2024 06:00:33 GMT, Hai-May Chao <[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 ...
>
> Hai-May Chao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Updated with John's comments
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 82:
> 80: private final Map<String,Reference<PrivateKeyEntry>> entryCacheMap;
> 81:
> 82: private final boolean ksP12;
Can we improve this name? Something like "cachePrivateKeys" would better
describe the purpose
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 112:
> 110: KeyStore keyStore = builder.getKeyStore();
> 111: String ksType = keyStore.getType();
> 112: if (ksType.equalsIgnoreCase("pkcs12")) {
I think we also need to check the provider; other providers might also
implement PKCS12 key store type, and we don't know if we can trust their
`getEntry` to return consistent data
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 113:
> 111: String ksType = keyStore.getType();
> 112: if (ksType.equalsIgnoreCase("pkcs12")) {
> 113: foundPKCS12 = true;
this will enable caching if _any_ of the keystores is PKCS12; we should only
enable caching when _all_ keystores are PKCS12
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1544815889
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1544818679
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1544818184
