RFR[10] 8186057: TLS interoperability testing between different Java versions

[sha.jiang at oracle.com]

Hi Artem,
Please find the updated webrev at: 
http://cr.openjdk.java.net/~jjiang/8186057/webrev.05/
And see my comments inline.

On 23/11/2017 02:19, Artem wrote:
> Hi John,
>
> Could you please upload the report to 
> http://cr.openjdk.java.net/~jjiang/8186057 ?
http://cr.openjdk.java.net/~jjiang/8186057/webrev.05/report/report.html

>
> One question about Compatibility.caseStatus(). What's the case when 
> you expect a timeout and no client output? Should it always result to 
> a test case failure?
I'm not sure understanding your question. Did you mean server or client 
timeout?
I think client should output something.

>
> Otherwise, looks fine to me.
>
> A couple of minor comments which you may want to address:
>
> 1. Cert.getCert(): it would be nice to add a comment which explains 
> what's returned in the array.
Some comments are added for method Cert.getCerts().

> It might be good to add an "else" block and throw an exception there.
DSA certificates are selected by default. If these certificates are 
unexpected, I think some cases should fail, finally the test should fail.

>
> 2. Client.java, lines 83-84: it's not necessary to print a stack trace 
> if you put the original exception to runtime exception. The same with 
> ProcessUtils.
Fixed.

>
> 3. JdkInfo: the fields of this class should be final since you don't 
> modify them then. If compilation fails because of complex logic in 
> constructor, then it may be a sign that the logic should be 
> simplified. Or, you can put the logic to a static factory method.
That if-else clause is removed and such fields are final now.

>
> 4. "sequence" field of JdkRelease looks redundant. A enum already 
> defines the order. The same with Protocol.
It looks the ordinal of enum constants is not recommended [1].

[1] https://docs.oracle.com/javase/9/docs/api/java/lang/Enum.html#ordinal--

Best regards,
John Jiang

>
> 5.
>
> On 11/14/2017 09:16 AM, sha.jiang at oracle.com wrote:
>> Hi Artem,
>> Please find the new webrev [1].
>> This test has been refactored significantly. It supports more cipher 
>> suites, and case combinations among the TLS communication parameters.
>> For more details, please look through README [2].
>>
>> [1] http://cr.openjdk.java.net/~jjiang/8186057/webrev.04/
>> [2] 
>> http://cr.openjdk.java.net/~jjiang/8186057/webrev.04/test/jdk/javax/net/ssl/compatibility/README.html
>>
>> Best regards,
>> John Jiang
>>
>> On 07/09/2017 18:47, sha.jiang at oracle.com wrote:
>>> Hi Artem,
>>>
>>> On 07/09/2017 16:28, Artem Smotrakov wrote:
>>>> In case of SNI, SSLParemeters.setServerNames() method (and others 
>>>> related to SNI) was introduced in 8. I don't think the code which 
>>>> use this method can be compiled with 6 and 7.
>>> All of Java source are compiled by a JDK 10 build, which is 
>>> specified by jtreg option "-jdk". Other testing JDKs, including 6 
>>> and 7, just use the .class files on runtime.
>>> That's why the below line exists in Compatibility.java,
>>> 32  * @compile -source 1.6 -target 1.6 Server.java Client.java 
>>> JdkUtils.java
>>>>
>>>> But if you want to test SNI with 8+, you need to call them.
>>>>
>>>> https://docs.oracle.com/javase/8/docs/api/javax/net/ssl/SSLParameters.html#setServerNames-java.util.List- 
>>>>
>>> If a feature, like ALPN and SNI, is not supported, such method would 
>>> not be called on runtime.
>>>
>>> Best regards,
>>> John Jiang
>>>>
>>>> Artem
>>>>
>>>> On 09/07/2017 11:24 AM, sha.jiang at oracle.com wrote:
>>>>> Hi Artem,
>>>>>
>>>>> On 07/09/2017 16:07, Artem Smotrakov wrote:
>>>>>>>
>>>>>>>> - Please use try-with-resources if possible (files, sockets, etc)
>>>>>>> The test uses only JDK 6-supported language features, but 
>>>>>>> try-with-resources is introduced by JDK 7.
>>>>>> Here we come to another issue. I believe that it would be good to 
>>>>>> use write clients for all JDK versions. If you use the same code 
>>>>>> for all Java versions, that means that you use only JDK 6 API. 
>>>>>> Let's consider the following:
>>>>>> - we want to test 8 vs 9
>>>>>> - 8 and 9 may have some API (or just functionality) which is not 
>>>>>> supported by 6 (for example, ALPN, SNI)
>>>>>>
>>>>>> If you write code which is compatible with 6, then you can use 
>>>>>> API from newer versions, but we still may want to test it.
>>>>> In fact, the current test has already covered ALPN, though only 
>>>>> JDK 9 and 10 support the associated methods, like 
>>>>> SSLParameters.getApplicationProtocols().
>>>>> ALPN-associated case combinations are ignored if a JDK doesn't 
>>>>> support this feature. Exactly, JdkUtils checks if a specific JDK 
>>>>> build contains method SSLParameters.getApplicationProtocols().
>>>>> I think the same approach could be applied for SNI in the future.
>>>>>
>>>>> Best regards,
>>>>> John Jiang
>>>>>>
>>>>>> Artem
>>>>>>>
>>>>>>> Best regards,
>>>>>>> John Jiang
>>>>>>>>
>>>>>>>> Artem
>>>>>>>>
>>>>>>>> On 09/07/2017 08:52 AM, sha.jiang at oracle.com wrote:
>>>>>>>>> Hi,
>>>>>>>>> Please review this test for checking the interop compatibility 
>>>>>>>>> on JSSE among different JDK releases (from 6 to 10).
>>>>>>>>> It covers the cases, like handshake, data exchange, client 
>>>>>>>>> authentication and APLN, on all TLS versions (if possible).
>>>>>>>>> And the selected TLS cipher suites are: 
>>>>>>>>> TLS_RSA_WITH_AES_128_CBC_SHA, TLS_DHE_DSS_WITH_AES_128_CBC_SHA 
>>>>>>>>> and TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA.
>>>>>>>>> For more details, please look though the README.
>>>>>>>>>
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~jjiang/8186057/webrev.00
>>>>>>>>> Issue: https://bugs.openjdk.java.net/browse/JDK-8186057
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> John Jiang
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>
>