RFR: 8322767: TLS full handshake is slow with PKCS12KeyStore and X509KeyManagerImpl [v2]

[Hai-May Chao]

On Mon, 25 Mar 2024 02:17:18 GMT, John Jiang <jjiang at openjdk.org> wrote:

>> 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 `{`.

Space added.

> 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`.

New method created.

> 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 `{`.

Fixed.

> 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.

Done as suggested.

> 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 `:`.

Space removed.

> 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.

"," added.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631064
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631147
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631089
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631262
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631176
PR Review Comment: https://git.openjdk.org/jdk/pull/17956#discussion_r1538631196