Add deprecation warnings for ssl config fallback by jaymode · Pull Request #36847 · elastic/elasticsearch

jaymode · 2018-12-19T16:12:45Z

SSL configuration fallback has long been present in security but is a
source of confusion due to the behavior. Ultimately, we plan to remove
support for fallback in the next major version so this commit provides
deprecation warnings for the current line of stable releases.

Relates #36846

SSL configuration fallback has long been present in security but is a source of confusion due to the behavior. Ultimately, we plan to remove support for fallback in the next major version so this commit provides deprecation warnings for the current line of stable releases.

elasticmachine · 2018-12-19T16:12:47Z

Pinging @elastic/es-security

tvernum

Will there be a follow up PR for the docs that explains why these are deprecated?

tvernum · 2018-12-28T02:05:48Z

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

            SecureSetting.secureString(key, LEGACY_TRUSTSTORE_PASSWORD_TEMPLATE.apply(key.replace("truststore.secure_password",
-                    "truststore.password")));
+                    "truststore.password")),
+                key.startsWith(XPackSettings.GLOBAL_SSL_PREFIX) ? new Property[] { Property.Deprecated } : new Property[0]);


Shouldn't the legacy (non-secure) password always be deprecated? It looks like this just got missed/lost at some point.

It is deprecated always. What you are seeing is the fallback setting being configured, which has its own properties; the properties here are the ones for the secure setting.

Of course. It really is a bit of a mess isn't it.

tvernum · 2018-12-28T02:17:14Z

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

+                List<String> urls = realm.getAsList("url");
+                if (urls.isEmpty() == false && urls.stream().anyMatch(s -> s.startsWith("ldaps://"))) {
+                    return true;
+                }


SAML uses ssl if the metadata path points to a https URL.
https://github.com/elastic/elasticsearch/blob/6.6/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/saml/SamlRealm.java#L515

tvernum · 2018-12-28T02:20:35Z

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

            SecureSetting.secureString(key, LEGACY_KEYSTORE_KEY_PASSWORD_TEMPLATE.apply(key.replace("keystore.secure_key_password",
-                    "keystore.key_password")));
+                    "keystore.key_password")),
+                key.startsWith(XPackSettings.GLOBAL_SSL_PREFIX) ? new Property[] { Property.Deprecated } : new Property[0]);


Per previous comment, we can probably make this deprecated in all cases.

This property is for the secure setting and not the legacy value

tvernum · 2018-12-28T02:21:00Z

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

-            SecureSetting.secureString(key, LEGACY_KEY_PASSWORD_TEMPLATE.apply(key.replace("secure_key_passphrase",
-                    "key_passphrase")));
+            SecureSetting.secureString(key, LEGACY_KEY_PASSWORD_TEMPLATE.apply(key.replace("secure_key_passphrase", "key_passphrase")),
+                key.startsWith(XPackSettings.GLOBAL_SSL_PREFIX) ? new Property[] { Property.Deprecated } : new Property[0]);


This property is for the secure setting and not the legacy value

jaymode · 2019-01-07T20:12:58Z

@tvernum I updated docs and added deprecation checks

jaymode · 2019-01-09T21:21:35Z

run gradle build tests 1

tvernum

LGTM

jkakavas

LGTM, a couple of minor suggestions

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

jkakavas · 2019-01-10T06:46:31Z

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

+        return false;
+    }
+
+    private void checkSSLConfigurationForFallback(String name, Settings settings, SSLConfiguration config) {


I think it might be nice to print the key missing from the actual config that falls back to the default value here instead of a descriptive name, but I'm not sure if it can be reliably computed and if it would actually make the deprecation log better, so I just bring it up for consideration

I agree that it would be nice, but I think the effort to compute this would probably outweigh the benefits

jkakavas · 2019-01-10T06:55:18Z

...t/java/org/elasticsearch/xpack/security/transport/ServerTransportFilterIntegrationTests.java

            "testnode",
            "/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.crt",
-            Arrays.asList("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.crt"));
+            Collections.singletonList("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.crt"));


👍 thanks for fixing these also

Co-Authored-By: jaymode <jaymode@users.noreply.github.com>

jaymode · 2019-01-10T20:35:12Z

run gradle build tests 2

Tests with failures:

org.elasticsearch.upgrades.WatchBackwardsCompatibilityIT.testWatcherRestart

jaymode · 2019-01-10T22:34:06Z

run gradle build tests 2

jaymode added 7 commits December 18, 2018 10:48

Merge branch '6.x' into deprecate_ssl_fallback

610f6f4

fix checkstyle

692c4a4

fix watcher httpclienttests

122053e

fix test ssl settings

a67ba76

more test fixes

a04f694

Merge branch '6.x' into deprecate_ssl_fallback

21ace21

jaymode added >deprecation :Security/TLS SSL/TLS, Certificates v6.7.0 labels Dec 19, 2018

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

jaymode added 2 commits December 20, 2018 09:44

Merge branch '6.x' into deprecate_ssl_fallback

07ed085

Merge branch '6.x' into deprecate_ssl_fallback

7697ef3

tvernum reviewed Dec 28, 2018

View reviewed changes

jaymode added 4 commits January 7, 2019 11:54

Merge branch '6.x' into deprecate_ssl_fallback

6484b1a

include saml realm

3b3eada

add deprecation in docs

930bfd6

add node deprecation checks

21acf04

jaymode mentioned this pull request Jan 7, 2019

Security: remove SSL settings fallback #36846

Merged

AthenaEryma mentioned this pull request Jan 7, 2019

Update Deprecation Info API for 7.0 breaking changes #36024

Closed

26 tasks

jaymode added 3 commits January 8, 2019 10:22

Merge branch '6.x' into deprecate_ssl_fallback

6a50e7a

fix test

6e7fca0

Merge branch '6.x' into deprecate_ssl_fallback

e3bf2b6

jaymode requested a review from tvernum January 9, 2019 21:21

tvernum approved these changes Jan 10, 2019

View reviewed changes

jkakavas approved these changes Jan 10, 2019

View reviewed changes

jkakavas and others added 2 commits January 10, 2019 09:29

use key for monitoring too

eaf88c5

Co-Authored-By: jaymode <jaymode@users.noreply.github.com>

Merge branch '6.x' into deprecate_ssl_fallback

9df9495

jaymode added 2 commits January 11, 2019 08:21

Merge branch '6.x' into deprecate_ssl_fallback

fe4d0bd

remove global ssl usage to avoid warning issues

ff93bd3

jaymode merged commit 680d3b3 into elastic:6.x Jan 14, 2019

jaymode deleted the deprecate_ssl_fallback branch January 14, 2019 16:54

tvernum mentioned this pull request Jan 24, 2019

Add TLS version changes to deprecation checks #37793

Merged

AthenaEryma mentioned this pull request Jan 25, 2019

Deprecation Info API cannot check filtered settings #37845

Closed

Conversation

jaymode commented Dec 19, 2018

Uh oh!

elasticmachine commented Dec 19, 2018

Uh oh!

tvernum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaymode commented Jan 7, 2019

Uh oh!

jaymode commented Jan 9, 2019

Uh oh!

tvernum left a comment

Choose a reason for hiding this comment

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaymode commented Jan 10, 2019

Uh oh!

jaymode commented Jan 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants