Potential Regression in 8.1.3 (#4c16ca8)

[zugazagoitia]

Describe the bug

Previously supported cipher suites are no longer supported since 8.1.3. I have not had time to debug this yet, but I am opening this issue to track the problem, it did not exist in 8.1.2.

To Reproduce

Running the test suite in the actions runner fails with the following error:

KeystoreLoadingTests > Loading a valid P12 keystore from a path FAILED
    io.javalin.util.JavalinException: java.lang.IllegalArgumentException: cipherSuite TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 is not supported.
        at app//io.javalin.Javalin.start(Javalin.java:186)
        at app//io.javalin.community.ssl.IntegrationTestClass.assertWorks(IntegrationTestClass.java:141)
        at app//io.javalin.community.ssl.IntegrationTestClass.assertSslWorks(IntegrationTestClass.java:152)
        at app//io.javalin.community.ssl.KeystoreLoadingTests.loadValidP12FromPath(KeystoreLoadingTests.java:85)

        Caused by:
        java.lang.IllegalArgumentException: cipherSuite TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 is not supported.
            at org.conscrypt.NativeCrypto.checkEnabledCipherSuites(NativeCrypto.java:1117)
            at org.conscrypt.SSLParametersImpl.setEnabledCipherSuites(SSLParametersImpl.java:264)
            at org.conscrypt.ConscryptEngine.setEnabledCipherSuites(ConscryptEngine.java:677)
            at java.base/javax.net.ssl.SSLEngine.setSSLParameters(SSLEngine.java:1331)
            at org.conscrypt.ConscryptEngine.setSSLParameters(ConscryptEngine.java:532)
            at org.conscrypt.Java8EngineWrapper.setSSLParameters(Java8EngineWrapper.java:66)
            at nl.altindag.ssl.sslcontext.FenixSSLContextSpi.getSSLEngine(FenixSSLContextSpi.java:84)
            at nl.altindag.ssl.sslcontext.FenixSSLContextSpi.engineCreateSSLEngine(FenixSSLContextSpi.java:68)
            at java.base/javax.net.ssl.SSLContext.createSSLEngine(SSLContext.java:373)
            at org.eclipse.jetty.util.ssl.SslContextFactory.checkConfiguration(SslContextFactory.java:220)
            at org.eclipse.jetty.util.ssl.SslContextFactory.doStart(SslContextFactory.java:215)
            at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)
            at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:171)
            at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:121)
            at org.eclipse.jetty.server.SslConnectionFactory.doStart(SslConnectionFactory.java:112)
            at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)
            at org.eclipse.jetty.util.component.ContainerLifeCycle.start(ContainerLifeCycle.java:171)
            at org.eclipse.jetty.util.component.ContainerLifeCycle.doStart(ContainerLifeCycle.java:121)
            at org.eclipse.jetty.server.AbstractConnector.doStart(AbstractConnector.java:367)
            at org.eclipse.jetty.server.AbstractNetworkConnector.doStart(AbstractNetworkConnector.java:75)
            at org.eclipse.jetty.server.ServerConnector.doStart(ServerConnector.java:228)
            at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)
            at org.eclipse.jetty.server.Server.doStart(Server.java:428)
            at org.eclipse.jetty.util.component.AbstractLifeCycle.start(AbstractLifeCycle.java:93)
            at io.javalin.jetty.JettyServer.start(JettyServer.kt:82)
            at io.javalin.Javalin.start(Javalin.java:171)
            ... 3 more

Environmental Data:

• Java Version: [11,17,18]
• Maven Version: N/A, Using gradle
• OS: [Windows, Linux, Mac]

Additional context

Actions Runner Logs: https://github.com/javalin/javalin-ssl/actions/runs/5606563554?pr=90
Ubuntu JDK18 runner logs: logs_491.zip

[Hakky54]

Hi @zugazagoitia

Thank you for the detailed issue description. This issue is related to the changes from the following PR: #360

I have cloned javalin-ssl to analyse the issue on my machine and I can confirm the same issue. Although the failing test on your side is now occuring duo the latest version of this library it is not actually a bug or a regression. Let me try to explain it.

The test is failing because it is saying TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 and TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 are not supported. These two ciphers are specified in io.javalin.community.ssl.TLSConfig. See here: https://github.com/javalin/javalin-ssl/blob/0239ba9e010a50b6ee16100c2aaf39d99763c4d2/src/main/java/io/javalin/community/ssl/TLSConfig.java#L31

By default with the default security provider of Java these two Ciphers are supported, the default supported list contains the following ciphers:

