RFR JDK-8206925,,Support the certificate_authorities extension

[Sean Mullan]

On 5/5/20 2:29 PM, Xuelei Fan wrote:
> Hi,
> 
> Could I get the following update reviewed?
> 
> RFE: https://bugs.openjdk.java.net/browse/JDK-8206925
> CSR: https://bugs.openjdk.java.net/browse/JDK-8244441

We have previously used the syntax "enable[Extension]" when naming 
system properties that enable optional extensions. Thus, it seems this 
name would be more consistent: 
"jdk.tls.client.enableCertificateAuthoritiesExtension"

However, it is a bit long, so maybe we could abbreviate it to CA: 
"jdk.tls.client.enableCAExtension"

Also, it is a bit unfortunate that we have to have a system property to 
enable it. Can we not enable it based on whether the configured 
X509TrustManager.getAcceptedIssuers returns a non-empty list?

> Release-note: https://bugs.openjdk.java.net/browse/JDK-8244460
> webrev: http://cr.openjdk.java.net/~xuelei/8206925/webrev.00/

* src/java.base/share/classes/sun/security/ssl/CertificateRequest.java

Missing copyright update.

* src/java.base/share/classes/sun/security/ssl/SSLExtension.java

   748             // Switch on certificate_authorities extention in 
ClientHello?

typo: s/extention/extension

Is the '?' at the end intentional or a typo?

* 
src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java

   70                 if (!authorities.contains(encodedPrincipal)) {
   71                     authorities.add(encodedPrincipal);
   72                 }

Is it really necessary to remove duplicates? Seems kind of expensive to 
iterate over the list every single time for what should be a rare case.

  108         X500Principal[] getAuthorities() {

Here you know the size of the array up front so you could avoid using a 
List and populate the array directly.

* test/jdk/sun/security/ssl/X509KeyManager/CertificateAuthorities.java

The test doesn't seem to do much, other than make sure you can make a 
connection if the extension is enabled. Can you test the scenario below 
where you can show that the extension addresses the issue where the 
certificate selected may not be the one the peer can accept?

--Sean

> 
> The "certificate_authorities" extension is an optional extension 
> introduced in TLS 1.3 and used to indicate the certificate authorities 
> (CAs) which an endpoint supports and which SHOULD be used by the 
> receiving endpoint to guide certificate selection.
> 
> In TLS 1.2, this function is built in the CertificateRequest handshake 
> massage.
> 
> This function is supported in TLS 1.2 and prior versions. However, it is 
> not implemented in the TLS 1.3 implementation. Without this function, 
> the authentication certificate selected may be not the one the peer 
> could accepted, when there are multiple certificates available.
> 
> Thanks,
> Xuelei