RFR: 8044609: javax.net.debug options not working and documented as expected [v8]
Bradford Wetmore
wetmore at openjdk.org
Thu Nov 14 02:46:45 UTC 2024
On Tue, 12 Nov 2024 11:49:25 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:
>
> 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
More information about the security-dev
mailing list