Re: RFR JDK-8206925,,Support the certificate_authorities extension

[Xuelei Fan]

Hi Alexey,

It a good information for interop.  Thank you!  I will see how to 
workaround it.

Thanks,
Xuelei

On 5/13/2020 3:00 AM, Alexey Bakhtin wrote:
Hello Xuelei,

I’m not a reviewer but I have some comment which could be helpful for your 
implementation.
We’ve developed CA Extension in the OpenJSSE provider [1] and found an issue 
with a third party server implementations.
According to RFC-8446 specification [2] the maximum size of the CA extension is 
2^16 bytes. The maximum TLS record size is 2^14 bytes. In case of handshake 
message is bigger then maximum TLS record size, it should be splitted into 
several records. In fact, some server implementations does not allow 
ClientHello message bigger than the Maximum TLS record size and aborts 
connection immediately with “illegal_parameter” fatal alert.
This issue can be easily reproduced on the client side:
  1) put additional certificates into cacerts file, about 200 certs in total,
  2) enable certificate_authorities extension in the ClientHello message
  3) connect to https://www.google.com

[1] - 
https://github.com/openjsse/openjsse/blob/master/src/main/java/org/openjsse/sun/security/ssl/CertificateAuthorityExtension.java
[2] - https://tools.ietf.org/html/rfc8446#page-45

Thank you
Alexey

On 13 May 2020, at 00:43, Xuelei Fan <xuelei....@oracle.com> wrote:

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