On Thu, 22 Feb 2024 01:14:24 GMT, Hai-May Chao <hc...@openjdk.org> 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
> 104.764 ? 3.237 ops/s
> SSLHandshak...
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 82:
> 80: private final Map<String,Reference<PrivateKeyEntry>> entryCacheMap;
> 81:
> 82: private boolean ksP12;
Could `ksP12` also be `final`?
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 106:
> 104: this.builders = builders;
> 105: uidCounter = new AtomicLong();
> 106: KeyStore keyStore = null;
It may be better to define `keyStore` in the below for-loop.
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 109:
> 107: boolean foundPKCS12 = false;
> 108:
> 109: for (int i = 0, n = builders.size(); i < n; i++) {
The index `i` just be used for retrieving the elements from the list, then it
can apply enhanced for-loop.
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 339:
> 337: return Arrays.equals(cachedCerts, newCerts);
> 338: } catch (Exception e) {
> 339: return false;
Should this exception be logged?
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 356:
> 354: }
> 355:
> 356: PrivateKeyEntry privateKeyEntry = (PrivateKeyEntry) newEntry;
Would you like to apply pattern matching for `instanceof`?
if (!(newEntry instanceof PrivateKeyEntry privateKeyEntry)) {
return false;
}
PrivateKey privateKey = privateKeyEntry.getPrivateKey();
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 410:
> 408: return mapEntryUpdated;
> 409: } catch (Exception e) {
> 410: return false;
Should this exception be logged?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1529787871
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1529792043
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1529796340
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1529803092
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1529801037
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1529810729
