Reject misconfigured/ambiguous SSL server config by tvernum · Pull Request #45892 · elastic/elasticsearch

tvernum · 2019-08-23T06:52:19Z

This commit makes it an error to start a node where either of the
server contexts (xpack.security.transport.ssl and
xpack.security.http.ssl) meet either of these conditions:

The server lacks a certificate/key pair (i.e. neither
ssl.keystore.path nor ssl.certificate are configured)
The server has some ssl configuration, but ssl.enabled is not
specified. This new validation does not care whether ssl.enabled is
true or false (though other validation might), it simply makes it
an error to configure server SSL without being explicit about
whether to enable that configuration.

Relates to #49280

This commit makes it an error to start a node where either of the server contexts (xpack.security.transport.ssl and xpack.security.http.ssl) meet either of these conditions: 1. The server lacks a certificate/key pair (i.e. neither ssl.keystore.path not ssl.certificate are configured) 2. The server has some ssl configuration, but ssl.enabled is not specified. This new validation does not care whether ssl.enabled is true or false (though other validation might), it simply makes it an error to configure server SSL without being explicit about whether to enable that configuration.

elasticmachine · 2019-08-23T06:52:21Z

Pinging @elastic/es-security

tvernum · 2019-08-23T06:52:58Z

This is 8.0 only, as it is a breaking change. I will raise a separate PR to backport to 7.x as a deprecation.

tvernum · 2019-08-27T07:49:43Z

@elasticmachine update branch

jkakavas

LGTM, left some suggestions

jkakavas · 2019-08-27T08:54:36Z

docs/reference/migration/migrate_8_0/security.asciidoc

+`xpack.security.http.ssl` without also configuring
+`xpack.security.http.ssl.enabled`.
+
+For example, the following configuration is invalid:


Maybe do this example with PEM files to further visually indicate that this doesn't apply only to keystores/truststores?

jkakavas · 2019-08-27T09:10:46Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

+            final SSLConfigurationSettings configurationSettings = SSLConfigurationSettings.withPrefix(prefix + ".");
+            if (isConfigurationValidForServerUsage(configuration) == false) {
+                throw new ElasticsearchSecurityException("invalid configuration for " + prefix +
+                    " - a server context requires a key and certificate, but these have not been configured; either [" +


Unsure how clear "server context" is. We could try "server SSL context" or "server SSL configuration" ?

jkakavas · 2019-08-27T09:13:26Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

+                throw new ElasticsearchSecurityException("invalid configuration for " + prefix +
+                    " - a server context requires a key and certificate, but these have not been configured; either [" +
+                    configurationSettings.x509KeyPair.keystorePath.getKey() + "] or [" +
+                    configurationSettings.x509KeyPair.certificatePath.getKey() + "] should be set");


Suggested change

configurationSettings.x509KeyPair.certificatePath.getKey() + "] should be set");

configurationSettings.x509KeyPair.keyPath.getKey() + " and " + configurationSettings.x509KeyPair.certificatePath.getKey() + "] should be set");

jkakavas · 2019-08-27T09:19:54Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

