Code Review Request, JDK-8171337 Check for correct SSLEngineImpl/SSLSocketImpl.setSSLParameters handshaker update method

[Xuelei Fan]

Hi Brad,

Please review this handshake update method miss-use fix:

   http://cr.openjdk.java.net/~xuelei/8171337/webrev.00/

The activation process of handshake may consider the parameters in a big 
picture and make adjustment accordingly.  Basically, SSL parameters 
should be configured before the handshake activated.


Thanks,
Xuelei

Re: RFR 8171353: New home for SecurityTools.java test utility

[Artem Smotrakov]

Hi Max,

I am fine with it.

Artem


On 12/16/2016 12:07 AM, Wang Weijun wrote:

Hi Artem

I hope you are OK with this change:

diff --git a/test/lib/security/SecurityTools.java 
b/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
rename from test/lib/security/SecurityTools.java
rename to test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
--- a/test/lib/security/SecurityTools.java
+++ b/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
@@ -20,13 +20,11 @@
   * or visit www.oracle.com if you need additional information or have any
   * questions.
   */
+package jdk.testlibrary;

  import java.util.ArrayList;
  import java.util.Collections;
  import java.util.List;
-import jdk.testlibrary.JDKToolLauncher;
-import jdk.testlibrary.OutputAnalyzer;
-import jdk.testlibrary.ProcessTools;

  public class SecurityTools {

diff --git a/test/sun/security/tools/keytool/PrintSSL.java 
b/test/sun/security/tools/keytool/PrintSSL.java
--- a/test/sun/security/tools/keytool/PrintSSL.java
+++ b/test/sun/security/tools/keytool/PrintSSL.java
@@ -25,7 +25,6 @@
   * @test
   * @bug 6480981 8160624
   * @summary keytool should be able to import certificates from remote SSL 
server
- * @library /lib/security
   * @library /lib/testlibrary
   * @run main/othervm PrintSSL
   */
@@ -37,6 +36,7 @@
  import javax.net.ssl.SSLServerSocketFactory;
  import javax.net.ssl.SSLSocket;
  import jdk.testlibrary.OutputAnalyzer;
+import jdk.testlibrary.SecurityTools;

  public class PrintSSL {

diff --git a/test/sun/security/tools/keytool/ReadJar.java 
b/test/sun/security/tools/keytool/ReadJar.java
--- a/test/sun/security/tools/keytool/ReadJar.java
+++ b/test/sun/security/tools/keytool/ReadJar.java
@@ -25,7 +25,6 @@
   * @test
   * @bug 6890872 8168882
   * @summary keytool -printcert to recognize signed jar files
- * @library /lib/security
   * @library /lib/testlibrary
   */

@@ -33,6 +32,7 @@
  import java.nio.file.Paths;
  import jdk.testlibrary.JarUtils;
  import jdk.testlibrary.OutputAnalyzer;
+import jdk.testlibrary.SecurityTools;

  public class ReadJar {

Thanks
Max

Re: Code Review Request JDK-8129988 JSSE should create a single instance of the cacerts KeyStore

[Sean Mullan]

Hi Xuelei,

First cut of a review at this. I still need to review TrustStoreManager, 
so will get back to you later on that. Looks very good so far.


* KeyStores.java

Do an 'hg rename' on this file, so you can see the diffs between this 
and the new name (TrustStoreUtil), and you preserve the history.


* TrustStoreUtil.java

  54 Set set = new HashSet();

Use <> operator.

  71 } catch (KeyStoreException e) {
  72 // ignore
  73 }

This should be rare, but maybe we want to log this.

* TrustStoreManager.java

  65 public static KeyStore getgetTrustedKeyStore() throws Exception {

s/getget/get/

* TrustManagerFactoryImpl.java

  85 abstract X509TrustManager getInstance(
  86 Collection trustedCerts) throws 
KeyStoreException;


It no longer makes sense for this method to throw KeyStoreException.

* X509TrustManagerImpl.java

  71 X509TrustManagerImpl(String validatorType,
  72 Collection trustedCerts) throws 
KeyStoreException {


No longer can throw KeyStoreException so it can be removed from throws 
clause.


 83 if (debug != null && Debug.isOn("trustmanager")) {
 84 showTrustedCerts();

Not sure why you made this change (and on line 99) because 
showTrustedCerts is still only called if debug is enabled.


--Sean

On 11/26/16 7:46 PM, Xuelei Fan wrote:

Hi,

Please review the performance enhancement update:

   http://cr.openjdk.java.net/~xuelei/8129988/webrev.00/

In SunJSSE provider, there are two ways to use the default trust store
(lib/security/cacerts), using the default SSLContext instance or using
the default trust manager.

The default SSLContext holds a strong reference to a collection of
trusted certificates in cacerts in static mode.  The default trust
manager reads the cacerts file and creates a KeyStore and parses the
certificates each time.

With the growth of cacerts, the loading and cache of trusted certificate
is not performance friendly.

In this fix, I'm trying to find a balance between CPU and memory: reuse
the loaded trusted certificates if possible and release the unused
trusted certificates if necessary.

Thanks,
Xuelei

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

[Vincent Ryan]

I’ve made the change to use activated() rather than started() in SSLEngineImpl 
and SSLSocketImpl
and also the suggestions for ServerHandshaker.

I’ve also updated the tests to check 
SSLEngine/SSLSocket.getHandshakeApplicationProtocolSelector.
All tests pass.

Thanks for all the review comments.



> On 16 Dec 2016, at 01:15, 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.  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: 

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

[Chris Hegarty]

On 16/12/16 03:43, Wang Weijun wrote:

Please take a review at

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

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


Thank you Max. Reviewed.

-Chris.

RFR 8171353: New home for SecurityTools.java test utility

[Wang Weijun]

Hi Artem

I hope you are OK with this change:

diff --git a/test/lib/security/SecurityTools.java 
b/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
rename from test/lib/security/SecurityTools.java
rename to test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
--- a/test/lib/security/SecurityTools.java
+++ b/test/lib/testlibrary/jdk/testlibrary/SecurityTools.java
@@ -20,13 +20,11 @@
  * or visit www.oracle.com if you need additional information or have any
  * questions.
  */
+package jdk.testlibrary;

 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import jdk.testlibrary.JDKToolLauncher;
-import jdk.testlibrary.OutputAnalyzer;
-import jdk.testlibrary.ProcessTools;

 public class SecurityTools {

diff --git a/test/sun/security/tools/keytool/PrintSSL.java 
b/test/sun/security/tools/keytool/PrintSSL.java
--- a/test/sun/security/tools/keytool/PrintSSL.java
+++ b/test/sun/security/tools/keytool/PrintSSL.java
@@ -25,7 +25,6 @@
  * @test
  * @bug 6480981 8160624
  * @summary keytool should be able to import certificates from remote SSL 
server
- * @library /lib/security
  * @library /lib/testlibrary
  * @run main/othervm PrintSSL
  */
@@ -37,6 +36,7 @@
 import javax.net.ssl.SSLServerSocketFactory;
 import javax.net.ssl.SSLSocket;
 import jdk.testlibrary.OutputAnalyzer;
+import jdk.testlibrary.SecurityTools;

 public class PrintSSL {

diff --git a/test/sun/security/tools/keytool/ReadJar.java 
b/test/sun/security/tools/keytool/ReadJar.java
--- a/test/sun/security/tools/keytool/ReadJar.java
+++ b/test/sun/security/tools/keytool/ReadJar.java
@@ -25,7 +25,6 @@
  * @test
  * @bug 6890872 8168882
  * @summary keytool -printcert to recognize signed jar files
- * @library /lib/security
  * @library /lib/testlibrary
  */

@@ -33,6 +32,7 @@
 import java.nio.file.Paths;
 import jdk.testlibrary.JarUtils;
 import jdk.testlibrary.OutputAnalyzer;
+import jdk.testlibrary.SecurityTools;

 public class ReadJar {

Thanks
Max