On Mon, 30 Aug 2021 14:56:29 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> 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.
Did you mean to change hc.peerRequestedCertSignSchemes to String? This field
would be used for algorithm constraints and more parameters are required there.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5257
