The default SSL engine enabled protocols should not prevent to use protocols declared by options

[johnoliver]

SSLHelper has:

  private static final String[] DEFAULT_ENABLED_PROTOCOLS = {"SSLv2Hello", "TLSv1", "TLSv1.1", "TLSv1.2"};

However later builds up the enabled protocol list with:

protocols.retainAll(Arrays.asList(engine.getEnabledProtocols()));

And since netty by default does not enable SSLv2Hello (ref ), SSLv2Hello in fact is not enabled, and I believe it is not possible to enable any protocols other than TLSv1/1.1/1.2.

This could be regarded as a feature as it prevents developers selecting insecure protocols, however is confusing that addEnabledSecureTransportProtocol will only actually work if its TLSv1/1.1/1.2.

Could be fixed by removing SSLv2Hello from the default list to make it less confusing, and maybe documenting what protocols are available. Or by re-enabling all protocols by changing

protocols.retainAll(Arrays.asList(engine.getEnabledProtocols()));

to

protocols.retainAll(Arrays.asList(engine.getSupportedProtocols()));

[johnoliver]

Having looked at the OpenSslEngine it seems that SSLv2Hello cant be disabled:

https://github.com/netty/netty/blob/4.1/handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java#L1353

So may have to go with the latter fix so the code actually does do what it should, as it may be just as confusing that it is not listed but OpenSsl adds it.

[vietj]

is it possible to provide a test for asserting this behavior ?

[vietj]

ping @johnoliver

[PhilLehmann]

Right now, SSLHelper is

1. taking the DEFAULT_ENABLED_PROTOCOLS as a basis protocol list
2. removing those protocols from that protocol list which are not already enabled in the SSLEngine by default (effectively leaving only those protocols in place which were enabled in the SSLEngine already)
3. if the developer added protocols to the options, the current protocol list is filtered to only leave those

This means that there is currently no way to add protocols not already enabled in both SSLHelper and SSLEngine by default - for my Java 8 setup, that is SSLv3 (disabled by both) and SSLv2Hello (enabled by SSLHelper, but disabled by SSLEngine).

As I need SSLv3 and SSLv2Hello support for a customer's legacy devices, I'd like to bring this forward. Let's not discuss the security implications here ☹️

I propose to move the DEFAULT_ENABLED_PROTOCOLS field from SSLHelper to TCPSSLOptions, where all other DEFAULT_* are. There, this field will be documented, so that developers know what the defaults are. A addEnabledSecureTransportProtocol(String protocol) method is already in place, a removeEnabledSecureTransportProtocol(String protocol) should be added. While the names are quite long, they are quite descriptive and the add-method is backwards compatible.

SSLHelper can then simply get that list, remove those protocols not supported by SSLEngine and tell SSLEngine to use the remainder of them.

TCPSSLOptions.addEnabledSecureTransportProtocol and getEnabledSecureTransportProtocols are being invoked/overwritten in several places:

• ClientOptionsBase, HttpClientOptions, HttpServerOptions, NetClientOptions, NetServerOptions: removeEnabledSecureTransportProtocol needs to be added
• EventBusOptionsConverter and TCPSSLOptionsConverter: nothing to do
• NetExamples: make it clearer by comments and code what is enabled and what not
• TCPSSLOptions constructor: take over other values
• SSLHelper: as described above
• SSLHelperTest, HttpTLSTest, NetTest: analyze how they worked until now; add tests for additional protocols if possible

[PhilLehmann]

@vietj : would you accept a PR adding this feature?

[vietj]

if the SSLHelper removes the protocols not supported by the engine how does it help you to use SSLv3 ?

[vietj]

ah you change by supported protocols as advocated by @johnoliver

[vietj]

yes I'm all for that, Vert.x should not get in your way if you want to shot in your feet :-)

[vietj]

fixed

[vietj]

@johnoliver @philrykoff I will remove SSLv2Hello from the list of default enabled protocols. Actually this protocol was already filtered by the previous code (as the engine supports it but does not enable it by default) : so the behavior will be the same for users. Also some tests don't pass with SSLv2Hello like the SNI tests (I think it does not supports SNI but I can't find evidence of it).

[vietj]

I will add a test that checks we can still use this protocol by setting it in the protocols list

[PhilLehmann]

Makes sense.