RFR: 8044609: javax.net.debug options not working and documented as expected [v19]

Bradford Wetmore wetmore at openjdk.org
Tue Sep 2 00:29:52 UTC 2025


On Mon, 1 Sep 2025 15:28:35 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 incrementally with one additional commit since the last revision:
> 
>   Initial review comments from Brad

Next set of comments.  Thanks for addressing the previous set!

src/java.base/share/classes/sun/security/ssl/Alert.java line 228:

> 226:         public void consume(ConnectionContext context,
> 227:                 ByteBuffer m) throws IOException {
> 228:             TransportContext tc = (TransportContext)context;

Copyright updates throughout.

I won't call out all cases, but noticed `Alert.java`, `AlpnExtension.java`, `CertificateAuthrotiesExtension.java`.  Some of these may have been done in 2024 in earlier versions of this PR (sorry!), but some are earlier (2023, 2022, etc.)

src/java.base/share/classes/sun/security/ssl/Alert.java line 231:

> 229: 
> 230:             AlertMessage am = new AlertMessage(tc, m);
> 231:             if (SSLLogger.logging && SSLLogger.isOn(SSLLogger.Opt.SSL)) {

You're likely doing a line-line update, but in earlier discussions, we talked about not having `ssl` be a filter category on of its own?  Did you decide not to got that route?

That is, every call site should be some concrete enum value like `Opt.HANDSHAKE` and not a general `Opt.SSL`?  For example:  `Alert.java` and `SSLCipher.java`.

This is more of a conversation starter than a request for change. 

There are some like the `Alert.java` sites that need to be assigned into the `Opt.HANDSHAKE` handshake category, but configuration items in `SSLCipher.java` don't fall cleanly into any existing category.  We could have a config category, but maybe it is better to just leave as `ssl`?  

> ./Alert.java
> ./DTLSInputRecord.java
> ./DTLSOutputRecord.java
> ./HandshakeOutStream.java
> ./KeyUpdate.java
> ./NamedGroup.java
> ./OutputRecord.java
> ./SessionTicketExtension.java
> ./SSLCipher.java
> ./SSLEngineImpl.java
> ./SSLEngineOutputRecord.java
> ./SSLSessionContextImpl.java
> ./SSLSocketImpl.java
> ./SSLSocketOutputRecord.java
> ./SSLTransport.java
> ./TransportContext.java
> ./Utilities.java
> ./X509Authentication.java


Thoughts?

src/java.base/share/classes/sun/security/ssl/CertificateRequest.java line 815:

> 813:                             hc.peerRequestedSignatureSchemes,               //  a CertificateVerify message later
> 814:                             ka, hc.negotiatedProtocol) != null
> 815:                             || SSLLogger.logWarning(SSLLogger.Opt.HANDSHAKE,

<=80.  

Would you mind tweaking the area around this too?  Thanks!

src/java.base/share/classes/sun/security/ssl/ClientHello.java line 434:

> 432:                     session = null;
> 433:                     if (SSLLogger.logging &&
> 434:                             SSLLogger.isOn(SSLLogger.Opt.HANDSHAKE_VERBOSE)) {

Not your fault, but I'm not sure I like this as a verbose message.

src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 79:

> 77:                     help();
> 78:                 }
> 79:                 logger = new SSLConsoleLogger("javax.net.ssl", p);

Would you mind adding a comment that this takes care of the `expand` keyword, as all of the other options are processed in the new way?  Thanks.

src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 128:

> 126:             // javax.net.debug would be misconfigured property with respect
> 127:             // to logging if value didn't contain "all" or "ssl"
> 128:             logging = Opt.ALL.on || Opt.SSL.on;

I don't have a strong feeling about this either way, but a usage question here.

If they have specified some args but not `ssl`.  e.g. 

    -Djavax.net.debug=trustmanager,keymanager

Should we:

1. quietly ignore as this is are doing
2. output a single log message saying logging is disabled.
3. a fatal error

src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 210:

> 208:         System.err.println("\tdefaultctx   print default SSL initialization");
> 209:         System.err.println("\thandshake    print each handshake message");
> 210:         System.err.println("\t    verbose   verbose handshake" +

This line in particular is lining up pretty ragged in the actual output.  Even in the github editor, it looks off due to the variable width fonts.  Maybe we can use tabs to help?  e.g.  the current:

	handshake    print each handshake message
	    verbose   verbose handshake message printing (widens handshake)
	keymanager   print key manager tracing
	record       enable per-record tracing
	    plaintext    hex dump of record plaintext (widens record)
	    packet       print raw SSL/TLS packets (widens record)

vs.

    System.err.println("\tdefaultctx\tprint default SSL initialization");
    System.err.println("\thandshake\tprint each handshake message");
    System.err.println("\t\tverbose\tverbose handshake" +
                            " message printing (widens handshake)");

src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 216:

> 214:         System.err.println("\t    plaintext    hex dump of record" +
> 215:                                 " plaintext (widens record)");
> 216:         System.err.println("\t    packet       print raw SSL/TLS" +

Order `packet` before `plaintext`?

-------------

PR Review: https://git.openjdk.org/jdk/pull/18764#pullrequestreview-3174518962
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314657243
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314658104
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314681552
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314684540
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314607893
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314601805
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314622404
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314609182


More information about the security-dev mailing list