RFR: 8295068: SSLEngine throws NPE parsing CertificateRequests

[Kevin Driver]

On Thu, 6 Jul 2023 07:05:46 GMT, Xue-Lei Andrew Fan <xuelei at openjdk.org> wrote:

>> JDK-8295068: an NPE is thrown when an invalid `id` is found to match up a `ClientCertificateType`; rather than throwing the `NPE`, we now throw an `IllegalArgumentException`. This does not seem to be a scenario where recovery is possible or desired, so the `IAE` should be the proper behavior.
>
> src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 134:
> 
>> 132:                     throw new IllegalArgumentException(id + " was " +
>> 133:                             "not a valid ClientCertificateType id");
>> 134:                 }
> 
> It may not comply to TLS specification if throwing exception here.  Unknown types should be ignored for compatibility.
> 
> - if (cct.isAvailable) {
> + if (cct != null && cct.isAvailable) {

@XueleiFan It looks like the relevant bit of the RFC (as you point out) is this: 


For historical reasons, the names of some client certificate types
      include the algorithm used to sign the certificate.  For example,
      in earlier versions of TLS, rsa_fixed_dh meant a certificate
      signed with RSA and containing a static DH key.  In TLS 1.2, this
      functionality has been obsoleted by the
      supported_signature_algorithms, and the certificate type no longer
      restricts the algorithm used to sign the certificate.  For
      example, if the server sends dss_fixed_dh certificate type and
      {{sha1, dsa}, {sha1, rsa}} signature types, the client MAY reply
      with a certificate containing a static DH key, signed with RSA-
      SHA1.


I had noticed the comments beginning on line 757 in CertificateRequest:


// For TLS 1.2, we no longer use the certificate_types field
// from the CertificateRequest message to directly determine
// the SSLPossession.  Instead, the choosePossession method
// will use the accepted signature schemes in the message to
// determine the set of acceptable certificate types to select from.


I noted that the id byte generated by the test case was 0x72, and this is definitely not a recognized/supported type in JSSE ["Values in the range 64-223 (decimal) inclusive are assigned via Specification Required"].

I chose `IllegalArgumentException` because it seemed values so far "out of range" would be undesirable, and perhaps I misinterpreted the commented paragraph above as implying that the `id` value needed to be one of the set of "acceptable certificate types". 

After referring back to the RFC, I agree that your proposed fix seems more in line with the intent of the RFC.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/14778#discussion_r1254616765