TLS_AES_256_GCM_SHA384
TLS_AES_128_GCM_SHA256
TLS_CHACHA20_POLY1305_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256
TLS_DHE_DSS_WITH_AES_256_GCM_SHA384
TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
TLS_DHE_DSS_WITH_AES_128_GCM_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
TLS_DHE_RSA_WITH_AES_256_CBC_SHA256
TLS_DHE_DSS_WITH_AES_256_CBC_SHA256
TLS_DHE_RSA_WITH_AES_128_CBC_SHA256
TLS_DHE_DSS_WITH_AES_128_CBC_SHA256
TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384
TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384
TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256
TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA384
TLS_ECDH_RSA_WITH_AES_256_CBC_SHA384
TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA256
TLS_ECDH_RSA_WITH_AES_128_CBC_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
TLS_DHE_RSA_WITH_AES_256_CBC_SHA
TLS_DHE_DSS_WITH_AES_256_CBC_SHA
TLS_DHE_RSA_WITH_AES_128_CBC_SHA
TLS_DHE_DSS_WITH_AES_128_CBC_SHA
TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA
TLS_ECDH_RSA_WITH_AES_256_CBC_SHA
TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA
TLS_ECDH_RSA_WITH_AES_128_CBC_SHA
TLS_RSA_WITH_AES_256_GCM_SHA384
TLS_RSA_WITH_AES_128_GCM_SHA256
TLS_RSA_WITH_AES_256_CBC_SHA256
TLS_RSA_WITH_AES_128_CBC_SHA256
TLS_RSA_WITH_AES_256_CBC_SHA
TLS_RSA_WITH_AES_128_CBC_SHA
TLS_EMPTY_RENEGOTIATION_INFO_SCSV

The failing ones are actually supported, however the custom security provider which is configured in io.javalin.community.ssl.util.SSLUtils is limiting down the scope of supported ciphers. The security provider by javalin-ssl is configured here: https://github.com/javalin/javalin-ssl/blob/0239ba9e010a50b6ee16100c2aaf39d99763c4d2/src/main/java/io/javalin/community/ssl/util/SSLUtils.java#L65

TLS_AES_128_GCM_SHA256
TLS_AES_256_GCM_SHA384
TLS_CHACHA20_POLY1305_SHA256
TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_PSK_WITH_CHACHA20_POLY1305_SHA256
TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA
TLS_ECDHE_PSK_WITH_AES_128_CBC_SHA
TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA
TLS_ECDHE_PSK_WITH_AES_256_CBC_SHA
TLS_RSA_WITH_AES_128_GCM_SHA256
TLS_RSA_WITH_AES_256_GCM_SHA384
TLS_RSA_WITH_AES_128_CBC_SHA
TLS_PSK_WITH_AES_128_CBC_SHA
TLS_RSA_WITH_AES_256_CBC_SHA
TLS_PSK_WITH_AES_256_CBC_SHA
SSL_RSA_WITH_3DES_EDE_CBC_SHA
TLS_EMPTY_RENEGOTIATION_INFO_SCSV
TLS_FALLBACK_SCSV

The second list of ciphers does not contain TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 which explains why it is failing, because it is not supported by the underlying security provider. So my suggestion is to eithr remove the custom security provider or remove TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 and TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 from the TLSConfig. Either of these two options will resolve your failing tests.

Probably you are curious why it was not failing before. The main reason is that the configured ciphers within the SSLFactory was not being used within the SSLContext. It was provided as a separate object as SSLParameters and it was provided within the SSLEngine, SSLSocketFactory and SSLServerSockerFactory however these 3 objects are being constructed within the SSLFactory. The latest code changes delegated the creation of these 3 objects to the SSLContext which will now use the custom ciphers suite. So your TLSConfig with a list of ciphers and protocols was being ignored when creating a server with TLS because it was creating the Server with the default SSLContext which was not able to use the custom ciphers in the earlier releases.

So what can I do on my side? I can filter out the not supported ciphers by explicitly validating which are not supported and log it. This will prevent you and the end-users of having a runtime exception and if they want to analyse the ssl configuration they can still see the not supported and filtered out ciphers in the logs.

What do you think? How shall we proceed and does it make sense to you?

[zugazagoitia]

The second list of ciphers does not contain TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 which explains why it is failing, because it is not supported by the underlying security provider. So my suggestion is to eithr remove the custom security provider or remove TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 and TLS_DHE_RSA_WITH_AES_128_GCM_SHA256 from the TLSConfig. Either of these two options will resolve your failing tests.

I don't want to change my TLSConfig because it's just a copy of Mozilla's ciphers recommendations.

I was under the impression Conscrypt was internally used by your library, it this is not the case I don't see why should I have a custom security provider instead of relying on the default one. I remember having some issue, but not what it was.

