On Fri, 22 Mar 2024 06:56:33 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 ...
>
> 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 168:
> 166: boolean mapEntryUpdated = processCredentials(builder,
> 167: i, alias);
> 168: if (!mapEntryUpdated){
Nit: need a space between `)` and `{`.
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 191:
> 189: @Override
> 190: public X509Certificate[] getCertificateChain(String alias) {
> 191: if (ksP12) {
It looks the new codes in method `getCertificateChain` and `getPrivateKey` are
quite similar.
They both mainly retrieve the `X509Credentials` from the cached map, though
finally the former returns certificate chain and the latter returns the private
key.
So, it may be better to abstract a method for retrieving the `X509Credentials`,
and this new method can be shared by the `getCertificateChain` and
`getPrivateKey`.
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 302:
> 300: //
> 301:
> 302: private String removeAliasIndex (String alias){
Nit: the space between `x` and `(` can be removed; a space is needed between
`)` and `{`.
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 368:
> 366: credentialsMap.put(builderAlias, cred);
> 367:
> 368: if (SSLLogger.isOn && SSLLogger.isOn("keymanager")) {
The method `processCredentials` itself logs for the success, however the
callers log for the failure (see line 169 and 388).
Especially, the logs on successful and failed cases display different contents.
The log for the failed case doesn't contain the builder index and entry alias.
Could it also log the failed cases in the body of this method?
That should log the messages before returning false.
Then, the callers may not need to log for the method.
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 369:
> 367:
> 368: if (SSLLogger.isOn && SSLLogger.isOn("keymanager")) {
> 369: SSLLogger.fine("found key for : " +
May not need the space between `for` and `:`.
src/java.base/share/classes/sun/security/ssl/X509KeyManagerImpl.java line 371:
> 369: SSLLogger.fine("found key for : " +
> 370: "keystore builder index = " + builderIndex +
> 371: " alias = " + alias, (Object[]) certs);
Suggestion: add a `,` after the first `"`. That can separate the parameters
`index` and `alias` clearly.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1536987602
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1537029010
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1537000363
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1537178746
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1537120663
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1537139654
