On Wed, 28 Jan 2026 16:23:07 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 incrementally with one additional
> commit since the last revision:
>
> Review comments from Brad
As discussed, I've been looking mainly at the conversion in the call sites.
We'll address the `Levels` and `SSLLogger.Opt.*` categories issues later.
- [JDK-8344158](https://bugs.openjdk.org/browse/JDK-8344158) category
consistency, and
- [JDK-8338702](https://bugs.openjdk.org/browse/JDK-8338702) System.Logger
doesn't output most JSSE debug messages
One mismerge, I think. Please check carefully.
Just have the `DebugPropertyValuesTest.java` to review, otherwise, looks pretty
good.
src/java.base/share/classes/sun/security/ssl/CertificateMessage.java line 1026:
> 1024: ka, hc.negotiatedProtocol) != null
> 1025: ||
> SSLLogger.logWarning(SSLLogger.Opt.HANDSHAKE,
> 1026: "Unable to produce
> CertificateVerify for key algorithm: " + ka))
Nitty nit nit: might as well take care of the <80 here too. Esp line1021. ;)
Had to vertically scroll just to see the line in side-by-side. Thanks!
There were a few others where things got a bit long (e.g.
`SessionTicketExtension`, `SSLCipher`), and noticed a couple really egregious
existing ones. I'll probably take care of them when I get to the category bug.
(P.S. Thank you for doing that in `CertificateRequest.java`!)
src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java line 847:
> 845: if (fragmentData.remaining() < 32) {
> 846: if (SSLLogger.isOn() &&
> 847: SSLLogger.isOn(SSLLogger.Opt.RECORD)) {
I think this looks right.
The consistency followup will look at this closer. There's several things I
think may need changing.
src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java line 1648:
> 1646: for (RecordFragment fragment : bufferedFragments) {
> 1647: if (fragment.contentType ==
> ContentType.CHANGE_CIPHER_SPEC.id) {
> 1648: if (hasFin) {
This appears to be another merge issue from a recent changeset (October).
8367059 removed the `if (hasFin)` check.
Please check this one closely. Here's the changeset ID.
436dc687ba2ead1662a4e0125cea0966fac825e5
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 212:
> 210: System.err.println();
> 211: System.err.println("The following filters can be used with
> ssl:");
> 212: System.err.printf(" %-14s %s%n", "defaultctx",
Minor nit for visibility:
Add an extra blank line between 211/212. Between `can be used with ssl:` and
`defaultctx...`
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 236:
> 234: System.err.printf(" %-14s %s%n", "trustmanager",
> 235: "print trust manager tracing");
> 236: System.err.printf("%nIf \"ssl\" is specified by itself," +
There's now an extra `%n` on this line. Maybe move to the new last line?
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 239:
> 237: " all non-widening filters are enabled.%n%n");
> 238: System.err.printf("%nAdding valid filter options to \"ssl\" will
> log" +
> 239: " messages to include%njust those filtered
> categories.%n");
So the intended behavior is the following:
If we have code like this:
> if (SSLLogger.isOn() && SSLLogger.isOn(SSLLogger.Opt.SSL)) {
then setting `javax.net.debug=ssl` or `ssl,trustmanager`, it will always output
this logging statement.
But if we have code like this:
> if (SSLLogger.isOn() && SSLLogger.isOn(SSLLogger.Opt.TRUSTMANAGER)) {
then `javax.net.debug=ssl` will output this (because only `ssl` was enabled),
but `javax.net.debug=ssl,keymanager` will not. That's because `keymanager` is
a valid name, thus only the `SSL` and `KEYMANAGER` options will print).
So we need to make sure that everything `SSL` and non-`SSL` is categorized
correctly, which will be the focus of
[JDK-8344158](https://bugs.openjdk.org/browse/JDK-8344158).
The wording here needs a slight update, and I'm not quite sure how to word
this. Ideas? :)
Adding valid filter options to ssl will output the high-level SSL/TLS debug
messages, plus only those messages in the specified categories.
test/jdk/sun/security/ssl/SSLEngineImpl/SSLEngineKeyLimit.java line 114:
> 112: " -Dtest.src=" + System.getProperty("test.src") +
> 113: " -Dtest.jdk=" +
> System.getProperty("test.jdk") +
> 114: " -Djavax.net.debug=ssl" +
I'm assuming that changing from `ssl,handshake` to just `ssl` is ok? I didn't
actually check. I notice this in several tests.
-------------
PR Review: https://git.openjdk.org/jdk/pull/18764#pullrequestreview-3719123589
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2748407425
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2748433796
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2748449980
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2747982555
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2738705046
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2738583278
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2748660217
