Security: remove SSL settings fallback #36846

jaymode · 2018-12-19T16:10:34Z

This commit removes the fallback for SSL settings. While this may be
seen as a non user friendly change, the intention behind this change
is to simplify the reasoning needed to understand what is actually
being used for a given SSL configuration. Each configuration now needs
to be explicitly specified as there is no global configuration or
fallback to some other configuration.

Closes #29797

This commit removes the fallback for SSL settings. While this may be seen as a non user friendly change, the intention behind this change is to simplify the reasoning needed to understand what is actually being used for a given SSL configuration. Each configuration now needs to be explicitly specified as there is no global configuration or fallback to some other configuration. Closes elastic#29797

elasticmachine · 2018-12-19T16:10:36Z

Pinging @elastic/es-security

gwbrown · 2018-12-27T23:11:18Z

I've added this to the list of things we should add deprecation checks for in #36024 - would you be able to either:

Add a deprecation check for this, or
Have a brief sync when you're available so I can better understand what the user impact of this change is and how to check for needed configuration changes?

jaymode · 2019-01-07T20:08:30Z

@gwbrown I added checks in the 6.x version #36847

docs/reference/settings/security-settings.asciidoc

jkakavas · 2019-01-10T10:16:09Z

docs/reference/settings/security-settings.asciidoc

@@ -1380,23 +1276,21 @@ a PKCS#12 container includes trusted certificate ("anchor") entries look for



Shouldn't we move the PKCS11 related settings to the http/transport related settings ? These feel strangely placed as is

I have them there as general information. @lcawl do you have a suggestion on where we should put this info?

If I'm understanding correctly, that PKCS#11 already appears under the appropriate HTTP and Transport sections. So unless there are other contexts where there would be PKCS#11 settings, I think it can be removed from the "default values..." section.

I think we reference PKCS#12 in those sections but not PKCS#11

Scratch that. I am wrong, we already have it there. So it can be removed from the defaults section

jkakavas · 2019-01-10T10:30:21Z

x-pack/docs/en/rest-api/security/ssl.asciidoc

+those configured in the `xpack.security.transport.ssl.truststore` and
+`xpack.security.transport.ssl.certificate_authorities` settings. It also
+includes certificates that are used for configuring server identity, such as
+`xpack.security.transport.ssl.keystore` and


Should we use the http layer as an example for certficate and keystore so that it becomes (even more) obvious that certificates used in both layers will be returned ?

jkakavas · 2019-01-10T11:09:47Z

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

