RFR: 8350582: Correct the parsing of the ssl value in javax.net.debug [v3]

[Bradford Wetmore]

On Thu, 6 Mar 2025 20:10:58 GMT, Sean Coffey <coffeys at openjdk.org> wrote:

>> Breaking the parent JDK-8044609 JBS issue into sub tasks. 
>> 
>> This patch addresses the main issue which is that `javax.net.debug=ssl ` option is completely broken since TLSv1.3 support was introduced.  This patch should be easier for backporting also.
>> 
>> Wider corrections can be followed up via parent bug.
>
> Sean Coffey has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Incorporate latest review feedback

Depending on the followup discussion of what `ssl` alone means.  

However, several suggestions below, one might help with test coverage.  If you take some/all, then I'll review again.  Should be quick.

test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValuesTest.java line 1:

> 1: /*

A suggestion that could ease/increase test coverage:

        HashMap<String, String> masterMap = new HashMap<>();

        // add one <String, String> for each output category.
        masterMap.put("handshake", "Produced ClientHello handshake message");
        // ...
        
        // for each testcase {
        //   missing = clone masterMap;  // start with the full map
        //   required = new HashMap();  // create an empty map
        //   for each key in the test case {
        //     missing.remove(value);
        //     required.add(value);
        //     }
        //   runTest();
        //   check each value String in required is present
        //   check each value String in missing is not
        // } 
   
You can then easily add test cases with:

    test("ssl","handshake");   // use "..." params if you don't want to do String parsing, and have multiple params

test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValuesTest.java line 27:

> 25:  * @test
> 26:  * @bug 8350582
> 27:  * @library /test/lib /javax/net/ssl/templates ../../

Is `../..` actually needed?  

It ran fine in another test directory several levels below this one.

test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValuesTest.java line 28:

> 26:  * @bug 8350582
> 27:  * @library /test/lib /javax/net/ssl/templates ../../
> 28:  * @summary Verify debug output for different javax.net.debug scenarios

Isn't the summary supposed to match the bugid's description?

test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValuesTest.java line 59:

> 57: 
> 58:     private static Stream<Arguments> patternMatches() {
> 59:         // "Plaintext before ENCRYPTION" comes from "ssl:record:plaintext" option

Minor nits.  >= 80 chars in some lines.  

You could leave as is, or shorten the searched-for message.  I'm ok if you really want to leave, as I don't expect lots of changes in this file.  (horizontal scrolling during reviews is my pet peeve)  

Or you could do:

`Produced ClientHello handshake message`
->
`Produced ClientHello handshake`

test/jdk/sun/security/ssl/SSLLogger/DebugPropertyValuesTest.java line 168:

> 166:     }
> 167: 
> 168:     public static void main(String[] args) throws Exception {

Is this here because for running outside of jtreg, say via the command line?

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

Marked as reviewed by wetmore (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/23781#pullrequestreview-2755088852
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r2036411141
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r2036335133
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r2036416941
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r2036338994
PR Review Comment: https://git.openjdk.org/jdk/pull/23781#discussion_r2036419493