Repository: netty/netty
Issue: OpenSslServerContext reinitializes the provided TrustManagerFactory with the key cert chain.
Labels: defect

AFAICT, this is incorrect behavior, as the provided trust manager factory should already be initialized.


Issue comments:
@freels hmmm... s you would argue that either the user provides a full initialised TrustManagerFactory and not certs or none and the certs ?

Yes. This would match the behavior of JdkSslServerContext as well.

In my case, I am trying to provide a trust manager initialized with a root CA cert that a server context will use to validate client certs. However, because OpenSslServerContext reinitializes the provided trust manager w/ the server's cert, it drops the root cert and trusts only its own server cert.

Also, if neither a trust manager or trusted cert are provided, I'm pretty sure it should be able to use a default trust manager (w/o the call to init) in order to use the system provided trust store. (I'm looking at https://github.com/netty/netty/blob/4.0/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java#L370-L380 )

Alright... Looking or if you like you could also do a pr ;) ?

> Am 14.10.2015 um 17:07 schrieb Matt Freels notifications@github.com:
> 
> Also, if neither a trust manager or trusted cert are provided, I'm pretty sure it should be able to use a default trust manager (w/o the call to init) in order to use the system provided trust store. (I'm looking at https://github.com/netty/netty/blob/4.0/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java#L370-L380 )
> 
> —
> Reply to this email directly or view it on GitHub.

I'd be happy to provide a PR sometime in the next few days... just wanted to make sure this was something to fix before spending more time on it. :)

If it is how it is done in Jdk one yes :)

> Am 14.10.2015 um 17:13 schrieb Matt Freels notifications@github.com:
> 
> I'd be happy to provide a PR sometime in the next few days... just wanted to make sure this was something to fix before spending more time on it. :)
> 
> —
> Reply to this email directly or view it on GitHub.

@freels nevermind... working on a fix, stay tuned :)

Awesome, thanks!

@freels please check https://github.com/netty/netty/pull/4387


Pull request review comments:

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java
index 99fead914bb..e3bf3ac7e4f 100644
--- a/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java
@@ -367,16 +367,13 @@ public OpenSslServerContext(
                     throw new SSLException("failed to set certificate: " + keyCertChainFile + " and " + keyFile, e);
                 }
                 try {
-                    if (trustManagerFactory == null) {
+                    if (trustCertChainFile != null) {
+                        trustManagerFactory = buildTrustManagerFactory(trustCertChainFile, trustManagerFactory);
+                    } else if (trustManagerFactory == null) {
                         // Mimic the way SSLContext.getInstance(KeyManager[], null, null) works
                         trustManagerFactory = TrustManagerFactory.getInstance(
                                 TrustManagerFactory.getDefaultAlgorithm());
-                    }
-                    if (trustCertChainFile != null) {
-                        trustManagerFactory = buildTrustManagerFactory(trustCertChainFile, trustManagerFactory);
-                    } else {
-                        KeyStore ks = buildKeyStore(keyCertChainFile, keyFile, keyPassword);
-                        trustManagerFactory.init(ks);
+                        trustManagerFactory.init((KeyStore) null);
                     }
 
                     final X509TrustManager manager = chooseTrustManager(trustManagerFactory.getTrustManagers());
@@ -484,16 +481,13 @@ void verify(OpenSslEngine engine, X509Certificate[] peerCerts, String auth)
                     }
                 }
                 try {
-                    if (trustManagerFactory == null) {
+                    if (trustCertChain != null) {
+                        trustManagerFactory = buildTrustManagerFactory(trustCertChain, trustManagerFactory);
+                    } else if (trustManagerFactory == null) {
                         // Mimic the way SSLContext.getInstance(KeyManager[], null, null) works
                         trustManagerFactory = TrustManagerFactory.getInstance(
                                 TrustManagerFactory.getDefaultAlgorithm());
-                    }
-                    if (trustCertChain != null) {
-                        trustManagerFactory = buildTrustManagerFactory(trustCertChain, trustManagerFactory);
-                    } else {
-                        KeyStore ks = buildKeyStore(keyCertChain, key, keyPassword.toCharArray());
-                        trustManagerFactory.init(ks);
+                        trustManagerFactory.init((KeyStore) null);
                     }
 
                     final X509TrustManager manager = chooseTrustManager(trustManagerFactory.getTrustManagers());


Test patch: