RFR[9] JDK-8164595: javax/net/ssl/FixingJavadocs/SSLSessionNulls.java fails intermittently with javax.net.ssl.SSLHandshakeException: Remote host terminated the handshake

[John Jiang]

Hi,
Please review this patch for test 
javax/net/ssl/FixingJavadocs/SSLSessionNulls.java.
It takes advantage of javax/net/ssl/templates/SSLTest.java to fix the 
intermittent SSLHandshakeException issue.


Webrev: http://cr.openjdk.java.net/~jjiang/8164595/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8164595

Best regards,
John Jiang

Re: Code review request, JDK-8168822, Document that algorithm restrictions do not apply to trusted certs

[Xuelei Fan]

New webrev:
http://cr.openjdk.java.net/~xuelei/8168822/webrev.01/

On 10/27/2016 8:34 AM, Wang Weijun wrote:

One question: I thought for TLS, you check twice. First using
jdk.tls.disabledAlgorithms on cipher suites etc, and second using
jdk.certpath.disabledAlgorithms on certificates. Why is
jdk.tls.disabledAlgorithms applied to cert at all?

jdk.tls.disabledAlgorithms also check certificates used during 
handshaking, not only cipher suites.



Thanks
Max

On 10/27/2016 8:30 AM, Wang Weijun wrote:

I don't think this applies to jdk.jar.disabledAlgorithms. While the
private key algorithm and key size are determined by the certificate, I
think they are always checked even if the end-entity cert is trusted
(For example, a trusted self-signed cert).


Make sense to me.  I removed the update on jdk.jar.disabledAlgorithms.

Thanks,
Xuelei


Thanks
Max

On 10/27/2016 8:04 AM, Xuelei Fan wrote:

Hi,

Please review the simple fix:

http://cr.openjdk.java.net/~xuelei/8168822/webrev/

Algorithm restrictions do not apply to trusted certs as the
application or customer has made the decision to trust the "trusted
cert".  However, this point is not explicit for general developers and
users.  We'd better to clarify this point explicitly.

In the update, I add a short note for each algorithm constraint security
properties:

   Note: Algorithm restrictions do not apply to trusted certificates.

Doc only update, no new regression test.

Thanks,
Xuelei

Re: Code review request, JDK-8168822, Document that algorithm restrictions do not apply to trusted certs

[Wang Weijun]

One question: I thought for TLS, you check twice. First using 
jdk.tls.disabledAlgorithms on cipher suites etc, and second using 
jdk.certpath.disabledAlgorithms on certificates. Why is 
jdk.tls.disabledAlgorithms applied to cert at all?


Thanks
Max

On 10/27/2016 8:30 AM, Wang Weijun wrote:

I don't think this applies to jdk.jar.disabledAlgorithms. While the
private key algorithm and key size are determined by the certificate, I
think they are always checked even if the end-entity cert is trusted
(For example, a trusted self-signed cert).

Thanks
Max

On 10/27/2016 8:04 AM, Xuelei Fan wrote:

Hi,

Please review the simple fix:

http://cr.openjdk.java.net/~xuelei/8168822/webrev/

Algorithm restrictions do not apply to trusted certs as the
application or customer has made the decision to trust the "trusted
cert".  However, this point is not explicit for general developers and
users.  We'd better to clarify this point explicitly.

In the update, I add a short note for each algorithm constraint security
properties:

   Note: Algorithm restrictions do not apply to trusted certificates.

Doc only update, no new regression test.

Thanks,
Xuelei

Re: Code review request, JDK-8168822, Document that algorithm restrictions do not apply to trusted certs

[Wang Weijun]

I don't think this applies to jdk.jar.disabledAlgorithms. While the 
private key algorithm and key size are determined by the certificate, I 
think they are always checked even if the end-entity cert is trusted 
(For example, a trusted self-signed cert).


Thanks
Max

On 10/27/2016 8:04 AM, Xuelei Fan wrote:

Hi,

Please review the simple fix:

http://cr.openjdk.java.net/~xuelei/8168822/webrev/

Algorithm restrictions do not apply to trusted certs as the
application or customer has made the decision to trust the "trusted
cert".  However, this point is not explicit for general developers and
users.  We'd better to clarify this point explicitly.

In the update, I add a short note for each algorithm constraint security
properties:

   Note: Algorithm restrictions do not apply to trusted certificates.

Doc only update, no new regression test.

Thanks,
Xuelei

Code review request, JDK-8168822, Document that algorithm restrictions do not apply to trusted certs

[Xuelei Fan]

Hi,

Please review the simple fix:

http://cr.openjdk.java.net/~xuelei/8168822/webrev/

Algorithm restrictions do not apply to trusted certs as the
application or customer has made the decision to trust the "trusted 
cert".  However, this point is not explicit for general developers and 
users.  We'd better to clarify this point explicitly.


In the update, I add a short note for each algorithm constraint security 
properties:


   Note: Algorithm restrictions do not apply to trusted certificates.

Doc only update, no new regression test.

