Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dropwizard reporting incorrectly enabled HTTPS protocols #3532

Closed
shahr opened this issue Oct 28, 2020 · 5 comments · Fixed by #3533
Closed

Dropwizard reporting incorrectly enabled HTTPS protocols #3532

shahr opened this issue Oct 28, 2020 · 5 comments · Fixed by #3533

Comments

@shahr
Copy link

shahr commented Oct 28, 2020

The deafult Dropwizard behaviour seems to enable TLSv1.1 (despite the documentation stating that TLSv1.1 is in the excludedProtocols by default). It does also not match what is bieng logged by Dropwizard.

Steps to reproduce (using Dropwizard v.2.0.13):

  1. Create a simple Dropwizard application. Add HTTPs configuration (do not set any values for supportedProtocols and excludedProtocols)
  2. Start up server. Server logs indicate:
    io.dropwizard.jersey.HttpsConnectorFactory: Enabled protocols: [TLSv1.2, TLSv1.3] io.dropwizard.jersey.HttpsConnectorFactory: Disabled protocols: [SSLv2Hello, SSLv3, TLSv1, TLSv1.1]
  3. curl --tlsv1.1 https://localhost - this actually works (using curl version: libcurl/7.29.0, NSS/3.44)

Further proof:

  • Add a ServerLifecycleListener with the serverStarted method as: server -> LOGGER.info(Arrays.asList(server.getBean(SslContextFactory.class).getSelectedProtocols()))
  • This will print TLSv1.1, TLSv1.2 and TLSv1.3

Notes:

  • If I explicitly set excludedProtocols with TLSv1.1, it gets disabled.
  • TLS 1.1 may still fail due to the default cipher suites prevent an algorithm agreement - but that is besides the point - the protocol is still enabled.
@joschi joschi added the bug label Oct 28, 2020
@joschi joschi added this to the 2.0.15 milestone Oct 28, 2020
@joschi joschi assigned joschi and unassigned joschi Oct 28, 2020
@joschi
Copy link
Member

joschi commented Oct 28, 2020

@shahr Thank you very much for reporting this!

That's an interesting find. We do indeed add TLSv1.1 to the list of excluded protocols:
https://github.com/dropwizard/dropwizard/blob/v2.0.14/dropwizard-jetty/src/main/java/io/dropwizard/jetty/HttpsConnectorFactory.java#L289-L290

This list of excluded protocols is then set on the SslContextFactory when it's being created:
https://github.com/dropwizard/dropwizard/blob/v2.0.14/dropwizard-jetty/src/main/java/io/dropwizard/jetty/HttpsConnectorFactory.java#L811-L813

A test like the following also dumps the correct selection of included and excluded protocols:

@Test
public void testDefaultExcludedProtocols() throws Exception {
  HttpsConnectorFactory factory = new HttpsConnectorFactory();
  factory.setKeyStorePassword("password"); // necessary to avoid a prompt for a password

  SslContextFactory sslContextFactory = factory.configureSslContextFactory(new SslContextFactory.Server());
  sslContextFactory.dump(System.out, "");
}

Output:

Server@13526e59[provider=null,keyStore=null,trustStore=null] - STOPPED
+> trustAll=false
+> Protocol Selections
|  +> Enabled size=2
|  |  +> TLSv1.2
|  |  +> TLSv1.3
|  +> Disabled size=4
|     +> SSLv2Hello - ConfigExcluded:'SSL.*'
|     +> SSLv3 - ConfigExcluded:'SSL.*' JVM:disabled
|     +> TLSv1 - ConfigExcluded:'TLSv1'
|     +> TLSv1.1 - ConfigExcluded:'TLSv1\.1'

And yet, any SslEngine created by this SslContextFactory will show TLSv1.1 in SslEngine#getEnabledProtocols(). 🤔

@shahr
Copy link
Author

shahr commented Oct 28, 2020

Yes, thank you for verifying - I was surprised by this because a cursory look at the code seemed like everything was configured correctly. For context, I was creating a verification that ensured servers were only using TLSv1.2 and when testing this I oddly discovered that the 1.1 protocol was still enabled.

I'm a bit time bound at the moment, but will try take a look into this a bit deeper to try understand where TLS 1.1 is bieng re-enabled.

@shahr
Copy link
Author

shahr commented Oct 28, 2020

As a side note - is it worth changing the logging to use SslEngine#getEnabledProtocols() (including cipher suites) to represent the real SSLEngine configuration?

@joschi
Copy link
Member

joschi commented Oct 28, 2020

This seems to be rooted in an inconsistency between SslContextFactory#dump() and which protocols are really selected in the built SSLContext and SSLEngine instances.

I've filed a bug report with the Jetty project for this:
jetty/jetty.project#5531

@shahr
Copy link
Author

shahr commented Oct 28, 2020

This explains why it seemed to be behaving correctly when I explicitly set the excludedProtocol list without using the regexp.

Thank you.

joschi added a commit that referenced this issue Oct 28, 2020
The default list of excluded protocols used in `HttpsConnectorFactory` wasn't working as expected.

Jetty currently doesn't support using regular expressions for supporte or excluded protocols.
This only works for supported and excluded cipher suites as of Jetty 9.4.33.v20201020.

The default list of excluded protocols now only contains valid and complete entries:

SSLv3, TLSv1, and TLSv1.1

Refs jetty/jetty.project#5531
Fixes #3532
joschi added a commit that referenced this issue Oct 29, 2020
…3533)

The default list of excluded protocols used in `HttpsConnectorFactory` wasn't working as expected.

Jetty currently doesn't support using regular expressions for supported or excluded protocols.
This only works for supported and excluded cipher suites as of Jetty 9.4.33.v20201020.

The default list of excluded protocols now only contains valid and complete entries:

SSLv3, TLSv1, and TLSv1.1

Refs jetty/jetty.project#5531
Fixes #3532
joschi added a commit that referenced this issue Oct 29, 2020
…3533)

The default list of excluded protocols used in `HttpsConnectorFactory` wasn't working as expected.

Jetty currently doesn't support using regular expressions for supported or excluded protocols.
This only works for supported and excluded cipher suites as of Jetty 9.4.33.v20201020.

The default list of excluded protocols now only contains valid and complete entries:

SSLv3, TLSv1, and TLSv1.1

Refs jetty/jetty.project#5531
Fixes #3532

(cherry picked from commit 206e858)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants