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.

Sign up for GitHub

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

Jump to bottom

Replace invalid default excludedProtocols in HttpsConnectorFactory #3533

Merged
merged 2 commits into from Oct 29, 2020
Merged

Replace invalid default excludedProtocols in HttpsConnectorFactory #3533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff1ea43
Replace invalid default excludedProtocols in HttpsConnectorFactory
joschi Oct 28, 2020
711c9ce
Let's add "SSLv2Hello" to be sure it's excluded
joschi Oct 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions docs/source/manual/configuration.rst
View file
Open in desktop
Expand Up @@ -470,10 +470,10 @@ validatePeers false Whether or not
implemented.
supportedProtocols (none) A list of protocols (e.g., ``SSLv3``, ``TLSv1``) which are supported. All
other protocols will be refused.
excludedProtocols ["SSL.*", "TLSv1", "TLSv1\\.1"] A list of protocols (e.g., ``SSLv3``, ``TLSv1``) which are excluded. These
protocols will be refused.
excludedProtocols ["SSLv2Hello", "SSLv3", A list of protocols (e.g., ``SSLv3``, ``TLSv1``) which are excluded. These
"TLSv1", "TLSv1.1"] protocols will be refused.
supportedCipherSuites (none) A list of cipher suites (e.g., ``TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256``) which
are supported. All other cipher suites will be refused
are supported. All other cipher suites will be refused.
excludedCipherSuites (none) A list of cipher suites (e.g., ``TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256``) which
are excluded. These cipher suites will be refused and exclusion takes higher
precedence than inclusion, such that if a cipher suite is listed in
Expand Down
54 changes: 27 additions & 27 deletions dropwizard-jetty/src/main/java/io/dropwizard/jetty/HttpsConnectorFactory.java
View file
Open in desktop
Expand Up @@ -182,7 +182,7 @@
* </tr>
* <tr>
* <td>{@code excludedProtocols}</td>
* <td>["SSL.*", "TLSv1", "TLSv1\.1"]</td>
* <td>["SSLv3", "TLSv1", "TLSv1.1"]</td>
* <td>
* A list of protocols (e.g., {@code SSLv3}, {@code TLSv1}) which are excluded. These
* protocols will be refused.
Expand All @@ -193,7 +193,7 @@
* <td>JVM default</td>
* <td>
* A list of cipher suites (e.g., {@code TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256}) which
* are supported. All other cipher suites will be refused
* are supported. All other cipher suites will be refused.
* </td>
* </tr>
* <tr>
Expand Down Expand Up @@ -287,7 +287,7 @@ public class HttpsConnectorFactory extends HttpConnectorFactory {
private List<String> supportedProtocols;

@Nullable
private List<String> excludedProtocols = Arrays.asList("SSL.*", "TLSv1", "TLSv1\\.1");
private List<String> excludedProtocols = Arrays.asList("SSLv2Hello", "SSLv3", "TLSv1", "TLSv1.1");

@Nullable
private List<String> supportedCipherSuites;
Expand Down Expand Up @@ -317,7 +317,7 @@ public String getEndpointIdentificationAlgorithm() {
}

@JsonProperty
public void setEndpointIdentificationAlgorithm(String endpointIdentificationAlgorithm) {
public void setEndpointIdentificationAlgorithm(@Nullable String endpointIdentificationAlgorithm) {
this.endpointIdentificationAlgorithm = endpointIdentificationAlgorithm;
}

Expand All @@ -328,7 +328,7 @@ public String getKeyStorePath() {
}

@JsonProperty
public void setKeyStorePath(String keyStorePath) {
public void setKeyStorePath(@Nullable String keyStorePath) {
this.keyStorePath = keyStorePath;
}

Expand All @@ -339,7 +339,7 @@ public String getKeyStorePassword() {
}

@JsonProperty
public void setKeyStorePassword(String keyStorePassword) {
public void setKeyStorePassword(@Nullable String keyStorePassword) {
this.keyStorePassword = keyStorePassword;
}

Expand All @@ -360,7 +360,7 @@ public String getKeyStoreProvider() {
}

@JsonProperty
public void setKeyStoreProvider(String keyStoreProvider) {
public void setKeyStoreProvider(@Nullable String keyStoreProvider) {
this.keyStoreProvider = keyStoreProvider;
}

Expand All @@ -381,7 +381,7 @@ public String getTrustStoreProvider() {
}

@JsonProperty
public void setTrustStoreProvider(String trustStoreProvider) {
public void setTrustStoreProvider(@Nullable String trustStoreProvider) {
this.trustStoreProvider = trustStoreProvider;
}

Expand All @@ -392,7 +392,7 @@ public String getKeyManagerPassword() {
}

@JsonProperty
public void setKeyManagerPassword(String keyManagerPassword) {
public void setKeyManagerPassword(@Nullable String keyManagerPassword) {
this.keyManagerPassword = keyManagerPassword;
}

Expand All @@ -403,7 +403,7 @@ public String getTrustStorePath() {
}

@JsonProperty
public void setTrustStorePath(String trustStorePath) {
public void setTrustStorePath(@Nullable String trustStorePath) {
this.trustStorePath = trustStorePath;
}

Expand All @@ -425,7 +425,7 @@ public Boolean getNeedClientAuth() {
}

@JsonProperty
public void setNeedClientAuth(Boolean needClientAuth) {
public void setNeedClientAuth(@Nullable Boolean needClientAuth) {
this.needClientAuth = needClientAuth;
}

Expand All @@ -436,7 +436,7 @@ public Boolean getWantClientAuth() {
}

@JsonProperty
public void setWantClientAuth(Boolean wantClientAuth) {
public void setWantClientAuth(@Nullable Boolean wantClientAuth) {
this.wantClientAuth = wantClientAuth;
}

Expand All @@ -447,7 +447,7 @@ public String getCertAlias() {
}

@JsonProperty
public void setCertAlias(String certAlias) {
public void setCertAlias(@Nullable String certAlias) {
this.certAlias = certAlias;
}

Expand All @@ -458,7 +458,7 @@ public File getCrlPath() {
}

@JsonProperty
public void setCrlPath(File crlPath) {
public void setCrlPath(@Nullable File crlPath) {
this.crlPath = crlPath;
}

Expand All @@ -469,7 +469,7 @@ public Boolean getEnableCRLDP() {
}

@JsonProperty
public void setEnableCRLDP(Boolean enableCRLDP) {
public void setEnableCRLDP(@Nullable Boolean enableCRLDP) {
this.enableCRLDP = enableCRLDP;
}

Expand All @@ -480,7 +480,7 @@ public Boolean getEnableOCSP() {
}

@JsonProperty
public void setEnableOCSP(Boolean enableOCSP) {
public void setEnableOCSP(@Nullable Boolean enableOCSP) {
this.enableOCSP = enableOCSP;
}

Expand All @@ -491,7 +491,7 @@ public Integer getMaxCertPathLength() {
}

@JsonProperty
public void setMaxCertPathLength(Integer maxCertPathLength) {
public void setMaxCertPathLength(@Nullable Integer maxCertPathLength) {
this.maxCertPathLength = maxCertPathLength;
}

Expand All @@ -502,7 +502,7 @@ public URI getOcspResponderUrl() {
}

@JsonProperty
public void setOcspResponderUrl(URI ocspResponderUrl) {
public void setOcspResponderUrl(@Nullable URI ocspResponderUrl) {
this.ocspResponderUrl = ocspResponderUrl;
}

Expand All @@ -513,7 +513,7 @@ public String getJceProvider() {
}

@JsonProperty
public void setJceProvider(String jceProvider) {
public void setJceProvider(@Nullable String jceProvider) {
this.jceProvider = jceProvider;
}

Expand All @@ -534,7 +534,7 @@ public List<String> getSupportedProtocols() {
}

@JsonProperty
public void setSupportedProtocols(List<String> supportedProtocols) {
public void setSupportedProtocols(@Nullable List<String> supportedProtocols) {
this.supportedProtocols = supportedProtocols;
}

Expand All @@ -545,7 +545,7 @@ public List<String> getExcludedProtocols() {
}

@JsonProperty
public void setExcludedProtocols(List<String> excludedProtocols) {
public void setExcludedProtocols(@Nullable List<String> excludedProtocols) {
this.excludedProtocols = excludedProtocols;
}

Expand All @@ -562,12 +562,12 @@ public List<String> getExcludedCipherSuites() {
}

@JsonProperty
public void setExcludedCipherSuites(List<String> excludedCipherSuites) {
public void setExcludedCipherSuites(@Nullable List<String> excludedCipherSuites) {
this.excludedCipherSuites = excludedCipherSuites;
}

@JsonProperty
public void setSupportedCipherSuites(List<String> supportedCipherSuites) {
public void setSupportedCipherSuites(@Nullable List<String> supportedCipherSuites) {
this.supportedCipherSuites = supportedCipherSuites;
}

Expand Down Expand Up @@ -762,12 +762,12 @@ protected SslContextFactory configureSslContextFactory(SslContextFactory factory
factory.setKeyManagerPassword(keyManagerPassword);
}

if (needClientAuth != null) {
factory.setNeedClientAuth(needClientAuth);
if (needClientAuth != null && factory instanceof SslContextFactory.Server) {
((SslContextFactory.Server) factory).setNeedClientAuth(needClientAuth);
}

if (wantClientAuth != null) {
factory.setWantClientAuth(wantClientAuth);
if (wantClientAuth != null && factory instanceof SslContextFactory.Server) {
((SslContextFactory.Server) factory).setWantClientAuth(wantClientAuth);
}

if (certAlias != null) {
Expand Down
50 changes: 34 additions & 16 deletions dropwizard-jetty/src/test/java/io/dropwizard/jetty/HttpsConnectorFactoryTest.java
View file
Open in desktop
Expand Up @@ -44,18 +44,18 @@
import static org.junit.jupiter.api.Assumptions.assumeFalse;
import static org.junit.jupiter.api.Assumptions.assumeTrue;

public class HttpsConnectorFactoryTest {
class HttpsConnectorFactoryTest {
private static final String WINDOWS_MY_KEYSTORE_NAME = "Windows-MY";
private final Validator validator = BaseValidator.newValidator();

@Test
public void isDiscoverable() throws Exception {
void isDiscoverable() {
assertThat(new DiscoverableSubtypeResolver().getDiscoveredSubtypes())
.contains(HttpsConnectorFactory.class);
}

@Test
public void testParsingConfiguration() throws Exception {
void testParsingConfiguration() throws Exception {
HttpsConnectorFactory https = new YamlConfigurationFactory<>(HttpsConnectorFactory.class, validator,
Jackson.newObjectMapper(), "dw-https")
.build(new File(Resources.getResource("yaml/https-connector.yml").toURI()));
Expand Down Expand Up @@ -90,7 +90,7 @@ public void testParsingConfiguration() throws Exception {
}

@Test
public void testSupportedProtocols() {
void testSupportedProtocols() {
List<String> supportedProtocols = Arrays.asList("SSLv3", "TLS1");

HttpsConnectorFactory factory = new HttpsConnectorFactory();
Expand All @@ -102,7 +102,7 @@ public void testSupportedProtocols() {
}

@Test
public void testExcludedProtocols() {
void testExcludedProtocols() {
List<String> excludedProtocols = Arrays.asList("SSLv3", "TLS1");

HttpsConnectorFactory factory = new HttpsConnectorFactory();
Expand All @@ -114,15 +114,33 @@ public void testExcludedProtocols() {
}

@Test
public void nonWindowsKeyStoreValidation() throws Exception {
void testDefaultExcludedProtocols() throws Exception {
HttpsConnectorFactory factory = new HttpsConnectorFactory();

SslContextFactory sslContextFactory = factory.configureSslContextFactory(new SslContextFactory.Server());
assertThat(sslContextFactory.getExcludeProtocols())
.containsExactlyElementsOf(factory.getExcludedProtocols());

sslContextFactory.start();
try {
assertThat(sslContextFactory.newSSLEngine().getEnabledProtocols())
.doesNotContainAnyElementsOf(factory.getExcludedProtocols())
.allSatisfy(protocol -> assertThat(protocol).doesNotStartWith("SSL"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we also want to ensure that enabled protocols only includes TLS v1.2/v1.3 and nothing else? Feels like a good test to fail if any older protocols are enabled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought about adding this, but then the test will start failing when run on JVMs which support other (newer) protocols in the future.

Or maybe some JVM only supports up to TLSv2. 🤔

} finally {
sslContextFactory.stop();
}
}

@Test
void nonWindowsKeyStoreValidation() {
HttpsConnectorFactory factory = new HttpsConnectorFactory();
Collection<String> properties = getViolationProperties(validator.validate(factory));
assertThat(properties.contains("validKeyStorePassword")).isEqualTo(true);
assertThat(properties.contains("validKeyStorePath")).isEqualTo(true);
}

@Test
public void windowsKeyStoreValidation() throws Exception {
void windowsKeyStoreValidation() {
HttpsConnectorFactory factory = new HttpsConnectorFactory();
factory.setKeyStoreType(WINDOWS_MY_KEYSTORE_NAME);
Collection<String> properties = getViolationProperties(validator.validate(factory));
Expand All @@ -131,7 +149,7 @@ public void windowsKeyStoreValidation() throws Exception {
}

@Test
public void canBuildContextFactoryWhenWindowsKeyStoreAvailable() throws Exception {
void canBuildContextFactoryWhenWindowsKeyStoreAvailable() {
// ignore test when Windows Keystore unavailable
assumeTrue(canAccessWindowsKeyStore());

Expand All @@ -142,7 +160,7 @@ public void canBuildContextFactoryWhenWindowsKeyStoreAvailable() throws Exceptio
}

@Test
public void windowsKeyStoreUnavailableThrowsException() throws Exception {
void windowsKeyStoreUnavailableThrowsException() {
assumeFalse(canAccessWindowsKeyStore());

final HttpsConnectorFactory factory = new HttpsConnectorFactory();
Expand All @@ -152,7 +170,7 @@ public void windowsKeyStoreUnavailableThrowsException() throws Exception {
}

@Test
public void testBuild() throws Exception {
void testBuild() throws Exception {
final HttpsConnectorFactory https = new HttpsConnectorFactory();
https.setBindHost("127.0.0.1");
https.setPort(8443);
Expand Down Expand Up @@ -196,15 +214,15 @@ public void testBuild() throws Exception {
assertThat(serverConnector.getServer()).isSameAs(server);
assertThat(serverConnector.getScheduler()).isInstanceOf(ScheduledExecutorScheduler.class);
assertThat(serverConnector.getExecutor()).isSameAs(threadPool);

final Jetty93InstrumentedConnectionFactory jetty93SslConnectionFacttory =
(Jetty93InstrumentedConnectionFactory) serverConnector.getConnectionFactory("ssl");
assertThat(jetty93SslConnectionFacttory).isInstanceOf(Jetty93InstrumentedConnectionFactory.class);
assertThat(jetty93SslConnectionFacttory.getTimer()).isSameAs(
metrics.timer("org.eclipse.jetty.server.HttpConnectionFactory.127.0.0.1.8443.connections"));
final SslContextFactory sslContextFactory = ((SslConnectionFactory) jetty93SslConnectionFacttory
.getConnectionFactory()).getSslContextFactory();

assertThat(getField(SslContextFactory.class, "_keyStoreResource", true).get(sslContextFactory))
.isEqualTo(Resource.newResource("/etc/app/server.ks"));
assertThat(sslContextFactory.getKeyStoreType()).isEqualTo("JKS");
Expand Down Expand Up @@ -235,7 +253,7 @@ public void testBuild() throws Exception {
assertThat(sslContextFactory.isValidatePeerCerts()).isTrue();
assertThat(sslContextFactory.getIncludeProtocols()).containsOnly("TLSv1.1", "TLSv1.2");
assertThat(sslContextFactory.getIncludeCipherSuites()).containsOnly("TLS_DHE_RSA.*", "TLS_ECDHE.*");

final ConnectionFactory httpConnectionFactory = serverConnector.getConnectionFactory("http/1.1");
assertThat(httpConnectionFactory).isInstanceOf(HttpConnectionFactory.class);
final HttpConfiguration httpConfiguration = ((HttpConnectionFactory) httpConnectionFactory)
Expand All @@ -250,7 +268,7 @@ public void testBuild() throws Exception {
}

@Test
public void partitionSupportOnlyEnable() {
void partitionSupportOnlyEnable() {
final String[] supported = {"SSLv2Hello", "SSLv3", "TLSv1", "TLSv1.1", "TLSv1.2"};
final String[] enabled = {"TLSv1", "TLSv1.1", "TLSv1.2"};
final Map<Boolean, List<String>> partition =
Expand All @@ -264,7 +282,7 @@ public void partitionSupportOnlyEnable() {
}

@Test
public void partitionSupportExclude() {
void partitionSupportExclude() {
final String[] supported = {"SSLv2Hello", "SSLv3", "TLSv1", "TLSv1.1", "TLSv1.2"};
final String[] enabled = {"SSLv3", "TLSv1", "TLSv1.1", "TLSv1.2"};
final String[] exclude = {"SSL.*"};
Expand All @@ -279,7 +297,7 @@ public void partitionSupportExclude() {
}

@Test
public void partitionSupportInclude() {
void partitionSupportInclude() {
final String[] supported = {"SSLv2Hello", "SSLv3", "TLSv1", "TLSv1.1", "TLSv1.2"};
final String[] enabled = {"SSLv3", "TLSv1", "TLSv1.1", "TLSv1.2"};
final String[] exclude = {"SSL*"};
Expand Down