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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

sha.jiang
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

Reply | Threaded
Open this post in threaded view
|

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

Artem Smotrakov
Hi John,

Thanks for starting working on this. I believe this tool is going to be
very helpful.

Let me skip some coding comments for a while, I have a couple of
comments about the design. The main idea is that it should cover as many
cases as it can. Even if it might look a bit redundant, I believe it is
actually not redundant because we basically never know what's going to
break down.

1. At the moment, you define cases in Compatibility.java

   59     private static final String[] CASE_TYPES = new String[] {
   60             Utils.CASE_HANDSHAKE,
   61             Utils.CASE_DATA_EXCHANGE,
   62             Utils.CASE_ALPN };

I believe there are much more many case which we can implement later.
But this approach doesn't looks flexible. For example, let's consider
that we'd like to use a couple of extensions in the same time. Let's say
we'd like to use SNI and ALPN. It seems like we have to create a
separate case for that, and add it manually to the array. Would it be
possible to generate cases automatically? For example, you can define
parameters of TLS connection, for example:
- handshake type: full, session-resumption
- use ALPN: true, false
- use SNI: true, false
- use client auth: true, false,
etc

Then you can build a Cartesian product of all parameters. Client and
sever should take parameters, and say if they support all of the or not
(for example, JDK 6 might not support all features that JDK 9 does). If
it does, client and server can configure themselves with those
parameters, and start handshaking.

