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

[Bradford Wetmore]

On Fri, 26 May 2023 21:39:29 GMT, Kevin Driver <kdriver at openjdk.org> wrote:

>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> 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/a0c80510...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

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.

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.

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.

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

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.

test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA12.java line 215:

> 213:         }
> 214:     }
> 215: 

Nits:  I generally delete multiple blank lines in situations like this.

        }
    }
}

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1207457534
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1207459701
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1207467826
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1207469384
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1207461770
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1207458791
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1207485286