Thanks,
Xuelei

Re: Code Review Request JDK-8161106 Improve SSLSocket test template

[Artem Smotrakov]

There is SSLTest.java which follows SSLSocketSample.java and can be used 
by other tests.


Artem


On 10/26/2016 09:45 AM, Xuelei Fan wrote:

The new test case is just a test in order to make sure this approach works in 
the testing environment.  I plan to remove both of the sample and template, and 
re-org them to a class that can be inherited from.

Xuelei


On 27 Oct 2016, at 12:31 AM, Bradford Wetmore  
wrote:

Xuelei,

Sorry that I didn't have time to look at this earlier.

Why did you create a new file SSLSocketSample.java instead of just updating 
SSLSocketTemplate.java?  Why should I use one vs the other?

IMHO, unless there's a good reason to keep both, we should just copy the 
contents of SSLSocketSample.java to SSLSocketTemplate.java, and remove 
SSLSocketSample.java.

Brad




On 7/24/2016 10:22 PM, Weijun Wang wrote:



On 7/25/2016 13:14, Xuelei Fan wrote:

On 7/25/2016 12:15 PM, Weijun Wang wrote:
Is it possible to use a single new CountDownLatch(2)?


Per the spec, the countDown() release all waiting threads if the count
reaches zero, and the await() will not return until the latch has
counted down to zero, or interrupted or timeout.  It's difficult to use
one instance of CountDownLatch(2) for two conditions.

Ah, yes. I forgot about that.


Also, I think comments on lines 145-149 and 199-203 are not really
necessary, the println() lines after them are quite clear.


The comments make the logic easier to understand, I think.  Let's keep
the comments if it is not a big concern of yours.

Sure.

So everything looks fine to me.

Thanks
Max


Thanks,
Xuelei


--Max


On 7/25/2016 11:38, Xuelei Fan wrote:
Hi Weijun,

Please review this update.  Per you suggestion, I updated to use
CountDownLatch for the synchronization between client and server.
CountDownLatch is more simple than ReentrantLock in the context.

http://cr.openjdk.java.net/~xuelei/8161106/webrev.03/

Thanks,
Xuelei

Re: RFR[9] JDK-8168064: sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java failed with "Received fatal alert: handshake_failure"

[Artem Smotrakov]

Hi John,

Looks good to me, thank you for the update.

Artem


On 10/26/2016 04:45 AM, John Jiang wrote:

Hi Artem,
Please take a look at this version: 
http://cr.openjdk.java.net/~jjiang/8168064/webrev.02/

It set a new Server peer.

Best regards,
John Jiang

On 2016/10/25 1:33, Artem Smotrakov wrote:

Hi John,

I think it is too late to set parameters for server socket in 
setServerApplication() because handshaking is already done at this 
point.


You can use setServerPeer() and setClientPeer() if you need to 
configure server socket. In this case, you need to follow 
SSLSocketSample.java to implement doServerSide().


Artem


On 10/24/2016 03:24 AM, John Jiang wrote:

Hi Artem,
Thanks for your review.
Would you like take a look at the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8168064/webrev.01/
I also modified SSLTest.java a bit to expose SSLServerSocket 
instance to support the fixing.


Best regards,
John Jiang


On 2016/10/22 1:50, Artem Smotrakov wrote:

Hi John,

It may be better to use SSLTest.java to avoid duplicate code. The 
class basically contains parts of SSLSocketSample.java


http://hg.openjdk.java.net/jdk9/dev/jdk/file/0fb9ba19a63a/test/javax/net/ssl/templates/SSLTest.java 



Here is an example

http://hg.openjdk.java.net/jdk9/dev/jdk/file/0fb9ba19a63a/test/sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java 



Artem


On 10/20/2016 10:13 PM, John Jiang wrote:

Hi,
Please review this patch for fixing an intermittent issue on test 
sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java.
The fix applies the pattern from 
test/javax/net/ssl/templates/SSLSocketSample.java


Webrev: http://cr.openjdk.java.net/~jjiang/8168064/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8168064

Best regards,
John Jiang

Re: Code Review Request JDK-8161106 Improve SSLSocket test template

[Xuelei Fan]

The new test case is just a test in order to make sure this approach works in 
the testing environment.  I plan to remove both of the sample and template, and 
re-org them to a class that can be inherited from.

Xuelei

> On 27 Oct 2016, at 12:31 AM, Bradford Wetmore  
> wrote:
> 
> Xuelei,
> 
> Sorry that I didn't have time to look at this earlier.
> 
> Why did you create a new file SSLSocketSample.java instead of just updating 
> SSLSocketTemplate.java?  Why should I use one vs the other?
> 
> IMHO, unless there's a good reason to keep both, we should just copy the 
> contents of SSLSocketSample.java to SSLSocketTemplate.java, and remove 
> SSLSocketSample.java.
> 
> Brad
> 
> 
> 
>> On 7/24/2016 10:22 PM, Weijun Wang wrote:
>> 
>> 
>>> On 7/25/2016 13:14, Xuelei Fan wrote:
 On 7/25/2016 12:15 PM, Weijun Wang wrote:
 Is it possible to use a single new CountDownLatch(2)?
 
