RFR: JDK-8210918, Add test to exercise server-side client hello processing

[Jamil Nimeh]

Hi Brad, thanks for the review.  Comments in-line.

On 9/21/2018 4:20 PM, Bradford Wetmore wrote:
> Take my "personal preference" comments with a grain of salt.
>
> This test may be part of a test template, so if some of my comments 
> (e.g. 340/412) are because of your test modification, you can ignore 
> them.
>
>
> CliHelloProcessing.java
> -----------------------
> My personal preference is to have the names written out for testing, 
> unless there is a real reason to not. 
> ClientHelloExtensionProcessing.java?  You can immediately tell what 
> the test is doing by the name.
>
> 74:  Would you please include a printout of what's actually being sent 
> here?  That would help people parse it instead of having to run it and 
> then print out the debug information.  This might also come in handy  
> to help folks understand what you tested against when other problems 
> with other extensions come up.
Yes, I think I could do that for each one.
>
> 74:  You might mention that this might happen when clients that don't 
> support PSK are used.
Good idea.  I will add that.
>
> 248:  You might mention that we know about names, but didn't support 
> it (IIRC), and would fail if we tried to use it.
OK
>
> 283:  Would it be better just to create a single static SSLContext, 
> and then create SSLEngines from that, rather that creating a bunch of 
> SSLContexts?  Same with the packet bufs.
I actually wanted to be able to control the kind of SSLContext so we 
could have engines created in future cases where a client might send a 
1.3 hello and we want to process it with engines that would only do 1.0, 
1.1 or 1.2.  Since I'm going to run one test per execution, it matters 
less now where it gets made since only one context and one buffer is 
being made.
>
> 336:  Personal preference again:  it could be just an Exception, 
> rather than RTE, since this is not a programming error.
I think this will end up being OBE since we're running one test per.  
But in cases where a failure should occur but does not I'll throw 
Exception if you prefer that.
>
>
> 340:   Why are passing proto, then not using it?
I was trying to think ahead for those cases where we want to force the 
server to be 1.2 only.
>
> 347:  If you're going to be running with debug on, maybe include the 
> packet dump as well, rather than a separate println here?
Oh, you mean inline with the debug stuff?  That was just because I find 
it easier to read our parsing without the hexdumps interlaced with the 
debug logging, but I still want to see the bytes I sent...so I let our 
debug parsing happen on stderr and have the packet dump go to stdout.
>
>
> 412:  You're passing back hsStatus, but not using it.  Suggest void.
OK, I can do that.
>
> Hope this helps.
>
> Brad
>
>
>
>
> On 9/21/2018 2:35 PM, Jamil Nimeh wrote:
>> Hello all,
>>
>> This adds a test that lets us send different kinds of client hellos 
>> to a JSSE server. It can be extended to handle different kinds of 
>> corner cases for client hello extension sets as well as fuzzing test 
>> cases in the future.  It also provides some extra test coverage for 
>> JDK-8210334 and JDK-8209916.
>>
>> Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8210918/webrev.01/
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8210918
>>
>> Thanks and have a good weekend,
>> --Jamil