RFR: 8262186: Call X509KeyManager.chooseClientAlias once for all key types [v2]

[Weijun Wang]

On Fri, 27 Aug 2021 22:00:47 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> 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);

Good suggestion.

One more thing: `checkedKeyTypes` only looks at `ss.keyAlgorithm`. I know the other checks (`SignatureScheme.getPreferableAlgorithm` and `X509Authentication.valueOf`) also only look at `ss.keyAlgorithm`, but are we going to check for more (Ex: group name) later? In the meantime, I would suggest changing the parameter type of these methods from `SignatureScheme` to `String` so we know only `keyAlgorithm` is checked.

> 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");`

Sure.

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

Do not fully understand. `chooseServerAlias` can only take one key type. How do I can it only once?

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

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