>>> Per the spec, the countDown() release all waiting threads if the count
>>> reaches zero, and the await() will not return until the latch has
>>> counted down to zero, or interrupted or timeout.  It's difficult to use
>>> one instance of CountDownLatch(2) for two conditions.
>> 
>> Ah, yes. I forgot about that.
>> 
>>> 
 Also, I think comments on lines 145-149 and 199-203 are not really
 necessary, the println() lines after them are quite clear.
 
>>> The comments make the logic easier to understand, I think.  Let's keep
>>> the comments if it is not a big concern of yours.
>> 
>> Sure.
>> 
>> So everything looks fine to me.
>> 
>> Thanks
>> Max
>> 
>>> 
>>> Thanks,
>>> Xuelei
>>> 
 --Max
 
> On 7/25/2016 11:38, Xuelei Fan wrote:
> Hi Weijun,
> 
> Please review this update.  Per you suggestion, I updated to use
> CountDownLatch for the synchronization between client and server.
> CountDownLatch is more simple than ReentrantLock in the context.
> 
>http://cr.openjdk.java.net/~xuelei/8161106/webrev.03/
> 
> Thanks,
> Xuelei
> 
>>> 

Re: Code Review Request JDK-8161106 Improve SSLSocket test template

[Bradford Wetmore]

Xuelei,

Sorry that I didn't have time to look at this earlier.

Why did you create a new file SSLSocketSample.java instead of just 
updating SSLSocketTemplate.java?  Why should I use one vs the other?


IMHO, unless there's a good reason to keep both, we should just copy the 
contents of SSLSocketSample.java to SSLSocketTemplate.java, and remove 
SSLSocketSample.java.


Brad



On 7/24/2016 10:22 PM, Weijun Wang wrote:



On 7/25/2016 13:14, Xuelei Fan wrote:

On 7/25/2016 12:15 PM, Weijun Wang wrote:

Is it possible to use a single new CountDownLatch(2)?


Per the spec, the countDown() release all waiting threads if the count
reaches zero, and the await() will not return until the latch has
counted down to zero, or interrupted or timeout.  It's difficult to use
one instance of CountDownLatch(2) for two conditions.


Ah, yes. I forgot about that.




Also, I think comments on lines 145-149 and 199-203 are not really
necessary, the println() lines after them are quite clear.


The comments make the logic easier to understand, I think.  Let's keep
the comments if it is not a big concern of yours.


Sure.

So everything looks fine to me.

Thanks
Max



Thanks,
Xuelei


--Max

On 7/25/2016 11:38, Xuelei Fan wrote:

Hi Weijun,

Please review this update.  Per you suggestion, I updated to use
CountDownLatch for the synchronization between client and server.
CountDownLatch is more simple than ReentrantLock in the context.

http://cr.openjdk.java.net/~xuelei/8161106/webrev.03/

Thanks,
Xuelei

Re: RFR[9] JDK-8168064: sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java failed with "Received fatal alert: handshake_failure"

[John Jiang]

Hi Artem,
Please take a look at this version: 
http://cr.openjdk.java.net/~jjiang/8168064/webrev.02/

It set a new Server peer.

Best regards,
John Jiang

On 2016/10/25 1:33, Artem Smotrakov wrote:

Hi John,

I think it is too late to set parameters for server socket in 
setServerApplication() because handshaking is already done at this point.


You can use setServerPeer() and setClientPeer() if you need to 
configure server socket. In this case, you need to follow 
SSLSocketSample.java to implement doServerSide().


Artem


On 10/24/2016 03:24 AM, John Jiang wrote:

Hi Artem,
Thanks for your review.
Would you like take a look at the updated webrev: 
http://cr.openjdk.java.net/~jjiang/8168064/webrev.01/
I also modified SSLTest.java a bit to expose SSLServerSocket instance 
to support the fixing.


Best regards,
John Jiang


On 2016/10/22 1:50, Artem Smotrakov wrote:

Hi John,

It may be better to use SSLTest.java to avoid duplicate code. The 
class basically contains parts of SSLSocketSample.java


http://hg.openjdk.java.net/jdk9/dev/jdk/file/0fb9ba19a63a/test/javax/net/ssl/templates/SSLTest.java 



Here is an example

http://hg.openjdk.java.net/jdk9/dev/jdk/file/0fb9ba19a63a/test/sun/net/www/protocol/https/HttpsClient/ProxyAuthTest.java 



Artem


On 10/20/2016 10:13 PM, John Jiang wrote:

Hi,
Please review this patch for fixing an intermittent issue on test 
sun/security/ssl/ServerHandshaker/AnonCipherWithWantClientAuth.java.
The fix applies the pattern from 
test/javax/net/ssl/templates/SSLSocketSample.java


Webrev: http://cr.openjdk.java.net/~jjiang/8168064/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8168064

Best regards,
John Jiang