Include realm type in Security Realm setting keys #30241

tvernum · 2018-04-30T03:51:40Z

This moves all Realm settings to an Affix definition.
However, because different realm types define different settings
(potentially conflicting settings) this requires that the realm type
become part of the setting key.

Thus, we now need to define realm settings as:

xpack.security.authc.realms:
  file.file1:
    order: 0

  native.native1:
    order: 1

This moves all Realm settings to an Affix definition. However, because different realm types define different settings (potentially conflicting settings) this requires that the realm type become part of the setting key. Thus, we now need to define realm settings as: xpack.security.authc.realms: file.file1: order: 0 native.native1: order: 1

elasticmachine · 2018-04-30T20:53:25Z

Pinging @elastic/es-security

jaymode

I like the direction this is going and would like to see if we can address the todos and get this incorporated

jaymode · 2018-05-16T14:32:35Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmConfig.java

+
+        @Override
+        public String toString() {
+            return type + '.' + name;


Hmm should we be consistent between this and Realm#toString?

jaymode · 2018-05-16T14:34:00Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmConfig.java

+    }
+
+    public static class RealmIdentifier {
+        final String type;


maybe we should just make these values private and use the getters?

jaymode · 2018-05-16T14:34:32Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmSettings.java

 /**
+ * TODO REALM-SETTINGS[TIM] This java doc is completely wrong now


Let's make sure we address this :)

jaymode · 2018-05-16T14:41:51Z

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

-    public Collection<? extends Setting<?>> getAllSettings() {
-        return allSettings;
+    /***
+     * TODO: this is horrible


can you elaborate?

I think the comment must have been for a previous version of that method. The current implementation isn't pretty, but it's no longer horrible.

jaymode · 2018-05-16T14:42:26Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -548,6 +549,7 @@ static Settings additionalSettings(final Settings settings, final boolean enable
            return settingsList;
        }

+


nit: unnecessary line

jaymode · 2018-05-16T15:01:33Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/InternalRealms.java

@@ -111,11 +111,14 @@ static boolean isStandardRealm(String type) {
    private InternalRealms() {
    }

+    /**
+     * TODO REALM-SETTINGS[TIM] This can be redone a lot now the realm settings are keyed by type


should we go ahead and redo this here?

jaymode · 2018-05-16T15:08:19Z

...src/main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java

@@ -453,7 +453,9 @@ protected void doRun() throws Exception {
                        public void onFailure(Exception e) {
                            IOUtils.closeWhileHandlingException(searchConnection);
                            listener.onFailure(e);
-                        };
+                        }


unnecessary change

jaymode · 2018-06-13T15:40:21Z

@tvernum what's the state of this? Please let me know if you're ready for review again.

jaymode · 2018-06-21T15:37:55Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmConfig.java

+        return globalSettings().getByPrefix(sslPrefix);
+    }
+
+    public static class RealmIdentifier {


can you add javadocs about this class?

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmConfig.java

jaymode · 2018-06-21T15:40:36Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/RealmSettings.java

-        final Map<String, Set<Setting<?>>> childSettings = getSettingsByRealm(extensions);
-        childSettings.forEach(RealmSettings::verify);
-        return validator(childSettings);
+    public static Settings get(Settings settings) {


The method name here is a bit ambiguous to me; maybe extract or something?

jaymode · 2018-06-21T15:44:35Z

...ain/java/org/elasticsearch/xpack/core/security/authc/ldap/PoolingSessionFactorySettings.java

+    }
+
+    public static Set<Setting.AffixSetting<?>> getSettings(String realmType) {
+        return Sets.newHashSet(


use Stream.of instead of Sets.newHashSet

jaymode · 2018-06-21T18:30:43Z

...c/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java

                    .build();
        } else {
            cache = null;
        }
    }

+    private <T> T setting(Function<String, Setting.AffixSetting<T>> settingFunction) {


can you add javadocs for this method

jaymode · 2018-06-21T18:31:14Z

x-pack/plugin/security/src/test/java/org/elasticsearch/test/SecuritySettingsSource.java

@@ -323,6 +324,17 @@ public static void addSecureSettings(Settings.Builder builder, Consumer<MockSecu
        }
    }

+    public static <T> String getSettingKey(Function<String, Setting.AffixSetting<T>> settingFactory,


add javadocs for these methods

tvernum · 2018-07-26T03:28:51Z

I've picked this up again and want to take this quiet period between 6.4 changes and 6.5 changes to get it merged.

I've just brought it back up to date with master, and will tackle outstanding review comments.

tvernum · 2018-11-08T09:34:31Z

To close this out, I've now raised PRs for all the refactors I want to get done on this on master.

I may still raise a change for 6.x to backport a few of the new methods from RealmSettings and RealmConfig so that it will be easier to do backports of features.

We changed the way realm settings are defined, and this affects custom realms in SecurityExtensions. This change adds those details to the breaking changes docs. Relates: #30241

Many realm tests were written to use separate setting objects for "global settings" and "realm settings". Since #30241 there is no distinction between these settings, so these tests can be cleaned up to use a single Settings object.

This removes an obsolete constructor that was still being called from some tests. Relates: elastic#30241

In elastic#30241 Realm settings were changed, but the Kerberos realm settings were not registered correctly. This change fixes the registration of those Kerberos settings. Also adds a new integration test that ensures every internal realm can be configured in a test cluster. Also fixes the QA test for kerberos. Resolves: elastic#35942

In #30241 Realm settings were changed, but the Kerberos realm settings were not registered correctly. This change fixes the registration of those Kerberos settings. Also adds a new integration test that ensures every internal realm can be configured in a test cluster. Also fixes the QA test for kerberos. Resolves: #35942

Realm settings were changed in elastic#30241 in a non-BWC way. If you try and start a 7.x node, using a 6.x config style, then the default error messages do not adquately describe the cause of the problem or the solution. This change detects the when realms are using the 6.x style and fails with a specific error message. This detection is a best-effort, and will detect issues when the realms have not be modified to use the 7.x style, but may not detect situations where the configuration was partially changed. e.g. We can detect this: xpack.security.authc: realms.pki1.type: pki realms.pki1.order: 3 realms.pki1.ssl.certificate_authorities: [ "ca.crt" ] But this (where the "order" has been updated, but the "ssl.*" has not) will fall back to the standard "unknown setting" check xpack.security.authc: realms.pki.pki1.order: 3 realms.pki1.ssl.certificate_authorities: [ "ca.crt" ] Closes: elastic#36026

Realm settings were changed in #30241 in a non-BWC way. If you try and start a 7.x node using a 6.x config style, then the default error messages do not adequately describe the cause of the problem, or the solution. This change detects the when realms are using the 6.x style and fails with a specific error message. This detection is a best-effort, and will detect issues when the realms have not been modified to use the 7.x style, but may not detect situations where the configuration was partially changed. e.g. We can detect this: xpack.security.authc: realms.pki1.type: pki realms.pki1.order: 3 realms.pki1.ssl.certificate_authorities: [ "ca.crt" ] But this (where the "order" has been updated, but the "ssl.*" has not) will fall back to the standard "unknown setting" check xpack.security.authc: realms.pki.pki1.order: 3 realms.pki1.ssl.certificate_authorities: [ "ca.crt" ] Closes: #36026

As part of elastic#30241 realm settings were changed to be true affix settings. In the process of this change, the "ssl." prefix was lost from the realm truststore password. It should be: xpack.security.authc.realms.<type>.<name>.ssl.truststore.password Due to a mismatch between the way we define SSL settings and load SSL contexts, there was no way to define this legacy password setting in a realm config. The settings validation would reject "ssl.truststore.password" but the SSL service would ignore "truststore.password" Resolves: elastic#41663

As part of #30241 realm settings were changed to be true affix settings. In the process of this change, the "ssl." prefix was lost from the realm truststore password. It should be: xpack.security.authc.realms.<type>.<name>.ssl.truststore.password Due to a mismatch between the way we define SSL settings and load SSL contexts, there was no way to define this legacy password setting in a realm config. The settings validation would reject "ssl.truststore.password" but the SSL service would ignore "truststore.password" Resolves: #41663

As part of elastic#30241 realm settings were changed to be true affix settings. In the process of this change, the "ssl." prefix was lost from the realm truststore password. It should be: xpack.security.authc.realms.<type>.<name>.ssl.truststore.password Due to a mismatch between the way we define SSL settings and load SSL contexts, there was no way to define this legacy password setting in a realm config. The settings validation would reject "ssl.truststore.password" but the SSL service would ignore "truststore.password" Resolves: elastic#41663 Backport of: elastic#42336

As part of #30241 realm settings were changed to be true affix settings. In the process of this change, the "ssl." prefix was lost from the realm truststore password. It should be: xpack.security.authc.realms.<type>.<name>.ssl.truststore.password Due to a mismatch between the way we define SSL settings and load SSL contexts, there was no way to define this legacy password setting in a realm config. The settings validation would reject "ssl.truststore.password" but the SSL service would ignore "truststore.password" Backport of: #42336

As part of elastic#30241 realm settings were changed to be true affix settings. In the process of this change, the "ssl." prefix was lost from the realm truststore password. It should be: xpack.security.authc.realms.<type>.<name>.ssl.truststore.password Due to a mismatch between the way we define SSL settings and load SSL contexts, there was no way to define this legacy password setting in a realm config. The settings validation would reject "ssl.truststore.password" but the SSL service would ignore "truststore.password" Resolves: elastic#41663

tvernum added >breaking v7.0.0 >refactoring :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Apr 30, 2018

tvernum requested review from jaymode and albertzaharovits April 30, 2018 03:51

Merge branch 'master' into sane-realm-settings

d05e496

hub-cap added :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) and removed :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Apr 30, 2018

Merge branch 'master' into sane-realm-settings

3e4555b

jaymode reviewed May 16, 2018

View reviewed changes

tvernum added 3 commits June 6, 2018 16:30

Merge branch 'master' into sane-realm-settings

20115d3

Clean up outstanding TODOs

d8173c2

Update new SAML test to settings change

cda4ef6

tvernum added 2 commits June 18, 2018 16:51

Merge branch 'master' into sane-realm-settings

1f56cf7

Fix merge / imports

fb19c22

jaymode requested changes Jun 21, 2018

View reviewed changes

jaymode mentioned this pull request Jun 25, 2018

Add warning on unsupported settings to elasticsearch keystore command #31489

Closed

Merge branch 'master' into sane-realm-settings

492beb7

tvernum added 7 commits July 26, 2018 17:35

Merge branch 'master' into sane-realm-settings

8a1f883

Merge branch 'master' into sane-realm-settings

f688fbe

Merge branch 'master' into sane-realm-settings

504688a

Remove production uses of deprecated constructor

9eddbad

Fix OpenLDAP QA tests

62f70f1

Merge branch 'master' into sane-realm-settings

7971a6d

Merge branch 'master' into sane-realm-settings

e22ef0d

tvernum mentioned this pull request Nov 8, 2018

Refactor realm tests to use single settings #35362

Merged

pgomulka pushed a commit to pgomulka/elasticsearch that referenced this pull request Nov 13, 2018

Remove deprecated RealmConfig constructor (elastic#35327)

a5a0db5

This removes an obsolete constructor that was still being called from some tests. Relates: elastic#30241

This was referenced Nov 28, 2018

Fix kerberos setting registration #35986

Merged

Improve error message if realms are in old format #36026

Closed

This was referenced Dec 14, 2018

Update Deprecation Info API for 7.0 breaking changes #36024

Closed

Deprecation check for Auth realm setting structure #36664

Merged

tvernum mentioned this pull request Dec 20, 2018

Improve error message for 6.x style realm settings #36876

Merged

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

albertzaharovits mentioned this pull request Apr 24, 2019

Add the capability for the LDAP and AD realms to bind using Kerberos credentials #41126

Closed

tvernum mentioned this pull request May 22, 2019

Fix settings prefix for realm truststore password #42336

Merged

tvernum mentioned this pull request May 23, 2019

[Backport 7.x] Fix settings prefix for realm truststore password #42413

Merged

tvernum mentioned this pull request May 23, 2019

[Backport 7.2] Fix settings prefix for realm truststore password #42415

Merged

codebrain mentioned this pull request Aug 5, 2019

[meta] 7.2 Release elastic/elasticsearch-net#3980

Closed

37 tasks

jkakavas mentioned this pull request Aug 22, 2019

metadata setting is not available for Active Directory realms #45848

Closed

lcawl mentioned this pull request Dec 9, 2019

[DOCS] Removes realm type security setting #50001

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include realm type in Security Realm setting keys #30241

Include realm type in Security Realm setting keys #30241

tvernum commented Apr 30, 2018

elasticmachine commented Apr 30, 2018

jaymode left a comment

jaymode May 16, 2018

jaymode May 16, 2018

jaymode May 16, 2018

jaymode May 16, 2018

tvernum Jun 6, 2018

jaymode May 16, 2018

jaymode May 16, 2018

jaymode May 16, 2018

jaymode commented Jun 13, 2018

jaymode Jun 21, 2018

jaymode Jun 21, 2018

jaymode Jun 21, 2018

jaymode Jun 21, 2018

jaymode Jun 21, 2018

tvernum commented Jul 26, 2018

tvernum commented Nov 8, 2018

		/**
		* TODO REALM-SETTINGS[TIM] This java doc is completely wrong now

		@@ -548,6 +549,7 @@ static Settings additionalSettings(final Settings settings, final boolean enable
		return settingsList;
		}

Include realm type in Security Realm setting keys #30241

Include realm type in Security Realm setting keys #30241

Conversation

tvernum commented Apr 30, 2018

elasticmachine commented Apr 30, 2018

jaymode left a 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

jaymode commented Jun 13, 2018

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 commented Jul 26, 2018

tvernum commented Nov 8, 2018