Repository: netty/netty
Issue: SSLContextBuilder builds client context for server and vice-versa
Labels: defect

Introduced with: https://github.com/netty/netty/issues/3531

SslContextBuilder constructor has a [boolean parameter `forServer`](https://github.com/netty/netty/blob/4d56028df5a7be31561442f45bc2141eb84aa5ad/handler/src/main/java/io/netty/handler/ssl/SslContextBuilder.java#L75), which is set to `false` in the [`forServer(...)` static factory method](https://github.com/netty/netty/blob/4d56028df5a7be31561442f45bc2141eb84aa5ad/handler/src/main/java/io/netty/handler/ssl/SslContextBuilder.java#L57) and set to `true` in the `forClient(...)` equivalent. Leading the `build()` method to build a client context for a server and a server context for a client. It turns out that it doesn't seem to have caused too much damage nor there are tests failing for that. So, while I could have sent a quick PR to fix that, I think it deserves a bit more testing to make sure that this sort of thing is caught/analysis to see if we really need those two types of contexts.


Issue comments:
@leogomes - Can you be more specific?  I looks like you are implying the context that is generated by `build()` is reversed.  However it looks to be correct https://github.com/netty/netty/blob/4d56028df5a7be31561442f45bc2141eb84aa5ad/handler/src/main/java/io/netty/handler/ssl/SslContextBuilder.java#L206.

Doh!

The value [is propagated](https://github.com/netty/netty/blob/216d5ee5836f9c879de3831ab034c50b6eb9ea17/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java#L193) to the JDK's [SSLEngine](http://docs.oracle.com/javase/7/docs/api/javax/net/ssl/SSLEngine.html#setUseClientMode%28boolean%29).

@Scottmitch, the [wrong value is passed to the contructor](https://github.com/netty/netty/blob/4d56028df5a7be31561442f45bc2141eb84aa5ad/handler/src/main/java/io/netty/handler/ssl/SslContextBuilder.java#L34).

@ejona86 - Heh...we flipped the variable name from `forClient` but missed these cases...oops...care to submit a PR?

We can address unit tests after we have it patched.

@Scottmitch, sure, I can do the PR.

Fixed with https://github.com/netty/netty/issues/3674.

Thanks for reporting @leogomes!

@Scottmitch Wouldn't it be good to keep this one open or create another one to make sure we add unit tests to avoid regression in this area?

@leogomes - Thanks for the reminder... https://github.com/netty/netty/issues/3686.


Pull request review comments:

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/SslContextBuilder.java b/handler/src/main/java/io/netty/handler/ssl/SslContextBuilder.java
index e935e7c8b32..fcc840e6ca8 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslContextBuilder.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslContextBuilder.java
@@ -31,7 +31,7 @@ public final class SslContextBuilder {
      * Creates a builder for new client-side {@link SslContext}.
      */
     public static SslContextBuilder forClient() {
-        return new SslContextBuilder(true);
+        return new SslContextBuilder(false);
     }
 
     /**
@@ -41,7 +41,7 @@ public static SslContextBuilder forClient() {
      * @param keyFile a PKCS#8 private key file in PEM format
      */
     public static SslContextBuilder forServer(File keyCertChainFile, File keyFile) {
-        return new SslContextBuilder(false).keyManager(keyCertChainFile, keyFile);
+        return new SslContextBuilder(true).keyManager(keyCertChainFile, keyFile);
     }
 
     /**
@@ -54,7 +54,7 @@ public static SslContextBuilder forServer(File keyCertChainFile, File keyFile) {
      */
     public static SslContextBuilder forServer(
             File keyCertChainFile, File keyFile, String keyPassword) {
-        return new SslContextBuilder(false).keyManager(keyCertChainFile, keyFile, keyPassword);
+        return new SslContextBuilder(true).keyManager(keyCertChainFile, keyFile, keyPassword);
     }
 
     private final boolean forServer;


Test patch: