On Wed, 27 Aug 2025 10:25:29 GMT, Sean Coffey <[email protected]> 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
