On Thu, 21 Nov 2024 13:01:41 GMT, Sean Coffey <coff...@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
