On Tue, 27 Jan 2026 11:44:06 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 48 commits:
>
> - correct NamedGroup.java merge
> - Merge branch 'master' into 8044609-ssl
> - fix up test bug ID
> - fix up files post merge
> - Merge branch 'master' into 8044609-ssl
> - prep for isOn() merge
> - Merge branch 'master' into 8044609-ssl
> - Merge branch 'master' into 8044609-ssl
> - Merge branch 'master' into 8044609-ssl
> - Incorporate review comments from Brad
> - ... and 38 more: https://git.openjdk.org/jdk/compare/c69275dd...6b5c692c
Nothing major. We did most of the heavy lifting last year. Without looking at
the calls sites yet, I like how the enum turned out. I am looking forward to
seeing how the enums actually fit.
Just a few nits+requested comments.
I've hit the `SSLLogger.java` code pretty hard, now off to the other 500+ call
sites. ;)
I'm going to check the Levels and the categories and see if we can make them
more consistent, with ears towards:
[JDK-8338702](https://bugs.openjdk.org/browse/JDK-8338702)
[JDK-8344158](https://bugs.openjdk.org/browse/JDK-8344158)
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 52:
> 50: /**
> 51: * Implementation of SSL logger.
> 52: *
If you feel up to it, you could add `<p>` where needed.
Also, lines 298, 302, and 308.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 58:
> 56: * and applications can customize and configure the logger or use external
> 57: * logging mechanisms. If the system property "javax.net.debug" is
> defined
> 58: * and non-empty, a private debug logger implemented in this class is
> used.
add something like...
the private debug logger in this class simply outputs to `System.err`.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 62:
> 60: public final class SSLLogger implements System.Logger {
> 61: private static final System.Logger logger;
> 62: // high level boolean to track whether "all" or "ssl" option
Minor nit:
This makes it sounds like the boolean state `true` might be `all` and `false`
is `ssl`.
Suggest maybe "to track whether logging is active (i.e. all/ssl)."
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 105:
> 103: if (Opt.HANDSHAKE.on && p.contains("verbose")) {
> 104: Opt.HANDSHAKE_VERBOSE.on = true;
> 105: }
Not absolutely needed given the options currently defined, but you could
`replaceFirst()` on these 3 as well.
Your call.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 130:
> 128: }
> 129: }
> 130: // javax.net.debug would be misconfigured property with
> respect
Nit: add extra line.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 149:
> 147: return logging;
> 148: }
> 149: /**
Nit: add a line.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 157:
> 155: }
> 156:
> 157: public static void severe(String msg, Object... params) {
Since we're down here, these don't really need to be `public`. Package-private
is fine.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 237:
> 235: System.err.printf("%nAdding valid filter options to \"ssl\" will
> log" +
> 236: " messages to include%njust those filtered
> categories.%n");
> 237: System.err.printf("%nIf \"ssl\" is specified by itself," +
Maybe reverse 235 and 237. ssl by itself, then ssl+filter? I've gone
back/forth on this quite a bit. Do you have a preference?
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 296:
> 294: return level != Level.OFF;
> 295: }
> 296: /**
Nit: add a line.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 311:
> 309: * Enabling subcomponents fine-tunes (filters) debug output.
> 310: */
> 311: public enum Opt {
Does this need to be public?
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 465:
> 463: formatParameters(parameters) :
> 464: Utilities.indent(formatParameters(parameters)))
> 465: };
Why did the indent get changed?
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 490:
> 488: isFirst = false;
> 489: } else {
> 490: builder.append(",\n");
Why did this get changed from `LINE_SEP` to `\n`. All of the other line
separators in this file are still `LINE_SEP`.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 494:
> 492:
> 493: if (parameter instanceof Throwable) {
> 494:
> builder.append(formatThrowable((Throwable)parameter));
Nit: from the Java Coding conventions, Section 8.2:
Casts should be followed by a blank. Examples:
myMethod((byte) aNum, (Object) x);
myFunc((int) (cp + 5), ((int) (i + 3))
I don't have a strong preference here.
If you do revert back, there are several places that got changed here.
-------------
PR Review: https://git.openjdk.org/jdk/pull/18764#pullrequestreview-3713815602
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734229865
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734137947
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734242861
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734786277
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734786894
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734896804
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734793448
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734912268
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734915243
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734920494
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734926518
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734982921
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2734985908