I will have a look at it tomorrow and get back to you.

Probably you are curious why it was not failing before. The main reason is that the configured ciphers within the SSLFactory was not being used within the SSLContext. It was provided as a separate object as SSLParameters and it was provided within the SSLEngine, SSLSocketFactory and SSLServerSockerFactory however these 3 objects are being constructed within the SSLFactory. The latest code changes delegated the creation of these 3 objects to the SSLContext which will now use the custom ciphers suite. So your TLSConfig with a list of ciphers and protocols was being ignored when creating a server with TLS because it was creating the Server with the default SSLContext which was not able to use the custom ciphers in the earlier releases.

So what can I do on my side? I can filter out the not supported ciphers by explicitly validating which are not supported and log it. This will prevent you and the end-users of having a runtime exception and if they want to analyse the ssl configuration they can still see the not supported and filtered out ciphers in the logs.

What do you think? How shall we proceed and does it make sense to you?

Thank you very much for your detailed and quick response.

What I understand is that this was a bug (or rather an unwanted/strange) behavior where ciphers were being ignored.

My intended use would be to provide a list of allowed ciphers rather than a strict set that must be present. It is fine if you don't want to change the API, I don't really mind how you implement it. It might be an option adding some kind of verb to specify when they're strict or not, something like requireCiphers,allowCiphers.

[Hakky54]

What I will do atleast is to ignore the not supported ciphers if a custom security provider is configured which does not have matching ciphers. That will prevent a runtime exception and atleast attempt a ssl handshake with the remaining possible ciphers.

I will attempt to release it this weak. I will keep you updated.

It might be an option adding some kind of verb to specify when they're strict or not, something like
requireCiphers,allowCiphers.

Actually the current setup works like that already. If you specify a cipher it will only use those option you have specified.

My intended use would be to provide a list of allowed ciphers rather than a strict set that must be present. It is fine if you don't want to change the API, I don't really mind how you implement it.

No worries, I will make sure it will be smooth as before and more solid then ever.

[Hakky54]

I made a new release yesterday and the issue should be resolved. The only suggestion I would provide is if you want to keep using the same ciphers then you should drop using that custom security provider. Can you give a try with the latest version?

[zugazagoitia]

I made a new release yesterday and the issue should be resolved. The only suggestion I would provide is if you want to keep using the same ciphers then you should drop using that custom security provider. Can you give a try with the latest version?

I will try and get back to you, thank you for the quick release!
It is now working fine, thank you!

Still, it is my understanding that different JVM implementations offer different security providers, with no standard set of supported ciphers and protocols that could be known in advance. Sometimes JVMs don't even follow the RFC names!

Why would a runtime exception be raised when a cipher is not available? The reason for having a set of ciphers instead of one is for compatibility and flexibility with the server and client, it is fine if some of them aren't implemented in the server. It would be the opposite case, only when using a custom security provider, that one could actually check in advance what ciphers are available to use.

I understand providing an API to have a strict check, but that can easily be done by the user by just checking the provider.

Am I missing something here that I don't understand?

[Hakky54]

It is now working fine, thank you!

Great to hear, thank you for confirming!

Still, it is my understanding that different JVM implementations offer different security providers, with no standard set of supported ciphers and protocols that could be known in advance

I think Oracle just provides specifications (JEPs) and a reference implementation (the OpenJDK) other vendors can provide their implementation for those specifications and in that way they are free to choose what part to implement. I would assume some have more or maybe less supported ciphers. So I am not sure what is best for the end-user... I would say having proper integration test would be really helpfull to just validate whether the server of client is actually using the requested cipher or else use a different security provider or another JDK from a different vendor.

Why would a runtime exception be raised when a cipher is not available? The reason for having a set of ciphers instead of one is for compatibility and flexibility with the server and client, it is fine if some of them aren't implemented in the server.

Yes it feels a bit redundant. It would be better to ignore the unsupported ones and just stick with the ones which are supported and attempt an ssl handshake. I am also seeing this runtime exception for the first time.

[zugazagoitia]

I understand it's the end user responsibility to validate their own JVM, as library developers we can't possibly make assumptions about what will be supported by the implementation and whatnot, I just wanted to provide a simple configuration following Mozilla's recommendations.

Thank you very much for your detailed and quick responses, I am glad this is solved now!

[Hakky54]

Yes it is indeed the responsibility of the end user as we as a library provider don't have the controle of this particular topic. The only thin we might be able to do is inform the user with either debug or warn log messages in my opinion.

I think you did well to have a default configuration based on Mozilla's recommendation which will be used if available or it will use now a subset of whatever is available which is still fine as it is using your configuration.