On Tue, 12 Nov 2024 11:49:25 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:
>
> keep expand option and add test coverage
More comments. I will have to look over the `SSLLogger` changes with fresher
eyes tomorrow. I had some great comments for you, but then I realized I
misread something. :)
When walking through the `SSLLogger.isOn()` calls, I noticed some
inconsistencies that aren't your fault, but should probably be addressed while
we're down here. The important ones are noted below.
That said, thoughout the existing code, I question whether the categorical
assignments are correct. It would be a good idea to have "someone" in dev have
a look and see if all these assignments are correctly defined. For example,
what does `slctx` have to do with `PredefinedDHParameterSpecs.java`. Should
session messages that are generated during handshaking be marked as `handshake`
or `session`? And are we consistently `verbose`ing? Should so many of these
be just `ssl`? I filed JDK-8344158 to track.
`DTLSInputRecord.java`: shouldn't there be a `ssl,record` for 837/861/897?
`SSLContextImpl.java`: 387/396, should these `handshake` instead of `sslctx`,
or should verbose be removed?
`SSLEngineImpl.java`: 336/618, should this be a `record` instead of `verbose`?
`SSLTransport.java`: same as `SSLEngineImpl.java` above? `ssl,verbose` isn't
right.
`SSLExtensions.java`: 146/et.al., should this all be `verbose`? This would
match what was done in some of the other code.
I noticed a lot of new <=80 char lines in many files. I started calling them
out, but stopped. I'd really like to keep the code readable when viewing
side-by-side diffs (e.g. github) without having to horizontal scroll, please!
Thanks.
For the CSR (JDK-8330987):
...per documentation in help output menu.
->
...per documentation in help output menu and the JSSE Reference Guide.
...for a standard client TLS connection.
->
...for a standard TLS connection.
You would be in a better position to advise about backport impacts, but it
would be nice to fix for everyone.
If you decide to update the wording for things like `expand` , be sure to
update here also.
src/java.base/share/classes/sun/security/ssl/DTLSInputRecord.java line 1075:
> 1073: if (!isDesired) {
> 1074: // Too old to use, discard this retransmitted record
> 1075: if (SSLLogger.isOn &&
> SSLLogger.isOn("ssl,handshake,verbose")) {
<=80 chars, please. Noticed this in several files/places.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 90:
> 88: && property.length() == 10
> 89: &&
> !Character.isLetterOrDigit(property.charAt(3))
> 90: && property.endsWith("expand"));
I believe this is not needed, and the code can be simplified using the same
style as I remember from the earlier stack. I'll look at this tomorrow.
src/java.base/share/classes/sun/security/ssl/SSLLogger.java line 103:
> 101: System.err.println("\tdefaultctx print default SSL
> initialization");
> 102: System.err.println("\tsslctx print SSLContext tracing");
> 103: System.err.println("\tsessioncache print session cache tracing");
Did the `sessioncache` category also get pulled? It used to be used whenever
we added/retrieved/deleted(expired) a `SSLSession` to/from the cache. (i.e.
when a handshake completed, or we are handshaking and we retrieved a session
from the cache to potentially resume.)
I don't see any usages of it now. If so, this is a defect and needs a bug to
track.
-------------
Changes requested by wetmore (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18764#pullrequestreview-2434697986
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1841442130
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1841353663
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1841367363
