RFR: 8044609: javax.net.debug options not working and documented as expected [v19]
Bradford Wetmore
wetmore at openjdk.org
Tue Sep 2 00:29:52 UTC 2025
On Mon, 1 Sep 2025 15:28:35 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:
>
> Initial review comments from Brad
Next set of comments. Thanks for addressing the previous set!
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.)
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?
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!
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.
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.
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
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)");
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`?
-------------
PR Review: https://git.openjdk.org/jdk/pull/18764#pullrequestreview-3174518962
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314657243
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314658104
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314681552
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314684540
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314607893
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314601805
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314622404
PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r2314609182
More information about the security-dev
mailing list