jdk
jdk copied to clipboard
openjdk
8044609: javax.net.debug options not working and documented as expected
The javax.net.debug TLS debug option is buggy since TLSv1.3 implementation was introduced many years ago.
Where "ssl" was previously a value to obtain all TLS debug traces (except network type dumps, verbose data), it now prints only a few lines for a standard client TLS connection.
The property parsing was also lax and allowed users to declare verbose logging options by themselves where the documentation stated that such verbose options were only meant to be used in conjunction with other TLS options :
System.err.println("help print the help messages");
System.err.println("expand expand debugging information");
System.err.println();
System.err.println("all turn on all debugging");
System.err.println("ssl turn on ssl debugging");
System.err.println();
System.err.println("The following can be used with ssl:");
System.err.println("\trecord enable per-record tracing");
System.err.println("\thandshake print each handshake message");
System.err.println("\tkeygen print key generation data");
System.err.println("\tsession print session activity");
System.err.println("\tdefaultctx print default SSL initialization");
System.err.println("\tsslctx print SSLContext tracing");
System.err.println("\tsessioncache print session cache tracing");
System.err.println("\tkeymanager print key manager tracing");
System.err.println("\ttrustmanager print trust manager tracing");
System.err.println("\tpluggability print pluggability tracing");
System.err.println();
System.err.println("\thandshake debugging can be widened with:");
System.err.println("\tdata hex dump of each handshake message");
System.err.println("\tverbose verbose handshake message printing");
System.err.println();
System.err.println("\trecord debugging can be widened with:");
System.err.println("\tplaintext hex dump of record plaintext");
System.err.println("\tpacket print raw SSL/TLS packets");
as part of this patch, I've also moved the log call to the more performant friendly System.Logger#log(java.lang.System.Logger.Level,java.util.function.Supplier) method.
the output has changed slightly with respect to that - less verbose
e.g. old style:
javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:24.302 GMT|SSLSocketOutputRecord.java:261|WRITE: TLSv1.2 handshake, length = 70
javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:24.302 GMT|SSLSocketOutputRecord.java:275|Raw write (
0000: 16 03 03 00 46 10 00 00 42 41 04 90 84 B1 78 70 ....F...BA....xp
0010: 08 E1 9B 40 AF 3C E7 81 2C 65 57 0C 81 C4 98 26 ...@.<..,eW....&
0020: 98 5B 12 20 B8 9A C3 36 6B 7A 51 0E B7 AA 32 D0 .[. ...6kzQ...2.
0030: 0D 82 36 56 3D 1C F0 EB 14 22 AF 2C 74 76 D7 86 ..6V=....".,tv..
0040: 65 B6 21 31 72 BD 2A D7 A6 91 A4 e.!1r.*....
)
javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:24.310 GMT|ChangeCipherSpec.java:115|Produced ChangeCipherSpec message
e.g. new format:
javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:46.440 GMT|SSLSocketOutputRecord.java:261|WRITE: TLSv1.2 handshake, length = 70
javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:46.441 GMT|SSLSocketOutputRecord.java:275|Raw write:
0000: 16 03 03 00 46 10 00 00 42 41 04 97 07 5E 48 78 ....F...BA...^Hx
0010: EC AB 59 78 13 0B CD 79 B9 43 89 DF 36 07 38 54 ..Yx...y.C..6.8T
0020: 2B E4 E7 D8 89 0B C0 D6 67 CB 4A 81 E9 E8 B7 50 +.......g.J....P
0030: A1 F4 3C 71 3E 67 26 24 95 4E 59 4E B6 5A 94 32 ..<q>g&$.NYN.Z.2
0040: AA AD C4 84 E2 28 71 B7 DB 43 E4 .....(q..C.
javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:46.447 GMT|ChangeCipherSpec.java:114|Produced ChangeCipherSpec message
note one line less per verbose entry and dropping of brackets around verbose output
new test case added to exercise javax.net.debug options including the use of the System.Logger option.
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [ ] Change requires CSR request JDK-8330987 to be approved
- [x] Commit message must refer to an issue
Issues
- JDK-8044609: javax.net.debug options not working and documented as expected (Bug - P3)
- JDK-8330987: javax.net.debug options not working and documented as expected (CSR)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18764/head:pull/18764
$ git checkout pull/18764
Update a local copy of the PR:
$ git checkout pull/18764
$ git pull https://git.openjdk.org/jdk.git pull/18764/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 18764
View PR using the GUI difftool:
$ git pr show -t 18764
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18764.diff
Using Webrev
:wave: Welcome back coffeys! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
❗ This change is not yet ready to be integrated. See the Progress checklist in the description for automated requirements.
@coffeys The following label will be automatically applied to this pull request:
security
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.
Webrevs
- 22: Full (53f9d757)
- 21: Full (4d4af430)
- 20: Full (f94c4e74)
- 19: Full - Incremental (ba4b1b26)
- 18: Full - Incremental (9399ee2c)
- 17: Full (71aa0211)
- 16: Full - Incremental (c8ebaf65)
- 15: Full (3116f43e)
- 14: Full (5da3c6cf)
- 13: Full (9d6bbfd4)
- 12: Full - Incremental (98b3b0c5)
- 11: Full (002d421f)
- 10: Full (a2e311fa)
- 09: Full - Incremental (b4512a90)
- 08: Full (6cf77100)
- 07: Full - Incremental (a11d50d5)
- 06: Full (fb724d3a)
- 05: Full (c44d524c)
- 04: Full - Incremental (4effe97f)
- 03: Full - Incremental (e7d42d4d)
- 02: Full - Incremental (1c4250ab)
- 01: Full - Incremental (277583cd)
- 00: Full (bc0d6107)
Tidied up the help menu output to capture current options
Changed logic to allow -Djavax.net.debug=ssl,handshake emit all ssl data (done today also)
Improved test case coverage
@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@coffeys This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.
Would you consider also fixing https://bugs.openjdk.org/browse/JDK-8340312 as part of this? There are no calls to SSLLogger.finer() so it is unused code at the moment. Consider removing finer() and changing finest() to use the TRACE logging level.
@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
new changes pushed which use the enum design approach for component token management. The new test coverage has been very useful in helping me validate the changes.
after checking with Brad, "javax.net.debug=ssl,typo" type syntax will be allowed - it'll just ignore the unknown option and log as if "ssl" was specified.
Regards the sub-component options used in the security implementation classes, I think it's better to cover this work via JDK-8344158 where a full audit can be done. I've only updated logging values where an illegal string option was present. "ssl" was missing in a bunch of them. Having the logging call site have correct syntax helps to enforce the rules with various options.
logged https://bugs.openjdk.org/browse/JDK-8344685 to track your request to add back the sessioncache option
@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@coffeys This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.
Seems to be agreement that the javax.net.debug property should behave in the following manner:
"all" indicates that all debug data will be logged "ssl" by itself indicates that all debug data except data from the "data" and "packet" categories will be logged "ssl" followed by any number of sub-components indicates that generic ssl data will be logged along will more specific data from the subcomponents specified.
As always, an empty value means a System Logger is used.
Overhaul of SSLLogger to represent debug levels via a new enum (SSLLogger.Opt) - this has a couple of benefits:
- ensure that calling sites use a documented setting instead of string values
- much simpler isOn(..) logic which can be a high volume call when logging is enabled.
the new isOn method boils down to much simpler logic :
public static boolean isOn(Opt option) {
return Opt.ALL.on || option.on;
}
test coverage also improved.
@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@coffeys this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:
git checkout 8044609-ssl
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push
@coffeys This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.
@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!