On Tue, 19 Nov 2024 20:56:13 GMT, Bradford Wetmore <[email protected]> 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
