On Fri, 27 Aug 2021 22:00:47 GMT, Xue-Lei Andrew Fan <xue...@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