@@ -231,14 +190,6 @@ private static TrustConfig createCertChainTrustConfig(Settings settings, KeyConf
            String trustStoreAlgorithm = SETTINGS_PARSER.truststoreAlgorithm.get(settings);
            SecureString trustStorePassword = SETTINGS_PARSER.truststorePassword.get(settings);
            return new StoreTrustConfig(trustStorePath, trustStoreType, trustStorePassword, trustStoreAlgorithm);
-        } else if (global == null && System.getProperty("javax.net.ssl.trustStore") != null


jkakavas · 2019-01-10T11:24:45Z

...lugin/security/src/test/java/org/elasticsearch/xpack/security/authc/saml/SamlRealmTests.java

@@ -123,19 +123,20 @@ public void testReadIdpMetadataFromHttps() throws Exception {
        final Path path = getDataPath("idp1.xml");
        final String body = new String(Files.readAllBytes(path), StandardCharsets.UTF_8);
        final MockSecureSettings mockSecureSettings = new MockSecureSettings();
-        mockSecureSettings.setString("xpack.ssl.secure_key_passphrase", "testnode");
+        mockSecureSettings.setString("xpack.security.transport.ssl.secure_key_passphrase", "testnode");


Doesn't really affect anything but we could use xpack.security.http key since the produced SSLContext is used in a MockWebServer

jkakavas · 2019-01-10T11:39:02Z

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/ssl/SSLClientAuthTests.java

+            if (value == null) {
+                try {
+                    if (key.startsWith("xpack.security.transport.ssl.")) {
+                        byte[] file = Streams.readAll(secureSettings.getFile(key));


Maybe move toBytesArray to ESTestCase and use this instead so that we don't introduce BC dependencies again?

jkakavas · 2019-01-10T11:41:08Z

...lugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/common/http/HttpClientTests.java

            Settings settings2 = Settings.builder()
-                .put("xpack.ssl.key", keyPath)
-                .put("xpack.ssl.certificate", certPath)
+                .put("xpack.security.transport.ssl.key", keyPath)


same as above, just for being easy to the eye use the http layer settings for the MockWebServer

jkakavas · 2019-01-10T11:42:04Z

...lugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/common/http/HttpClientTests.java

        try (HttpClient client = new HttpClient(settings, new SSLService(settings, environment), null)) {
            MockSecureSettings secureSettings = new MockSecureSettings();
            // We can't use the client created above for the server since it only defines a truststore
-            secureSettings.setString("xpack.ssl.secure_key_passphrase", "testnode-no-subjaltname");
+            secureSettings.setString("xpack.security.transport.ssl.secure_key_passphrase", "testnode-no-subjaltname");


same as above, just for being easy to the eye use the http layer settings for the MockWebServer

tvernum

LGTM.
It looks like it was a massive effort to work through all those tests.

jaymode · 2019-01-11T15:31:41Z

It looks like it was a massive effort to work through all those tests.

Yes it was. So much that merging 6+ months worth of changes was better than starting fresh.

jaymode · 2019-01-14T16:56:21Z

run gradle build tests 1

* master: (28 commits) Introduce retention lease serialization (elastic#37447) Update Delete Watch to allow unknown fields (elastic#37435) Make finalize step of recovery source non-blocking (elastic#37388) Update the default for include_type_name to false. (elastic#37285) Security: remove SSL settings fallback (elastic#36846) Adding mapping for hostname field (elastic#37288) Relax assertSameDocIdsOnShards assertion Reduce recovery time with compress or secure transport (elastic#36981) Implement ccr file restore (elastic#37130) Fix Eclipse specific compilation issue (elastic#37419) Performance fix. Reduce deprecation calls for the same bulk request (elastic#37415) [ML] Use String rep of Version in map for serialisation (elastic#37416) Cleanup Deadcode in Rest Tests (elastic#37418) Mute IndexShardRetentionLeaseTests.testCommit elastic#37420 unmuted test Remove unused index store in directory service Improve CloseWhileRelocatingShardsIT (elastic#37348) Fix ClusterBlock serialization and Close Index API logic after backport to 6.x (elastic#37360) Update the scroll example in the docs (elastic#37394) Update analysis.asciidoc (elastic#37404) ...

Update the security configuration with the changes brought in elastic/elasticsearch#36846

Update the security configuration with the changes brought in elastic/elasticsearch#36846 Relates #12

jaymode added 9 commits April 26, 2018 12:45

Merge branch 'master' into no_ssl_fallback

b2b4970

Merge branch 'master' into no_ssl_fallback

481c460

dont enable ssl unless we really need to

51b7660

cleanup

5e31047

fix pkiauthtests

b7f0f46

rollback pem -> jks changes

48b1adf

update docs

df1819a

Merge branch 'master' into no_ssl_fallback

b40aa14

jaymode added >enhancement >breaking v7.0.0 :Security/TLS SSL/TLS, Certificates labels Dec 19, 2018

jaymode requested review from tvernum and jkakavas December 19, 2018 16:10

jaymode mentioned this pull request Dec 19, 2018

Add deprecation warnings for ssl config fallback #36847

Merged

jaymode added 2 commits December 20, 2018 09:55

Merge branch 'master' into no_ssl_fallback

32f334d

Merge branch 'master' into no_ssl_fallback

6bbcbf9

gwbrown mentioned this pull request Dec 27, 2018

Update Deprecation Info API for 7.0 breaking changes #36024

Closed

26 tasks

jaymode added 2 commits January 7, 2019 13:09

Merge branch 'master' into no_ssl_fallback

79219e2

docs

f7db301

jkakavas reviewed Jan 10, 2019

View reviewed changes

jaymode added 3 commits January 10, 2019 13:04

Merge branch 'master' into no_ssl_fallback

367ac1d

address feedback

0861087

fix test

c641ade

tvernum approved these changes Jan 11, 2019

View reviewed changes

Merge branch 'master' into no_ssl_fallback

a2bd36b

jaymode added 3 commits January 11, 2019 09:46

Merge branch 'master' into no_ssl_fallback

8ed61db

Merge branch 'master' into no_ssl_fallback

74f200d

pkcs11 docs

337d694

Merge branch 'master' into no_ssl_fallback

8bc6232

jaymode merged commit f3edbe2 into elastic:master Jan 14, 2019

jaymode deleted the no_ssl_fallback branch January 14, 2019 21:06

dliappis added a commit to dliappis/rally-teams that referenced this pull request Jan 16, 2019

Declare TLS key/certs separately for http/transport

205f169

Update the security configuration with the changes brought in elastic/elasticsearch#36846

dliappis mentioned this pull request Jan 16, 2019

Declare TLS key/certs separately for http/transport elastic/rally-teams#12

Merged

dliappis added a commit to elastic/rally-teams that referenced this pull request Jan 16, 2019

Declare TLS key/certs separately for http/transport

ef1ff09

Update the security configuration with the changes brought in elastic/elasticsearch#36846 Relates #12

jkakavas mentioned this pull request Jan 22, 2019

[CI] Cannot load security plugin under 8FIPS #37682

Closed

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security: remove SSL settings fallback #36846

Security: remove SSL settings fallback #36846

jaymode commented Dec 19, 2018

elasticmachine commented Dec 19, 2018

gwbrown commented Dec 27, 2018

jaymode commented Jan 7, 2019

jkakavas Jan 10, 2019

jaymode Jan 10, 2019

lcawl Jan 11, 2019

jaymode Jan 11, 2019

jaymode Jan 11, 2019

jkakavas Jan 10, 2019

jkakavas Jan 10, 2019

jkakavas Jan 10, 2019

jkakavas Jan 10, 2019

jkakavas Jan 10, 2019

jkakavas Jan 10, 2019

tvernum left a comment

jaymode commented Jan 11, 2019

jaymode commented Jan 14, 2019

		@@ -1380,23 +1276,21 @@ a PKCS#12 container includes trusted certificate ("anchor") entries look for

Security: remove SSL settings fallback #36846

Security: remove SSL settings fallback #36846

Conversation

jaymode commented Dec 19, 2018

elasticmachine commented Dec 19, 2018

gwbrown commented Dec 27, 2018

jaymode commented Jan 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum left a comment

Choose a reason for hiding this comment

jaymode commented Jan 11, 2019

jaymode commented Jan 14, 2019