Custom cipher list with BoringSSL breaks in 4.1.31.Final

[njhill]

Expected behavior

TLS continues to work in 4.1.31.Final when providing an explicit list of ciphers via SslContextBuilder.ciphers(...).

Actual behavior

After upgrading, my SslContextBuilder.build() throws:

Caused by: java.lang.IllegalArgumentException: unsupported cipher suite: TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384(ECDHE-ECDSA-AES256-SHA384)
       at io.netty.handler.ssl.CipherSuiteConverter.convertToCipherStrings(CipherSuiteConverter.java:418)
       at io.netty.handler.ssl.ReferenceCountedOpenSslContext.<init>(ReferenceCountedOpenSslContext.java:252)
       ... 34 more

It looks like this is due to changes in #8293, in particular this check in CipherSuiteConverter.convertToCipherStrings() which throws if false. Should it just continue instead of throwing so that the current cipherSuite is excluded from the lists being built (since a later cipher in the list may be available)? I verified that it works as before with this modification.

Minimal yet complete reproducer code (or URL to code)

KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
kmf.init(null,null);
String[] ciphers = ((SSLSocketFactory)SSLSocketFactory.getDefault()).getDefaultCipherSuites();
SslContextBuilder.forServer(kmf).sslProvider(SslProvider.OPENSSL) 
    .ciphers(Arrays.asList(ciphers)).build();

Netty version

4.1.31.Final

JVM version (e.g. java -version)

java version "1.8.0_181"
Java(TM) SE Runtime Environment (build 1.8.0_181-b13)
Java HotSpot(TM) 64-Bit Server VM (build 25.181-b13, mixed mode)

OS version (e.g. uname -a)

Linux 3.10.0-862.14.4.el7.x86_64

[normanmaurer]

And this did not throw before ? I wonder why as I think the check was always there or I am missing something ?

…

Am 30.10.2018 um 07:27 schrieb Nick Hill ***@***.***>: Expected behavior TLS continues to work in 4.1.31.Final when providing an explicit list of ciphers via SslContextBuilder.ciphers(...). Actual behavior After upgrading, my SslContextBuilder.build() throws: Caused by: java.lang.IllegalArgumentException: unsupported cipher suite: TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384(ECDHE-ECDSA-AES256-SHA384) at io.netty.handler.ssl.CipherSuiteConverter.convertToCipherStrings(CipherSuiteConverter.java:418) at io.netty.handler.ssl.ReferenceCountedOpenSslContext.<init>(ReferenceCountedOpenSslContext.java:252) ... 34 more It looks like this is due to changes in #8293, in particular this check in CipherSuiteConverter.convertToCipherStrings() which throws if false. Should it just continue instead of throwing so that the current cipherSuite is excluded from the lists being built (since a later cipher in the list may be available)? I verified that it works as before with this modification. Minimal yet complete reproducer code (or URL to code) KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm()); kmf.init(null,null); String[] ciphers = ((SSLSocketFactory)SSLSocketFactory.getDefault()).getDefaultCipherSuites(); SslContextBuilder.forServer(kmf).sslProvider(SslProvider.OPENSSL) .ciphers(Arrays.asList(ciphers)).build(); Netty version 4.1.31.Final JVM version (e.g. java -version) java version "1.8.0_181" Java(TM) SE Runtime Environment (build 1.8.0_181-b13) Java HotSpot(TM) 64-Bit Server VM (build 25.181-b13, mixed mode) OS version (e.g. uname -a) Linux 3.10.0-862.14.4.el7.x86_64 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[normanmaurer]

Seems like before we only did the check in ReferenceCountedSslEngine. I just wonder what is more correct as it could also be surprising to configure something and then have Netty partial ignore it

[normanmaurer]

@njhill actually yeah let’s just do what you proposed as this should give the least surprises to people. Will change and do a release after it

[normanmaurer]

@njhill actually no... I think it works as expected now as this is consistent across SSLEngine implementations:

    @Test
    public void test() throws Exception {
        KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
        kmf.init(null,null);
        String[] ciphers = ((SSLSocketFactory)SSLSocketFactory.getDefault()).getDefaultCipherSuites();
        List<String> cipherList = new ArrayList<String>();
        Collections.addAll(cipherList, ciphers);
        cipherList.add("CustomCipher");
        SSLEngine client = null;
        try {
            clientSslCtx = SslContextBuilder.forServer(kmf).sslProvider(sslClientProvider())
                                            .ciphers(cipherList).build();
            client = clientSslCtx.newEngine(UnpooledByteBufAllocator.DEFAULT);
            fail();
        } catch (IllegalArgumentException expected) {
            
        } catch (SSLException expected) {

        } finally {
            cleanupClientSslEngine(client);
        }
    }

This now pass with all SSLEngine implementations we test (JDK, netty-tcnative, netty-tcnative-boringssl-static, conscrypt). WDYT ?

[normanmaurer]

@njhill #8443

[njhill]

Thanks @normanmaurer and sorry for leaving you hanging, I was debugging late and open this right before I fell asleep :)

To answer your questions - yes it worked before with openssl (boringssl) and still works with the JDK provider (since the ciphers are valid in that case).

Now I understand more what's going on I agree the new behaviour seems reasonable, however my code actually does this:

String[] ciphers = ((SSLSocketFactory)SSLSocketFactory.getDefault()).getDefaultCipherSuites();
SslContextBuilder.forServer(kmf).sslProvider(OpenSsl.supportsKeyManagerFactory()
        ? SslProvider.OPENSSL : SslProvider.JDK).ciphers(cipherList).build();

So the difference now is that the cipher suite list needs to be tailored depending on the provider type provided, rather than having the builder choose the valid subset in each case. This sounds fine to me, since I guess like you said it will catch unsupported ones earlier in the case they are included unintentionally.

[normanmaurer]

As discussed works as expected