8044609: javax.net.debug options not working and documented as expected #18764

coffeys · 2024-04-12T15:56:24Z

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 style:

javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:24.302 GMT|SSLSocketOutputRecord.java:261|WRITE: TLSv1.2 handshake, length = 70
javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:24.302 GMT|SSLSocketOutputRecord.java:275|Raw write (
  0000: 16 03 03 00 46 10 00 00   42 41 04 90 84 B1 78 70  ....F...BA....xp
  0010: 08 E1 9B 40 AF 3C E7 81   2C 65 57 0C 81 C4 98 26  ...@.<..,eW....&
  0020: 98 5B 12 20 B8 9A C3 36   6B 7A 51 0E B7 AA 32 D0  .[. ...6kzQ...2.
  0030: 0D 82 36 56 3D 1C F0 EB   14 22 AF 2C 74 76 D7 86  ..6V=....".,tv..
  0040: 65 B6 21 31 72 BD 2A D7   A6 91 A4                 e.!1r.*....
)
javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:24.310 GMT|ChangeCipherSpec.java:115|Produced ChangeCipherSpec message

e.g. new format:

javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:46.440 GMT|SSLSocketOutputRecord.java:261|WRITE: TLSv1.2 handshake, length = 70
javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:46.441 GMT|SSLSocketOutputRecord.java:275|Raw write:
  0000: 16 03 03 00 46 10 00 00   42 41 04 97 07 5E 48 78  ....F...BA...^Hx
  0010: EC AB 59 78 13 0B CD 79   B9 43 89 DF 36 07 38 54  ..Yx...y.C..6.8T
  0020: 2B E4 E7 D8 89 0B C0 D6   67 CB 4A 81 E9 E8 B7 50  +.......g.J....P
  0030: A1 F4 3C 71 3E 67 26 24   95 4E 59 4E B6 5A 94 32  ..<q>g&$.NYN.Z.2
  0040: AA AD C4 84 E2 28 71 B7   DB 43 E4                 .....(q..C.
javax.net.ssl|DEBUG|10|main|2024-04-12 15:47:46.447 GMT|ChangeCipherSpec.java:114|Produced ChangeCipherSpec message

note one line less per verbose entry and dropping of brackets around verbose output

new test case added to exercise javax.net.debug options including the use of the System.Logger option.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Change requires CSR request JDK-8330987 to be approved
Commit message must refer to an issue

Issues

JDK-8044609: javax.net.debug options not working and documented as expected (Bug - P3)
JDK-8330987: javax.net.debug options not working and documented as expected (CSR)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18764/head:pull/18764
$ git checkout pull/18764

Update a local copy of the PR:
$ git checkout pull/18764
$ git pull https://git.openjdk.org/jdk.git pull/18764/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18764

View PR using the GUI difftool:
$ git pr show -t 18764

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18764.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-12T15:57:07Z

👋 Welcome back coffeys! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-12T17:09:50Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2024-04-12T17:11:32Z

@coffeys The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-04-12T17:13:54Z

Webrevs

…overage

coffeys · 2024-04-17T14:10:20Z

Tidied up the help menu output to capture current options
Changed logic to allow -Djavax.net.debug=ssl,handshake emit all ssl data (done today also)
Improved test case coverage

src/java.base/share/classes/sun/security/ssl/CertificateMessage.java

bridgekeeper · 2024-06-20T17:54:51Z

@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2024-08-12T19:24:17Z

@coffeys This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

bradfordwetmore · 2024-09-04T03:18:33Z

src/java.base/share/classes/sun/security/ssl/SSLLogger.java

        System.err.println("\thandshake    print each handshake message");
-        System.err.println("\tkeygen       print key generation data");
+        System.err.println("\tkeymanager   print key manager tracing");
+        System.err.println("\tpluggability print pluggability tracing");


Shouldn't this just be removed completely, or marked with "(obsolete)" and moved to the bottom?

/open

good catch. Looks like there hasn't been a use case for years with this option. Removed it.

coffeys · 2024-09-04T09:40:25Z

/open

openjdk · 2024-09-04T09:41:30Z

@coffeys This pull request is now open

bradfordwetmore

The issues noted in the previous main comment apparently got missed.

SSLEngineImpl/SSLContextImpl/etc.

This changeset was mostly <=80 and a help comment.

Thanks.

coffeys · 2024-11-21T13:06:00Z

new changes pushed which use the enum design approach for component token management. The new test coverage has been very useful in helping me validate the changes.

after checking with Brad, "javax.net.debug=ssl,typo" type syntax will be allowed - it'll just ignore the unknown option and log as if "ssl" was specified.

Regards the sub-component options used in the security implementation classes, I think it's better to cover this work via JDK-8344158 where a full audit can be done. I've only updated logging values where an illegal string option was present. "ssl" was missing in a bunch of them. Having the logging call site have correct syntax helps to enforce the rules with various options.

logged https://bugs.openjdk.org/browse/JDK-8344685 to track your request to add back the sessioncache option

bradfordwetmore

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.

bradfordwetmore · 2024-11-22T18:40:27Z

src/java.base/share/classes/sun/security/ssl/SSLLogger.java

+                for (ComponentToken o : ComponentToken.values()) {
+                    if (tmpProperty.contains(o.component)) {
+                        activeComponents.add(o);
+                        // remove the pattern to avoid it being reused


Brilliant! Great idea!

bradfordwetmore · 2024-11-22T18:45:16Z

src/java.base/share/classes/sun/security/ssl/SSLLogger.java

+                    }
+                }
+                // some rules to check
+                if ((activeComponents.contains(ComponentToken.PLAINTEXT)


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

bradfordwetmore · 2024-11-22T19:07:34Z

src/java.base/share/classes/sun/security/ssl/SSLLogger.java

            }
-            isOn = true;
+            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

bradfordwetmore · 2024-11-22T19:13:03Z

src/java.base/share/classes/sun/security/ssl/SSLLogger.java

-        } else if (property.isEmpty()) {     // use System.Logger
+        }
+
+        if (property.isEmpty() || property.equals("all")) {


Then here this becomes:

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

bradfordwetmore · 2024-11-22T19:32:32Z

src/java.base/share/classes/sun/security/ssl/SSLLogger.java

+            return true;
+        }
+
+        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.

No, the ssl option is designed to return all debug statements except those that are extra verbose (data, packet, plaintext). At least that's how the old (JDK 7) and newer TLSv1.3 stacks have been implemented. (with the exception of bug 8044609 which is being addressed here!)

The record, handshake, keygen, session, defaultctx, sslctx, keymanager, trustmanager options have always struck me as odd. I reckon no one uses them. The only logic I can see with them is as filtering operations, since the all or ssl option is mandatory.

I think the filtering option (if we choose to keep them) is the only purpose we can have for these sub options.

I can't imagine telling users to use ssl for some TLS debugging but turn on record or handshake or keygen or session or defaultctx or sslctx or keymanager or trustmanager options if you fancy other bits of TLS debugging.

Happy to address your CSR comments shortly. Let's agree on the implementation some more first. Nice suggestion on that EMPTYALL value. I'll get that change in shortly.

I can't imagine telling users...if you fancy other bits of TLS debugging.

That's exactly what I thought it was previously. Have a look at the usage examples in the JSSE Ref Guide, ...To view the hexadecimal dumps of each handshake message, and to print trust manager tracing...

This is just like what is done in the JDK java.security.debug property in Troubleshooting Security

bradfordwetmore · 2024-11-22T19:43:00Z

src/java.base/share/classes/sun/security/ssl/SSLLogger.java

+            this.component = this.toString().toLowerCase(Locale.ROOT);
+        }
+
+        static boolean isSslFilteringEnabled() {


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

bradfordwetmore · 2024-11-22T19:43:48Z

src/java.base/share/classes/sun/security/ssl/SSLLogger.java

@@ -62,6 +62,7 @@ public final class SSLLogger {
    private static final System.Logger logger;
    private static final String property;
    public static final boolean isOn;
+    static EnumSet<ComponentToken> activeComponents = EnumSet.noneOf(ComponentToken.class);


This could be private/final.

bridgekeeper · 2024-12-24T03:16:34Z

@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2025-01-21T05:09:43Z

@coffeys This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

coffeys · 2025-04-30T12:53:32Z

/open

openjdk · 2025-04-30T12:54:11Z

@coffeys This pull request is now open

coffeys · 2025-04-30T13:11:31Z

Seems to be agreement that the javax.net.debug property should behave in the following manner:

"all" indicates that all debug data will be logged
"ssl" by itself indicates that all debug data except data from the "data" and "packet" categories will be logged
"ssl" followed by any number of sub-components indicates that generic ssl data will be logged along will more specific data from the subcomponents specified.

As always, an empty value means a System Logger is used.

Overhaul of SSLLogger to represent debug levels via a new enum (SSLLogger.Opt) - this has a couple of benefits:

ensure that calling sites use a documented setting instead of string values
much simpler isOn(..) logic which can be a high volume call when logging is enabled.

the new isOn method boils down to much simpler logic :

    public static boolean isOn(Opt option) {
        return Opt.ALL.on || option.on;
    }

test coverage also improved.

bridgekeeper · 2025-05-28T18:56:20Z

@coffeys This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

coffeys added 3 commits April 12, 2024 14:54

Initial commit

d87c974

remove file

c1a842f

Tighten up ssl parsing

bc0d610

openjdk bot changed the title ~~8044609~~ 8044609: javax.net.debug "ssl" options are not working and documented as expected. Apr 12, 2024

openjdk bot added the rfr Pull request is ready for review label Apr 12, 2024

openjdk bot added the security [email protected] label Apr 12, 2024

Allow ssl,<option> type syntax as currently done. Improve test case c…

277583c

…overage

openjdk bot added the csr Pull request needs approved CSR before integration label Apr 23, 2024

indentation

27178fb

mpdonova reviewed May 8, 2024

View reviewed changes

src/java.base/share/classes/sun/security/ssl/CertificateMessage.java Outdated Show resolved Hide resolved

coffeys changed the title ~~8044609: javax.net.debug "ssl" options are not working and documented as expected.~~ 8044609: javax.net.debug options not working and documented as expected May 8, 2024

coffeys added 3 commits May 21, 2024 09:34

Merge branch 'master' into 8044609-ssl

826ad29

all ssl mode only if ssl specified by itself

1c4250a

Merge branch 'master' into 8044609-ssl

a1003b9

Merge branch 'master' into 8044609-ssl

e7d42d4

bridgekeeper bot closed this Aug 12, 2024

bradfordwetmore reviewed Sep 4, 2024

View reviewed changes

coffeys added 2 commits September 4, 2024 08:15

Merge branch 'master' into 8044609-ssl

902453c

Remove pluggability help reference

4effe97

openjdk bot reopened this Sep 4, 2024

update test to comply with new debug output requirements

32ec47b

bridgekeeper bot added the oca Needs verification of OCA signatory status label Nov 19, 2024

openjdk bot removed the rfr Pull request is ready for review label Nov 19, 2024

bridgekeeper bot removed the oca Needs verification of OCA signatory status label Nov 19, 2024

openjdk bot added the rfr Pull request is ready for review label Nov 19, 2024

bradfordwetmore reviewed Nov 19, 2024

View reviewed changes

enum Options and logic

b4512a9

bradfordwetmore reviewed Nov 22, 2024

View reviewed changes

coffeys added 3 commits November 25, 2024 20:16

EMPTYALL enum addition

f25f83a

Fix up isOn values

b3abd98

Merge branch 'master' into 8044609-ssl

a2e311f

bridgekeeper bot closed this Jan 21, 2025

coffeys added 7 commits April 25, 2025 10:10

Merge branch 'master' into 8044609-ssl

c3b9a28

minor refactor and clean up

3114fea

convert to enum options and refactor

055c8a7

enums refactoring and line width correction

ceee6b8

Merge branch 'master' into 8044609-ssl

4010674

remove legacy test and minor fix ups

990075f

erroneous edit to test file

002d421

openjdk bot reopened this Apr 30, 2025

openjdk bot removed the rfr Pull request is ready for review label Apr 30, 2025

remove whitespace

98b3b0c

openjdk bot added the rfr Pull request is ready for review label Apr 30, 2025

graalvmbot mentioned this pull request Apr 30, 2025

[JDK-8355981] Adapt JDK-8044609: javax.net.debug options not working and documented as expected oracle/graal#11108

Open

8044609: javax.net.debug options not working and documented as expected #18764

Are you sure you want to change the base?

8044609: javax.net.debug options not working and documented as expected #18764

Uh oh!

Conversation

coffeys commented Apr 12, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewing

Uh oh!

bridgekeeper bot commented Apr 12, 2024

Uh oh!

openjdk bot commented Apr 12, 2024

Uh oh!

openjdk bot commented Apr 12, 2024

Uh oh!

mlbridge bot commented Apr 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

coffeys commented Apr 17, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bridgekeeper bot commented Jun 20, 2024

Uh oh!

bridgekeeper bot commented Aug 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coffeys Sep 4, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coffeys commented Sep 4, 2024 • edited by bridgekeeper bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Sep 4, 2024

Uh oh!

bradfordwetmore left a comment

Choose a reason for hiding this comment

Uh oh!

coffeys commented Nov 21, 2024

Uh oh!

bradfordwetmore left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Dec 24, 2024

Uh oh!

bridgekeeper bot commented Jan 21, 2025

Uh oh!

coffeys commented Apr 30, 2025

Uh oh!

openjdk bot commented Apr 30, 2025

Uh oh!

coffeys commented Apr 30, 2025

Uh oh!

bridgekeeper bot commented May 28, 2025

Uh oh!

Uh oh!

coffeys commented Apr 12, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Apr 12, 2024 •

edited

Loading

coffeys commented Apr 17, 2024 •

edited by bridgekeeper bot

Loading

coffeys Sep 4, 2024 •

edited by bridgekeeper bot

Loading

coffeys commented Sep 4, 2024 •

edited by bridgekeeper bot

Loading