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

Bradford Wetmore bradford.wetmore at oracle.com
Fri Sep 21 23:20:23 UTC 2018


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.

74:  You might mention that this might happen when clients that don't 
support PSK are used.

248:  You might mention that we know about names, but didn't support it 
(IIRC), and would fail if we tried to use it.

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.

336:  Personal preference again:  it could be just an Exception, rather 
than RTE, since this is not a programming error.

340:   Why are passing proto, then not using it?

347:  If you're going to be running with debug on, maybe include the 
packet dump as well, rather than a separate println here?

412:  You're passing back hsStatus, but not using it.  Suggest void.

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



More information about the security-dev mailing list