RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v21]

[Kevin Driver]

On Fri, 26 May 2023 23:00:40 GMT, Bradford Wetmore <wetmore at openjdk.org> wrote:

>> Kevin Driver has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 23 additional commits since the last revision:
>> 
>>  - Merge branch 'master' of github.com:openjdk/jdk into JDK-8294985
>>  - run tests with othervm
>>  - working testcase for CertificateRequest for TLS1.2
>>  - new version of second test
>>  - initial commit of second test
>>  - reintroduce bug id to test header
>>  - Merge remote-tracking branch 'upstream/master' into JDK-8294985
>>  - additional code review comments
>>  - rename class and remove bug id from test header
>>  - removing block that isn't reached
>>  - ... and 13 more: https://git.openjdk.org/jdk/compare/257152e8...a0bfd0c7
>
> test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 28:
> 
>> 26:  * @bug 8294985
>> 27:  * @library /test/lib
>> 28:  * @summary verify correct exception handling in the event of an unparseable
> 
> The @summary should generally match the bug synopsis:
> 
> SSLEngine throws IAE during parsing of X500Principal

addressed in https://github.com/openjdk/jdk/pull/13466/commits/9bd9b4583444f2b469bd39e445424d01524601f9

> test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 29:
> 
>> 27:  * @library /test/lib
>> 28:  * @summary verify correct exception handling in the event of an unparseable
>> 29:  *  DN in the peer CA
> 
> Generally this should be indented as well.  4 spaces minimum, or to be at the same position as the word "verify."  Your choice.

addressed in https://github.com/openjdk/jdk/pull/13466/commits/9bd9b4583444f2b469bd39e445424d01524601f9

> test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 49:
> 
>> 47: 
>> 48:     // Test was originally written for TLSv1.2
>> 49:     private static final String proto = "TLSv1.2";
> 
> I would suggest changing this to "TLSv1.3", just so it's immediately clear that you're testing the 1.3 code, not the 1.2 which is in the other test.

addressed in https://github.com/openjdk/jdk/pull/13466/commits/9bd9b4583444f2b469bd39e445424d01524601f9

> test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 65:
> 
>> 63:             + "/../../../../javax/net/ssl/etc/keystore";
>> 64: 
>> 65:     // the following contains a certificate with an invalid/unparseable
> 
> "The following ClientHello handshake message..." so that folks aren't wondering what this binary data actually is.

addressed in https://github.com/openjdk/jdk/pull/13466/commits/9bd9b4583444f2b469bd39e445424d01524601f9

> test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 67:
> 
>> 65:     // the following contains a certificate with an invalid/unparseable
>> 66:     // distinguished name
>> 67:     private static final byte[] payload = Base64.getDecoder().decode(
> 
> Minor nit:  It's a little inconsistent to have similar tests use two different encodings.  i.e. base64 vs. hex.   The advantage to base64 is that the decoder exists in JDK 8 which this bug will be backported to.  The HexConvert wasn't added until JDK 17, so when this is backported, they will have to convert it or add their own Hex converter (or use the BigInteger trick).

addressed in https://github.com/openjdk/jdk/pull/13466/commits/9bd9b4583444f2b469bd39e445424d01524601f9

> test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA12.java line 28:
> 
>> 26:  * @bug 8294985
>> 27:  * @library /test/lib
>> 28:  * @summary verify correct exception handling in the event of an unparseable
> 
> Same as above:
> 
> The @summary should generally match the bug synopsis:
> 
> SSLEngine throws IAE during parsing of X500Principal
> 
> and indention below.

addressed in https://github.com/openjdk/jdk/pull/13466/commits/9bd9b4583444f2b469bd39e445424d01524601f9

> test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA12.java line 215:
> 
>> 213:         }
>> 214:     }
>> 215: 
> 
> Nits:  I generally delete multiple blank lines in situations like this.
> 
>         }
>     }
> }

addressed in https://github.com/openjdk/jdk/pull/13466/commits/9bd9b4583444f2b469bd39e445424d01524601f9

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1210387386
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1210388001
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1210388504
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1210388799
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1210389671
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1210387703
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1210389289