On Thu, 6 Jul 2023 07:05:46 GMT, Xue-Lei Andrew Fan <xue...@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
