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

[Bradford Wetmore]

On Thu, 21 Nov 2024 13:01:41 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:
> 
>   enum Options and logic

More suggestions which I think will ease user understanding of the option combinations, and simplify the code as well.

Some of my other comments from last week should probably be addressed in this bug, e.g. `TLSInputRecord/SSLContextImpl/SSLEngineImpl/SSLTransport` having only `verbose`, as several other of this type were also already cleaned up in previous changesets for this bug.

Also note the CSR comments.  I can mark that as reviewed when ready.

Thanks.

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

> 63:     private static final String property;
> 64:     public static final boolean isOn;
> 65:     static EnumSet<ComponentToken> activeComponents = EnumSet.noneOf(ComponentToken.class);

This could be private/final.

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

> 81:                     if (tmpProperty.contains(o.component)) {
> 82:                         activeComponents.add(o);
> 83:                         // remove the pattern to avoid it being reused

Brilliant!  Great idea!

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

> 87:                 }
> 88:                 // some rules to check
> 89:                 if ((activeComponents.contains(ComponentToken.PLAINTEXT)

This logic is so much easier to parse.  Great job here too!

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

> 99:                 }
> 100:             }
> 101:             isOn = (property.isEmpty() || property.equals("all"))

What would you think about including `EMPTYALL` as an enum option, rather than a separate String property using separate processing checks?  This would simplify the logic, and you no longer need to store `property` in the class as it's no longer needed.  e.g.:

    isOn = activeComponents.contains(ComponentToken.EMPTYALL) 
        || activeComponents.contains(ComponentToken.SSL));

This would help in the `isOn()` section, where you just check for `EMPTYALL`

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

> 152:         }
> 153: 
> 154:         if (property.isEmpty() || property.equals("all")) {

Then here this becomes:

    if (activeComponents.contains(EMPTYALL)) {
        return true;
    }

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

> 157:         }
> 158: 
> 159:         if (checkPoints.equals("ssl")) {

I always thought that the `ssl` options were additive.

The presence of `ssl` in the property outputs those debug statements which only have `ssl` defined.  The presence of additional options turns on additional debug info. i.e. The property `ssl,handshake` turns on both `ssl` and `ssl,handshake` statements.  `ssl,handshake,verbose` turns on `ssl`, `ssl,handshake`, and `ssl,handshake,verbose` statements.

I **think** this style would eliminate these special cases, and the code would just go to the `split`/options loop after the EMPTYALL check.

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

> 258:         }
> 259: 
> 260:         static boolean isSslFilteringEnabled() {

This is probably no longer needed if you agree with my additive comment above.

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

PR Review: https://git.openjdk.org/jdk/pull/18764#pullrequestreview-2455367485
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1854562176
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1854482516
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1854491877
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1854520382
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1854526604
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1854551107
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1854561270