Repository: netty/netty
Issue: Adding ability to use more standard JSSE options when using JdkSslContext
Labels: improvement

(Please note the case difference between `SSLContext` and `SslContext` when reading this.)

I was trying to pass various SSL/TLS settings (checking the host name, setting a client certificate and a custom trust store) when using a `JdkSslClientContext` with Netty 4.0.29, when I realised it was actually quite difficult to pass an existing `javax.net.ssl.SSLContext` to the existing classes (`JdkSslContext` and its subclasses). (The code seems fairly similar in the latest 4.0 and 4.1 branches.)

It is currently possible to build a `JdkSslClientContext` using a `TrustManagerFactory` or a list of trusted certificates, a key file or a `KeyManagerFactory`, but there doesn't seem to be anywhere to set all this from an existing `SSLContext` instance (which is one of the main JSSE classes). I realise these can be very useful and convenient, but they make it difficult to use existing `SSLContext` classes (which seems more natural when you're used to the JSSE). (The package protection and `final` modifiers also make it quite tricky to subclass these classes to make such modifications.)

In addition, without any specific settings, it doesn't seem to use the "set" default `SSLContext`. Indeed, it initialises a new `TrustManagerFactory` with `(null, null, null)`, but that's not necessarily the same as `SSLContext.getDefault()` (which can be set with `SSLContext.setDefault(...)` since Java 6). From a JSSE point of view, this seems counter intuitive (since the default values would effectively have different settings from what you would get with `SSLServerSocketFactory.getDefault()`, for example).

Another parameter that would be interesting to pass would be an `SSLParameters` instance. It can provide many settings (otherwise repeated) such as the cipher suites. More importantly, it can provide hostname verification for free since Java 7.

Although Netty 4.x only relies on Java 6, being able to use an `SSLParameters` instance would let code that uses Netty and Java 7 or 8 pass this to enforce hostname verification (for HTTPS):

```
SSLParameters sslParameters = new SSLParameters();
sslParameters.setEndpointIdentificationAlgorithm("HTTPS");
```

