Code Review Request JDK-8170329 New SSLSocket testing template

[Artem Smotrakov]

Hi Xuelei,

I am not sure how the updates in SimpleValidator relate to the template 
for JSSE tests. 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.

SSLSocketTemplate.java

- Fields in ContextParameters can be final
- 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.
- 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.
- 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. It also might make sense to make 
createSSLContext() a part of public API which I think would make the 
template more flexible.
- 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.

ProxyAuthTest.java

- line 153, I believe try-with-resources doesn't make it worse.
- lines 114-123, this code is used quite often by tests, why don't we 
keep it in SSLSocketTemplate?

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
>>>>