RFR: 8262186: Callback semantics of the method X509KeyManager.chooseClientAlias(...) [v2]

[Xue-Lei Andrew Fan]

On Fri, 27 Aug 2021 14:36:52 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> This code change collects all key types and runs `chooseClientAlias` only once.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   reorg src, new test case

src/java.base/share/classes/sun/security/ssl/CertificateMessage.java line 1047:

> 1045: 
> 1046:             Collection<String> checkedKeyTypes = new HashSet<>();
> 1047:             LinkedHashSet<String> allAuths = new LinkedHashSet<>();

I was wondering if it is necessary to use a linked hash set here.  Maybe, we can reuse the checkedKeyTypes for all checked key types, no matter it is supported or unsupported.  If a type is checked, ignore it.  Otherwise, add this type as checked and then move forward.  Then, we could use the checkedKeyTypes array list for supported key types, and avoid duplicated validation of duplicated types.  And the allAuths could be a array list for available authentications.


if (ss.keyAlgorithm in checkedKeyTypes) {
   // log: "unsupported or duplicated authentication scheme"
   continue;
}

checkedKeyTypes.add(ss.keyAlgorithm);
 ...
availableAuthns.add(ss.keyAlgorithm);

src/java.base/share/classes/sun/security/ssl/CertificateMessage.java line 1082:

> 1080:                     continue;
> 1081:                 }
> 1082:                 allAuths.add(ss.keyAlgorithm);

As the use of HashSet, duplicated key algorithms could be filter out here.  It's a good point.  But the checking of the previous steps are still duplicated.  We may be able to have an improvement to avoid the duplicated checking.  See the comment at the declaration of allAuths.

src/java.base/share/classes/sun/security/ssl/CertificateMessage.java line 1090:

> 1088:                 if (SSLLogger.isOn && SSLLogger.isOn("ssl,handshake")) {
> 1089:                     SSLLogger.warning(
> 1090:                             "Unavailable authentication scheme: " + allAuths);

The message may be missleading as it only shows a part of the request authentication scheme.  It is different from the previous code, which check the scheme one by one.  I may just log the message as:

`SSLLogger.warning("No available authentication scheme");`

src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 735:

> 733: 
> 734:             Collection<String> checkedKeyTypes = new HashSet<>();
> 735:             LinkedHashSet<String> allAuths = new LinkedHashSet<>();

See the comments in CertificateMessage.java.

src/java.base/share/classes/sun/security/ssl/X509Authentication.java line 235:

> 233:                     chc.peerSupportedAuthorities == null ? null :
> 234:                             chc.peerSupportedAuthorities.clone(),
> 235:                     (SSLSocket) chc.conContext.transport);

Do you want to use pattern matching so as to avoid the cast?

`if (chc.conContext.transport instanceof SSLSocketImpl sslSocket)`

src/java.base/share/classes/sun/security/ssl/X509Authentication.java line 236:

> 234:                             chc.peerSupportedAuthorities.clone(),
> 235:                     (SSLSocket) chc.conContext.transport);
> 236:         } else if (chc.conContext.transport instanceof SSLEngineImpl) {

And one more line that pattern matching could be used.

src/java.base/share/classes/sun/security/ssl/X509Authentication.java line 289:

> 287:         X509ExtendedKeyManager km = shc.sslContext.getX509KeyManager();
> 288:         String serverAlias = null;
> 289:         for (String keyType : keyTypes) {

What do you think if we update the createServerPossession to call chooseServerAlias only once?  A similar problem could occur in server side, I think.  Keeping the behavior consistent between client and server may easy the key manager development and customization.

src/java.base/share/classes/sun/security/ssl/X509Authentication.java line 295:

> 293:                                 shc.peerSupportedAuthorities.clone(),
> 294:                         (SSLSocket) shc.conContext.transport);
> 295:             } else if (shc.conContext.transport instanceof SSLEngineImpl) {

Two more lines that pattern matching could be used.

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

PR: https://git.openjdk.java.net/jdk/pull/5257