RFR[10] 8186057: TLS interoperability testing between different Java versions
Artem
artem.smotrakov at oracle.com
Thu Nov 23 17:06:30 UTC 2017
Hi John,
A couple of more (probably minor) comments. Addressing them might
simplify the code and make it more readable. Although, one might argue
with it. So, feel free to ignore them. The only thing which I would like
to clarify is the questing about timeouts below.
I see you use multi-lined boolean expressions and ternary operators
(especially when they are long). It might make the code shorter, but I
believe in most cases it doesn't make it more readable.
If you really want to make the code shorter (I don't think it should be
the goal), you can also consider dropping "else" blocks if you just
return values.
I would also avoid implicit converting an integer to a string by
concatenation of the integer with an empty string. You can use
String.valueOf() instead.
Please also see inline.
On 11/23/2017 04:54 PM, sha.jiang at oracle.com 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
Thank you. It might be better to add some space between columns.
>>
>> 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.
My questing is - should timeout be always considered as a failure? Why
don't you just check if server or client result is FAIL?
if (clientStatus == FAIL || serverStatus == FAIL) {
return FAIL;
}
return PASS;
>
>>
>> Otherwise, looks fine to me.
>>
>> A couple of minor comments which you may want to address:
>>
>> 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--
I don't see that it says that using "ordinal" is not recommended. I
think "sequence" just duplicates of "ordinal".
Artem
>
> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>
More information about the security-dev
mailing list