RFR: 8044609: javax.net.debug options not working and documented as expected [v18]
Bradford Wetmore
wetmore at openjdk.org
Sat Aug 30 03:09:52 UTC 2025
On Wed, 27 Aug 2025 10:25:29 GMT, Sean Coffey <coffeys at openjdk.org> wrote:
>> 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...
>
> Sean Coffey has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 35 commits:
>
> - Merge branch 'master' into 8044609-ssl
> - 1 file omitted during merge
> - Merge branch 'master' into 8044609-ssl
> - Merge branch 'master' into 8044609-ssl
> - Merge branch 'master' into 8044609-ssl
> - remove whitespace
> - erroneous edit to test file
> - remove legacy test and minor fix ups
> - Merge branch 'master' into 8044609-ssl
> - enums refactoring and line width correction
> - ... and 25 more: https://git.openjdk.org/jdk/compare/2b44ed70...71aa0211
Initial comments for current SSLLogger.
Continuing next week.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 114:
> 112: // enable all subcomponents. "ssl" logs all activity
> 113: // except for the "data" and "packet" categories
> 114: if (Opt.SSL.on && !Opt.isAnySubComponentEnabled()) {
`isAnySubComponentEnabled()` and `enableAllSubComponents()` both call `subCompoenentList()`, so if isAnySC is false, then the list of subcomponents will be generated twice back-to-back. Since these methods are not used anywhere else, maybe do a single call instead:
if (Opt.SSL.on) {
ifSSLOnlyEnableAllSubcompoenents() // needs a better name!
}
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 166:
> 164: } else {
> 165: try {
> 166: logger.log(level, () -> msg + ":\n" + SSLSimpleFormatter.formatParameters(params));
Can I trouble you to shorten this line to <=80?
It makes side/side comparisons and printing so much easier on the wrists and eyes.
Would you mind also shortening lines 324-25?
Thanks.
e.g. [SSLLogger-longlines.txt](https://github.com/user-attachments/files/22055476/SSLLogger-longlines.txt)
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 198:
> 196: System.err.println("all turn on all debugging");
> 197: System.err.println("ssl turn on ssl debugging");
> 198: System.err.println();
The ordering of the options seem random. Might suggest either alphabetical or by functional area (e.g. key/trustmanager together)
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 199:
> 197: System.err.println("ssl turn on ssl debugging");
> 198: System.err.println();
> 199: System.err.println("The following can be used with ssl:");
Maybe `...to select/filter specific types of output...` or something to indicate that it's a narrowing/filtering operation. ;)
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 203:
> 201: System.err.println("\thandshake print each handshake message");
> 202: System.err.println("\tkeymanager print key manager tracing");
> 203: System.err.println("\trecord enable per-record tracing");
What would you think of listing/indenting the suboptions here instead of down below? e.g.
record enable per-record tracing
plaintext hex dump of record plaintext (widens record)
packet print raw SSL/TLS packets (widens record)
same with `verbose`
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 206:
> 204: System.err.println("\trespmgr print OCSP response tracing");
> 205: System.err.println("\tsession print session activity");
> 206: System.err.println("\tdefaultctx print default SSL initialization");
`defaultctx` listed twice
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 208:
> 206: System.err.println("\tdefaultctx print default SSL initialization");
> 207: System.err.println("\tsslctx print SSLContext tracing");
> 208: System.err.println("\tsessioncache print session cache tracing");
Missing `sessioncache` in the enum. In a previous discussion, you said you would be filing a separate bug to fix this? Maybe add it now to the enum, and we can figure the call sites later.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 209:
> 207: System.err.println("\tsslctx print SSLContext tracing");
> 208: System.err.println("\tsessioncache print session cache tracing");
> 209: System.err.println("\tkeymanager print key manager tracing");
`keymanager` listed twice
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 211:
> 209: System.err.println("\tkeymanager print key manager tracing");
> 210: System.err.println("\ttrustmanager print trust manager tracing");
> 211: System.err.println("\tpluggability print pluggability tracing");
Remove `pluggability`. I think this was removed once already, but found it's way back.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 223:
> 221: }
> 222:
> 223: public enum Opt {
A quick comment about the enum's purpose might be appreciated by someone new. "list of options, whether they are active..."
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 242:
> 240:
> 241: Opt() {
> 242: this.component = this.toString().toLowerCase(Locale.ROOT);
Why `Locale.ROOT` instead of `Locale.English` throughout the rest of the class?
-------------
PR Review: https://git.openjdk.org/jdk/pull/18764#pullrequestreview-3170762803
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311743949
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311700232
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311720446
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311720377
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311719679
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311710549
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311713541
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311715372
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311713610
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311725748
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2311735622
More information about the security-dev
mailing list