Repository: netty/netty
Issue: JdkSslContext produces invalid supported ciphers
Labels: defect

### Expected behavior

JdkSslContext should produce valid ciphers that are actually supported by the platform.
Typically, it should be possible to use them in a custom `CipherSuiteFilter` to configure a `SSLEngine`.

### Actual behavior

Netty 4.1.13.Final introduced a workaround for IBM certs naming scheme.
This workaround is based on IBM's statement:

> According to IBM's documentation [2] the "SSL_" prefix is "interchangeable" with the "TLS_" prefix.

This statement is wrong and transformed names cause `SSLEngine#setEnabledCipherSuites` to crash.

On my machine, the following ciphers get transformed with the `SSL` prefix being replaced with `TLS` and cause SSLEngine to crash:

* SSL_DH_anon_WITH_DES_CBC_SHA
* SSL_DHE_DSS_WITH_3DES_EDE_CBC_SHA
* SSL_DH_anon_WITH_3DES_EDE_CBC_SHA
* SSL_DHE_RSA_WITH_DES_CBC_SHA
* SSL_RSA_WITH_DES_CBC_SHA
* SSL_DHE_DSS_WITH_DES_CBC_SHA
* SSL_RSA_WITH_NULL_SHA
* SSL_RSA_WITH_3DES_EDE_CBC_SHA
* SSL_RSA_WITH_NULL_MD5
* SSL_DHE_RSA_WITH_3DES_EDE_CBC_SHA

### Steps to reproduce

See reproducer below.

### Minimal yet complete reproducer code (or URL to code)

```java
SSLContext sslcontext = SSLContext.getInstance("TLS");
sslcontext.init(null, null, null);
SSLEngine sslEngine = sslcontext.createSSLEngine();

// works
sslEngine.setEnabledCipherSuites(new String[] {"SSL_DH_anon_WITH_DES_CBC_SHA"});

// crashes with IllegalArgumentException: Unsupported ciphersuite TLS_DH_anon_WITH_DES_CBC_SHA
sslEngine.setEnabledCipherSuites(new String[] {"TLS_DH_anon_WITH_DES_CBC_SHA"});
```

### Netty version

Tested on 4.1.20.Final

### JVM version (e.g. `java -version`)

Java HotSpot(TM) 64-Bit Server VM (build 25.152-b16, mixed mode)

### OS version (e.g. `uname -a`)

Not relevant

Issue comments:
@slandelle are you sure this was introduced in 4.1.20 ?
@normanmaurer This renaming was introduced in 449befa0034e2cf77b3033f62d4f2153cef5a1d7 which is tagged as 4.1.20. 
My bad, I didn't get what those tags meant in the GH interface. I guess it's from 4.1.13.
I guess this went unnoticed as `SslUtils#DEFAULT_CIPHER_SUITES` only contains names prefixed with TLS.
cc @Scottmitch @rkapsi
http://docs.oracle.com/javase/8/docs/technotes/guides/security/StandardNames.html#ciphersuites
> The names mentioned in the TLS RFCs prefixed with TLS_ are functionally equivalent to the JSSE cipher suites prefixed with SSL_.

Arg. I guess the names aren't quite interchangeable. See https://github.com/netty/netty/pull/7677

Pull request review comments:
Shouldn't you fall back to adding the original name here?
the original name has already been added above on line 97, and is known to be supported because it's returned by `engine.getSupportedCipherSuites()`.
Oh right

Patch that resolved the issue:
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 e027036146a..41fd9c6f6a8 100644
--- a/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/JdkSslContext.java
@@ -105,7 +105,13 @@ public class JdkSslContext extends SslContext {
             //[3] https://www.ibm.com/developerworks/community/forums/html/topic?id=9b5a56a9-fa46-4031-b33b-df91e28d77c2
             //[4] https://www.ibm.com/developerworks/rfe/execute?use_case=viewRfe&CR_ID=71770
             if (supportedCipher.startsWith("SSL_")) {
-                SUPPORTED_CIPHERS.add("TLS_" + supportedCipher.substring("SSL_".length()));
+                final String tlsPrefixedCipherName = "TLS_" + supportedCipher.substring("SSL_".length());
+                try {
+                    engine.setEnabledCipherSuites(new String[]{tlsPrefixedCipherName});
+                    SUPPORTED_CIPHERS.add(tlsPrefixedCipherName);
+                } catch (IllegalArgumentException ignored) {
+                    // The cipher is not supported ... move on to the next cipher.
+                }
             }
         }
         List<String> ciphers = new ArrayList<String>();


Test patch:
diff --git a/handler/src/test/java/io/netty/handler/ssl/SslContextTest.java b/handler/src/test/java/io/netty/handler/ssl/SslContextTest.java
index 76526a5b259..1c0032885ac 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SslContextTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SslContextTest.java
@@ -18,12 +18,20 @@
 import org.junit.Assert;
 import org.junit.Test;
 
-import javax.net.ssl.SSLException;
 import java.io.File;
 import java.io.IOException;
+import java.security.KeyManagementException;
+import java.security.NoSuchAlgorithmException;
 import java.security.PrivateKey;
 import java.security.spec.InvalidKeySpecException;
 
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLException;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assume.assumeNotNull;
+
 public abstract class SslContextTest {
 
     @Test(expected = IOException.class)
@@ -85,5 +93,26 @@ public void testSslServerWithUnencryptedPrivateKeyEmptyPass() throws SSLExceptio
         newServerContext(crtFile, keyFile, "");
     }
 
+    @Test
+    public void testSupportedCiphers() throws KeyManagementException, NoSuchAlgorithmException, SSLException {
+        SSLContext jdkSslContext = SSLContext.getInstance("TLS");
+        jdkSslContext.init(null, null, null);
+        SSLEngine sslEngine = jdkSslContext.createSSLEngine();
+
+        String unsupportedCipher = "TLS_DH_anon_WITH_DES_CBC_SHA";
+        IllegalArgumentException exception = null;
+        try {
+            sslEngine.setEnabledCipherSuites(new String[] {unsupportedCipher});
+        } catch (IllegalArgumentException e) {
+            exception = e;
+        }
+        assumeNotNull(exception);
+        File keyFile = new File(getClass().getResource("test_unencrypted.pem").getFile());
+        File crtFile = new File(getClass().getResource("test.crt").getFile());
+
+        SslContext sslContext = newServerContext(crtFile, keyFile, null);
+        assertFalse(sslContext.cipherSuites().contains(unsupportedCipher));
+    }
+
     protected abstract SslContext newServerContext(File crtFile, File keyFile, String pass) throws SSLException;
 }