RFR 8171340: HttpNegotiateServer/java test should not use system proxy on Mac

[Wang Weijun]

Please take a review at

   http://cr.openjdk.java.net/~weijun/8171340/webrev.00/

All "openConnection()" modified to "openConnection(Proxy.NO_PROXY)".

Everything else is whitespace change.

Thanks
Max

Re: [9] RFR 8170282: Enable ALPN parameters to be supplied during the TLS handshake

[Xuelei Fan]

On 12/15/2016 5:15 PM, Bradford Wetmore wrote:

(Xuelei, question for you below)

Hi Vinnie,

There is no test yet for SSLEngine/SSLSocket:

public BiFunction
getHandshakeApplicationProtocolSelector() {

Tests are passing at 100% with latest jdk.patch.


SSLSocketImpl.java
==
2672:  Sorry, but I think what you want here is:

 if ((handshaker != null) && !handshaker.started()) {
->
 if ((handshaker != null) && !handshaker.activated()) {

Xuelei can confirm.

More safe to use activated().

Xuelei


BTW, I just filed:

8171337: Check for
 correct SSLEngineImpl/SSLSocketImpl.setSSLParameters
 handshaker update method

as I think setSSLParameters may be using the wrong method.


SSLEngineImpl.java
==
2275:  I misspoke earlier today.  Please add a similar change that you
made in SSLSocket (2671-2674).

if ((handshaker != null) && !handshaker.activated()) {


ServerHandshaker.java
=
536:  Minor nit:  suggest renaming hasCallback to something a little
more descriptive.  By the time you drop 400 lines, you may have
forgotten the variable meaning.  hasAPCallback?

535:  A comments describing your current logic would be nice.

   if (there is a callback) {
  use the callback
   } else {
  use the list
   }

Rest looks good!  Thanks.

Brad



On 12/15/2016 4:39 PM, Vincent Ryan wrote:

Thanks Brad for those review comments.

I’ve make some implementation changes and updated the existing ALPN
tests.
No public API changes.

A new webrev is available at:
http://cr.openjdk.java.net/~vinnie/8170282/webrev.02/




On 9 Dec 2016, at 23:27, Bradford Wetmore > wrote:

API looks good.

SSLEngineImpl/SSLSocketImpl.java

212/229:  I might suggest a more descriptive variable name, like
applicationSelector.  "selector" is a bit ambiguous.

450/1379:
getHandshakeApplicationProtocolSelector());
->
selector);

Xuelei wrote:

> This method would work in server side only.  You mention this point
> in the apiNote part.  I'd like to spec this point in the beginning
> part.

If you do something like this, then you need to be careful to mention
something like "application protocols such as ALPN would do this on
the server side..."  NPN is the reverse, where the server offers the
strings, and the client selects.

> and application developer know what kind of information can be
> retrieved from the handshake session reliably.

Whatever you put in here, make sure it can be changed later in case we
are able revisit the selection mechanism.

> The current application protocol selection scenarios looks like:

In my previous response, I suggested a different approach where
everything ALPN is done together.  I think it may be similar to your
N1-4.

I look forward to the ServerHandshaker change next week.

Brad


On 12/9/2016 1:08 PM, Vincent Ryan wrote:

Thanks for your detailed review Brad. I’ve generated a new webrev:
   http://cr.openjdk.java.net/~vinnie/8170282/webrev.01/




On 9 Dec 2016, at 01:34, Bradford Wetmore
>
wrote:


Hi Vinnie,

On 12/8/2016 2:18 PM, Vincent Ryan wrote:

The Java Servlet Expect Group reported that they have identified a
specific HTTP2 server use-case that cannot
be easily addressed using the existing ALPN APIs.

This changeset fixes that problem. It supports a new callback
mechanism to allow TLS server applications
to set an application protocol during the TLS handshake.
Specifically it allows the cipher suite chosen by the
TLS protocol implementation to be examined by the TLS server
application before it sets the application protocol.
Additional TLS parameters are also available for inspection in the
callback function.

This new mechanism is available only to TLS server applications.
TLS clients will continue to use the existing ALPN APIs.


Technically, the API could be used for NPN (though it's pretty much
dead), so that would be a listing the options on the server side,
and selection on client side.


Bug: https://bugs.openjdk.java.net/browse/JDK-8170282
Webrev: http://cr.openjdk.java.net/~vinnie/8170282/webrev.00/


SSLEngineImpl.java/SSLSocketImpl.java
=
Please use the standard handshaker initialization pattern where the
Handshaker is initialized with all of the data/mechanisms needed for
a complete handshake.  This will will ensure consistent handshake
results and avoid potential strange race conditions.  (There's some
corresponding API comments below.)

You would register your callback after the
handshaker.setApplicationProtocols() calls at (currently) line 444
in the SSLEngineImpl code.


SSLEngine.java/SSLSocket.java
=
I would suggest putting an introduction to this addition in the
class description section, that application values can be set 

Re: [9] RFR 8170282: Enable ALPN parameters to be supplied during the TLS handshake

[Bradford Wetmore]

(Xuelei, question for you below)

Hi Vinnie,

There is no test yet for SSLEngine/SSLSocket:

public BiFunction
getHandshakeApplicationProtocolSelector() {

Tests are passing at 100% with latest jdk.patch.


SSLSocketImpl.java
==
2672:  Sorry, but I think what you want here is:

 if ((handshaker != null) && !handshaker.started()) {
->
 if ((handshaker != null) && !handshaker.activated()) {

Xuelei can confirm.  BTW, I just filed:

8171337: Check for
 correct SSLEngineImpl/SSLSocketImpl.setSSLParameters
 handshaker update method

as I think setSSLParameters may be using the wrong method.


SSLEngineImpl.java
==
2275:  I misspoke earlier today.  Please add a similar change that you 
made in SSLSocket (2671-2674).


if ((handshaker != null) && !handshaker.activated()) {


ServerHandshaker.java
=
536:  Minor nit:  suggest renaming hasCallback to something a little 
more descriptive.  By the time you drop 400 lines, you may have 
forgotten the variable meaning.  hasAPCallback?


535:  A comments describing your current logic would be nice.

   if (there is a callback) {
  use the callback
   } else {
  use the list
   }

Rest looks good!  Thanks.

Brad



On 12/15/2016 4:39 PM, Vincent Ryan wrote:

Thanks Brad for those review comments.

I’ve make some implementation changes and updated the existing ALPN tests.
No public API changes.

A new webrev is available at:
http://cr.openjdk.java.net/~vinnie/8170282/webrev.02/




On 9 Dec 2016, at 23:27, Bradford Wetmore > wrote:

API looks good.

SSLEngineImpl/SSLSocketImpl.java

212/229:  I might suggest a more descriptive variable name, like
applicationSelector.  "selector" is a bit ambiguous.

450/1379:
getHandshakeApplicationProtocolSelector());
->
selector);

Xuelei wrote:

> This method would work in server side only.  You mention this point
> in the apiNote part.  I'd like to spec this point in the beginning
> part.

If you do something like this, then you need to be careful to mention
something like "application protocols such as ALPN would do this on
the server side..."  NPN is the reverse, where the server offers the
strings, and the client selects.

> and application developer know what kind of information can be
> retrieved from the handshake session reliably.

Whatever you put in here, make sure it can be changed later in case we
are able revisit the selection mechanism.

> The current application protocol selection scenarios looks like:

In my previous response, I suggested a different approach where
everything ALPN is done together.  I think it may be similar to your N1-4.

I look forward to the ServerHandshaker change next week.

Brad


On 12/9/2016 1:08 PM, Vincent Ryan wrote:

Thanks for your detailed review Brad. I’ve generated a new webrev:
   http://cr.openjdk.java.net/~vinnie/8170282/webrev.01/




On 9 Dec 2016, at 01:34, Bradford Wetmore
>
wrote:


Hi Vinnie,

On 12/8/2016 2:18 PM, Vincent Ryan wrote:

The Java Servlet Expect Group reported that they have identified a
specific HTTP2 server use-case that cannot
be easily addressed using the existing ALPN APIs.

This changeset fixes that problem. It supports a new callback
mechanism to allow TLS server applications
to set an application protocol during the TLS handshake.
Specifically it allows the cipher suite chosen by the
TLS protocol implementation to be examined by the TLS server
application before it sets the application protocol.
Additional TLS parameters are also available for inspection in the
callback function.

This new mechanism is available only to TLS server applications.
TLS clients will continue to use the existing ALPN APIs.


Technically, the API could be used for NPN (though it's pretty much
dead), so that would be a listing the options on the server side,
and selection on client side.


Bug: https://bugs.openjdk.java.net/browse/JDK-8170282
Webrev: http://cr.openjdk.java.net/~vinnie/8170282/webrev.00/


SSLEngineImpl.java/SSLSocketImpl.java
=
Please use the standard handshaker initialization pattern where the
Handshaker is initialized with all of the data/mechanisms needed for
a complete handshake.  This will will ensure consistent handshake
results and avoid potential strange race conditions.  (There's some
corresponding API comments below.)

You would register your callback after the
handshaker.setApplicationProtocols() calls at (currently) line 444
in the SSLEngineImpl code.


SSLEngine.java/SSLSocket.java
=
I would suggest putting an introduction to this addition in the
class description section, that application values can be set using
SSLParameters.setApplication...() and selected with the default
algorithm, or 

Re: [9] RFR 8170282: Enable ALPN parameters to be supplied during the TLS handshake

[Vincent Ryan]

Thanks Brad for those review comments.

I’ve make some implementation changes and updated the existing ALPN tests.
No public API changes.

A new webrev is available at:
http://cr.openjdk.java.net/~vinnie/8170282/webrev.02/ 




> On 9 Dec 2016, at 23:27, Bradford Wetmore  wrote:
> 
> API looks good.
> 
> SSLEngineImpl/SSLSocketImpl.java
> 
> 212/229:  I might suggest a more descriptive variable name, like 
> applicationSelector.  "selector" is a bit ambiguous.
> 
> 450/1379:
> getHandshakeApplicationProtocolSelector());
> ->
> selector);
> 
> Xuelei wrote:
> 
> > This method would work in server side only.  You mention this point
> > in the apiNote part.  I'd like to spec this point in the beginning
> > part.
> 
> If you do something like this, then you need to be careful to mention 
> something like "application protocols such as ALPN would do this on the 
> server side..."  NPN is the reverse, where the server offers the strings, and 
> the client selects.
> 
> > and application developer know what kind of information can be
> > retrieved from the handshake session reliably.
> 
> Whatever you put in here, make sure it can be changed later in case we are 
> able revisit the selection mechanism.
> 
> > The current application protocol selection scenarios looks like:
> 
> In my previous response, I suggested a different approach where everything 
> ALPN is done together.  I think it may be similar to your N1-4.
> 
> I look forward to the ServerHandshaker change next week.
> 
> Brad
> 
> 
> On 12/9/2016 1:08 PM, Vincent Ryan wrote:
>> Thanks for your detailed review Brad. I’ve generated a new webrev:
>>http://cr.openjdk.java.net/~vinnie/8170282/webrev.01/
>> 
>> 
>> 
>>> On 9 Dec 2016, at 01:34, Bradford Wetmore  
>>> wrote:
>>> 
>>> 
>>> Hi Vinnie,
>>> 
>>> On 12/8/2016 2:18 PM, Vincent Ryan wrote:
 The Java Servlet Expect Group reported that they have identified a 
 specific HTTP2 server use-case that cannot
 be easily addressed using the existing ALPN APIs.
 
 This changeset fixes that problem. It supports a new callback mechanism to 
 allow TLS server applications
 to set an application protocol during the TLS handshake. Specifically it 
 allows the cipher suite chosen by the
 TLS protocol implementation to be examined by the TLS server application 
 before it sets the application protocol.
 Additional TLS parameters are also available for inspection in the 
 callback function.
 
 This new mechanism is available only to TLS server applications. TLS 
 clients will continue to use the existing ALPN APIs.
>>> 
>>> Technically, the API could be used for NPN (though it's pretty much dead), 
>>> so that would be a listing the options on the server side, and selection on 
>>> client side.
>>> 
 Bug: https://bugs.openjdk.java.net/browse/JDK-8170282
 Webrev: http://cr.openjdk.java.net/~vinnie/8170282/webrev.00/
>>> 
>>> SSLEngineImpl.java/SSLSocketImpl.java
>>> =
>>> Please use the standard handshaker initialization pattern where the 
>>> Handshaker is initialized with all of the data/mechanisms needed for a 
>>> complete handshake.  This will will ensure consistent handshake results and 
>>> avoid potential strange race conditions.  (There's some corresponding API 
>>> comments below.)
>>> 
>>> You would register your callback after the 
>>> handshaker.setApplicationProtocols() calls at (currently) line 444 in the 
>>> SSLEngineImpl code.
>>> 
>>> 
>>> SSLEngine.java/SSLSocket.java
>>> =
>>> I would suggest putting an introduction to this addition in the class 
>>> description section, that application values can be set using 
>>> SSLParameters.setApplication...() and selected with the default algorithm, 
>>> or that a more accurate selection mechanism can be created by registering 
>>> the callback that could look at any Handshake in progress and make 
>>> appropriate decisions.
>>> 
>>> 1339:
>>> Registers the callback function that selects an application protocol
>>> value during the SSL/TLS/DTLS handshake.
>>> ->
>>> Registers a callback function that selects an application protocol
>>> value for a SSL/TLS/DTLS handshake.  The function overrides any values set 
>>> using {@link SSLParameters#setApplicationProtocols 
>>> SSLParameters.setApplicationProtocols}.
>>> 
>>> and remove the corresponding section from the return docs (in the {@code 
>>> String section}).
>>> 
>>> the function's first argument enables the current
>>> handshake settings to be inspected.
>>> ->
>>> the function's first argument allows the current SSLEngine(SSLSocket) to be 
>>> inspected, including the handshake session and configuration settings.
>>> 
>>> If null is returned, or a value that was not advertised
>>> then the underlying protocol will determine 

Re: [9] RFR: 8166531: sun/security/ssl/SocketCreation/SocketCreation.java fails intermittently

[Artem Smotrakov]

Hi Xuelei,

I am not sure either, but I would like to try. If it doesn't work, we 
can go and re-write the test to follow SSLSocketTemplate. Actually, I 
don't think it is really possible because SSLSocketTemplate uses 
connect() method which is not used by the test.


Artem


On 12/15/2016 02:04 PM, Xuelei Fan wrote:

I'm not sure this update would work.

As this is a special test, it might be possible to copy/past and 
update the template code (or old sample code) here as we did to use 
the old template.


Xuelei

On 12/15/2016 11:23 AM, Artem Smotrakov wrote:

Hello,

Please review the patch below for
sun/security/ssl/SocketCreation/SocketCreation.java test.

The test has been observed to fail intermittently, but I couldn't
reproduce it by running the test in a loop. The test creates sockets for
TLS connection in different ways. Even if the test is very old, I
wouldn't like to change its original behavior. It might be better to use
SSLSocketTemplate here, but it would require a significant update to
SSLSocketTemplate, and splitting the test to 5 tests. Instead of this, I
am proposing to add some simple synchronization on client side where the
test was seen to fail. If it doesn't work, we can try to use
SSLSocketTemplate then.

Bug: https://bugs.openjdk.java.net/browse/JDK-8166531
Webrev: http://cr.openjdk.java.net/~asmotrak/8166531/webrev.00/

Artem

Re: [9] RFR: 8166531: sun/security/ssl/SocketCreation/SocketCreation.java fails intermittently

[Xuelei Fan]

I'm not sure this update would work.

As this is a special test, it might be possible to copy/past and update 
the template code (or old sample code) here as we did to use the old 
template.


Xuelei

On 12/15/2016 11:23 AM, Artem Smotrakov wrote:

Hello,

Please review the patch below for
sun/security/ssl/SocketCreation/SocketCreation.java test.

The test has been observed to fail intermittently, but I couldn't
reproduce it by running the test in a loop. The test creates sockets for
TLS connection in different ways. Even if the test is very old, I
wouldn't like to change its original behavior. It might be better to use
SSLSocketTemplate here, but it would require a significant update to
SSLSocketTemplate, and splitting the test to 5 tests. Instead of this, I
am proposing to add some simple synchronization on client side where the
test was seen to fail. If it doesn't work, we can try to use
SSLSocketTemplate then.

Bug: https://bugs.openjdk.java.net/browse/JDK-8166531
Webrev: http://cr.openjdk.java.net/~asmotrak/8166531/webrev.00/

Artem

[PATCH] 8170876: NPE in Cipher with java.security.debug=provider

[Adam Petcher]

I'm continuing my quest to rid engine classes of NullPointerException 
due to calling getName() on a null provider. This patch fixes Cipher 
(which fails in a test using NullCipher) and all other engine classes 
that have this pattern. I found them by searching the codebase for 
references to Provider::getName. This fix does not address NPEs caused 
by calls to other methods of Provider, or calls that occur outside the 
engine classes (e.g. someEngine.getProvider().getName()).


I augmented an existing test so it will check for the NPE in Cipher. The 
diff also containsa fix for a small typo in the NullProvider test for 
Signature.


Bug: https://bugs.openjdk.java.net/browse/JDK-8170876

Diff:

diff --git a/src/java.base/share/classes/java/security/KeyStore.java 
b/src/java.base/share/classes/java/security/KeyStore.java

--- a/src/java.base/share/classes/java/security/KeyStore.java
+++ b/src/java.base/share/classes/java/security/KeyStore.java
@@ -824,10 +824,14 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("KeyStore." + type.toUpperCase() + " type 
from: " +

-this.provider.getName());
+getProviderName());
 }
 }

