Selected protocols in SslContextFactory#dump() and actual SSLContext inconsistent #5531

joschi · 2020-10-28T22:21:04Z

Jetty version

9.4.33.v20201020

Java version

openjdk version "1.8.0_272"
OpenJDK Runtime Environment (Zulu 8.50.0.21-CA-macosx) (build 1.8.0_272-b17)
OpenJDK 64-Bit Server VM (Zulu 8.50.0.21-CA-macosx) (build 25.272-b17, mixed mode)

OS type/version

macOS 10.15.7

Description

When a list of protocols used with or SslContextFactory#excludeProtocols() contains a regular expression, the respective protocols will be shown as excluded in the output of SslContextFactory#dump(), but they won't necessarily be excluded in the actual SSLEngine instance created by SslContextFactory#newSSLEngine().

The same is true for the inverse case when using SslContextFactory#includeProtocols().

While the list of included/excluded ciphers is actually being processed to allow regular expressions (https://github.com/eclipse/jetty.project/blob/jetty-9.4.33.v20201020/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java#L1420-L1453), the list of included/excluded protocols is not processed in the same way.

SslContextFactory#dump() is passing the list of excluded protocols to SslSelectionDump:
https://github.com/eclipse/jetty.project/blob/jetty-9.4.33.v20201020/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java#L420-L424

There, any regular expressions the respective list will be compiled and matched against the list of supported protocols and removed accordingly, so that they appear to excluded:
https://github.com/eclipse/jetty.project/blob/jetty-9.4.33.v20201020/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslSelectionDump.java#L86-L102

Unfortunately, when the actual SSLContext is being configured, the list of excluded protocols is passed from SslContextFactory as-is to remove the protocols from the list of selected protocols of the SSLContext:
https://github.com/eclipse/jetty.project/blob/jetty-9.4.33.v20201020/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java#L1377-L1378

This leads to a discrepancy between which enabled and disabled protocols are listed in the output of SslContextFactory#dump() and which protocols the SSLContext and SSLEngine actually support.

Example unit test and output

@Test
public void testDefaultExcludedProtocols() throws Exception {
    SslContextFactory sslContextFactory = new SslContextFactory.Server();
    sslContextFactory.setExcludeProtocols("SSL.*", "TLSv1", "TLSv1\\.[01]");
    sslContextFactory.dump(System.out, "");

    sslContextFactory.start();
    try {
        assertThat(sslContextFactory.newSSLEngine().getEnabledProtocols()).doesNotContain("TLSv1.1");
    } finally {
        sslContextFactory.stop();
    }
}

2020-10-28 21:55:52.984:INFO::main: Logging initialized @878ms to org.eclipse.jetty.util.log.StdErrLog
Server@12591ac8[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\.[01]'
+> Cipher Suite Selections
   +> Enabled size=27
   |  +> TLS_AES_128_GCM_SHA256
   |  +> TLS_AES_256_GCM_SHA384
   |  +> TLS_DHE_DSS_WITH_AES_128_CBC_SHA256
   |  +> TLS_DHE_DSS_WITH_AES_128_GCM_SHA256
   |  +> TLS_DHE_DSS_WITH_AES_256_CBC_SHA256
   |  +> TLS_DHE_DSS_WITH_AES_256_GCM_SHA384
   |  +> TLS_DHE_RSA_WITH_AES_128_CBC_SHA256
   |  +> TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
   |  +> TLS_DHE_RSA_WITH_AES_256_CBC_SHA256
   |  +> TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
   |  +> TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
   |  +> TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256
   |  +> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA384
   |  +> TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
   |  +> TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
   |  +> TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256
   |  +> TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384
   |  +> TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
   |  +> TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA256
   |  +> TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256
   |  +> TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA384
   |  +> TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384
   |  +> TLS_ECDH_RSA_WITH_AES_128_CBC_SHA256
   |  +> TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256
   |  +> TLS_ECDH_RSA_WITH_AES_256_CBC_SHA384
   |  +> TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384
   |  +> TLS_EMPTY_RENEGOTIATION_INFO_SCSV
   +> Disabled size=18
      +> TLS_DHE_DSS_WITH_AES_128_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_DHE_DSS_WITH_AES_256_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_DHE_RSA_WITH_AES_128_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_DHE_RSA_WITH_AES_256_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_ECDH_RSA_WITH_AES_128_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_ECDH_RSA_WITH_AES_256_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$'
      +> TLS_RSA_WITH_AES_128_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$', ConfigExcluded:'^TLS_RSA_.*$'
      +> TLS_RSA_WITH_AES_128_CBC_SHA256 - ConfigExcluded:'^TLS_RSA_.*$'
      +> TLS_RSA_WITH_AES_128_GCM_SHA256 - ConfigExcluded:'^TLS_RSA_.*$'
      +> TLS_RSA_WITH_AES_256_CBC_SHA - ConfigExcluded:'^.*_(MD5|SHA|SHA1)$', ConfigExcluded:'^TLS_RSA_.*$'
      +> TLS_RSA_WITH_AES_256_CBC_SHA256 - ConfigExcluded:'^TLS_RSA_.*$'
      +> TLS_RSA_WITH_AES_256_GCM_SHA384 - ConfigExcluded:'^TLS_RSA_.*$'


java.lang.AssertionError: 
Expecting
 <["TLSv1.2", "TLSv1.1"]>
not to contain
 <["TLSv1.1"]>
but found
 <["TLSv1.1"]>

The text was updated successfully, but these errors were encountered:

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

…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

…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)

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

joakime · 2020-10-29T19:00:33Z

public void testDefaultExcludedProtocols() throws Exception {
    SslContextFactory sslContextFactory = new SslContextFactory.Server();
    sslContextFactory.setExcludeProtocols("SSL.*", "TLSv1", "TLSv1\\.[01]");
    sslContextFactory.dump(System.out, "");

    sslContextFactory.start();
    try {
        assertThat(sslContextFactory.newSSLEngine().getEnabledProtocols()).doesNotContain("TLSv1.1");
    } finally {
        sslContextFactory.stop();
    }
}

That's an invalid test.

You cannot use the dump THEN start the SslContextFactory and expect sane results.
The dump isn't valid until the SslContextFactory is started.

I added a testcase based on your report, it works as designed, and correctly.
Both the new SSLEngine and the dump report the same values. (where TLSv1.1 is excluded)

https://github.com/eclipse/jetty.project/blob/cff4771375ee5120fdd11a82a3e090cc3ecd6135/jetty-util/src/test/java/org/eclipse/jetty/util/ssl/SslContextFactoryTest.java#L99-L130

joakime · 2020-10-29T19:01:32Z

Going to test this on a few different OS combinations now.

joakime · 2020-10-29T20:19:53Z

Results from OSX 10.15.7

Using AdoptOpenJDK 8u272 - PASS
Using AdoptOpenJDK 11.0.9 - PASS

Results from Windows 10

Using AdoptOpenJDK 8u272 - PASS
Using AdoptOpenJDK 11.0.9 - PASS

Refs jetty/jetty.project#5531

joschi · 2020-10-29T21:46:34Z

@joakime Thanks for checking this bug report!

The dump isn't valid until the SslContextFactory is started.

I didn't find anything in the Javadoc about that: SslContextFactory#dump(Appendable, String).

Would it make sense to throw an IllegalStateException or emit an error message instead of an invalid dump in this case?

I added a testcase based on your report, it works as designed, and correctly.

Are you sure the test is working correctly? The assertion for "Confirm behavior in engine" is failing for me.

Here's the minimal project to reproduce the issue:
https://github.com/joschi/jetty-issue-5531-excluded-protocols/

Test case: SslContextFactoryTest

The only relevant difference is that I'm using AssertJ instead of Hamcrest matchers.
Printing the list of enabled protocols at the end of the test with System.out.println() confirmed that the assertion is working (and failing) correctly.

@Test
public void testDumpExcludedProtocols() throws Exception {
    SslContextFactory.Server cf = new SslContextFactory.Server();
    cf.setKeyStorePassword("storepwd");
    cf.setKeyManagerPassword("keypwd");
    cf.setExcludeProtocols("SSL.*", "TLSv1", "TLSv1\\.[01]");
    cf.start();

    // Confirm behavior in engine
    String[] enabledProtocols = cf.newSSLEngine().getEnabledProtocols();
    assertThat(enabledProtocols).doesNotContain("TLSv1.1");
}

Failing output on Linux (Ubuntu 18.04.5), macOS 10.15.7, Windows (Windows Server 2019 10.0.17763) with Java (Azul Zulu) 1.8.0_272, 11.0.9, and 15.0.1:

https://github.com/joschi/jetty-issue-5531-excluded-protocols/actions/runs/336689872

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

joakime · 2020-10-30T01:29:26Z

Wait a sec.
There's multiple things not right at this point.

First, and most "duh" moment of my own, is that .setExcludeProtocols() doesn't support regex. (even says so in the javadoc)
I need to fix that, so I opened #5535.
The next is that the assertion I have is wrong.
I have to fix that too.

Commit c969fba addresses both the bad usage of .setExcludeProtocols and the bad assertion.

joschi · 2020-10-30T07:08:28Z

@joakime Thanks for looking further into this!

First, and most "duh" moment of my own, is that .setExcludeProtocols() doesn't support regex. (even says so in the javadoc)

That's what I tried to convey in my original post of this issue:

While the list of included/excluded ciphers is actually being processed to allow regular expressions (https://github.com/eclipse/jetty.project/blob/jetty-9.4.33.v20201020/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java#L1420-L1453), the list of included/excluded protocols is not processed in the same way.

But great that this is going to be fixed. 😃

joakime · 2020-10-30T15:32:44Z

Opened PR #5538

joakime · 2020-10-30T18:14:22Z

Merged PR #5538 into jetty-9.4.x

joschi mentioned this issue Oct 28, 2020

Dropwizard reporting incorrectly enabled HTTPS protocols dropwizard/dropwizard#3532

Closed

joschi mentioned this issue Oct 28, 2020

Replace invalid default excludedProtocols in HttpsConnectorFactory dropwizard/dropwizard#3533

Merged

joakime self-assigned this Oct 29, 2020

joakime added the Bug For general bugs on Jetty side label Oct 29, 2020

joakime added a commit that referenced this issue Oct 29, 2020

Issue #5531 - Test excluded protocol behavior

cff4771

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

joakime added More Info Required Not a bug and removed Bug For general bugs on Jetty side labels Oct 29, 2020

joschi added a commit to joschi/jetty-issue-5531-excluded-protocols that referenced this issue Oct 29, 2020

Minimal project to verify excluded protocols in SslContextFactory

7d537e9

Refs jetty/jetty.project#5531

joschi added a commit to joschi/jetty-issue-5531-excluded-protocols that referenced this issue Oct 29, 2020

Minimal project to verify excluded protocols in SslContextFactory

8cca87f

Refs jetty/jetty.project#5531

joakime mentioned this issue Oct 30, 2020

Support regex in SslContextFactory include/exclude of protocols #5535

Closed

joakime added a commit that referenced this issue Oct 30, 2020

Issue #5531 - Using .setExcludeProtocols correctly.

c969fba

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>

joakime mentioned this issue Oct 30, 2020

Issue #5535 - Adding support for regex include/exclude of Protocols to SslContextFactory #5538

Merged

joakime linked a pull request Oct 30, 2020 that will close this issue

Issue #5535 - Adding support for regex include/exclude of Protocols to SslContextFactory #5538

Merged

joakime closed this as completed Oct 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selected protocols in SslContextFactory#dump() and actual SSLContext inconsistent #5531

Selected protocols in SslContextFactory#dump() and actual SSLContext inconsistent #5531

joschi commented Oct 28, 2020

joakime commented Oct 29, 2020

joakime commented Oct 29, 2020

joakime commented Oct 29, 2020

joschi commented Oct 29, 2020

joakime commented Oct 30, 2020

joschi commented Oct 30, 2020

joakime commented Oct 30, 2020

joakime commented Oct 30, 2020

Selected protocols in SslContextFactory#dump() and actual SSLContext inconsistent #5531

Selected protocols in SslContextFactory#dump() and actual SSLContext inconsistent #5531

Comments

joschi commented Oct 28, 2020

joakime commented Oct 29, 2020

joakime commented Oct 29, 2020

joakime commented Oct 29, 2020

joschi commented Oct 29, 2020

joakime commented Oct 30, 2020

joschi commented Oct 30, 2020

joakime commented Oct 30, 2020

joakime commented Oct 30, 2020