2. With the approach above, it might be possible to avoid those nested
loops:

   93       for (String caseType : CASE_TYPES) {
   94             for (String protocol : PROTOCOLS) {
   95                 for (String cipherSuite : CIPHER_SUITES) {
   96                     for (JdkInfo serverJdk : jdkInfos) {

3. I believe we should cover all supported cipher suites. Test run is
going to take more time, but it think it's okay. Another option is to
introduce two modes:
- light: run the test with a couple of cipher suites
- heavy: run the test with all supported cipher suites


A couple of comments about the code:
- What's the reason of using disabled SHA1 and RSA with 1024-bit keys?
You might use stronger algorithms and key sizes, and avoid modifying
java.security file
- Please use try-with-resources if possible (files, sockets, etc)

Artem

On 09/07/2017 08:52 AM, [hidden email] 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
>

Reply | Threaded
Open this post in threaded view
|

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

sha.jiang
Hi Artem,
Thanks for your comments!
Please see inline.

On 07/09/2017 14:32, Artem Smotrakov wrote:

> Hi John,
>
> Thanks for starting working on this. I believe this tool is going to
> be very helpful.
>
> Let me skip some coding comments for a while, I have a couple of
> comments about the design. The main idea is that it should cover as
> many cases as it can. Even if it might look a bit redundant, I believe
> it is actually not redundant because we basically never know what's
> going to break down.
>
> 1. At the moment, you define cases in Compatibility.java
>
>   59     private static final String[] CASE_TYPES = new String[] {
>   60             Utils.CASE_HANDSHAKE,
>   61             Utils.CASE_DATA_EXCHANGE,
>   62             Utils.CASE_ALPN };
>
> I believe there are much more many case which we can implement later.
> But this approach doesn't looks flexible. For example, let's consider
> that we'd like to use a couple of extensions in the same time. Let's
> say we'd like to use SNI and ALPN. It seems like we have to create a
> separate case for that, and add it manually to the array. Would it be
> possible to generate cases automatically? For example, you can define
> parameters of TLS connection, for example:
> - handshake type: full, session-resumption
> - use ALPN: true, false
> - use SNI: true, false
> - use client auth: true, false,
> etc
>
> Then you can build a Cartesian product of all parameters. Client and
> sever should take parameters, and say if they support all of the or
> not (for example, JDK 6 might not support all features that JDK 9
> does). If it does, client and server can configure themselves with
> those parameters, and start handshaking.
Frankly, what case combinations would be tested is a problem. Currently,
I don't expand the combinations too much.
For example, I don't make combinations for data exchange and ALPN, and
client auth always be enabled.
That's why I don't use complex data structures.
If the test requirements or case combinations could be confirmed, the
data structures would be enriched.

> 2. With the approach above, it might be possible to avoid those nested
> loops:
>
>   93       for (String caseType : CASE_TYPES) {
>   94             for (String protocol : PROTOCOLS) {
>   95                 for (String cipherSuite : CIPHER_SUITES) {
>   96                     for (JdkInfo serverJdk : jdkInfos) {
But, how to build the above Cartesian product automatically (not by manual)?

> 3. I believe we should cover all supported cipher suites. Test run is
> going to take more time, but it think it's okay. Another option is to
> introduce two modes:
> - light: run the test with a couple of cipher suites
> - heavy: run the test with all supported cipher suites
I hesitate to do that.
Different JDK builds and TLS versions supports different cipher suites.
And many cipher suites use the same algorithms on key exchange,
signature, encryption/decryption and authentication, then is it
necessary to check each of them?
Although we don't know "what's going to break down", I'm not sure a
single test could check every point on TLS communication.
> A couple of comments about the code:
> - What's the reason of using disabled SHA1 and RSA with 1024-bit keys?
> You might use stronger algorithms and key sizes, and avoid modifying
> java.security file
Different JDK builds possibly support different algorithms and have
different restrictions on such algorithms and key sizes. Weaker or
stronger, that is relative. And this point should not be a focus of the
test.
So, I just pick the ones that are common in history, and provides an
alternative java.security file to avoid possible broken.

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

Best regards,
John Jiang

>
> Artem
>
> On 09/07/2017 08:52 AM, [hidden email] 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
>>
>
>

Reply | Threaded
Open this post in threaded view
|

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

Artem Smotrakov
Hi John,

Please see inline.


On 09/07/2017 10:52 AM, [hidden email] wrote:

>>
>> Then you can build a Cartesian product of all parameters. Client and
>> sever should take parameters, and say if they support all of the or
>> not (for example, JDK 6 might not support all features that JDK 9
>> does). If it does, client and server can configure themselves with
>> those parameters, and start handshaking.
> Frankly, what case combinations would be tested is a problem.
> Currently, I don't expand the combinations too much.
> For example, I don't make combinations for data exchange and ALPN, and
> client auth always be enabled.
> That's why I don't use complex data structures.
> If the test requirements or case combinations could be confirmed, the
> data structures would be enriched.
That's fine to start with limited amount of parameters, but I believe we
can design the test to be able to extend it easily in future. That's my
main point here.

>
>> 2. With the approach above, it might be possible to avoid those
>> nested loops:
>>
>>   93       for (String caseType : CASE_TYPES) {
>>   94             for (String protocol : PROTOCOLS) {
>>   95                 for (String cipherSuite : CIPHER_SUITES) {
>>   96                     for (JdkInfo serverJdk : jdkInfos) {
> But, how to build the above Cartesian product automatically (not by
> manual)?
"automatically" means writing some code of course, but I believe we
don't have to define such an array of cases manually.
>
>> 3. I believe we should cover all supported cipher suites. Test run is
>> going to take more time, but it think it's okay. Another option is to
>> introduce two modes:
>> - light: run the test with a couple of cipher suites
>> - heavy: run the test with all supported cipher suites
> I hesitate to do that.
> Different JDK builds and TLS versions supports different cipher suites.
That's fine. If you test JDK 6 against 9, you can use only cipher suites
which are supported by 6.
> And many cipher suites use the same algorithms on key exchange,
> signature, encryption/decryption and authentication, then is it
> necessary to check each of them?
I think it would be worth to be able to do that at least. Again, we
never know what can go wrong.
> Although we don't know "what's going to break down", I'm not sure a
> single test could check every point on TLS communication.
Right, we can't test everything, but we can to our best. If we know that
the test can be easily enhanced in this way, it's worth to do that.

>> A couple of comments about the code:
>> - What's the reason of using disabled SHA1 and RSA with 1024-bit
>> keys? You might use stronger algorithms and key sizes, and avoid
>> modifying java.security file
> Different JDK builds possibly support different algorithms and have
> different restrictions on such algorithms and key sizes. Weaker or
> stronger, that is relative. And this point should not be a focus of
> the test.
> So, I just pick the ones that are common in history, and provides an
> alternative java.security file to avoid possible broken.
That makes sense.
>
>> - 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.

Artem

>
> Best regards,
> John Jiang
>>
>> Artem
>>
>> On 09/07/2017 08:52 AM, [hidden email] 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
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

sha.jiang
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, [hidden email] 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
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

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

Artem Smotrakov
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.

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-

Artem

On 09/07/2017 11:24 AM, [hidden email] 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, [hidden email] 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
>>>>>
>>>>
>>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

sha.jiang
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, [hidden email] 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, [hidden email] 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
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

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

sha.jiang
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, [hidden email] 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, [hidden email] 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, [hidden email] 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
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

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

Artem Smotrakov
Hi John,

Could you please upload the report to
http://cr.openjdk.java.net/~jjiang/8186057 ?

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?

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. It might be good to add an "else" block
and throw an exception there.

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.

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.

4. "sequence" field of JdkRelease looks redundant. A enum already
defines the order. The same with Protocol.

5.

On 11/14/2017 09:16 AM, [hidden email] 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, [hidden email] 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, [hidden email] 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, [hidden email] 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
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

sha.jiang
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, [hidden email] 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, [hidden email] 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, [hidden email] 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, [hidden email] 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
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>
>

Reply | Threaded
Open this post in threaded view
|

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

Artem Smotrakov
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, [hidden email] 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, [hidden email] 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, [hidden email] 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, [hidden email] 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, [hidden email] 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

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

sha.jiang
Hi Artem,
The test is updated again:
http://cr.openjdk.java.net/~jjiang/8186057/webrev.06/
The report table is decorated.
And please see more comments inline.

On 24/11/2017 01:06, Artem wrote:

> 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 not using ternary operator (conditional operator), it has to use
if-else block. That would not be better.
Especially, I don't use complex expressions with conditional operator.
And the coding indent should comply with our convention.

>
> 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.
String (String arg) {
     if (args.equals("foo")) {
         return 'F';
     } else if (args.equals("bar")) {
         return "B";
     } else {
         return "";
     }
}

Do you mean change the above codes to the below style?

String (String arg) {
     if (args.equals("foo")) {
         return 'F';
     } else if (args.equals("bar")) {
         return "B";
     }
     return "";
}

Frankly, I'm not sure which one is better.

>
> 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.
If that, do you suggest to change the following codes (in
Compatibility.java) to use String.valueOf() for boolean values?
  101                 props.put(Utils.PROP_SUPPORTS_SNI_ON_SERVER,
  102                         serverJdk.supportsSNI + "");
  103                 props.put(Utils.PROP_SUPPORTS_ALPN_ON_SERVER,
  104                         serverJdk.supportsALPN + "");
Here, serverJdk.supportsSNI and serverJdk.supportsALPN are boolean values.

  128                     props.put(Utils.PROP_PORT, port + "");
Here, port is an integer, but many existing JDK codes just use [port +
""] instead of [String.valueOf(port)].

>
> Please also see inline.
>
>
> On 11/23/2017 04:54 PM, [hidden email] 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.
I add a style for the table and make it more friendly.
http://cr.openjdk.java.net/~jjiang/8186057/webrev.06/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.
> 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;
Some cases are negative cases, their status should be EXPECTED_FAIL.
A server or client timeout would lead to the case to fail, if the
failure is not expected.
If a server starts normally, but client doesn't start due to a negative
case, the server should get timeout. But this failure is expected.
If a server starts normally, and client get a unexpected timeout, this
failure would make the case to fail.

>>
>>>
>>> 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".
The following words are copied from the description of method
Enum.ordinal():
"Most programmers will have no use for this method. It is designed for
use by sophisticated enum-based data structures, such as EnumSet and
EnumMap."

In addition, Effective Java (2nd) Item #33 also advise us not using
ordinal index.

Best regards,
John Jiang
Reply | Threaded
Open this post in threaded view
|

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

Artem Smotrakov
Hi John,

Let me skip most of cosmetic/style comments. I think you got my points,
I'll let you decide if you want to address them or not. Or, you may want
to ask for other's opinions.

Let's focus on the question about timeout below.

On 11/24/2017 08:57 AM, [hidden email] wrote:
>>>> 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.
> I add a style for the table and make it more friendly.
> http://cr.openjdk.java.net/~jjiang/8186057/webrev.06/report/report.html
The report looks much better now, thank you.

>
>>>>
>>>> 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;
> Some cases are negative cases, their status should be EXPECTED_FAIL.
That's fine, I got that.
> A server or client timeout would lead to the case to fail, if the
> failure is not expected.
> If a server starts normally, but client doesn't start due to a
> negative case, the server should get timeout. But this failure is
> expected.
> If a server starts normally, and client get a unexpected timeout, this
> failure would make the case to fail.
This logic looks unnecessary complex to me. The more complex it is, the
more likely we get a bug there. I think TIMEOUT status is not necessary
here. If a failure is expected on one side, the other side should fail
as well (no matter if it's a timeout or an exception).

If I were you, I would simplify this method like this

if (client == EXPECTED_FAIL && server == EXPECTED_FAIL) {
     return EXPECTED_FAIL;
}

if (client == PASS && server == PASS) {
     return PASS;
}

return FAIL;

If I am not missing something, the pseudo-code above looks better and
simpler.

>
>>>
>>>>
>>>> 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".
> The following words are copied from the description of method
> Enum.ordinal():
> "Most programmers will have no use for this method. It is designed for
> use by sophisticated enum-based data structures, such as EnumSet and
> EnumMap."
I don't think it says that "ordinal" is not recommended to use :)
>
> In addition, Effective Java (2nd) Item #33 also advise us not using
> ordinal index.
I respect and appreciate Mr. Bloch's book, but my point is that
"ordinal" and "sequence" have exact the same meaning, so "sequence" just
duplicates it, and as a result, it looks redundant :)

http://hg.openjdk.java.net/jdk10/master/file/be620a591379/src/java.base/share/classes/java/lang/Enum.java#l91

...
     private final int ordinal;
...
     public final int ordinal() {
         return ordinal;
     }
...

Artem
>
> Best regards,
> John Jiang