+                    configurationSettings.x509KeyPair.certificatePath.getKey() + "] should be set");
+            }
+        } else if (settings.hasValue(enabledSetting) == false) {
+            final List<String> sslKeys = settings.keySet().stream()


Suggested change

final List<String> sslKeys = settings.keySet().stream()

final List<String> sslConfigKeys = settings.keySet().stream()

to make it immediately obvious when looking at the code these are not SSL Keys but configuration keys

bizybot

LGTM, Thank you.

We added a few tests with invalid config while this PR was stagnant

tvernum · 2019-11-05T05:39:00Z

@elasticmachine update branch

This commit adds a deprecation warning when starting a node where either of the server contexts (xpack.security.transport.ssl and xpack.security.http.ssl) meet either of these conditions: 1. The server lacks a certificate/key pair (i.e. neither ssl.keystore.path not ssl.certificate are configured) 2. The server has some ssl configuration, but ssl.enabled is not specified. This new validation does not care whether ssl.enabled is true or false (though other validation might), it simply makes it an error to configure server SSL without being explicit about whether to enable that configuration. Backport of: elastic#45892

This commit adds a deprecation warning when starting a node where either of the server contexts (xpack.security.transport.ssl and xpack.security.http.ssl) meet either of these conditions: 1. The server lacks a certificate/key pair (i.e. neither ssl.keystore.path not ssl.certificate are configured) 2. The server has some ssl configuration, but ssl.enabled is not specified. This new validation does not care whether ssl.enabled is true or false (though other validation might), it simply makes it an error to configure server SSL without being explicit about whether to enable that configuration. Backport of: #45892

…settings (#77092) In 8.0 we prevent the server from starting up if certain SSL properties are misconfigured or ambiguous. Specifically: 1) The server lacks a certificate/key pair (i.e. neither ssl.keystore.path nor ssl.key/ssl.certificate are configured) 2) The server has some ssl configuration, but ssl.enabled is not specified. This commit adds a check to the deprecation info API for these changes. Relates #42404 #45892

tvernum added >enhancement >breaking :Security/TLS SSL/TLS, Certificates v8.0.0 labels Aug 23, 2019

tvernum requested review from bizybot and jkakavas August 23, 2019 06:52

Merge branch 'master' into tls-missing-key-material

e25a370

jkakavas approved these changes Aug 27, 2019

View reviewed changes

bizybot approved these changes Aug 28, 2019

View reviewed changes

tvernum added 6 commits October 15, 2019 14:34

Merge branch 'master' into tls-missing-key-material

26b0944

Address feedback

d9b73a6

Fix broken tests

535094f

We added a few tests with invalid config while this PR was stagnant

Merge branch 'master' into tls-missing-key-material

a681ed7

Update error message

7077b90

Fix HLRC tests

fced860

tvernum mentioned this pull request Oct 18, 2019

Better error messages for common TLS problems #43079

Open

37 tasks

tvernum added 4 commits November 5, 2019 00:46

Merge branch 'master' into tls-missing-key-material

8666968

Switch boolean setting to string

5b74feb

Add transport.ssl.enabled to HLRC build

fd7a95e

Merge branch 'master' into tls-missing-key-material

1791f16

tvernum merged commit eb3c57b into elastic:master Nov 7, 2019

tvernum added the backport pending label Nov 7, 2019

tvernum mentioned this pull request Nov 19, 2019

Deprecate misconfigured SSL server config #49280

Merged

tvernum removed the backport pending label Nov 22, 2019

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

jakelandis mentioned this pull request Feb 22, 2021

DRAFT [META] REST Compatible API V7 completeness #68905

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

masseyke mentioned this pull request Aug 9, 2021

Update Deprecation Info API checks for 8.0 #42404

Closed

80 tasks

masseyke mentioned this pull request Aug 31, 2021

Adding deprecation info API check for misconfigured or ambiguous SSL settings #77092

Merged

lockewritesdocs mentioned this pull request Sep 16, 2021

[DOCS] Add TLS config validation to 7.6 breaking changes #77932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject misconfigured/ambiguous SSL server config#45892

Reject misconfigured/ambiguous SSL server config#45892
tvernum merged 12 commits intoelastic:masterfrom
tvernum:tls-missing-key-material

tvernum commented Aug 23, 2019 •

edited by jrodewig

Loading

Uh oh!

elasticmachine commented Aug 23, 2019

Uh oh!

tvernum commented Aug 23, 2019

Uh oh!

tvernum commented Aug 27, 2019

Uh oh!

jkakavas left a comment

Uh oh!

jkakavas Aug 27, 2019

Uh oh!

jkakavas Aug 27, 2019

Uh oh!

jkakavas Aug 27, 2019

Uh oh!

jkakavas Aug 27, 2019

Uh oh!

bizybot left a comment

Uh oh!

tvernum commented Nov 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	configurationSettings.x509KeyPair.certificatePath.getKey() + "] should be set");
	configurationSettings.x509KeyPair.keyPath.getKey() + " and " + configurationSettings.x509KeyPair.certificatePath.getKey() + "] should be set");

	final List<String> sslKeys = settings.keySet().stream()
	final List<String> sslConfigKeys = settings.keySet().stream()

Conversation

tvernum commented Aug 23, 2019 • edited by jrodewig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 23, 2019

Uh oh!

tvernum commented Aug 23, 2019

Uh oh!

tvernum commented Aug 27, 2019

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

jkakavas Aug 27, 2019

Choose a reason for hiding this comment

Uh oh!

jkakavas Aug 27, 2019

Choose a reason for hiding this comment

Uh oh!

jkakavas Aug 27, 2019

Choose a reason for hiding this comment

Uh oh!

jkakavas Aug 27, 2019

Choose a reason for hiding this comment

Uh oh!

bizybot left a comment

Choose a reason for hiding this comment

Uh oh!

tvernum commented Nov 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tvernum commented Aug 23, 2019 •

edited by jrodewig

Loading