Would you be interested in contributions and pull requests in that area? (That last point might be relevant w.r.t. issue #3534.)


Issue comments:
@harbulot As a workaround, you can specify protocols, cipher suites and algorithm downstream, on each `SSLEngine` you get from `SslContext.newEngine`, before passing them to a `SslHandler`.

FYI, that's how things are done in AsyncHttpClient.

> Another parameter that would be interesting to pass would be an SSLParameters instance.

Usually, you don't override the existing parameters, you "edit" them by getting a copy of the current ones, change what you want and set them back. So, instead of passing a value, I suggest passing a function/SAM interface.

@slandelle Regarding the workaround, this is indeed possible, but it would certainly be better to address the problems upstream instead of having to tweak each library that uses Netty (a number of which don't fully expose useful SSL settings unfortunately).

> > Another parameter that would be interesting to pass would be an SSLParameters instance.
> 
> Usually, you don't override the existing parameters, you "edit" them by getting a copy of the current ones, change what you want and set them back. So, instead of passing a value, I suggest passing a function/SAM interface.

I think there's a use for both. From an `SSLContext`/`SSLParameters` point of view, it would still make sense to be able to set the `SSLParameters` instance, in particular thanks to the fact `SSLContext.getDefaultSSLParameters` returns a _copy_ of the default `SSLParameters`.

Hence, you could do something like this:

```
SSLContext sslContext = /* ... create and init as required */
SSLParameters sslParameters = sslContext.getDefaultSSLParameters();
sslParameters.setProtocols(...);
sslParameters.setCipherSuites(...);
sslParameters.setEndpointIdentificationAlgorithm("HTTPS");

JdkSslClientContext jdkSslContext = new JdkSslClientContext(sslContext, sslParameters);
```

You are right there is a issue of precedence regarding default settings. In particular, `SSLEngine.setSSLParameters(...)` will not change the instance's protocols and ciphers suites if those values are `null` in the `SSLParameters` instance, but it will change the other values. (See [`SSLEngine`](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/javax/net/ssl/SSLEngine.java#l1237) and [`SSLEngineImpl`](http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/687fd7c7986d/src/share/classes/sun/security/ssl/SSLEngineImpl.java#l2093) implementations.)

The `JdkSslContext` code would indeed need to choose where it gets its settings from (and where precedence apply). I'd suggest something like:

```
sslEngine.setEnabledCipherSuites(...); // and other settings...
if (sslParameters != null) {
    sslEngine.setSSLParameters(sslParameters);
}
```

That way, users using `SSLParameters` would override all the other settings (except protocols and cipher suite if they haven't set them in their `SSLParameters` instance). Users not using `SSLParameters` would use the current behaviour.

That said, it would certainly also make sense to have a function / SAM interface for improved customisation indeed. It seems it would make sense to have both mechanisms.

Maybe something along these lines could work (pseudo-code not to be taken too literally, of course):

```
sslEngine.setEnabledCipherSuites(...); // and other settings...
SSLParameters sslParameters = this.sslParameters; // null if not set
if (sslParametersCallback != null) {
    sslParameters = sslParametersCallback.call(sslParameters != null ? 
            sslParameters : sslEngine.getSSLParameters);
}
if (sslParameters != null) {
    sslEngine.setSSLParameters(sslParameters);
}
```

If you have a SAM, you can decide to ignore the existing input and return a fresh instance instead.

@harbulot let me work on this

You can now create a `JdkSslContext` from a pre-configured `SSLContext`. See https://github.com/netty/netty/pull/5113


Pull request review comments:
@trustin @Scottmitch asked if this is a breaking change but I think no as the constructor was package private. WDYT ?

Doesn't sound like a breaking change to me because there was no way for a user to extend this class.

- use -> Use
- reference -> use

- use -> Use
- reference -> use


Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/JdkSslClientContext.java b/handler/src/main/java/io/netty/handler/ssl/JdkSslClientContext.java
index ed9e9a0a891..90e6914439b 100644
--- a/handler/src/main/java/io/netty/handler/ssl/JdkSslClientContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/JdkSslClientContext.java
@@ -29,11 +29,13 @@
 
 /**
  * A client-side {@link SslContext} which uses JDK's SSL/TLS implementation.
+ *
+ * @deprecated Use {@link SslContextBuilder} to create {@link JdkSslContext} instances and only
+ * use {@link JdkSslContext} in your code.
  */
+@Deprecated
 public final class JdkSslClientContext extends JdkSslContext {
 
-    private final SSLContext ctx;
-
     /**
      * Creates a new instance.
      *
@@ -245,26 +247,20 @@ public JdkSslClientContext(File trustCertCollectionFile, TrustManagerFactory tru
             File keyCertChainFile, File keyFile, String keyPassword, KeyManagerFactory keyManagerFactory,
             Iterable<String> ciphers, CipherSuiteFilter cipherFilter, JdkApplicationProtocolNegotiator apn,
             long sessionCacheSize, long sessionTimeout) throws SSLException {
-        super(ciphers, cipherFilter, apn, ClientAuth.NONE);
-        try {
-            ctx = newSSLContext(toX509Certificates(trustCertCollectionFile), trustManagerFactory,
-                                toX509Certificates(keyCertChainFile), toPrivateKey(keyFile, keyPassword),
-                                keyPassword, keyManagerFactory, sessionCacheSize, sessionTimeout);
-        } catch (Exception e) {
-            if (e instanceof SSLException) {
-                throw (SSLException) e;
-            }
-            throw new SSLException("failed to initialize the client-side SSL context", e);
-        }
+        super(newSSLContext(toX509CertificatesInternal(
+                trustCertCollectionFile), trustManagerFactory,
+                toX509CertificatesInternal(keyCertChainFile), toPrivateKeyInternal(keyFile, keyPassword),
+                keyPassword, keyManagerFactory, sessionCacheSize, sessionTimeout), true,
+                ciphers, cipherFilter, apn, ClientAuth.NONE);
     }
 
     JdkSslClientContext(X509Certificate[] trustCertCollection, TrustManagerFactory trustManagerFactory,
                         X509Certificate[] keyCertChain, PrivateKey key, String keyPassword,
                         KeyManagerFactory keyManagerFactory, Iterable<String> ciphers, CipherSuiteFilter cipherFilter,
                         ApplicationProtocolConfig apn, long sessionCacheSize, long sessionTimeout) throws SSLException {
-        super(ciphers, cipherFilter, toNegotiator(apn, false), ClientAuth.NONE);
-        ctx = newSSLContext(trustCertCollection, trustManagerFactory, keyCertChain, key, keyPassword,
-                            keyManagerFactory, sessionCacheSize, sessionTimeout);
+        super(newSSLContext(trustCertCollection, trustManagerFactory, keyCertChain, key, keyPassword,
+                keyManagerFactory, sessionCacheSize, sessionTimeout), true,
+                ciphers, cipherFilter, toNegotiator(apn, false), ClientAuth.NONE);
     }
 
     private static SSLContext newSSLContext(X509Certificate[] trustCertCollection,
@@ -298,14 +294,4 @@ private static SSLContext newSSLContext(X509Certificate[] trustCertCollection,
             throw new SSLException("failed to initialize the client-side SSL context", e);
         }
     }
-
-    @Override
-    public boolean isClient() {
-        return true;
-    }
-
-    @Override
-    public SSLContext context() {
-        return ctx;
-    }
 }
diff --git a/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java b/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java
index 4389f8aef0a..bf54d1484aa 100644
--- a/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java
@@ -51,7 +51,7 @@
 /**
  * An {@link SslContext} which uses JDK's SSL/TLS implementation.
  */
-public abstract class JdkSslContext extends SslContext {
+public class JdkSslContext extends SslContext {
 
     private static final InternalLogger logger = InternalLoggerFactory.getInstance(JdkSslContext.class);
 
@@ -140,20 +140,60 @@ private static void addIfSupported(Set<String> supported, List<String> enabled,
     private final List<String> unmodifiableCipherSuites;
     private final JdkApplicationProtocolNegotiator apn;
     private final ClientAuth clientAuth;
+    private final SSLContext sslContext;
+    private final boolean isClient;
 
-    JdkSslContext(Iterable<String> ciphers, CipherSuiteFilter cipherFilter, JdkApplicationProtocolNegotiator apn,
-                  ClientAuth clientAuth) {
+    /**
+     * Creates a new {@link JdkSslContext} from a pre-configured {@link SSLContext}.
+     *
+     * @param sslContext the {@link SSLContext} to use.
+     * @param isClient {@code true} if this context should create {@link SSLEngine}s for client-side usage.
+     * @param clientAuth the {@link ClientAuth} to use. This will only be used when {@param isClient} is {@code false}.
+     */
+    public JdkSslContext(SSLContext sslContext, boolean isClient,
+                         ClientAuth clientAuth) {
+        this(sslContext, isClient, null, IdentityCipherSuiteFilter.INSTANCE,
+                JdkDefaultApplicationProtocolNegotiator.INSTANCE, clientAuth);
+    }
+
+    /**
+     * Creates a new {@link JdkSslContext} from a pre-configured {@link SSLContext}.
+     *
+     * @param sslContext the {@link SSLContext} to use.
+     * @param isClient {@code true} if this context should create {@link SSLEngine}s for client-side usage.
+     * @param ciphers the ciphers to use or {@code null} if the standart should be used.
+     * @param cipherFilter the filter to use.
+     * @param apn the {@link ApplicationProtocolConfig} to use.
+     * @param clientAuth the {@link ClientAuth} to use. This will only be used when {@param isClient} is {@code false}.
+     */
+    public JdkSslContext(SSLContext sslContext, boolean isClient, Iterable<String> ciphers,
+                         CipherSuiteFilter cipherFilter, ApplicationProtocolConfig apn,
+                         ClientAuth clientAuth) {
+        this(sslContext, isClient, ciphers, cipherFilter, toNegotiator(apn, !isClient), clientAuth);
+    }
+
+    JdkSslContext(SSLContext sslContext, boolean isClient, Iterable<String> ciphers, CipherSuiteFilter cipherFilter,
+                  JdkApplicationProtocolNegotiator apn, ClientAuth clientAuth) {
         this.apn = checkNotNull(apn, "apn");
         this.clientAuth = checkNotNull(clientAuth, "clientAuth");
         cipherSuites = checkNotNull(cipherFilter, "cipherFilter").filterCipherSuites(
                 ciphers, DEFAULT_CIPHERS, SUPPORTED_CIPHERS);
         unmodifiableCipherSuites = Collections.unmodifiableList(Arrays.asList(cipherSuites));
+        this.sslContext = checkNotNull(sslContext, "sslContext");
+        this.isClient = isClient;
     }
 
     /**
      * Returns the JDK {@link SSLContext} object held by this context.
      */
-    public abstract SSLContext context();
+    public final SSLContext context() {
+        return sslContext;
+    }
+
+    @Override
+    public final boolean isClient() {
+        return isClient;
+    }
 
     /**
      * Returns the JDK {@link SSLSessionContext} object held by this context.
@@ -210,7 +250,7 @@ private SSLEngine configureAndWrapEngine(SSLEngine engine) {
     }
 
     @Override
-    public JdkApplicationProtocolNegotiator applicationProtocolNegotiator() {
+    public final JdkApplicationProtocolNegotiator applicationProtocolNegotiator() {
         return apn;
     }
 
diff --git a/handler/src/main/java/io/netty/handler/ssl/JdkSslServerContext.java b/handler/src/main/java/io/netty/handler/ssl/JdkSslServerContext.java
index c80665cb159..4434c3a41c4 100644
--- a/handler/src/main/java/io/netty/handler/ssl/JdkSslServerContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/JdkSslServerContext.java
@@ -30,11 +30,13 @@
 
 /**
  * A server-side {@link SslContext} which uses JDK's SSL/TLS implementation.
+ *
+ * @deprecated Use {@link SslContextBuilder} to create {@link JdkSslContext} instances and only
+ * use {@link JdkSslContext} in your code.
  */
+@Deprecated
 public final class JdkSslServerContext extends JdkSslContext {
 
-    private final SSLContext ctx;
-
     /**
      * Creates a new instance.
      *
@@ -210,17 +212,10 @@ public JdkSslServerContext(File trustCertCollectionFile, TrustManagerFactory tru
             File keyCertChainFile, File keyFile, String keyPassword, KeyManagerFactory keyManagerFactory,
             Iterable<String> ciphers, CipherSuiteFilter cipherFilter, JdkApplicationProtocolNegotiator apn,
             long sessionCacheSize, long sessionTimeout) throws SSLException {
-        super(ciphers, cipherFilter, apn, ClientAuth.NONE);
-        try {
-            ctx = newSSLContext(toX509Certificates(trustCertCollectionFile), trustManagerFactory,
-                                toX509Certificates(keyCertChainFile), toPrivateKey(keyFile, keyPassword),
-                                keyPassword, keyManagerFactory, sessionCacheSize, sessionTimeout);
-        } catch (Exception e) {
-            if (e instanceof SSLException) {
-                throw (SSLException) e;
-            }
-            throw new SSLException("failed to initialize the server-side SSL context", e);
-        }
+        super(newSSLContext(toX509CertificatesInternal(trustCertCollectionFile), trustManagerFactory,
+                toX509CertificatesInternal(keyCertChainFile), toPrivateKeyInternal(keyFile, keyPassword),
+                keyPassword, keyManagerFactory, sessionCacheSize, sessionTimeout), false,
+                ciphers, cipherFilter, apn, ClientAuth.NONE);
     }
 
     JdkSslServerContext(X509Certificate[] trustCertCollection, TrustManagerFactory trustManagerFactory,
@@ -228,9 +223,9 @@ public JdkSslServerContext(File trustCertCollectionFile, TrustManagerFactory tru
                         KeyManagerFactory keyManagerFactory, Iterable<String> ciphers, CipherSuiteFilter cipherFilter,
                         ApplicationProtocolConfig apn, long sessionCacheSize, long sessionTimeout,
                         ClientAuth clientAuth) throws SSLException {
-        super(ciphers, cipherFilter, toNegotiator(apn, true), clientAuth);
-        ctx = newSSLContext(trustCertCollection, trustManagerFactory, keyCertChain, key,
-                            keyPassword, keyManagerFactory, sessionCacheSize, sessionTimeout);
+        super(newSSLContext(trustCertCollection, trustManagerFactory, keyCertChain, key,
+                keyPassword, keyManagerFactory, sessionCacheSize, sessionTimeout), false,
+                ciphers, cipherFilter, toNegotiator(apn, true), clientAuth);
     }
 
     private static SSLContext newSSLContext(X509Certificate[] trustCertCollection,
@@ -272,13 +267,4 @@ private static SSLContext newSSLContext(X509Certificate[] trustCertCollection,
         }
     }
 
-    @Override
-    public boolean isClient() {
-        return false;
-    }
-
-    @Override
-    public SSLContext context() {
-        return ctx;
-    }
 }
diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java
index ee5536f3904..a38e28192dc 100644
--- a/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java
@@ -570,22 +570,6 @@ private static long newBIO(ByteBuf buffer) throws Exception {
         return bio;
     }
 
-    static PrivateKey toPrivateKeyInternal(File keyFile, String keyPassword) throws SSLException {
-        try {
-            return SslContext.toPrivateKey(keyFile, keyPassword);
-        } catch (Exception e) {
-            throw new SSLException(e);
-        }
-    }
-
-    static X509Certificate[] toX509CertificatesInternal(File file) throws SSLException {
-        try {
-            return SslContext.toX509Certificates(file);
-        } catch (CertificateException e) {
-            throw new SSLException(e);
-        }
-    }
-
     static void checkKeyManagerFactory(KeyManagerFactory keyManagerFactory) {
         if (keyManagerFactory != null) {
             throw new IllegalArgumentException(
diff --git a/handler/src/main/java/io/netty/handler/ssl/SslContext.java b/handler/src/main/java/io/netty/handler/ssl/SslContext.java
index 16b95d1eded..b6c092610a1 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslContext.java
@@ -1018,4 +1018,20 @@ static TrustManagerFactory buildTrustManagerFactory(
 
         return trustManagerFactory;
     }
+
+    static PrivateKey toPrivateKeyInternal(File keyFile, String keyPassword) throws SSLException {
+        try {
+            return toPrivateKey(keyFile, keyPassword);
+        } catch (Exception e) {
+            throw new SSLException(e);
+        }
+    }
+
+    static X509Certificate[] toX509CertificatesInternal(File file) throws SSLException {
+        try {
+            return toX509Certificates(file);
+        } catch (CertificateException e) {
+            throw new SSLException(e);
+        }
+    }
 }


Test patch: