On Fri, 27 Aug 2021 14:36:52 GMT, Weijun Wang <wei...@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