+private String getProviderName() {
+return (provider == null) ? "(no provider)" : provider.getName();
+}
+
 /**
  * Returns a keystore object of the specified type.
  *
diff --git 
a/src/java.base/share/classes/java/security/MessageDigest.java 
b/src/java.base/share/classes/java/security/MessageDigest.java

--- a/src/java.base/share/classes/java/security/MessageDigest.java
+++ b/src/java.base/share/classes/java/security/MessageDigest.java
@@ -430,13 +430,17 @@
 return digest();
 }

+private String getProviderName() {
+return (provider == null) ? "(no provider)" : provider.getName();
+}
+
 /**
  * Returns a string representation of this message digest object.
  */
 public String toString() {
 ByteArrayOutputStream baos = new ByteArrayOutputStream();
 PrintStream p = new PrintStream(baos);
-p.print(algorithm+" Message Digest from "+provider.getName()+", ");
+p.print(algorithm+" Message Digest from "+getProviderName()+", ");
 switch (state) {
 case INITIAL:
 p.print("");
diff --git a/src/java.base/share/classes/java/security/SecureRandom.java 
b/src/java.base/share/classes/java/security/SecureRandom.java

--- a/src/java.base/share/classes/java/security/SecureRandom.java
+++ b/src/java.base/share/classes/java/security/SecureRandom.java
@@ -310,10 +310,14 @@

 if (!skipDebug && pdebug != null) {
 pdebug.println("SecureRandom." + algorithm +
-" algorithm from: " + this.provider.getName());
+" algorithm from: " + getProviderName());
 }
 }

