From 790ac1b2b1d151eb1a1e4db313d6ecf1181608c3 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Thu, 20 Dec 2018 13:13:27 +1100 Subject: [PATCH] Improve error message for 6.x style realm settings 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 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: #36026 --- .../xpack/core/ssl/SSLService.java | 19 ++- .../xpack/security/Security.java | 45 ++++++- .../xpack/security/SecurityTests.java | 118 +++++++++++------- 3 files changed, 127 insertions(+), 55 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java index 1a7641ef64b88..79ccaeaae1a9e 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java @@ -619,12 +619,19 @@ private static Map getRealmsSSLSettings(Settings settings) { final Map sslSettings = new HashMap<>(); final String prefix = "xpack.security.authc.realms."; final Map settingsByRealmType = settings.getGroups(prefix); - settingsByRealmType.forEach((realmType, typeSettings) -> - typeSettings.getAsGroups().forEach((realmName, realmSettings) -> { - Settings realmSSLSettings = realmSettings.getByPrefix("ssl."); - // Put this even if empty, so that the name will be mapped to the global SSL configuration - sslSettings.put(prefix + realmType + "." + realmName + ".ssl", realmSSLSettings); - }) + settingsByRealmType.forEach((realmType, typeSettings) -> { + final Optional nonDottedSetting = typeSettings.keySet().stream().filter(k -> k.indexOf('.') == -1).findAny(); + if (nonDottedSetting.isPresent()) { + logger.warn("Skipping any SSL configuration from realm [{}{}] because the key [{}] is not in the correct format", + prefix, realmType, nonDottedSetting.get()); + } else { + typeSettings.getAsGroups().forEach((realmName, realmSettings) -> { + Settings realmSSLSettings = realmSettings.getByPrefix("ssl."); + // Put this even if empty, so that the name will be mapped to the global SSL configuration + sslSettings.put(prefix + realmType + "." + realmName + ".ssl", realmSSLSettings); + }); + } + } ); return sslSettings; } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 3a735332a6b8b..ad9f1d7aa948c 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -115,6 +115,8 @@ import org.elasticsearch.xpack.core.security.authc.DefaultAuthenticationFailureHandler; import org.elasticsearch.xpack.core.security.authc.InternalRealmsSettings; import org.elasticsearch.xpack.core.security.authc.Realm; +import org.elasticsearch.xpack.core.security.authc.RealmConfig; +import org.elasticsearch.xpack.core.security.authc.RealmSettings; import org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken; import org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField; import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl; @@ -244,6 +246,7 @@ import java.util.function.Predicate; import java.util.function.Supplier; import java.util.function.UnaryOperator; +import java.util.stream.Collectors; import static java.util.Collections.emptyList; import static java.util.Collections.singletonList; @@ -296,7 +299,7 @@ public Security(Settings settings, final Path configPath) { this.env = transportClientMode ? null : new Environment(settings, configPath); this.enabled = XPackSettings.SECURITY_ENABLED.get(settings); if (enabled && transportClientMode == false) { - validateAutoCreateIndex(settings); + runStartupChecks(settings); // we load them all here otherwise we can't access secure settings since they are closed once the checks are // fetched final List checks = new ArrayList<>(); @@ -315,6 +318,12 @@ public Security(Settings settings, final Path configPath) { this.bootstrapChecks = Collections.emptyList(); } this.securityExtensions.addAll(extensions); + + } + + private static void runStartupChecks(Settings settings) { + validateAutoCreateIndex(settings); + validateRealmSettings(settings); } @Override @@ -781,6 +790,40 @@ public Map getProcessors(Processor.Parameters paramet return Collections.singletonMap(SetSecurityUserProcessor.TYPE, new SetSecurityUserProcessor.Factory(parameters.threadContext)); } + /** + * Realm settings were changed in 7.0. This method validates that the settings in use on this node match the new style of setting. + * In 6.x a realm config would be + * 
+     *   xpack.security.authc.realms.file1.type: file
+     *   xpack.security.authc.realms.file1.order: 0
+     *
+ * In 7.x this realm should be + * 
+     *   xpack.security.authc.realms.file.file1.order: 0
+     *
+ * If confronted with an old style config, the ES Settings validation would simply fail with an error such as + * unknown setting [xpack.security.authc.realms.file1.order]. This validation method provides an error that is easier to + * understand and take action on. + */ + static void validateRealmSettings(Settings settings) { + final Set badRealmSettings = settings.keySet().stream() + .filter(k -> k.startsWith(RealmSettings.PREFIX)) + .filter(key -> { + final String suffix = key.substring(RealmSettings.PREFIX.length()); + // suffix-part, only contains a single '.' + return suffix.indexOf('.') == suffix.lastIndexOf('.'); + }) + .collect(Collectors.toSet()); + if (badRealmSettings.isEmpty() == false) { + String sampleRealmSetting = RealmSettings.realmSettingPrefix(new RealmConfig.RealmIdentifier("file", "my_file")) + "order"; + throw new IllegalArgumentException("Incorrect realm settings found. " + + "Realm settings have been changed to include the type as part of the setting key.\n" + + "For example '" + sampleRealmSetting + "'\n" + + "Found invalid config: " + Strings.collectionToDelimitedString(badRealmSettings, ", ") + "\n" + + "Please see the breaking changes documentation." + ); + } + } static boolean indexAuditLoggingEnabled(Settings settings) { if (XPackSettings.AUDIT_ENABLED.get(settings)) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java index e9924b9d85245..8674a5b295085 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java @@ -35,6 +35,7 @@ import org.elasticsearch.xpack.core.security.SecurityExtension; import org.elasticsearch.xpack.core.security.SecurityField; import org.elasticsearch.xpack.core.security.authc.Realm; +import org.elasticsearch.xpack.core.security.authc.RealmSettings; import org.elasticsearch.xpack.core.security.authc.file.FileRealmSettings; import org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField; import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl; @@ -66,8 +67,8 @@ import static org.elasticsearch.cluster.metadata.IndexMetaData.INDEX_FORMAT_SETTING; import static org.elasticsearch.discovery.DiscoveryModule.ZEN2_DISCOVERY_TYPE; import static org.elasticsearch.discovery.DiscoveryModule.ZEN_DISCOVERY_TYPE; -import static org.elasticsearch.xpack.security.support.SecurityIndexManager.SECURITY_INDEX_NAME; import static org.elasticsearch.xpack.security.support.SecurityIndexManager.INTERNAL_INDEX_FORMAT; +import static org.elasticsearch.xpack.security.support.SecurityIndexManager.SECURITY_INDEX_NAME; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; @@ -99,9 +100,9 @@ private Collection createComponents(Settings testSettings, SecurityExten throw new IllegalStateException("Security object already exists (" + security + ")"); } Settings settings = Settings.builder() - .put("xpack.security.enabled", true) - .put(testSettings) - .put("path.home", createTempDir()).build(); + .put("xpack.security.enabled", true) + .put(testSettings) + .put("path.home", createTempDir()).build(); Environment env = TestEnvironment.newEnvironment(settings); licenseState = new TestUtils.UpdatableLicenseState(settings); SSLService sslService = new SSLService(settings, env); @@ -159,7 +160,7 @@ public void testCustomRealmExtension() throws Exception { public void testCustomRealmExtensionConflict() throws Exception { IllegalArgumentException e = expectThrows(IllegalArgumentException.class, - () -> createComponents(Settings.EMPTY, new DummyExtension(FileRealmSettings.TYPE))); + () -> createComponents(Settings.EMPTY, new DummyExtension(FileRealmSettings.TYPE))); assertEquals("Realm type [" + FileRealmSettings.TYPE + "] is already registered", e.getMessage()); } @@ -181,8 +182,8 @@ public void testDisabledByDefault() throws Exception { public void testIndexAuditTrail() throws Exception { Settings settings = Settings.builder() - .put(XPackSettings.AUDIT_ENABLED.getKey(), true) - .put(Security.AUDIT_OUTPUTS_SETTING.getKey(), "index").build(); + .put(XPackSettings.AUDIT_ENABLED.getKey(), true) + .put(Security.AUDIT_OUTPUTS_SETTING.getKey(), "index").build(); Collection components = createComponents(settings); AuditTrailService service = findComponent(AuditTrailService.class, components); assertNotNull(service); @@ -192,8 +193,8 @@ public void testIndexAuditTrail() throws Exception { public void testIndexAndLoggingAuditTrail() throws Exception { Settings settings = Settings.builder() - .put(XPackSettings.AUDIT_ENABLED.getKey(), true) - .put(Security.AUDIT_OUTPUTS_SETTING.getKey(), "index,logfile").build(); + .put(XPackSettings.AUDIT_ENABLED.getKey(), true) + .put(Security.AUDIT_OUTPUTS_SETTING.getKey(), "index,logfile").build(); Collection components = createComponents(settings); AuditTrailService service = findComponent(AuditTrailService.class, components); assertNotNull(service); @@ -204,8 +205,8 @@ public void testIndexAndLoggingAuditTrail() throws Exception { public void testUnknownOutput() { Settings settings = Settings.builder() - .put(XPackSettings.AUDIT_ENABLED.getKey(), true) - .put(Security.AUDIT_OUTPUTS_SETTING.getKey(), "foo").build(); + .put(XPackSettings.AUDIT_ENABLED.getKey(), true) + .put(Security.AUDIT_OUTPUTS_SETTING.getKey(), "foo").build(); IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> createComponents(settings)); assertEquals("Unknown audit trail output [foo]", e.getMessage()); } @@ -218,9 +219,9 @@ public void testHttpSettingDefaults() throws Exception { public void testTransportSettingNetty4Both() { Settings both4 = Security.additionalSettings(Settings.builder() - .put(NetworkModule.TRANSPORT_TYPE_KEY, SecurityField.NAME4) - .put(NetworkModule.HTTP_TYPE_KEY, SecurityField.NAME4) - .build(), true, false); + .put(NetworkModule.TRANSPORT_TYPE_KEY, SecurityField.NAME4) + .put(NetworkModule.HTTP_TYPE_KEY, SecurityField.NAME4) + .build(), true, false); assertFalse(NetworkModule.TRANSPORT_TYPE_SETTING.exists(both4)); assertFalse(NetworkModule.HTTP_TYPE_SETTING.exists(both4)); } @@ -229,13 +230,13 @@ public void testTransportSettingValidation() { final String badType = randomFrom("netty4", "other", "security1"); Settings settingsTransport = Settings.builder().put(NetworkModule.TRANSPORT_TYPE_KEY, badType).build(); IllegalArgumentException badTransport = expectThrows(IllegalArgumentException.class, - () -> Security.additionalSettings(settingsTransport, true, false)); + () -> Security.additionalSettings(settingsTransport, true, false)); assertThat(badTransport.getMessage(), containsString(SecurityField.NAME4)); assertThat(badTransport.getMessage(), containsString(NetworkModule.TRANSPORT_TYPE_KEY)); Settings settingsHttp = Settings.builder().put(NetworkModule.HTTP_TYPE_KEY, badType).build(); IllegalArgumentException badHttp = expectThrows(IllegalArgumentException.class, - () -> Security.additionalSettings(settingsHttp, true, false)); + () -> Security.additionalSettings(settingsHttp, true, false)); assertThat(badHttp.getMessage(), containsString(SecurityField.NAME4)); assertThat(badHttp.getMessage(), containsString(NetworkModule.HTTP_TYPE_KEY)); } @@ -249,21 +250,21 @@ public void testSettingFilter() throws Exception { public void testFilteredSettings() throws Exception { createComponents(Settings.EMPTY); final List> realmSettings = security.getSettings().stream() - .filter(s -> s.getKey().startsWith("xpack.security.authc.realms")) - .collect(Collectors.toList()); + .filter(s -> s.getKey().startsWith("xpack.security.authc.realms")) + .collect(Collectors.toList()); Arrays.asList( - "bind_dn", "bind_password", - "hostname_verification", - "truststore.password", "truststore.path", "truststore.algorithm", - "keystore.key_password").forEach(suffix -> { + "bind_dn", "bind_password", + "hostname_verification", + "truststore.password", "truststore.path", "truststore.algorithm", + "keystore.key_password").forEach(suffix -> { final List> matching = realmSettings.stream() - .filter(s -> s.getKey().endsWith("." + suffix)) - .collect(Collectors.toList()); + .filter(s -> s.getKey().endsWith("." + suffix)) + .collect(Collectors.toList()); assertThat("For suffix " + suffix, matching, Matchers.not(empty())); matching.forEach(setting -> assertThat("For setting " + setting, - setting.getProperties(), Matchers.hasItem(Setting.Property.Filtered))); + setting.getProperties(), Matchers.hasItem(Setting.Property.Filtered))); }); } @@ -290,7 +291,7 @@ public void testTLSJoinValidator() throws Exception { TestUtils.putLicense(builder, license); ClusterState state = ClusterState.builder(ClusterName.DEFAULT).metaData(builder.build()).build(); EnumSet productionModes = EnumSet.of(License.OperationMode.GOLD, License.OperationMode.PLATINUM, - License.OperationMode.STANDARD); + License.OperationMode.STANDARD); if (productionModes.contains(license.operationMode()) && tlsOn == false && "single-node".equals(discoveryType) == false) { IllegalStateException ise = expectThrows(IllegalStateException.class, () -> validator.accept(node, state)); assertEquals("TLS setup is required for license type [" + license.operationMode().name() + "]", ise.getMessage()); @@ -340,18 +341,18 @@ public void testIndexJoinValidator_Old_And_Rolling() throws Exception { assertNotNull(joinValidator); DiscoveryNode node = new DiscoveryNode("foo", buildNewFakeTransportAddress(), Version.CURRENT); IndexMetaData indexMetaData = IndexMetaData.builder(SECURITY_INDEX_NAME) - .settings(settings(Version.V_6_1_0).put(INDEX_FORMAT_SETTING.getKey(), INTERNAL_INDEX_FORMAT - 1)) - .numberOfShards(1).numberOfReplicas(0) - .build(); + .settings(settings(Version.V_6_1_0).put(INDEX_FORMAT_SETTING.getKey(), INTERNAL_INDEX_FORMAT - 1)) + .numberOfShards(1).numberOfReplicas(0) + .build(); DiscoveryNode existingOtherNode = new DiscoveryNode("bar", buildNewFakeTransportAddress(), Version.V_6_1_0); DiscoveryNodes discoveryNodes = DiscoveryNodes.builder().add(existingOtherNode).build(); ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT) - .nodes(discoveryNodes) - .metaData(MetaData.builder().put(indexMetaData, true).build()).build(); + .nodes(discoveryNodes) + .metaData(MetaData.builder().put(indexMetaData, true).build()).build(); IllegalStateException e = expectThrows(IllegalStateException.class, - () -> joinValidator.accept(node, clusterState)); + () -> joinValidator.accept(node, clusterState)); assertThat(e.getMessage(), equalTo("Security index is not on the current version [6] - " + - "The Upgrade API must be run for 7.x nodes to join the cluster")); + "The Upgrade API must be run for 7.x nodes to join the cluster")); } public void testIndexJoinValidator_FullyCurrentCluster() throws Exception { @@ -361,14 +362,14 @@ public void testIndexJoinValidator_FullyCurrentCluster() throws Exception { DiscoveryNode node = new DiscoveryNode("foo", buildNewFakeTransportAddress(), Version.CURRENT); int indexFormat = randomBoolean() ? INTERNAL_INDEX_FORMAT : INTERNAL_INDEX_FORMAT - 1; IndexMetaData indexMetaData = IndexMetaData.builder(SECURITY_INDEX_NAME) - .settings(settings(Version.V_6_1_0).put(INDEX_FORMAT_SETTING.getKey(), indexFormat)) - .numberOfShards(1).numberOfReplicas(0) - .build(); + .settings(settings(Version.V_6_1_0).put(INDEX_FORMAT_SETTING.getKey(), indexFormat)) + .numberOfShards(1).numberOfReplicas(0) + .build(); DiscoveryNode existingOtherNode = new DiscoveryNode("bar", buildNewFakeTransportAddress(), Version.CURRENT); DiscoveryNodes discoveryNodes = DiscoveryNodes.builder().add(existingOtherNode).build(); ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT) - .nodes(discoveryNodes) - .metaData(MetaData.builder().put(indexMetaData, true).build()).build(); + .nodes(discoveryNodes) + .metaData(MetaData.builder().put(indexMetaData, true).build()).build(); joinValidator.accept(node, clusterState); } @@ -379,14 +380,14 @@ public void testIndexUpgradeValidatorWithUpToDateIndex() throws Exception { Version version = randomBoolean() ? Version.CURRENT : Version.V_6_1_0; DiscoveryNode node = new DiscoveryNode("foo", buildNewFakeTransportAddress(), Version.CURRENT); IndexMetaData indexMetaData = IndexMetaData.builder(SECURITY_INDEX_NAME) - .settings(settings(version).put(INDEX_FORMAT_SETTING.getKey(), INTERNAL_INDEX_FORMAT)) - .numberOfShards(1).numberOfReplicas(0) - .build(); + .settings(settings(version).put(INDEX_FORMAT_SETTING.getKey(), INTERNAL_INDEX_FORMAT)) + .numberOfShards(1).numberOfReplicas(0) + .build(); DiscoveryNode existingOtherNode = new DiscoveryNode("bar", buildNewFakeTransportAddress(), version); DiscoveryNodes discoveryNodes = DiscoveryNodes.builder().add(existingOtherNode).build(); ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT) - .nodes(discoveryNodes) - .metaData(MetaData.builder().put(indexMetaData, true).build()).build(); + .nodes(discoveryNodes) + .metaData(MetaData.builder().put(indexMetaData, true).build()).build(); joinValidator.accept(node, clusterState); } @@ -398,7 +399,7 @@ public void testIndexUpgradeValidatorWithMissingIndex() throws Exception { DiscoveryNode existingOtherNode = new DiscoveryNode("bar", buildNewFakeTransportAddress(), Version.V_6_1_0); DiscoveryNodes discoveryNodes = DiscoveryNodes.builder().add(existingOtherNode).build(); ClusterState clusterState = ClusterState.builder(ClusterName.DEFAULT) - .nodes(discoveryNodes).build(); + .nodes(discoveryNodes).build(); joinValidator.accept(node, clusterState); } @@ -409,15 +410,15 @@ public void testGetFieldFilterSecurityEnabled() throws Exception { Map permissionsMap = new HashMap<>(); FieldPermissions permissions = new FieldPermissions( - new FieldPermissionsDefinition(new String[]{"field_granted"}, Strings.EMPTY_ARRAY)); + new FieldPermissionsDefinition(new String[] { "field_granted" }, Strings.EMPTY_ARRAY)); IndicesAccessControl.IndexAccessControl indexGrantedAccessControl = new IndicesAccessControl.IndexAccessControl(true, permissions, - Collections.emptySet()); + Collections.emptySet()); permissionsMap.put("index_granted", indexGrantedAccessControl); IndicesAccessControl.IndexAccessControl indexAccessControl = new IndicesAccessControl.IndexAccessControl(false, - FieldPermissions.DEFAULT, Collections.emptySet()); + FieldPermissions.DEFAULT, Collections.emptySet()); permissionsMap.put("index_not_granted", indexAccessControl); IndicesAccessControl.IndexAccessControl nullFieldPermissions = - new IndicesAccessControl.IndexAccessControl(true, null, Collections.emptySet()); + new IndicesAccessControl.IndexAccessControl(true, null, Collections.emptySet()); permissionsMap.put("index_null", nullFieldPermissions); IndicesAccessControl index = new IndicesAccessControl(true, permissionsMap); threadContext.putTransient(AuthorizationServiceField.INDICES_PERMISSIONS_KEY, index); @@ -446,4 +447,25 @@ public void testGetFieldFilterSecurityEnabledLicenseNoFLS() throws Exception { assertNotSame(MapperPlugin.NOOP_FIELD_FILTER, fieldFilter); assertSame(MapperPlugin.NOOP_FIELD_PREDICATE, fieldFilter.apply(randomAlphaOfLengthBetween(3, 6))); } + + public void testValidateRealmsWhenSettingsAreInvalid() { + final Settings settings = Settings.builder() + .put(RealmSettings.PREFIX + "my_pki.type", "pki") + .put(RealmSettings.PREFIX + "ldap1.type", "ldap") + .build(); + final IllegalArgumentException iae = expectThrows(IllegalArgumentException.class, () -> Security.validateRealmSettings(settings)); + assertThat(iae.getMessage(), containsString("Incorrect realm settings")); + assertThat(iae.getMessage(), containsString("breaking changes doc")); + assertThat(iae.getMessage(), containsString(RealmSettings.PREFIX + "my_pki.type")); + assertThat(iae.getMessage(), containsString(RealmSettings.PREFIX + "ldap1.type")); + } + + public void testValidateRealmsWhenSettingsAreCorrect() { + final Settings settings = Settings.builder() + .put(RealmSettings.PREFIX + "pki.my_pki.order", 0) + .put(RealmSettings.PREFIX + "ldap.ldap1.order", 1) + .build(); + Security.validateRealmSettings(settings); + // no-exception + } }