On Fri, 26 May 2023 23:00:40 GMT, Bradford Wetmore <wetm...@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
