Code Review Request JDK-8170329 New SSLSocket testing template

[Xue-Lei Fan]

Thanks for the review, Artem.

On 12/2/2016 2:41 PM, Artem Smotrakov wrote:
> Hi Xuelei,
>
> I am not sure how the updates in SimpleValidator relate to the template
> for JSSE tests.
The certificates generated for the template have the same subject and 
issuer for RSA, DSA and EC algorithms.  If using the SimpleValidator, it 
cannot find the right root cert for a particular end entity certificate 
any more.  The issue is not exposed in the past because the certs in etc 
directory are self-signed and version 1 certificates.  It's not common 
to use version 1 and self-signed certs for general JSSE testing, so I 
move to version 3 and use end entity certificate for authentication in 
this update.

> It might be better to separate those changes if I am not
> missing something. This update in SimpleValidator looks okay to me, but
> taking into account Sean's comments below, I'll let someone who is more
> knowledgeable review it as well. I would prefer not to mix it with
> updates for SSLSocketTemplate.
>
It saves time and easier to backport if using one bug.  I'm OK to fix 
them separately.  Let's whether Sean or Weijun can have free cycle for 
the review of this part.

> SSLSocketTemplate.java
>
> - Fields in ContextParameters can be final
Good suggestion!

> - Looks like JSSE test are supposed to extend SSLSocketTemplate. I
> suppose serverCondition/clientCondition should not be static then. Some
> JSSE test may be safely run in same JVM mode, I think it is better to
> make them non-static.
Good catch!

> - Why did you remove Peer and Application interfaces? I think those
> interfaces make SSLSocketTemplate more flexible since it allows override
> doServerSide/doClientSide logic if necessary - it doesn't seem to be
> worse. If there is no strong reason to remove those interfaces, I would
> prefer to keep them with default static/stateless
> doServerSide/doClientSide versions.
I agree with you that The Peer and Application interfaces are more 
flexible.  I have no strong reason to remove them.  For this update, I 
focus more on simple and minimal sub-classes.  The Peer and Application 
interfaces need some complicated coding (condition, threading, etc) in 
sub-classes, so the design does not fall into the scope.  I may prefer 
to remove them at this update and see what happens if we moving forward 
with more test update.  We can add them back or create a more flexible 
one if we need the flexibility in the future.

> - It may be better to pass a ContextParameters instance to
> createSSLContext() method because we may want to use more parameters
> while creating an SSL context.
Yes.

> It also might make sense to make
> createSSLContext() a part of public API which I think would make the
> template more flexible.
We have had the protected createClient/ServerSSLContext() methods.

> - Exceptions are printed out in startServer/startClient methods, it
> doesn't look necessary to use suppressed exceptions and initCause()
> method. What was wrong with the code in runTest() method? The code in
> runTest() method looks shorter and cleaner to me.
>
The main issue of runTest() is that it does not throw the original 
exception.  Exception tacks have more information for debugging.  I want 
to keep the good side of Brad's previous hardness on this point.

> ProxyAuthTest.java
>
> - line 153, I believe try-with-resources doesn't make it worse.
Right.

> - lines 114-123, this code is used quite often by tests, why don't we
> keep it in SSLSocketTemplate?
>
Good point!  I don't like it in SSLSocketTemplate as it is used for HTTP 
connections only, but it may be worthy to have a sub-template for HTTPS 
client testing.  What do you think if we address this enhancement in a 
new bug?

Thanks,
Xuelei

> Artem
>
> On 12/02/2016 11:23 AM, Xue-Lei Fan wrote:
>> On 11/29/2016 5:22 AM, Sean Mullan wrote:
>>> On 11/27/16 7:43 AM, Xuelei Fan wrote:
>>>> On 11/27/2016 6:04 PM, Wang Weijun wrote:
>>>>> This is not only a test update.
>>>>>
>>>> No, I happened to find an implementation issue with the new test, so
>>>> fix
>>>> it altogether.  The issue is that the simple validator
>>>> (SimpleValidator.java) does not support SKID/AKID during cert path
>>>> build.  If two trusted certs has the same subject,  the simple
>>>> validator
>>>> may not be able to find the right one.
>>>
>>> We have had issues in the PKIX CertPathBuilder with matching on
>>> AKID/SKID when building certpaths, so we want to be careful not to
>>> introduce a similar issue. See this bug for more information:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8072463
>>>
>>> I have not reviewed the fix enough to know if this issue applies here
>>> but please double-check it.
>>>
>> The KID are used for best effort matching in this update.  If no KIDs
>> get matched, the previous behavior is reserved. Should be safe, I think.
>>
>> Xuelei
>>
>>> --Sean
>>>
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>>>> On Nov 27, 2016, at 9:35 AM, Xuelei Fan <xuelei.fan at oracle.com>
>>>>>> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Please review this test update:
>>>>>>
>>>>>>   http://cr.openjdk.java.net/~xuelei/8170329/webrev.00/
>>>>>>
>>>>>> The new template (SSLSocketTemplate.java) could be used to avoid the
>>>>>> anti-free-port issues.  By using sub-classes of it, the new one can
>>>>>> simplify the general SSLSocket test code significantly.
>>>>>>
>>>>>> Thanks,
>>>>>> Xuelei
>>>>>
>