Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v30]
On Fri, 9 Jun 2023 19:48:13 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > fixed server payload once more Latest change looks good. - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1585183418
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v30]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: fixed server payload once more - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/e69abb3f..50a45b5d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=29 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=28-29 Stats: 9 lines in 1 file changed: 0 ins; 2 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v29]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: uniformity in keystore paths - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/6f8021f7..e69abb3f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=28 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=27-28 Stats: 2 lines in 2 files changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v28]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: test case now catching the proper variety of SSLException (the message is now correct) - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/78ad62aa..6f8021f7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=27 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=26-27 Stats: 40 lines in 1 file changed: 9 ins; 3 del; 28 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v27]
> 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 31 additional commits since the last revision: - format server payload to 80 chars - Merge remote-tracking branch 'upstream/master' into JDK-8294985 - latest version of TLS1.2 test - undo import changes - undo import changes - Merge branch 'master' of github.com:openjdk/jdk into JDK-8294985 - wrapping - addressed remaining code review comments - Merge branch 'master' of github.com:openjdk/jdk into JDK-8294985 - run tests with othervm - ... and 21 more: https://git.openjdk.org/jdk/compare/815bad3d...78ad62aa - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/b90cee85..78ad62aa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=26 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=25-26 Stats: 53610 lines in 743 files changed: 44001 ins; 7004 del; 2605 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v26]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: latest version of TLS1.2 test - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/af035c39..b90cee85 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=25 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=24-25 Stats: 83 lines in 2 files changed: 37 ins; 36 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v25]
On Wed, 31 May 2023 07:14:39 GMT, Daniel Jeliński wrote: >> When we put in the debug field into the template, it was to allow folks to >> quickly add debugging output to their test, but in general it won't be >> turned on during regular test runs. > > I'd like to enable this in the test runs as well; otherwise the changes to > `toString` method will not be tested. Ah! Thanks for clarifying. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211849050
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v25]
On Wed, 31 May 2023 06:27:41 GMT, Bradford Wetmore wrote: >> test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 29: >> >>> 27: * @library /test/lib >>> 28: * @summary SSLEngine throws IAE during parsing of X500Principal >>> 29: * @run main/othervm TestBadDNForPeerCA >> >> Suggestion: >> >> * @run main/othervm TestBadDNForPeerCA >> * @run main/othervm -Djavax.net.debug=all TestBadDNForPeerCA >> >> and then remove the `debug` field. > > When we put in the debug field into the template, it was to allow folks to > quickly add debugging output to their test, but in general it won't be turned > on during regular test runs. I'd like to enable this in the test runs as well; otherwise the changes to `toString` method will not be tested. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211196467
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v25]
On Tue, 30 May 2023 19:24:09 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with two additional > commits since the last revision: > > - undo import changes > - undo import changes test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA12.java line 66: > 64: > 65: // this contains a server response with invalid DNs > 66: private static final byte[] serverPayload = > Base64.getDecoder().decode( I executed this test with debug output enabled, and didn't see any invalid DNs in the test output. The exception thrown was `Fatal (CERTIFICATE_UNKNOWN): No trusted certificate found`. Are you using the right server payload here? - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211198613
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v25]
On Wed, 31 May 2023 05:10:34 GMT, Daniel Jeliński wrote: >> Kevin Driver has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - undo import changes >> - undo import changes > > test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 29: > >> 27: * @library /test/lib >> 28: * @summary SSLEngine throws IAE during parsing of X500Principal >> 29: * @run main/othervm TestBadDNForPeerCA > > Suggestion: > > * @run main/othervm TestBadDNForPeerCA > * @run main/othervm -Djavax.net.debug=all TestBadDNForPeerCA > > and then remove the `debug` field. When we put in the debug field into the template, it was to allow folks to quickly add debugging output to their test, but in general it won't be turned on during regular test runs. > test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 160: > >> 158: serverIn = ByteBuffer.allocateDirect(65536); >> 159: >> 160: cTOs = ByteBuffer.allocateDirect(65536); > > not used - you immediately overwrite this value in runTest He updated the Template code but didn't take all of the unused stuff out. I'm ok with it either way. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211146002 PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211147351
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v25]
On Tue, 30 May 2023 19:24:09 GMT, Kevin Driver wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - undo import changes
> - undo import changes
test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 29:
> 27: * @library /test/lib
> 28: * @summary SSLEngine throws IAE during parsing of X500Principal
> 29: * @run main/othervm TestBadDNForPeerCA
Suggestion:
* @run main/othervm TestBadDNForPeerCA
* @run main/othervm -Djavax.net.debug=all TestBadDNForPeerCA
and then remove the `debug` field.
test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 55:
> 53: private SSLEngine serverEngine; // server Engine
> 54: private ByteBuffer serverIn;// read side of serverEngine
> 55: private ByteBuffer clientOut;// read side of serverEngine
Not used
test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 140:
> 138: System.out.println("injecting client hello");
> 139:
> 140: for (int i = 0; i < 10; i++) { //retry if survived
Is this loop really needed?
test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 160:
> 158: serverIn = ByteBuffer.allocateDirect(65536);
> 159:
> 160: cTOs = ByteBuffer.allocateDirect(65536);
not used - you immediately overwrite this value in runTest
test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA12.java line 29:
> 27: * @library /test/lib
> 28: * @summary SSLEngine throws IAE during parsing of X500Principal
> 29: * @run main/othervm TestBadDNForPeerCA12
Suggestion:
* @run main/othervm TestBadDNForPeerCA12
* @run main/othervm -Djavax.net.debug=all TestBadDNForPeerCA12
and then remove the debug field.
test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA12.java line 182:
> 180: clientOut = ByteBuffer.wrap("Hi Server, I'm Client".getBytes());
> 181:
> 182: sTOc = ByteBuffer.allocateDirect(65536);
not used - you immediately overwrite this value in runTest
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211096065
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211098585
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211100260
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211099134
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211097088
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211101845
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v25]
On Tue, 30 May 2023 19:24:09 GMT, Kevin Driver wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - undo import changes
> - undo import changes
These are minor nits, can go as is.
test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA12.java line 147:
> 145: createBuffers();
> 146:
> 147: System.out.println("forcing client hello");
"Create" rather than "forcing?"
test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA12.java line 148:
> 146:
> 147: System.out.println("forcing client hello");
> 148: //sTOc = ByteBuffer.wrap(serverHello);
Might as well delete this. Dead code is confusing.
test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA12.java line 161:
> 159:
> 160: sTOc.compact();
> 161: cTOs.compact();
It doesn't really matter since the code will bomb out, but I don't think this
line is doing what you expected. cTOs is already pointing at the beginning of
the Buffer.
-
Marked as reviewed by wetmore (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1452190853
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211077478
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211077197
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1211080473
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v25]
On Tue, 30 May 2023 19:24:09 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with two additional > commits since the last revision: > > - undo import changes > - undo import changes My concerns for tests were addressed. Thanks! - Marked as reviewed by xuelei (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1451732655
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v25]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with two additional commits since the last revision: - undo import changes - undo import changes - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/2a35a7cc..af035c39 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=24 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=23-24 Stats: 28 lines in 2 files changed: 3 ins; 14 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v24]
On Tue, 30 May 2023 15:23:39 GMT, Sean Mullan 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 26 additional >> commits since the last revision: >> >> - Merge branch 'master' of github.com:openjdk/jdk into JDK-8294985 >> - wrapping >> - addressed remaining code review comments >> - 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 >> - ... and 16 more: https://git.openjdk.org/jdk/compare/bbf628a3...2a35a7cc > > src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java > line 28: > >> 26: package sun.security.ssl; >> 27: >> 28: import sun.security.ssl.SSLExtension.ExtensionConsumer; > > Please keep the previous import order for consistency with other code in this > package. All code in `sun.security.ssl` generally orders the imports as > `java`, `javax`, then internal (`sun`), etc. we need a code style template for IDEs :) - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1210712213
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v24]
On Tue, 30 May 2023 14:59:11 GMT, Kevin Driver 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 26 additional > commits since the last revision: > > - Merge branch 'master' of github.com:openjdk/jdk into JDK-8294985 > - wrapping > - addressed remaining code review comments > - 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 > - ... and 16 more: https://git.openjdk.org/jdk/compare/5217509f...2a35a7cc I'll defer to Brad for a review of the tests, but otherwise looks good to me. - Marked as reviewed by mullan (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1451270775
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v24]
On Tue, 30 May 2023 14:59:11 GMT, Kevin Driver 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 26 additional > commits since the last revision: > > - Merge branch 'master' of github.com:openjdk/jdk into JDK-8294985 > - wrapping > - addressed remaining code review comments > - 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 > - ... and 16 more: https://git.openjdk.org/jdk/compare/38960d7b...2a35a7cc src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java line 28: > 26: package sun.security.ssl; > 27: > 28: import sun.security.ssl.SSLExtension.ExtensionConsumer; Please keep the previous import order for consistency with other code in this package. All code in `sun.security.ssl` generally orders the imports as `java`, `javax`, then internal (`sun`), etc. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1210451045
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v24]
> 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 26 additional commits since the last revision: - Merge branch 'master' of github.com:openjdk/jdk into JDK-8294985 - wrapping - addressed remaining code review comments - 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 - ... and 16 more: https://git.openjdk.org/jdk/compare/805511ea...2a35a7cc - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/16d6d74e..2a35a7cc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=23 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=22-23 Stats: 2692 lines in 105 files changed: 1642 ins; 582 del; 468 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v23]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: wrapping - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/9bd9b458..16d6d74e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=22 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=21-22 Stats: 22 lines in 1 file changed: 21 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v21]
On Fri, 26 May 2023 23:00:40 GMT, Bradford Wetmore 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
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v22]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: addressed remaining code review comments - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/a0bfd0c7..9bd9b458 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=21 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=20-21 Stats: 46 lines in 2 files changed: 1 ins; 38 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v21]
On Fri, 26 May 2023 21:39:29 GMT, Kevin Driver 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
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v19]
On Tue, 23 May 2023 18:15:57 GMT, Xue-Lei Andrew Fan 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 17 additional >> commits since the last revision: >> >> - 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 >> - fix bug id in test header >> - reworked example into a jtreg test >> - whitespace adjustments >> - all review comments applied >> - optimize imports and change toString >> - review comments addressed >> - ... and 7 more: https://git.openjdk.org/jdk/compare/47d94ef0...d9f0c667 > > test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 29: > >> 27: * @summary verify correct exception handling in the event of an >> unparseable >> 28: * DN in the peer CA >> 29: */ > > Please run tests for JSSE/TLS in othervm mode. Otherwise, the behavior may > be impacted with each other, and the failure debugging is pretty hard. For > an example, please refer to > test/jdk/javax/net/ssl/SSLEngine/IllegalHandshakeMessage.java See: https://github.com/openjdk/jdk/pull/13466/commits/67506a21231793df250e08ea090e6c3461d7b4f8 - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1207388554
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v21]
> 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/2fcd42f3...a0bfd0c7 - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/b1382e57..a0bfd0c7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=20 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=19-20 Stats: 28953 lines in 542 files changed: 21073 ins; 3066 del; 4814 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v20]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with four additional commits since the last revision: - working testcase for CertificateRequest for TLS1.2 - new version of second test - initial commit of second test - reintroduce bug id to test header - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/d9f0c667..b1382e57 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=19 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=18-19 Stats: 217 lines in 2 files changed: 217 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v19]
On Mon, 22 May 2023 19:41:25 GMT, Kevin Driver 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 17 additional > commits since the last revision: > > - 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 > - fix bug id in test header > - reworked example into a jtreg test > - whitespace adjustments > - all review comments applied > - optimize imports and change toString > - review comments addressed > - ... and 7 more: https://git.openjdk.org/jdk/compare/1950bd92...d9f0c667 Changes requested by xuelei (Reviewer). test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 29: > 27: * @summary verify correct exception handling in the event of an > unparseable > 28: * DN in the peer CA > 29: */ Please run tests for JSSE/TLS in othervm mode. Otherwise, the behavior may be impacted with each other, and the failure debugging is pretty hard. - PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1440319320 PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1202812974
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v19]
On Tue, 23 May 2023 16:48:52 GMT, Kevin Driver wrote: >> test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 27: >> >>> 25: * @test >>> 26: * @library /test/lib >>> 27: * @summary verify correct exception handling in the event of an >>> unparseable >> >> Missing @bug field. > > I was asked to remove it previously. I can add it back. Just looking for a > consistent POV here. Generally, the @bug tag should be there to easy maintenance. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1202809318
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v19]
On Tue, 23 May 2023 15:09:39 GMT, Bradford Wetmore 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 17 additional >> commits since the last revision: >> >> - 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 >> - fix bug id in test header >> - reworked example into a jtreg test >> - whitespace adjustments >> - all review comments applied >> - optimize imports and change toString >> - review comments addressed >> - ... and 7 more: https://git.openjdk.org/jdk/compare/98f1821e...d9f0c667 > > test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 27: > >> 25: * @test >> 26: * @library /test/lib >> 27: * @summary verify correct exception handling in the event of an >> unparseable > > Missing @bug field. I was asked to remove it previously. I can add it back. Just looking for a consistent POV here. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1202704091
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v19]
On Mon, 22 May 2023 19:41:25 GMT, Kevin Driver 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 17 additional > commits since the last revision: > > - 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 > - fix bug id in test header > - reworked example into a jtreg test > - whitespace adjustments > - all review comments applied > - optimize imports and change toString > - review comments addressed > - ... and 7 more: https://git.openjdk.org/jdk/compare/6825bcbe...d9f0c667 test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java line 27: > 25: * @test > 26: * @library /test/lib > 27: * @summary verify correct exception handling in the event of an > unparseable Missing @bug field. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1202513519
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v19]
> 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 17 additional commits since the last revision: - 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 - fix bug id in test header - reworked example into a jtreg test - whitespace adjustments - all review comments applied - optimize imports and change toString - review comments addressed - ... and 7 more: https://git.openjdk.org/jdk/compare/bdf0d8ad...d9f0c667 - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/10c18fa1..d9f0c667 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=18 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=17-18 Stats: 21455 lines in 699 files changed: 12606 ins; 6112 del; 2737 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v15]
On Fri, 19 May 2023 17:49:07 GMT, Kevin Driver wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - fix bug id in test header
> - reworked example into a jtreg test
test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 48:
> 46:
> 47: // Test was originally written for TLSv1.2
> 48: private static String proto = "TLSv1.2";
Many of these could final. Some of these are no longer used.
test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 86:
> 84: * Main entry point for this demo.
> 85: */
> 86: public static void main(String args[]) throws Exception {
Nit. C Style args here instead of Java style.
test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 104:
> 102: }
> 103:
> 104: static Test8294985 fuzzdemoref = null;
No longer used.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1199338944
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1199339757
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1199339499
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v18]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: additional code review comments - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/8b672b0c..10c18fa1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=17 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=16-17 Stats: 12 lines in 1 file changed: 2 ins; 4 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v18]
On Fri, 19 May 2023 19:00:39 GMT, Sean Mullan wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> additional code review comments
>
> test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 28:
>
>> 26: * @bug 8164879
>> 27: * @library /test/lib
>> 28: * @summary test for proper exception handling
>
> Suggest adding more details here, ex: "Check that an improperly encoded CA
> distinguished name causes a handshake failure"
addressed
> test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 45:
>
>> 43:
>> 44:
>> 45: public class Test8294985 {
>
> I would avoid putting the bug number in the test name and use something more
> descriptive, like InvalidEncodedCaName.
addressed
> test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 64:
>
>> 62: + "/../../../../javax/net/ssl/etc/keystore";
>> 63:
>> 64: private static byte[] payload = Base64.getDecoder().decode(
>
> Can you add a comment as to what is in this payload?
addressed
> test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 91:
>
>> 89: }
>> 90:
>> 91: System.out.println("payload len:" + payload.length);
>
> Is this println necessary?
addressed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1200954942
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1200955022
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1200955117
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1200955187
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v15]
On Fri, 19 May 2023 20:21:16 GMT, Bradford Wetmore wrote:
>> Kevin Driver has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - fix bug id in test header
>> - reworked example into a jtreg test
>
> test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 48:
>
>> 46:
>> 47: // Test was originally written for TLSv1.2
>> 48: private static String proto = "TLSv1.2";
>
> Many of these could final. Some of these are no longer used.
addressed
> test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 86:
>
>> 84: * Main entry point for this demo.
>> 85: */
>> 86: public static void main(String args[]) throws Exception {
>
> Nit. C Style args here instead of Java style.
addressed
> test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 104:
>
>> 102: }
>> 103:
>> 104: static Test8294985 fuzzdemoref = null;
>
> No longer used.
addressed
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1200955290
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1200955420
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1200955349
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v15]
On Fri, 19 May 2023 17:49:07 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with two additional > commits since the last revision: > > - fix bug id in test header > - reworked example into a jtreg test Mostly nits here. Take or leave... - PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1435046400
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v17]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: rename class and remove bug id from test header - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/40a35bc1..8b672b0c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=16 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=15-16 Stats: 8 lines in 1 file changed: 1 ins; 2 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v16]
On Fri, 19 May 2023 19:38:09 GMT, Kevin Driver wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional
> commit since the last revision:
>
> removing block that isn't reached
test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 28:
> 26: * @bug 8164879
> 27: * @library /test/lib
> 28: * @summary test for proper exception handling
Suggest adding more details here, ex: "Check that an improperly encoded CA
distinguished name causes a handshake failure"
test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 45:
> 43:
> 44:
> 45: public class Test8294985 {
I would avoid putting the bug number in the test name and use something more
descriptive, like InvalidEncodedCaName.
test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 64:
> 62: + "/../../../../javax/net/ssl/etc/keystore";
> 63:
> 64: private static byte[] payload = Base64.getDecoder().decode(
Can you add a comment as to what is in this payload?
test/jdk/sun/security/ssl/SSLEngineImpl/Test8294985.java line 91:
> 89: }
> 90:
> 91: System.out.println("payload len:" + payload.length);
Is this println necessary?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r119928
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1199283420
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1199283669
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1199284615
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v16]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: removing block that isn't reached - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/d4ffde32..40a35bc1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=15 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=14-15 Stats: 16 lines in 1 file changed: 0 ins; 16 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v15]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with two additional commits since the last revision: - fix bug id in test header - reworked example into a jtreg test - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/cae5e1bc..d4ffde32 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=14 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=13-14 Stats: 204 lines in 1 file changed: 204 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v14]
On Thu, 18 May 2023 23:28:24 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > whitespace adjustments New code LGTM, minus the one outstanding test issue. Pending that, I can sponsor. - Marked as reviewed by wetmore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1433626170
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v13]
On Thu, 18 May 2023 22:33:30 GMT, Bradford Wetmore wrote: >> > src="https://github.com/openjdk/jdk/assets/1783591/c549e612-5ec7-47ce-add5-828cf79c31d9";> > > Slacked with Kevin and pointed out locations. Slacked with Kevin and resolved my concern. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198406416
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v14]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: whitespace adjustments - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/335fb052..cae5e1bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=13 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=12-13 Stats: 21 lines in 2 files changed: 8 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v13]
On Thu, 18 May 2023 22:02:55 GMT, Kevin Driver wrote: >> This might be something on your end @bradfordwetmore. I opened this in my >> IDE and went to the end of the line... It was right under the "String >> clientAlias" capital "S". > > src="https://github.com/openjdk/jdk/assets/1783591/c549e612-5ec7-47ce-add5-828cf79c31d9";> Slacked with Kevin and pointed out locations. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198365130
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v13]
On Thu, 18 May 2023 21:52:37 GMT, Bradford Wetmore wrote: >> Kevin Driver has updated the pull request incrementally with one additional >> commit since the last revision: >> >> all review comments applied > > src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 385: > >> 383: X509ExtendedKeyManager km = >> chc.sslContext.getX509KeyManager(); >> 384: String clientAlias = null; >> 385: > > Many of these lines are > 80 chars, which makes side-by-side challenging. > Please keep line changes to a max of 80 chars. > > Didn't check the other file, but think I saw similar things there. This might be something on your end @bradfordwetmore. I opened this in my IDE and went to the end of the line... It was right under the "String clientAlias" capital "S". - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198346964
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v13]
On Thu, 18 May 2023 22:00:35 GMT, Kevin Driver wrote: >> src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line >> 385: >> >>> 383: X509ExtendedKeyManager km = >>> chc.sslContext.getX509KeyManager(); >>> 384: String clientAlias = null; >>> 385: >> >> Many of these lines are > 80 chars, which makes side-by-side challenging. >> Please keep line changes to a max of 80 chars. >> >> Didn't check the other file, but think I saw similar things there. > > This might be something on your end @bradfordwetmore. I opened this in my IDE > and went to the end of the line... It was right under the "String > clientAlias" capital "S". https://github.com/openjdk/jdk/assets/1783591/c549e612-5ec7-47ce-add5-828cf79c31d9";> - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198348358
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v13]
On Thu, 18 May 2023 17:49:07 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > all review comments applied src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 385: > 383: X509ExtendedKeyManager km = > chc.sslContext.getX509KeyManager(); > 384: String clientAlias = null; > 385: Many of these lines are > 80 chars, which makes side-by-side challenging. Please keep line changes to a max of 80 chars. Didn't check the other file, but think I saw similar things there. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198340417
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v13]
On Thu, 18 May 2023 17:49:07 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > all review comments applied Marked as reviewed by xuelei (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1433195956
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v13]
On Thu, 18 May 2023 17:49:07 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > all review comments applied Marked as reviewed by mullan (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1433176661
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v13]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: all review comments applied - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/e33a9b0b..335fb052 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=12 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=11-12 Stats: 15 lines in 2 files changed: 8 ins; 0 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v11]
On Thu, 18 May 2023 16:58:50 GMT, Kevin Driver wrote:
>> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
>> line 290:
>>
>>> 288: shc.peerSupportedAuthorities = spec.getAuthorities();
>>> 289: } catch (IllegalArgumentException iae) {
>>> 290: shc.conContext.fatal(Alert.DECODE_ERROR,
>>> "X500Principal could not be parsed", iae);
>>
>> In the context, it may be easier to catch the idea if the message is about
>> the authorities, and easier to update getAuthorities() implementation, for
>> example X500Principal is not used any longer, if needed in the future.
>>
>> - "X500Principal could not be parsed"
>> + "Peer authorities could not be parsed"
>
> I'm inclined to keep the current version. It seems more specific in guiding
> the caller to the fix needed. However, I understand your point.
>
> @seanjmullan comments?
I tend to agree with Xuelei in that we should try to use terms as specified in
the TLS RFCs in error messages as that will give a user a better indication of
where the issue is. I would even be a bit more specific and suggest:
"The distinguished names of the peer's certificate authorities could not be
parsed"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198073492
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v11]
On Thu, 18 May 2023 16:42:45 GMT, Xue-Lei Andrew Fan wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments addressed
>
> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
> line 148:
>
>> 146: builder.append(principal.toString());
>> 147: } catch (IllegalArgumentException iae) {
>> 148: builder.append("unparseable X500Principal: " +
>> iae.getMessage());
>
> Throwable.getMessage() may return `null`. I may use iae.toString() instead.
>
> `builder.append("unparseable X500Principal: " + iae);`
`toString` would be called implicitly.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198052098
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v11]
On Thu, 18 May 2023 16:48:34 GMT, Xue-Lei Andrew Fan wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments addressed
>
> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
> line 290:
>
>> 288: shc.peerSupportedAuthorities = spec.getAuthorities();
>> 289: } catch (IllegalArgumentException iae) {
>> 290: shc.conContext.fatal(Alert.DECODE_ERROR, "X500Principal
>> could not be parsed", iae);
>
> In the context, it may be easier to catch the idea if the message is about
> the authorities, and easier to update getAuthorities() implementation, for
> example X500Principal is not used any longer, if needed in the future.
>
> - "X500Principal could not be parsed"
> + "Peer authorities could not be parsed"
I'm inclined to keep the current version. It seems more specific in guiding the
caller to the fix needed. However, I understand your point.
@seanjmullan comments?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198061874
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v12]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: optimize imports and change toString - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/5a20371e..e33a9b0b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=10-11 Stats: 33 lines in 2 files changed: 14 ins; 5 del; 14 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v11]
On Thu, 18 May 2023 16:15:39 GMT, Kevin Driver wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comments addressed
src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
line 290:
> 288: shc.peerSupportedAuthorities = spec.getAuthorities();
> 289: } catch (IllegalArgumentException iae) {
> 290: shc.conContext.fatal(Alert.DECODE_ERROR, "X500Principal
> could not be parsed", iae);
In the context, it may be easier to catch the idea if the message is about the
authorities, and easier to update getAuthorities() implementation, for example
X500Principal is not used any longer, if needed in the future.
- "X500Principal could not be parsed"
+ "Peer authorities could not be parsed"
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198051462
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v11]
On Thu, 18 May 2023 16:15:39 GMT, Kevin Driver wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional
> commit since the last revision:
>
> review comments addressed
src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
line 148:
> 146: builder.append(principal.toString());
> 147: } catch (IllegalArgumentException iae) {
> 148: builder.append("unparseable X500Principal: " +
> iae.getMessage());
Throwable.getMessage() may return `null`. I may use iae.toString() instead.
`builder.append("unparseable X500Principal: " + iae);`
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198044980
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v11]
On Thu, 18 May 2023 16:15:39 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > review comments addressed src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java line 33: > 31: import java.text.MessageFormat; > 32: import java.util.*; > 33: import javax.net.ssl.SSLException; This import is not needed anymore. src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 35: > 33: import java.util.*; > 34: import javax.net.ssl.SSLEngine; > 35: import javax.net.ssl.SSLException; This import is not needed anymore. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198042070 PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198043800
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]
On Wed, 17 May 2023 15:49:27 GMT, Xue-Lei Andrew Fan wrote: >>> Do you have any plans to write a test? If not, the bug needs a `noreg` >>> label. >> >> As discussed internally, the test that surfaced this issue will be >> incorporated into regular testing. I have added `noreg-other` since none of >> the other labels seemed quite appropriate. > >> @driverkt Please do not rebase or force-push to an active PR as it >> invalidates existing review comments. Note for future reference, the bots >> always squash all changes into a single commit automatically as part of the >> integration. See [OpenJDK Developers’ >> Guide](https://openjdk.org/guide/#working-with-pull-requests) for more >> information. > > @driverkt I'm not very sure. But per this message and the webrevs, it looks > like the git use for the PR request might be able to improved by working on > git branches, so that you can push the commit for every little change, and > avoid force-push. @XueleiFan @seanjmullan all comments have been addressed. - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1553300216
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v11]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: review comments addressed - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/0aa93e2a..5a20371e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=09-10 Stats: 15 lines in 2 files changed: 3 ins; 1 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v10]
On Thu, 18 May 2023 12:42:20 GMT, Sean Mullan wrote:
>> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
>> line 126:
>>
>>> 124: }
>>> 125:
>>> 126: X500Principal[] getAuthorities() throws
>>> IllegalArgumentException {
>>
>> IAE is unchecked exception, and should not be throwing explicitly in method
>> signature/statement. I'm not sure if this throwing is really helpful for
>> caller to check the exception.
>
> Yes, I agree with that comment. I suggest adding a comment above the method
> to remind callers they may need to catch IAE, something like:
>
> // This method will throw IllegalArgumentException if the X500Principal
> cannot be parsed.
Agreed. It's atypical (at least) to include a RuntimeException in the method
signature.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1198005604
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v10]
On Thu, 18 May 2023 04:04:17 GMT, Xue-Lei Andrew Fan wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> rework based upon code review comments
>
> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
> line 289:
>
>> 287: shc.peerSupportedAuthorities = spec.getAuthorities();
>> 288: } catch (IllegalArgumentException iae) {
>> 289: shc.conContext.fatal(Alert.DECODE_ERROR, "X500Principal
>> could not be parsed");
>
> To easy debugging, I may use the exception with fatal(Alert alert, String
> diagnostic, Throwable cause). Considering this point, I may prefer to throw
> SSLException in getAuthorities() method.
>
>
> X500Principal[] getAuthorities() throws SSLException {
> ...
> try {
> shc.peerSupportedAuthorities = spec.getAuthorities();
> } catch (SSLException ssle) {
> shc.conContext.fatal(Alert.DECODE_ERROR, "Cannot parse peer supported
> authorities", ssle);
> }
> @XueleiFan Seems like an unnecessary wrapping of IAE when it is going to be
> rethrown anyway.
I'm fine if IAE get checked for every call to getAuthorities().
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197881532
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v10]
On Wed, 17 May 2023 21:54:20 GMT, Kevin Driver wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional
> commit since the last revision:
>
> rework based upon code review comments
src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 738:
> 736: try {
> 737: chc.peerSupportedAuthorities = crm.getAuthorities();
> 738: }
Move the `catch` line up to line 738.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197776053
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v10]
On Thu, 18 May 2023 03:51:55 GMT, Xue-Lei Andrew Fan wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> rework based upon code review comments
>
> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
> line 126:
>
>> 124: }
>> 125:
>> 126: X500Principal[] getAuthorities() throws
>> IllegalArgumentException {
>
> IAE is unchecked exception, and should not be throwing explicitly in method
> signature/statement. I'm not sure if this throwing is really helpful for
> caller to check the exception.
Yes, I agree with that comment. I suggest adding a comment above the method to
remind callers they may need to catch IAE, something like:
// This method will throw IllegalArgumentException if the X500Principal cannot
be parsed.
> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
> line 147:
>
>> 145: builder.append(principal.toString());
>> 146: } catch (IllegalArgumentException iae) {
>> 147: builder.append("unparseable X500Principal");
>
> I may use the iae message as well for better debugging.
Yes, suggest:
`builder.append("unparseable X500Principal: " + iae.getMessage());`
> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
> line 289:
>
>> 287: shc.peerSupportedAuthorities = spec.getAuthorities();
>> 288: } catch (IllegalArgumentException iae) {
>> 289: shc.conContext.fatal(Alert.DECODE_ERROR, "X500Principal
>> could not be parsed");
>
> To easy debugging, I may use the exception with fatal(Alert alert, String
> diagnostic, Throwable cause). Considering this point, I may prefer to throw
> SSLException in getAuthorities() method.
>
>
> X500Principal[] getAuthorities() throws SSLException {
> ...
> try {
> shc.peerSupportedAuthorities = spec.getAuthorities();
> } catch (SSLException ssle) {
> shc.conContext.fatal(Alert.DECODE_ERROR, "Cannot parse peer supported
> authorities", ssle);
> }
@XueleiFan Seems like an unnecessary wrapping of IAE when it is going to be
rethrown anyway.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197768251
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197771043
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197773420
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v10]
On Wed, 17 May 2023 21:54:20 GMT, Kevin Driver wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional
> commit since the last revision:
>
> rework based upon code review comments
Similar comments for update in CertificateRequest.java
Similar comments for update in CertificateRequest.java
src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
line 126:
> 124: }
> 125:
> 126: X500Principal[] getAuthorities() throws IllegalArgumentException
> {
IAE is unchecked exception, and should not be throwing explicitly in method
signature/statement. I'm not sure if this throwing is really helpful for
caller to check the exception.
src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
line 147:
> 145: builder.append(principal.toString());
> 146: } catch (IllegalArgumentException iae) {
> 147: builder.append("unparseable X500Principal");
I may use the iae message as well for better debugging.
src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
line 289:
> 287: shc.peerSupportedAuthorities = spec.getAuthorities();
> 288: } catch (IllegalArgumentException iae) {
> 289: shc.conContext.fatal(Alert.DECODE_ERROR, "X500Principal
> could not be parsed");
To easy debugging, I may use the exception with fatal(Alert alert, String
diagnostic, Throwable cause). Considering this point, I may prefer to throw
SSLException in getAuthorities() method.
X500Principal[] getAuthorities() throws SSLException {
...
try {
shc.peerSupportedAuthorities = spec.getAuthorities();
} catch (SSLException ssle) {
shc.conContext.fatal(Alert.DECODE_ERROR, "Cannot parse peer supported
authorities", ssle);
}
-
Changes requested by xuelei (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1431961001
PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1431970382
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197326576
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197327083
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1197334523
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]
On Fri, 12 May 2023 20:14:56 GMT, Sean Mullan wrote: >> Kevin Driver has refreshed the contents of this pull request, and previous >> commits have been removed. Incremental views are not available. > > Do you have any plans to write a test? If not, the bug needs a `noreg` label. @seanjmullan @XueleiFan - ready for another round of reviews - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1552140738
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v10]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: rework based upon code review comments - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/3ffdfd63..0aa93e2a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=08-09 Stats: 52 lines in 2 files changed: 17 ins; 15 del; 20 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v9]
> 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 six additional commits since the last revision: - Merge branch 'master' of github.com:openjdk/jdk into JDK-8294985 - update copyright - reworking the fix in light of encouragement to change the problematic method signature - Update src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java Co-authored-by: Daniel Jelinski - updated copyright - fixes JDK-8294985: throw an SSLException wrapping the IAE - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/d02cfd1c..3ffdfd63 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=07-08 Stats: 1799 lines in 11 files changed: 1789 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]
On Fri, 12 May 2023 20:29:47 GMT, Kevin Driver wrote: >> Do you have any plans to write a test? If not, the bug needs a `noreg` label. > >> Do you have any plans to write a test? If not, the bug needs a `noreg` label. > > As discussed internally, the test that surfaced this issue will be > incorporated into regular testing. I have added `noreg-other` since none of > the other labels seemed quite appropriate. > @driverkt Please do not rebase or force-push to an active PR as it > invalidates existing review comments. Note for future reference, the bots > always squash all changes into a single commit automatically as part of the > integration. See [OpenJDK Developers’ > Guide](https://openjdk.org/guide/#working-with-pull-requests) for more > information. @driverkt I'm not very sure. But per this message and the webrevs, it looks like the git use for the PR request might be able to improved by working on git branches, so that you can push the commit for every little change, and avoid force-push. - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1551644675
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v8]
> 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 pull request now contains five commits: - update copyright - reworking the fix in light of encouragement to change the problematic method signature - Update src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java Co-authored-by: Daniel Jelinski - updated copyright - fixes JDK-8294985: throw an SSLException wrapping the IAE - Changes: https://git.openjdk.org/jdk/pull/13466/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=07 Stats: 50 lines in 2 files changed: 32 ins; 0 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v7]
> 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 five additional commits since the last revision: - update copyright - reworking the fix in light of encouragement to change the problematic method signature - Update src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java Co-authored-by: Daniel Jelinski - updated copyright - fixes JDK-8294985: throw an SSLException wrapping the IAE - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/0017c094..fa31ddb6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=05-06 Stats: 10844 lines in 494 files changed: 6394 ins; 1103 del; 3347 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]
On Mon, 15 May 2023 19:37:44 GMT, Sean Mullan wrote: >> It is not easy to understand the final behavior if throwing SSLException >> here. I would like to call `TransportContext.fatal()` directly to make the >> behavior more accuracy, by using Alert.DECODE_ERROR. > > You will need to pass in `TransportContext` as a parameter if you do that, > unless you go back changing the callers of `getAuthorities()` to catch > `IllegalArgumentException`. I'm now thinking it is better for the callers of > `getAuthorities()` to catch `IllegalArgumentException` and then call `fatal`. > > One other minor comment: > > - I would remove the word "successfully" as this is a failure case so it is > implied. Sean, agreed. I had considered the above previously. Also, I'll remove "successfully" with the next round of edits. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1194292927
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]
On Mon, 15 May 2023 19:17:18 GMT, Xue-Lei Andrew Fan wrote: >> Yes, let's wait for @XueleiFan > > It is not easy to understand the final behavior if throwing SSLException > here. I would like to call `TransportContext.fatal()` directly to make the > behavior more accuracy, by using Alert.DECODE_ERROR. You will need to pass in `TransportContext` as a parameter if you do that, unless you go back changing the callers of `getAuthorities()` to catch `IllegalArgumentException`. I'm now thinking it is better for the callers of `getAuthorities()` to catch `IllegalArgumentException` and then call `fatal`. One other minor comment: - I would remove the word "successfully" as this is a failure case so it is implied. - PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1194286609
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]
On Fri, 12 May 2023 20:30:04 GMT, Kevin Driver wrote:
>> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
>> line 136:
>>
>>> 134: } catch (IllegalArgumentException iae) {
>>> 135: throw new SSLException("X500Principal could not be
>>> parsed " +
>>> 136: "successfully", iae);
>>
>> Is it ok to throw a general `SSLException` here? Or do you need to call
>> `TransportContext.fatal()` so that additional cleanup happens? Perhaps
>> @XueleiFan would know.
>
> Yes, let's wait for @XueleiFan
It is not easy to understand the final behavior if throwing SSLException here.
I would like to call `TransportContext.fatal()` directly to make the behavior
more accuracy, by using Alert.DECODE_ERROR.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1194266764
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]
On Fri, 12 May 2023 20:14:56 GMT, Sean Mullan wrote:
> Do you have any plans to write a test? If not, the bug needs a `noreg` label.
As discussed internally, the test that surfaced this issue will be incorporated
into regular testing.
> src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
> line 136:
>
>> 134: } catch (IllegalArgumentException iae) {
>> 135: throw new SSLException("X500Principal could not be
>> parsed " +
>> 136: "successfully", iae);
>
> Is it ok to throw a general `SSLException` here? Or do you need to call
> `TransportContext.fatal()` so that additional cleanup happens? Perhaps
> @XueleiFan would know.
Yes, let's wait for @XueleiFan
-
PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1546257007
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1192769497
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]
On Thu, 11 May 2023 20:43:49 GMT, Kevin Driver wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - update copyright
> - reworking the fix in light of encouragement to change the problematic
> method signature
Do you have any plans to write a test? If not, the bug needs a `noreg` label.
src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
line 136:
> 134: } catch (IllegalArgumentException iae) {
> 135: throw new SSLException("X500Principal could not be
> parsed " +
> 136: "successfully", iae);
Is it ok to throw a general `SSLException` here? Or do you need to call
`TransportContext.fatal()` so that additional cleanup happens? Perhaps
@XueleiFan would know.
-
PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1546242429
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1192759621
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v6]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with two additional commits since the last revision: - update copyright - reworking the fix in light of encouragement to change the problematic method signature - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/87b47a03..0017c094 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=04-05 Stats: 54 lines in 2 files changed: 31 ins; 5 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v5]
On Thu, 11 May 2023 16:40:07 GMT, Kevin Driver 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 pull request now contains three commits: > > - Update > src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java > >Co-authored-by: Daniel Jelinski > - updated copyright > - fixes JDK-8294985: throw an SSLException wrapping the IAE Not sure if you have more changes coming, but there are still a few other places where IAE could be thrown. I would change `getAuthorities()` (in both CertificateRequest.java and CertificateAuthoritiesExtension.java) to catch `IllegalArgumentException` and rethrow it as an `SSLException`, as this will ensure all existing and future calls to this method are handled consistently. Also, I would change the `toString()` method to also catch IAE but not propagate it, instead print something like "unparseable X500Principal". - PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1423140554
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v5]
> 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 three additional commits since the last revision: - Update src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java Co-authored-by: Daniel Jelinski - updated copyright - fixes JDK-8294985: throw an SSLException wrapping the IAE - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/33366412..87b47a03 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=03-04 Stats: 99417 lines in 1498 files changed: 79732 ins; 8161 del; 11524 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v3]
On Tue, 9 May 2023 16:30:22 GMT, Xue-Lei Andrew Fan wrote: >> As for other examples of the `X500Principal(byte[] ..)` constructor being >> called in TLS packages, here are the ones that don't seem to be handled >> nicely currently: >> >> - `consume` in >> `CertificateAuthoritiesExtension.CRCertificateAuthoritiesConsumer` (could >> throw IAE, which is an uncaught RuntimeException) >> - `toString` in `CertificateAuthoritiesExtension.CertificateAuthoritiesSpec` >> (could throw IAE, which is an uncaught RuntimeException) >> - `consume` in `CertificateRequest.T10CertificateRequestConsumer` (could >> throw IAE, which is an uncaught RuntimeException) >> - `toString` in `CertificateRequest.T10CertificateRequestMessage` (could >> throw IAE, which is an uncaught RuntimeException) >> - `consume` in `CertificateRequest.T12CertificateRequestConsumer` (could >> throw IAE, which is an uncaught RuntimeException) >> - `toString` in `CertificateRequest.T12CertificateRequestMessage` (could >> throw IAE, which is an uncaught RuntimeException) >> >> I will look at making related changes in these spots as well. >> >> @XueleiFan wrt your other question about updating the `getAuthorities` >> method, I considered this, but it looks like it would involve changing a >> method signature for that method. This may be fine, but similar changes >> would need to be made in all the above places anyway, I suspect, unless we >> can pass information about the context in order to throw an >> `SSL(Protocol)Exception` and have that bubble-up to where `IOException`s are >> usually checked. >> >> @seanjmullan @XueleiFan thoughts on that? > >> I will look at making related changes in these spots as well. >> > OK. > >> @XueleiFan wrt your other question about updating the `getAuthorities` >> method, I considered this, but it looks like it would involve changing a >> method signature for that method. > > Changing the signature should be fine as it is a internal method. But I'm > fine if the calls to getAuthorities() have considered the impact of illegal > values of X500Principal. > > Anyway, this is a typical example to show how hard to use runtime exception. > From the viewpoint of X500Principal, and unchecked exception should be thrown > for invalid input values. But for the caller, it may need to check the input > values for sure everything is good. However, an unchecked exception cannot > be detected by Java compiler and thus the checking of unchecked exception > could be missed. I would suggest catching the IAE and rethrowing as an IOE (with the IAE as the cause) if the data is coming from the network. If it is not coming from the network, then don't necessarily rethrow as the IAE could indicate a bug in the JSSE code. I agree with @XueleiFan that it is ok to change internal method signatures, by adding more parameters or throwing exceptions. - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1542441464
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v3]
On Wed, 3 May 2023 22:11:20 GMT, Kevin Driver wrote: > I will look at making related changes in these spots as well. > OK. > @XueleiFan wrt your other question about updating the `getAuthorities` > method, I considered this, but it looks like it would involve changing a > method signature for that method. Changing the signature should be fine as it is a internal method. But I'm fine if the calls to getAuthorities() have considered the impact of illegal values of X500Principal. Anyway, this is a typical example to show how hard to use runtime exception. From the viewpoint of X500Principal, and unchecked exception should be thrown for invalid input values. But for the caller, it may need to check the input values for sure everything is good. However, an unchecked exception cannot be detected by Java compiler and thus the checking of unchecked exception could be missed. - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1540501498
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v4]
> 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 pull request now contains three commits: - Update src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java Co-authored-by: Daniel Jelinski - updated copyright - fixes JDK-8294985: throw an SSLException wrapping the IAE - Changes: https://git.openjdk.org/jdk/pull/13466/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=03 Stats: 8 lines in 1 file changed: 6 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v3]
On Fri, 28 Apr 2023 19:15:59 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java > > Co-authored-by: Daniel Jelinski As for other examples of the `X500Principal(byte[] ..)` constructor being called in TLS packages, here are the ones that don't seem to be handled nicely currently: - `consume` in `CertificateAuthoritiesExtension.CRCertificateAuthoritiesConsumer` (could throw IAE, which is an uncaught RuntimeException) - `toString` in `CertificateAuthoritiesExtension.CertificateAuthoritiesSpec` (could throw IAE, which is an uncaught RuntimeException) - `consume` in `CertificateRequest.T10CertificateRequestConsumer` (could throw IAE, which is an uncaught RuntimeException) - `toString` in `CertificateRequest.T10CertificateRequestMessage` (could throw IAE, which is an uncaught RuntimeException) - `consume` in `CertificateRequest.T12CertificateRequestConsumer` (could throw IAE, which is an uncaught RuntimeException) - `toString` in `CertificateRequest.T12CertificateRequestMessage` (could throw IAE, which is an uncaught RuntimeException) I will look at making related changes in these spots as well. @XueleiFan wrt your other question about updating the `getAuthorities` method, I considered this, but it looks like it would involve changing a method signature for that method. This may be fine, but similar changes would need to be made in all the above places anyway, I suspect, unless we can pass information about the context in order to throw an `SSL(Protocol)Exception` and have that bubble-up to where `IOException`s are usually checked. @seanjmullan @XueleiFan thoughts on that? - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1533818757
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v3]
On Fri, 28 Apr 2023 19:15:59 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java > > Co-authored-by: Daniel Jelinski Yes, I think we should check other calls in the TLS code to `new X500Principal()` that take the encoded bytes from the network to see if similar changes are needed. I would also pass the cause to the `fatal()` method as this will provide additional information as to the reason of the parsing failure for debugging purposes. - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1529997195
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v3]
On Fri, 28 Apr 2023 19:15:59 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > Update > src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java > > Co-authored-by: Daniel Jelinski There are two blocks called CertificateAuthoritiesSpec.getAuthorities(). Another call is in the CRCertificateAuthoritiesConsumer inner class. Did you check if both should be updated? Or Is it possible to update the getAuthorities() implementation directly? There are similar code in CertificateRequest. Did you have a chance to look at if it is impacted as well? Basically, the issue is caused by the X500Principal constructor with DER bytes. It may be good to check the call to the constructor and handle the IAE accordingly as well, for example in the CertificateAuthoritiesSpec.toString() method. - Changes requested by xuelei (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1407415460
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal
On Fri, 14 Apr 2023 05:00:48 GMT, Xue-Lei Andrew Fan wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > @driverkt Is [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) in > WJBS a closed bug? If it is cannot be public, would you mind add more > description about what problem this PR is addressing? Since this has already been marked `/integrate`, I can sponsor next week, in case @XueleiFan has any last minute comments. - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1528476828
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal
On Fri, 14 Apr 2023 05:00:48 GMT, Xue-Lei Andrew Fan wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > @driverkt Is [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) in > WJBS a closed bug? If it is cannot be public, would you mind add more > description about what problem this PR is addressing? @XueleiFan now that the bug is no longer marked confidential, please feel free to review. - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1527985858
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v3]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java Co-authored-by: Daniel Jelinski - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/d4a73407..968a2882 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=01-02 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v2]
On Fri, 14 Apr 2023 19:03:16 GMT, Kevin Driver wrote:
>> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985)
>
> Kevin Driver has updated the pull request incrementally with one additional
> commit since the last revision:
>
> updated copyright
src/java.base/share/classes/sun/security/ssl/CertificateAuthoritiesExtension.java
line 284:
> 282: shc.peerSupportedAuthorities = spec.getAuthorities();
> 283: } catch (IllegalArgumentException iae) {
> 284: throw new SSLException(iae);
Suggestion:
throw shc.conContext.fatal(Alert.ILLEGAL_PARAMETER,
"Certificate authority distinguished name is not
valid");
-
PR Review Comment: https://git.openjdk.org/jdk/pull/13466#discussion_r1180543939
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v2]
On Fri, 14 Apr 2023 19:03:16 GMT, Kevin Driver wrote: >> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) > > Kevin Driver has updated the pull request incrementally with one additional > commit since the last revision: > > updated copyright Try this: "/issue JDK-8294985" (I don't know if only the PR owner can do that one...) - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1527704070
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v2]
On Fri, 14 Apr 2023 04:33:15 GMT, Bradford Wetmore wrote: > Please update the copyright date to include "2022, 2023," > > Otherwise, LGTM. Fixed in https://github.com/openjdk/jdk/pull/13466/commits/d4a7340718dcdb769ad5eb6c8708e43284c14548. - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1509089968
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal [v2]
> Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Kevin Driver has updated the pull request incrementally with one additional commit since the last revision: updated copyright - Changes: - all: https://git.openjdk.org/jdk/pull/13466/files - new: https://git.openjdk.org/jdk/pull/13466/files/df4fbebc..d4a73407 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=13466&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13466.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13466/head:pull/13466 PR: https://git.openjdk.org/jdk/pull/13466
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal
On Thu, 13 Apr 2023 18:49:48 GMT, Kevin Driver wrote: > Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Is [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) in JBS a closed bug? - PR Comment: https://git.openjdk.org/jdk/pull/13466#issuecomment-1507925845
Re: RFR: 8294985: SSLEngine throws IAE during parsing of X500Principal
On Thu, 13 Apr 2023 18:49:48 GMT, Kevin Driver wrote: > Fixes: [JDK-8294985](https://bugs.openjdk.org/browse/JDK-8294985) Please update the copyright date to include "2022, 2023," Otherwise, LGTM. - Marked as reviewed by wetmore (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/13466#pullrequestreview-1384584301
