RFR JDK-8206925,,Support the certificate_authorities extension

[Xuelei Fan]

Updated webrev: http://cr.openjdk.java.net/~xuelei/8206925/webrev.01/

On 5/12/2020 12:40 PM, Sean Mullan wrote:
> 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"
> 
"enableCAExtension" looks fine, but it is not as instinctive as 
"indicateCertificateAuthorities".

We used to use "enableXXExtension" because normally there is only one 
behavior for the extension.  However, for the Certificate Authorities 
extension, it could be requested by server side to indicate client cert 
selection, or by client side to indicate server cert selection.  It is 
not straightforward to know if "enableCAExtension" means accepting 
server request, or produce client request.

It is not expected to use this extension regularly.

Please let me know if you still prefer to use "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?
> 
We can do that on server side, but there are compatibility impact on 
client behavior if we did it in client side.  See #2 in the 
"Specification" section.

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

> * 
> 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.
> 
Good point!  Update here, and revert the update for 
CertificateRequest.java as well.

>   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.
> 
Good catch!

> * 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?
> 
With this update, the certificate selected must be the one the peer can 
accept.  It may be helpful to add test cases to make sure the connection 
is terminated if no certification can be selected.  I will try to add 
more test cases.

Thanks,
Xuelei

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