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
