From c287d89bea1b2158c9eb11a224967033ce87308f Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Fri, 24 Aug 2018 12:11:49 +1000 Subject: [PATCH 1/6] [SECURITY] Auth scheme preference Some browsers (eg. Firefox) behave differently when presented with multiple auth schemes in 'WWW-Authenticate' header. The expected behavior is that browser select the most secure auth scheme before trying others, but Firefox selects the first presented auth scheme and tries the next one's sequentially. As the interpretation is something we do not control, we can at least present the auth schemes in most to least secure order as server's preference. This commit adds the changes to collect and sort the auth schemes presented by most to least secure. We support 'negotiate', 'bearer' and 'basic' schemes and the order is hardcoded with no customization. If there are user requests we will make this configurable. Unit test to verify the `WWW-Authenticate` header values are sorted by server preference as more secure to least secure auth schemes. Testing done with Firefox, Chrome, Internet Explorer 11. Closes#32699 --- .../xpack/security/Security.java | 52 ++++++++++++++++--- .../xpack/security/SecurityTests.java | 49 +++++++++++++++-- 2 files changed, 90 insertions(+), 11 deletions(-) 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 2a392478cbcd9..68a6790243365 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 @@ -236,11 +236,13 @@ import java.util.Locale; import java.util.Map; import java.util.Set; +import java.util.TreeMap; import java.util.function.BiConsumer; import java.util.function.Function; 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; @@ -512,23 +514,39 @@ private AuthenticationFailureHandler createAuthenticationFailureHandler(final Re if (failureHandler == null) { logger.debug("Using default authentication failure handler"); final Map> defaultFailureResponseHeaders = new HashMap<>(); + final Map> sortedWWWAuthenticateHeaderValues = new TreeMap<>(); realms.asList().stream().forEach((realm) -> { Map> realmFailureHeaders = realm.getAuthenticationFailureHeaders(); realmFailureHeaders.entrySet().stream().forEach((e) -> { String key = e.getKey(); - e.getValue().stream() - .filter(v -> defaultFailureResponseHeaders.computeIfAbsent(key, x -> new ArrayList<>()).contains(v) == false) - .forEach(v -> defaultFailureResponseHeaders.get(key).add(v)); + if (key.equalsIgnoreCase("WWW-Authenticate")) { + /* + * Some browsers (eg. Firefox) behave differently when presented with multiple + * auth schemes in 'WWW-Authenticate' header. The expected behavior is that browser + * select the most secure auth scheme before trying others, but Firefox selects the + * first presented auth scheme and tries the next one's sequentially. As the + * interpretation is something we do not control, we can at least present + * the auth schemes in most to least secure order as server's preference. + */ + e.getValue().stream() + .forEach(v -> { + sortedWWWAuthenticateHeaderValues.computeIfAbsent(authSchemePriority(v), t -> new HashSet<>()).add(v); + }); + } else { + e.getValue().stream() + .filter(v -> defaultFailureResponseHeaders.computeIfAbsent(key, x -> new ArrayList<>()) + .contains(v) == false) + .forEach(v -> defaultFailureResponseHeaders.get(key).add(v)); + } }); }); if (TokenService.isTokenServiceEnabled(settings)) { String bearerScheme = "Bearer realm=\"" + XPackField.SECURITY + "\""; - if (defaultFailureResponseHeaders.computeIfAbsent("WWW-Authenticate", x -> new ArrayList<>()) - .contains(bearerScheme) == false) { - defaultFailureResponseHeaders.get("WWW-Authenticate").add(bearerScheme); - } + sortedWWWAuthenticateHeaderValues.computeIfAbsent(authSchemePriority(bearerScheme), v -> new HashSet<>()).add(bearerScheme); } + defaultFailureResponseHeaders.put("WWW-Authenticate", + sortedWWWAuthenticateHeaderValues.values().stream().flatMap(coll -> coll.stream()).collect(Collectors.toList())); failureHandler = new DefaultAuthenticationFailureHandler(defaultFailureResponseHeaders); } else { logger.debug("Using authentication failure handler from extension [" + extensionName + "]"); @@ -536,6 +554,26 @@ private AuthenticationFailureHandler createAuthenticationFailureHandler(final Re return failureHandler; } + /** + * For given 'WWW-Authenticate' header value returns the priority based on + * the auth-scheme. Lower number denotes more secure and preferred + * auth-scheme than the higher number. + * + * @param headerValue string starting with auth-scheme name + * @return integer value denoting priority for given auth scheme. + */ + private static Integer authSchemePriority(final String headerValue) { + if (headerValue.regionMatches(true, 0, "negotiate", 0, "negotiate".length())) { + return 0; + } else if (headerValue.regionMatches(true, 0, "bearer", 0, "bearer".length())) { + return 1; + } else if (headerValue.regionMatches(true, 0, "basic", 0, "basic".length())) { + return 2; + } else { + return 3; + } + } + @Override public Settings additionalSettings() { return additionalSettings(settings, enabled, transportClientMode); 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 857b1694ac87a..00c6e29a1455f 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 @@ -5,7 +5,9 @@ */ package org.elasticsearch.xpack.security; +import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.Version; +import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; @@ -21,19 +23,23 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; import org.elasticsearch.license.License; import org.elasticsearch.license.TestUtils; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.plugins.MapperPlugin; +import org.elasticsearch.rest.RestRequest; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.VersionUtils; +import org.elasticsearch.test.rest.FakeRestRequest; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.watcher.ResourceWatcherService; import org.elasticsearch.xpack.core.XPackSettings; import org.elasticsearch.xpack.core.security.SecurityExtension; import org.elasticsearch.xpack.core.security.SecurityField; +import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authc.Realm; import org.elasticsearch.xpack.core.security.authc.file.FileRealmSettings; import org.elasticsearch.xpack.core.security.authc.ldap.support.SessionFactorySettings; @@ -45,10 +51,15 @@ import org.elasticsearch.xpack.security.audit.AuditTrailService; import org.elasticsearch.xpack.security.audit.index.IndexAuditTrail; import org.elasticsearch.xpack.security.audit.logfile.LoggingAuditTrail; +import org.elasticsearch.xpack.security.authc.AuthenticationService; import org.elasticsearch.xpack.security.authc.Realms; +import org.elasticsearch.xpack.security.authc.kerberos.KerberosTestCase; import org.junit.Before; import java.io.IOException; +import java.net.InetSocketAddress; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -66,6 +77,7 @@ 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.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; import static org.mockito.Mockito.mock; @@ -92,10 +104,13 @@ private Collection createComponents(Settings testSettings, SecurityExten if (security != null) { throw new IllegalStateException("Security object already exists (" + security + ")"); } - Settings settings = Settings.builder() - .put("xpack.security.enabled", true) - .put(testSettings) - .put("path.home", createTempDir()).build(); + Settings.Builder settingsBuilder = Settings.builder(); + settingsBuilder.put("xpack.security.enabled", true); + settingsBuilder.put(testSettings); + if (Strings.hasText(settingsBuilder.get("path.home")) == false) { + settingsBuilder.put("path.home", createTempDir()); + } + Settings settings = settingsBuilder.build(); Environment env = TestEnvironment.newEnvironment(settings); licenseState = new TestUtils.UpdatableLicenseState(settings); SSLService sslService = new SSLService(settings, env); @@ -424,4 +439,30 @@ public void testGetFieldFilterSecurityEnabledLicenseNoFLS() throws Exception { assertNotSame(MapperPlugin.NOOP_FIELD_FILTER, fieldFilter); assertSame(MapperPlugin.NOOP_FIELD_PREDICATE, fieldFilter.apply(randomAlphaOfLengthBetween(3, 6))); } + + public void testDefaultFailureHandlerFailedAuthenticationRespondsWithSortedWWWAuthenticateHeaderValues() throws Exception { + final Path temp = createTempDir(); + final Path configDir = Files.createDirectories(temp.resolve("config")); + KerberosTestCase.writeKeyTab(configDir.resolve("es.keytab"), null); + final Settings settings = Settings.builder() + .put("xpack.security.authc.token.enabled", "true") + .put("xpack.security.authc.realms.kerb1.type", "kerberos") + .put("xpack.security.authc.realms.kerb1.order", "2") + .put("xpack.security.authc.realms.kerb1.keytab.path", "es.keytab") + .put("path.home", temp).build(); + final Collection components = createComponents(settings); + + final AuthenticationService authcService = findComponent(AuthenticationService.class, components); + final RestRequest restRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withRemoteAddress(mock( + InetSocketAddress.class)).build(); + + final ElasticsearchSecurityException e = expectThrows(ElasticsearchSecurityException.class, () -> { + PlainActionFuture future = new PlainActionFuture<>(); + authcService.authenticate(restRequest, future); + future.actionGet(); + }); + assertThat(e.getHeader("WWW-Authenticate"), contains(containsString("Negotiate"), containsString("Bearer"), containsString( + "Basic"))); + } + } From ce1011890ae8a8be50c0b20945c5778609e39d06 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Wed, 29 Aug 2018 10:35:26 +1000 Subject: [PATCH 2/6] Address review comment. Moved the logic to sort auth schemes in the failure handler constructor and the test to failure handler tests. --- .../DefaultAuthenticationFailureHandler.java | 40 ++++++++++++-- ...aultAuthenticationFailureHandlerTests.java | 20 +++++++ .../xpack/security/Security.java | 52 +++---------------- .../xpack/security/SecurityTests.java | 49 ++--------------- 4 files changed, 68 insertions(+), 93 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java index d6f678a2dcb90..b59a5a63a3850 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java @@ -12,9 +12,12 @@ import org.elasticsearch.transport.TransportMessage; import org.elasticsearch.xpack.core.XPackField; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Map.Entry; +import java.util.stream.Collectors; import static org.elasticsearch.xpack.core.security.support.Exceptions.authenticationError; @@ -44,12 +47,43 @@ public DefaultAuthenticationFailureHandler() { * be sent as failure response. * @see Realm#getAuthenticationFailureHeaders() */ - public DefaultAuthenticationFailureHandler(Map> failureResponseHeaders) { + public DefaultAuthenticationFailureHandler(final Map> failureResponseHeaders) { if (failureResponseHeaders == null || failureResponseHeaders.isEmpty()) { - failureResponseHeaders = Collections.singletonMap("WWW-Authenticate", + this.defaultFailureResponseHeaders = Collections.singletonMap("WWW-Authenticate", Collections.singletonList("Basic realm=\"" + XPackField.SECURITY + "\" charset=\"UTF-8\"")); + } else { + this.defaultFailureResponseHeaders = Collections.unmodifiableMap(failureResponseHeaders.entrySet().stream().collect(Collectors + .toMap(entry -> entry.getKey(), entry -> { + if (entry.getKey().equalsIgnoreCase("WWW-Authenticate")) { + List values = new ArrayList<>(entry.getValue()); + Collections.sort(values, (o1, o2) -> authSchemePriority(o1).compareTo(authSchemePriority(o2))); + return Collections.unmodifiableList(values); + } else { + return Collections.unmodifiableList(entry.getValue()); + } + }))); + } + } + + /** + * For given 'WWW-Authenticate' header value returns the priority based on + * the auth-scheme. Lower number denotes more secure and preferred + * auth-scheme than the higher number. + * + * @param headerValue string starting with auth-scheme name + * @return integer value denoting priority for given auth scheme. + */ + // package scope for testing + static Integer authSchemePriority(final String headerValue) { + if (headerValue.regionMatches(true, 0, "negotiate", 0, "negotiate".length())) { + return 0; + } else if (headerValue.regionMatches(true, 0, "bearer", 0, "bearer".length())) { + return 1; + } else if (headerValue.regionMatches(true, 0, "basic", 0, "basic".length())) { + return 2; + } else { + return 3; } - this.defaultFailureResponseHeaders = Collections.unmodifiableMap(failureResponseHeaders); } @Override diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java index 2598461c37280..b8304cb9272bf 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java @@ -21,6 +21,7 @@ import java.util.List; import java.util.Map; +import static org.elasticsearch.xpack.core.security.authc.DefaultAuthenticationFailureHandler.authSchemePriority; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -107,8 +108,27 @@ public void testExceptionProcessingRequest() { } + public void testSortsWWWAuthenticateHeaderValues() { + final String basicAuthScheme = "Basic realm=\"" + XPackField.SECURITY + "\" charset=\"UTF-8\""; + final String bearerAuthScheme = "Bearer realm=\"" + XPackField.SECURITY + "\""; + final String negotiateAuthScheme = randomFrom("Negotiate", "Negotiate Ijoijksdk"); + final Map> failureResponeHeaders = new HashMap<>(); + failureResponeHeaders.put("WWW-Authenticate", Arrays.asList(basicAuthScheme, bearerAuthScheme, negotiateAuthScheme)); + final DefaultAuthenticationFailureHandler failuerHandler = new DefaultAuthenticationFailureHandler(failureResponeHeaders); + + final ElasticsearchSecurityException ese = failuerHandler.exceptionProcessingRequest(Mockito.mock(RestRequest.class), new Exception("other error"), + new ThreadContext(Settings.builder().build())); + + assertThat(ese, is(notNullValue())); + assertThat(ese.getHeader("WWW-Authenticate"), is(notNullValue())); + assertThat(ese.getMessage(), equalTo("error attempting to authenticate request")); + assertWWWAuthenticateWithSchemes(ese, basicAuthScheme, bearerAuthScheme, negotiateAuthScheme); + } + private void assertWWWAuthenticateWithSchemes(final ElasticsearchSecurityException ese, final String... schemes) { assertThat(ese.getHeader("WWW-Authenticate").size(), is(schemes.length)); + // Auth scheme are ordered as more secure to least secure + Arrays.sort(schemes, (o1, o2) -> authSchemePriority(o1).compareTo(authSchemePriority(o2))); assertThat(ese.getHeader("WWW-Authenticate"), contains(schemes)); } } 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 68a6790243365..2a392478cbcd9 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 @@ -236,13 +236,11 @@ import java.util.Locale; import java.util.Map; import java.util.Set; -import java.util.TreeMap; import java.util.function.BiConsumer; import java.util.function.Function; 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; @@ -514,39 +512,23 @@ private AuthenticationFailureHandler createAuthenticationFailureHandler(final Re if (failureHandler == null) { logger.debug("Using default authentication failure handler"); final Map> defaultFailureResponseHeaders = new HashMap<>(); - final Map> sortedWWWAuthenticateHeaderValues = new TreeMap<>(); realms.asList().stream().forEach((realm) -> { Map> realmFailureHeaders = realm.getAuthenticationFailureHeaders(); realmFailureHeaders.entrySet().stream().forEach((e) -> { String key = e.getKey(); - if (key.equalsIgnoreCase("WWW-Authenticate")) { - /* - * Some browsers (eg. Firefox) behave differently when presented with multiple - * auth schemes in 'WWW-Authenticate' header. The expected behavior is that browser - * select the most secure auth scheme before trying others, but Firefox selects the - * first presented auth scheme and tries the next one's sequentially. As the - * interpretation is something we do not control, we can at least present - * the auth schemes in most to least secure order as server's preference. - */ - e.getValue().stream() - .forEach(v -> { - sortedWWWAuthenticateHeaderValues.computeIfAbsent(authSchemePriority(v), t -> new HashSet<>()).add(v); - }); - } else { - e.getValue().stream() - .filter(v -> defaultFailureResponseHeaders.computeIfAbsent(key, x -> new ArrayList<>()) - .contains(v) == false) - .forEach(v -> defaultFailureResponseHeaders.get(key).add(v)); - } + e.getValue().stream() + .filter(v -> defaultFailureResponseHeaders.computeIfAbsent(key, x -> new ArrayList<>()).contains(v) == false) + .forEach(v -> defaultFailureResponseHeaders.get(key).add(v)); }); }); if (TokenService.isTokenServiceEnabled(settings)) { String bearerScheme = "Bearer realm=\"" + XPackField.SECURITY + "\""; - sortedWWWAuthenticateHeaderValues.computeIfAbsent(authSchemePriority(bearerScheme), v -> new HashSet<>()).add(bearerScheme); + if (defaultFailureResponseHeaders.computeIfAbsent("WWW-Authenticate", x -> new ArrayList<>()) + .contains(bearerScheme) == false) { + defaultFailureResponseHeaders.get("WWW-Authenticate").add(bearerScheme); + } } - defaultFailureResponseHeaders.put("WWW-Authenticate", - sortedWWWAuthenticateHeaderValues.values().stream().flatMap(coll -> coll.stream()).collect(Collectors.toList())); failureHandler = new DefaultAuthenticationFailureHandler(defaultFailureResponseHeaders); } else { logger.debug("Using authentication failure handler from extension [" + extensionName + "]"); @@ -554,26 +536,6 @@ private AuthenticationFailureHandler createAuthenticationFailureHandler(final Re return failureHandler; } - /** - * For given 'WWW-Authenticate' header value returns the priority based on - * the auth-scheme. Lower number denotes more secure and preferred - * auth-scheme than the higher number. - * - * @param headerValue string starting with auth-scheme name - * @return integer value denoting priority for given auth scheme. - */ - private static Integer authSchemePriority(final String headerValue) { - if (headerValue.regionMatches(true, 0, "negotiate", 0, "negotiate".length())) { - return 0; - } else if (headerValue.regionMatches(true, 0, "bearer", 0, "bearer".length())) { - return 1; - } else if (headerValue.regionMatches(true, 0, "basic", 0, "basic".length())) { - return 2; - } else { - return 3; - } - } - @Override public Settings additionalSettings() { return additionalSettings(settings, enabled, transportClientMode); 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 00c6e29a1455f..857b1694ac87a 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 @@ -5,9 +5,7 @@ */ package org.elasticsearch.xpack.security; -import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.Version; -import org.elasticsearch.action.support.PlainActionFuture; import org.elasticsearch.client.Client; import org.elasticsearch.cluster.ClusterName; import org.elasticsearch.cluster.ClusterState; @@ -23,23 +21,19 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.util.concurrent.ThreadContext; -import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; import org.elasticsearch.license.License; import org.elasticsearch.license.TestUtils; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.plugins.MapperPlugin; -import org.elasticsearch.rest.RestRequest; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.test.VersionUtils; -import org.elasticsearch.test.rest.FakeRestRequest; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.watcher.ResourceWatcherService; import org.elasticsearch.xpack.core.XPackSettings; import org.elasticsearch.xpack.core.security.SecurityExtension; import org.elasticsearch.xpack.core.security.SecurityField; -import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authc.Realm; import org.elasticsearch.xpack.core.security.authc.file.FileRealmSettings; import org.elasticsearch.xpack.core.security.authc.ldap.support.SessionFactorySettings; @@ -51,15 +45,10 @@ import org.elasticsearch.xpack.security.audit.AuditTrailService; import org.elasticsearch.xpack.security.audit.index.IndexAuditTrail; import org.elasticsearch.xpack.security.audit.logfile.LoggingAuditTrail; -import org.elasticsearch.xpack.security.authc.AuthenticationService; import org.elasticsearch.xpack.security.authc.Realms; -import org.elasticsearch.xpack.security.authc.kerberos.KerberosTestCase; import org.junit.Before; import java.io.IOException; -import java.net.InetSocketAddress; -import java.nio.file.Files; -import java.nio.file.Path; import java.util.Arrays; import java.util.Collection; import java.util.Collections; @@ -77,7 +66,6 @@ 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.hamcrest.Matchers.containsString; -import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; import static org.mockito.Mockito.mock; @@ -104,13 +92,10 @@ private Collection createComponents(Settings testSettings, SecurityExten if (security != null) { throw new IllegalStateException("Security object already exists (" + security + ")"); } - Settings.Builder settingsBuilder = Settings.builder(); - settingsBuilder.put("xpack.security.enabled", true); - settingsBuilder.put(testSettings); - if (Strings.hasText(settingsBuilder.get("path.home")) == false) { - settingsBuilder.put("path.home", createTempDir()); - } - Settings settings = settingsBuilder.build(); + Settings settings = Settings.builder() + .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); @@ -439,30 +424,4 @@ public void testGetFieldFilterSecurityEnabledLicenseNoFLS() throws Exception { assertNotSame(MapperPlugin.NOOP_FIELD_FILTER, fieldFilter); assertSame(MapperPlugin.NOOP_FIELD_PREDICATE, fieldFilter.apply(randomAlphaOfLengthBetween(3, 6))); } - - public void testDefaultFailureHandlerFailedAuthenticationRespondsWithSortedWWWAuthenticateHeaderValues() throws Exception { - final Path temp = createTempDir(); - final Path configDir = Files.createDirectories(temp.resolve("config")); - KerberosTestCase.writeKeyTab(configDir.resolve("es.keytab"), null); - final Settings settings = Settings.builder() - .put("xpack.security.authc.token.enabled", "true") - .put("xpack.security.authc.realms.kerb1.type", "kerberos") - .put("xpack.security.authc.realms.kerb1.order", "2") - .put("xpack.security.authc.realms.kerb1.keytab.path", "es.keytab") - .put("path.home", temp).build(); - final Collection components = createComponents(settings); - - final AuthenticationService authcService = findComponent(AuthenticationService.class, components); - final RestRequest restRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withRemoteAddress(mock( - InetSocketAddress.class)).build(); - - final ElasticsearchSecurityException e = expectThrows(ElasticsearchSecurityException.class, () -> { - PlainActionFuture future = new PlainActionFuture<>(); - authcService.authenticate(restRequest, future); - future.actionGet(); - }); - assertThat(e.getHeader("WWW-Authenticate"), contains(containsString("Negotiate"), containsString("Bearer"), containsString( - "Basic"))); - } - } From a5ba1f65836a031c8f71a412aa6d6ffabd01c137 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Thu, 30 Aug 2018 08:46:41 +1000 Subject: [PATCH 3/6] Address review comments --- .../authc/DefaultAuthenticationFailureHandler.java | 3 +-- .../DefaultAuthenticationFailureHandlerTests.java | 13 +++++-------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java index b59a5a63a3850..ed6e04a9e3d66 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java @@ -73,8 +73,7 @@ public DefaultAuthenticationFailureHandler(final Map> failu * @param headerValue string starting with auth-scheme name * @return integer value denoting priority for given auth scheme. */ - // package scope for testing - static Integer authSchemePriority(final String headerValue) { + private static Integer authSchemePriority(final String headerValue) { if (headerValue.regionMatches(true, 0, "negotiate", 0, "negotiate".length())) { return 0; } else if (headerValue.regionMatches(true, 0, "bearer", 0, "bearer".length())) { diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java index b8304cb9272bf..08841f0a6067c 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java @@ -21,7 +21,6 @@ import java.util.List; import java.util.Map; -import static org.elasticsearch.xpack.core.security.authc.DefaultAuthenticationFailureHandler.authSchemePriority; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; @@ -51,7 +50,7 @@ public void testAuthenticationRequired() { if (testDefault) { assertWWWAuthenticateWithSchemes(ese, basicAuthScheme); } else { - assertWWWAuthenticateWithSchemes(ese, basicAuthScheme, bearerAuthScheme); + assertWWWAuthenticateWithSchemes(ese, bearerAuthScheme, basicAuthScheme); } } @@ -84,12 +83,12 @@ public void testExceptionProcessingRequest() { assertThat(ese.getHeader("WWW-Authenticate"), is(notNullValue())); assertThat(ese, is(sameInstance(cause))); if (withAuthenticateHeader == false) { - assertWWWAuthenticateWithSchemes(ese, basicAuthScheme, bearerAuthScheme, negotiateAuthScheme); + assertWWWAuthenticateWithSchemes(ese, negotiateAuthScheme, bearerAuthScheme, basicAuthScheme); } else { if (selectedScheme.contains("Negotiate ")) { assertWWWAuthenticateWithSchemes(ese, selectedScheme); } else { - assertWWWAuthenticateWithSchemes(ese, basicAuthScheme, bearerAuthScheme, negotiateAuthScheme); + assertWWWAuthenticateWithSchemes(ese, negotiateAuthScheme, bearerAuthScheme, basicAuthScheme); } } assertThat(ese.getMessage(), equalTo("unauthorized")); @@ -103,7 +102,7 @@ public void testExceptionProcessingRequest() { assertThat(ese, is(notNullValue())); assertThat(ese.getHeader("WWW-Authenticate"), is(notNullValue())); assertThat(ese.getMessage(), equalTo("error attempting to authenticate request")); - assertWWWAuthenticateWithSchemes(ese, basicAuthScheme, bearerAuthScheme, negotiateAuthScheme); + assertWWWAuthenticateWithSchemes(ese, negotiateAuthScheme, bearerAuthScheme, basicAuthScheme); } } @@ -122,13 +121,11 @@ public void testSortsWWWAuthenticateHeaderValues() { assertThat(ese, is(notNullValue())); assertThat(ese.getHeader("WWW-Authenticate"), is(notNullValue())); assertThat(ese.getMessage(), equalTo("error attempting to authenticate request")); - assertWWWAuthenticateWithSchemes(ese, basicAuthScheme, bearerAuthScheme, negotiateAuthScheme); + assertWWWAuthenticateWithSchemes(ese, negotiateAuthScheme, bearerAuthScheme, basicAuthScheme); } private void assertWWWAuthenticateWithSchemes(final ElasticsearchSecurityException ese, final String... schemes) { assertThat(ese.getHeader("WWW-Authenticate").size(), is(schemes.length)); - // Auth scheme are ordered as more secure to least secure - Arrays.sort(schemes, (o1, o2) -> authSchemePriority(o1).compareTo(authSchemePriority(o2))); assertThat(ese.getHeader("WWW-Authenticate"), contains(schemes)); } } From aa2ea72f5b8a5a665dc866794340b751e612bc74 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Thu, 30 Aug 2018 12:38:59 +1000 Subject: [PATCH 4/6] Remove unused import --- .../core/security/authc/DefaultAuthenticationFailureHandler.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java index ed6e04a9e3d66..736b9378e3876 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandler.java @@ -16,7 +16,6 @@ import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.Map.Entry; import java.util.stream.Collectors; import static org.elasticsearch.xpack.core.security.support.Exceptions.authenticationError; From aed90c3e4e02da421165850b9f587ce2d6bca768 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Thu, 30 Aug 2018 15:39:34 +1000 Subject: [PATCH 5/6] forgot to run precommit, addressed the issue --- .../authc/DefaultAuthenticationFailureHandlerTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java index 08841f0a6067c..9ad489da9dcfe 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java @@ -115,7 +115,7 @@ public void testSortsWWWAuthenticateHeaderValues() { failureResponeHeaders.put("WWW-Authenticate", Arrays.asList(basicAuthScheme, bearerAuthScheme, negotiateAuthScheme)); final DefaultAuthenticationFailureHandler failuerHandler = new DefaultAuthenticationFailureHandler(failureResponeHeaders); - final ElasticsearchSecurityException ese = failuerHandler.exceptionProcessingRequest(Mockito.mock(RestRequest.class), new Exception("other error"), + final ElasticsearchSecurityException ese = failuerHandler.exceptionProcessingRequest(Mockito.mock(RestRequest.class), null, new ThreadContext(Settings.builder().build())); assertThat(ese, is(notNullValue())); From 48277abe4ae0f1c6871f483d985f1e2062f222df Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Thu, 6 Sep 2018 15:53:42 +1000 Subject: [PATCH 6/6] Address review comments --- .../authc/DefaultAuthenticationFailureHandlerTests.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java index 9ad489da9dcfe..15593f0b82ea5 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/DefaultAuthenticationFailureHandlerTests.java @@ -112,7 +112,9 @@ public void testSortsWWWAuthenticateHeaderValues() { final String bearerAuthScheme = "Bearer realm=\"" + XPackField.SECURITY + "\""; final String negotiateAuthScheme = randomFrom("Negotiate", "Negotiate Ijoijksdk"); final Map> failureResponeHeaders = new HashMap<>(); - failureResponeHeaders.put("WWW-Authenticate", Arrays.asList(basicAuthScheme, bearerAuthScheme, negotiateAuthScheme)); + final List supportedSchemes = Arrays.asList(basicAuthScheme, bearerAuthScheme, negotiateAuthScheme); + Collections.shuffle(supportedSchemes, random()); + failureResponeHeaders.put("WWW-Authenticate", supportedSchemes); final DefaultAuthenticationFailureHandler failuerHandler = new DefaultAuthenticationFailureHandler(failureResponeHeaders); final ElasticsearchSecurityException ese = failuerHandler.exceptionProcessingRequest(Mockito.mock(RestRequest.class), null,