RFR: 8044609: javax.net.debug options not working and documented as expected [v8]

[Sean Coffey]

On Tue, 19 Nov 2024 20:56:13 GMT, Bradford Wetmore <wetmore at openjdk.org> wrote:

>> `sslexpandhandshakeverbose` works in the new implementation. 
>> 
>> the `sslOn `boolean is a fast path helper. We fall back to matching per debug option if required.
>> 
>> I'm going to suggest that we shouldn't have to accept `ssl|moohaha|handshake2354432verbose@#%^%SeanWasHere,keygen` as a valid property value. The lax parsing currently used is somewhat to blame for javax.net.debug parsing being so badly broken since its introduction in the new TLSv1.3 stack in 2018.  i.e. passing the widely documented `ssl` is broken today. I'm not as concerned as some code that might be trying to use the `sslexpandhandshakeverbose` example!
>> 
>> can we document some reasonable separators ? We could go with an EnumSet design approach but I do think it's time we specify a valid separator if so.
>
> IMHO, the JSSE docs are pretty clear (even tho functionally it's been broken for quite some time, sigh). 
> 
>> You do not have to have a separator between options, although a separator such as a colon (:) or a comma (,) helps readability. It does not matter what separators you use, and the ordering of the option keywords is also not important. 
> 
> No separators needed, no ordering enforced.
> 
> Could be something like this:
> 
>             EnumSet<Options> activeOptions = EnumSet.noneOf(Options.class);
>             for (Options e : Options.values()) {
>                 if (property.contains(e.toString())) {
>                     activeOptions.add(e);
>                 }
>             }
>         -----------
>        public static boolean isOn(String checkPoints) {
>             ...etc...
>             String[] options = checkPoints.split(",");
>             for (String option : options) {
>                 option = option.trim().toLowerCase(Locale.ROOT);
>                 if (!activeOptions.contains(option.valueOf(option)) {
>                     return false;
>                 }
>             }
>             return true;
>         }

The docs and implementation can be updated to a new specification if we choose to adopt a more sensible token parsing scheme.  Perhaps we can study that in a follow on issue.

 I've looked at the EnumSet option before also. IMO, it's yields maximum benefit if we start moving to a `"Debug.isOn(Option.SSL_HANDSHAKE)"` type call pattern in logging code in place of `"Debug.isOn("ssl, handshake")"`

I still see some issues with the current proposal (.e.g `"-Djavax.net.debug=sslctx"` would be an illegal option and yet it would turn on all ssl debug logs. Likewise `-Djavax.net.debug=typossl` has the odd behaviour of turning on all ssl debug code. It just strikes me as a broken system.

let me have a look at see if there' a hybrid solution that can be introduced to satisfy concerns.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18764#discussion_r1850188399