+private String getProviderName() {
+return (provider == null) ? "(no provider)" : provider.getName();
+}
+
 /**
  * Returns a {@code SecureRandom} object that implements the specified
  * Random Number Generator (RNG) algorithm.
diff --git a/src/java.base/share/classes/javax/crypto/Cipher.java 
b/src/java.base/share/classes/javax/crypto/Cipher.java

--- a/src/java.base/share/classes/javax/crypto/Cipher.java
+++ b/src/java.base/share/classes/javax/crypto/Cipher.java
@@ -611,6 +611,10 @@
 return getInstance(transformation, p);
 }

+private String getProviderName() {
+return (provider == null)  ? "(no provider)" : provider.getName();
+}
+
 /**
  * Returns a {@code Cipher} object that implements the specified
  * transformation.
@@ -1278,7 +1282,7 @@
 if (!skipDebug && pdebug != null) {
 pdebug.println("Cipher." + transformation + " " +
 getOpmodeString(opmode) + " algorithm from: " +
-this.provider.getName());
+getProviderName());
 }
 }

@@ -1421,7 +1425,7 @@
 if (!skipDebug && pdebug != null) {
 pdebug.println("Cipher." + transformation + " " +
 getOpmodeString(opmode) + " algorithm from: " +
-this.provider.getName());
+getProviderName());
 }
 }

@@ -1564,7 +1568,7 @@
 if (!skipDebug && pdebug != null) {
 pdebug.println("Cipher." + transformation + " " +
 getOpmodeString(opmode) + " algorithm from: " +
-this.provider.getName());
+getProviderName());
 }
 }

@@ -1754,7 +1758,7 @@
 if (!skipDebug && pdebug != null) {
 pdebug.println("Cipher." + transformation + " " +
 getOpmodeString(opmode) + " algorithm from: " +
-this.provider.getName());
+getProviderName());
 }
 }

diff --git a/src/java.base/share/classes/javax/crypto/KeyAgreement.java 
b/src/java.base/share/classes/javax/crypto/KeyAgreement.java

--- 

[9] RFR: 8166531: sun/security/ssl/SocketCreation/SocketCreation.java fails intermittently

[Artem Smotrakov]

Hello,

Please review the patch below for 
sun/security/ssl/SocketCreation/SocketCreation.java test.


The test has been observed to fail intermittently, but I couldn't 
reproduce it by running the test in a loop. The test creates sockets for 
TLS connection in different ways. Even if the test is very old, I 
wouldn't like to change its original behavior. It might be better to use 
SSLSocketTemplate here, but it would require a significant update to 
SSLSocketTemplate, and splitting the test to 5 tests. Instead of this, I 
am proposing to add some simple synchronization on client side where the 
test was seen to fail. If it doesn't work, we can try to use 
SSLSocketTemplate then.


Bug: https://bugs.openjdk.java.net/browse/JDK-8166531
Webrev: http://cr.openjdk.java.net/~asmotrak/8166531/webrev.00/

Artem