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

Sean Coffey coffeys at openjdk.org
Tue Sep 2 14:33:55 UTC 2025 

On Mon, 1 Sep 2025 23:33:43 GMT, Bradford Wetmore <wetmore at openjdk.org> wrote:

>> Sean Coffey has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Initial review comments from Brad
>
> 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.)

Scanned all updated files - I think I should have them all with 2025 now.

> 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?

Yes - For the refactoring of the component values in calling code, I pretty much stuck to what was already there. I think allowing code to call via the Opt.SSL value is ok  - some TLS code is generic to all components. *like TLS configuration data perhaps)

Yes - some calls probably deserve to be tidied off to a specific filter value - probably best for a follow on JBS issue.

> 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!

done

> 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.

In agreement. This sort of message strikes me as more in the `HANDSHAKE `category

Will I log a follow on JBS issue to have all call sites of `Opt `examined ?

> 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.

I've added a comment and tweaked the code here. We can pass boolean in SSLConsoleLogger constructor call

> 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

I'm leaning towards option 1. no logging gets enabled in misconfigured values. I've test code covering this scenario. 

option 2 might be useful and if there's a strong desire for that, perhaps we can alter in follow on JDK issue.
option 3 sounds a bit heavy handed, Don't think we need to have a fatal error. Also, since the load time for loading SSLLogger is variable, it could be dangerous (e.g. app runs fine for 1st 10 mins and then trigger an error etc)

> 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)");

Fair point. the tab approach crossed my mind yesterday but I stuck with original design. Just tried with tabs but it was quite ugly (too many spaces) - I've used the printf functionality. how does this look ?
help             print this help message and exit
expand           expanded (less compact) output format


all              turn on all debugging
ssl              turn on ssl debugging

The following filters can be used with ssl:
    defaultctx     print default SSL initialization
    handshake      print each handshake message
      verbose        -verbose handshake message printing (widens handshake)
    keymanager     print key manager tracing
    record         enable per-record tracing
      packet         -print raw SSL/TLS packets (widens record)
      plaintext      -hex dump of record plaintext (widens record)
    respmgr        print OCSP response tracing
    session        print session activity
    sessioncache   print session cache tracing
    sslctx         print SSLContext tracing
    trustmanager   print trust manager tracing

> 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`?

fixed. thanks!

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2316095683
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2316093383
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2316110651
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2316277275
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2315725718
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2315735365
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2316085193
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2316086202

More information about the security-dev mailing list