From 80064652f810a6bf9e2f13d2cdb88376506a3ab7 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Wed, 11 Sep 2019 14:33:17 +1000 Subject: [PATCH] Fallback to realm authc if ApiKey fails (#46552) This changes API-Key authentication to always fallback to the realm chain if the API key is not valid. The previous behaviour was inconsistent and would terminate on some failures, but continue to the realm chain for others. Backport of: #46538 --- .../xpack/security/authc/ApiKeyService.java | 22 +-- .../security/authc/ApiKeyServiceTests.java | 134 +++++++++++++++--- .../authc/AuthenticationServiceTests.java | 1 - 3 files changed, 126 insertions(+), 31 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java index 781feadabdc..b74e03bacc6 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java @@ -337,15 +337,16 @@ public class ApiKeyService { } if (credentials != null) { + final String docId = credentials.getId(); final GetRequest getRequest = client - .prepareGet(SECURITY_MAIN_ALIAS, SINGLE_MAPPING_NAME, credentials.getId()) + .prepareGet(SECURITY_MAIN_ALIAS, SINGLE_MAPPING_NAME, docId) .setFetchSource(true) .request(); executeAsyncWithOrigin(ctx, SECURITY_ORIGIN, getRequest, ActionListener.wrap(response -> { if (response.isExists()) { try (ApiKeyCredentials ignore = credentials) { final Map source = response.getSource(); - validateApiKeyCredentials(source, credentials, clock, listener); + validateApiKeyCredentials(docId, source, credentials, clock, listener); } } else { credentials.close(); @@ -440,17 +441,22 @@ public class ApiKeyService { /** * Validates the ApiKey using the source map + * @param docId the identifier of the document that was retrieved from the security index * @param source the source map from a get of the ApiKey document * @param credentials the credentials provided by the user * @param listener the listener to notify after verification */ - void validateApiKeyCredentials(Map source, ApiKeyCredentials credentials, Clock clock, + void validateApiKeyCredentials(String docId, Map source, ApiKeyCredentials credentials, Clock clock, ActionListener listener) { + final String docType = (String) source.get("doc_type"); final Boolean invalidated = (Boolean) source.get("api_key_invalidated"); - if (invalidated == null) { - listener.onResponse(AuthenticationResult.terminate("api key document is missing invalidated field", null)); + if ("api_key".equals(docType) == false) { + listener.onResponse( + AuthenticationResult.unsuccessful("document [" + docId + "] is [" + docType + "] not an api key", null)); + } else if (invalidated == null) { + listener.onResponse(AuthenticationResult.unsuccessful("api key document is missing invalidated field", null)); } else if (invalidated) { - listener.onResponse(AuthenticationResult.terminate("api key has been invalidated", null)); + listener.onResponse(AuthenticationResult.unsuccessful("api key has been invalidated", null)); } else { final String apiKeyHash = (String) source.get("api_key_hash"); if (apiKeyHash == null) { @@ -484,7 +490,7 @@ public class ApiKeyService { listener.onResponse(AuthenticationResult.unsuccessful("invalid credentials", null)); } else { apiKeyAuthCache.invalidate(credentials.getId(), listenableCacheEntry); - validateApiKeyCredentials(source, credentials, clock, listener); + validateApiKeyCredentials(docId, source, credentials, clock, listener); } }, listener::onFailure), threadPool.generic(), threadPool.getThreadContext()); @@ -534,7 +540,7 @@ public class ApiKeyService { authResultMetadata.put(API_KEY_ID_KEY, credentials.getId()); listener.onResponse(AuthenticationResult.success(apiKeyUser, authResultMetadata)); } else { - listener.onResponse(AuthenticationResult.terminate("api key is expired", null)); + listener.onResponse(AuthenticationResult.unsuccessful("api key is expired", null)); } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java index 031f5ccec06..88cbc0a8069 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java @@ -45,6 +45,7 @@ import org.junit.Before; import java.io.IOException; import java.nio.charset.StandardCharsets; import java.time.Clock; +import java.time.Duration; import java.time.Instant; import java.time.temporal.ChronoUnit; import java.util.Arrays; @@ -56,6 +57,7 @@ import java.util.Map; import static org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR; import static org.hamcrest.Matchers.arrayContaining; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; @@ -159,25 +161,110 @@ public class ApiKeyServiceTests extends ESTestCase { assertThat(auth.getUser(), nullValue()); } - public void mockKeyDocument(ApiKeyService service, String id, String key, User user) throws IOException { - final Authentication authentication = new Authentication(user, new RealmRef("realm1", "native", "node01"), null, Version.CURRENT); - final XContentBuilder docSource = service.newDocument(new SecureString(key.toCharArray()), "test", authentication, - Collections.singleton(SUPERUSER_ROLE_DESCRIPTOR), Instant.now(), Instant.now().plusSeconds(3600), null, Version.CURRENT); + public void testAuthenticationFailureWithInvalidatedApiKey() throws Exception { + final Settings settings = Settings.builder().put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true).build(); + final ApiKeyService service = createApiKeyService(settings); + final String id = randomAlphaOfLength(12); + final String key = randomAlphaOfLength(16); + + mockKeyDocument(service, id, key, new User("hulk", "superuser"), true, Duration.ofSeconds(3600)); + + final AuthenticationResult auth = tryAuthenticate(service, id, key); + assertThat(auth.getStatus(), is(AuthenticationResult.Status.CONTINUE)); + assertThat(auth.getUser(), nullValue()); + assertThat(auth.getMessage(), containsString("invalidated")); + } + + public void testAuthenticationFailureWithInvalidCredentials() throws Exception { + final Settings settings = Settings.builder().put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true).build(); + final ApiKeyService service = createApiKeyService(settings); + + final String id = randomAlphaOfLength(12); + final String realKey = randomAlphaOfLength(16); + final String wrongKey = "#" + realKey.substring(1); + + mockKeyDocument(service, id, realKey, new User("hulk", "superuser")); + + final AuthenticationResult auth = tryAuthenticate(service, id, wrongKey); + assertThat(auth.getStatus(), is(AuthenticationResult.Status.CONTINUE)); + assertThat(auth.getUser(), nullValue()); + assertThat(auth.getMessage(), containsString("invalid credentials")); + } + + public void testAuthenticationFailureWithExpiredKey() throws Exception { + final Settings settings = Settings.builder().put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true).build(); + final ApiKeyService service = createApiKeyService(settings); + + final String id = randomAlphaOfLength(12); + final String key = randomAlphaOfLength(16); + + mockKeyDocument(service, id, key, new User("hulk", "superuser"), false, Duration.ofSeconds(-1)); + + final AuthenticationResult auth = tryAuthenticate(service, id, key); + assertThat(auth.getStatus(), is(AuthenticationResult.Status.CONTINUE)); + assertThat(auth.getUser(), nullValue()); + assertThat(auth.getMessage(), containsString("expired")); + } + + /** + * We cache valid and invalid responses. This test verifies that we handle these correctly. + */ + public void testMixingValidAndInvalidCredentials() throws Exception { + final Settings settings = Settings.builder().put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true).build(); + final ApiKeyService service = createApiKeyService(settings); + + final String id = randomAlphaOfLength(12); + final String realKey = randomAlphaOfLength(16); + + mockKeyDocument(service, id, realKey, new User("hulk", "superuser")); + + for (int i = 0; i < 3; i++) { + final String wrongKey = "=" + randomAlphaOfLength(14) + "@"; + AuthenticationResult auth = tryAuthenticate(service, id, wrongKey); + assertThat(auth.getStatus(), is(AuthenticationResult.Status.CONTINUE)); + assertThat(auth.getUser(), nullValue()); + assertThat(auth.getMessage(), containsString("invalid credentials")); + + auth = tryAuthenticate(service, id, realKey); + assertThat(auth.getStatus(), is(AuthenticationResult.Status.SUCCESS)); + assertThat(auth.getUser(), notNullValue()); + assertThat(auth.getUser().principal(), is("hulk")); + } + } + + private void mockKeyDocument(ApiKeyService service, String id, String key, User user) throws IOException { + mockKeyDocument(service, id, key, user, false, Duration.ofSeconds(3600)); + } + + private void mockKeyDocument(ApiKeyService service, String id, String key, User user, boolean invalidated, + Duration expiry) throws IOException { + final Authentication authentication = new Authentication(user, new RealmRef("realm1", "native", + "node01"), null, Version.CURRENT); + XContentBuilder docSource = service.newDocument(new SecureString(key.toCharArray()), "test", authentication, + Collections.singleton(SUPERUSER_ROLE_DESCRIPTOR), Instant.now(), Instant.now().plus(expiry), null, + Version.CURRENT); + if (invalidated) { + Map map = XContentHelper.convertToMap(BytesReference.bytes(docSource), true, XContentType.JSON).v2(); + map.put("api_key_invalidated", true); + docSource = XContentBuilder.builder(XContentType.JSON.xContent()).map(map); + } SecurityMocks.mockGetRequest(client, id, BytesReference.bytes(docSource)); } private AuthenticationResult tryAuthenticate(ApiKeyService service, String id, String key) throws Exception { final ThreadContext threadContext = threadPool.getThreadContext(); - final String header = "ApiKey " + Base64.getEncoder().encodeToString((id + ":" + key).getBytes(StandardCharsets.UTF_8)); - threadContext.putHeader("Authorization", header); + try (ThreadContext.StoredContext ignore = threadContext.stashContext()) { + final String header = "ApiKey " + Base64.getEncoder().encodeToString((id + ":" + key).getBytes(StandardCharsets.UTF_8)); + threadContext.putHeader("Authorization", header); - final PlainActionFuture future = new PlainActionFuture<>(); - service.authenticateWithApiKeyIfPresent(threadContext, future); + final PlainActionFuture future = new PlainActionFuture<>(); + service.authenticateWithApiKeyIfPresent(threadContext, future); - final AuthenticationResult auth = future.get(); - assertThat(auth, notNullValue()); - return auth; + final AuthenticationResult auth = future.get(); + assertThat(auth, notNullValue()); + return auth; + } } public void testValidateApiKey() throws Exception { @@ -186,6 +273,7 @@ public class ApiKeyServiceTests extends ESTestCase { final char[] hash = hasher.hash(new SecureString(apiKey.toCharArray())); Map sourceMap = new HashMap<>(); + sourceMap.put("doc_type", "api_key"); sourceMap.put("api_key_hash", new String(hash)); sourceMap.put("role_descriptors", Collections.singletonMap("a role", Collections.singletonMap("cluster", "all"))); sourceMap.put("limited_by_role_descriptors", Collections.singletonMap("limited role", Collections.singletonMap("cluster", "all"))); @@ -200,7 +288,7 @@ public class ApiKeyServiceTests extends ESTestCase { ApiKeyService.ApiKeyCredentials creds = new ApiKeyService.ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(apiKey.toCharArray())); PlainActionFuture future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); AuthenticationResult result = future.get(); assertNotNull(result); assertTrue(result.isAuthenticated()); @@ -214,7 +302,7 @@ public class ApiKeyServiceTests extends ESTestCase { sourceMap.put("expiration_time", Clock.systemUTC().instant().plus(1L, ChronoUnit.HOURS).toEpochMilli()); future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); result = future.get(); assertNotNull(result); assertTrue(result.isAuthenticated()); @@ -228,7 +316,7 @@ public class ApiKeyServiceTests extends ESTestCase { sourceMap.put("expiration_time", Clock.systemUTC().instant().minus(1L, ChronoUnit.HOURS).toEpochMilli()); future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); result = future.get(); assertNotNull(result); assertFalse(result.isAuthenticated()); @@ -236,7 +324,7 @@ public class ApiKeyServiceTests extends ESTestCase { sourceMap.remove("expiration_time"); creds = new ApiKeyService.ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(randomAlphaOfLength(15).toCharArray())); future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); result = future.get(); assertNotNull(result); assertFalse(result.isAuthenticated()); @@ -244,7 +332,7 @@ public class ApiKeyServiceTests extends ESTestCase { sourceMap.put("api_key_invalidated", true); creds = new ApiKeyService.ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(randomAlphaOfLength(15).toCharArray())); future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); result = future.get(); assertNotNull(result); assertFalse(result.isAuthenticated()); @@ -344,6 +432,7 @@ public class ApiKeyServiceTests extends ESTestCase { final char[] hash = hasher.hash(new SecureString(apiKey.toCharArray())); Map sourceMap = new HashMap<>(); + sourceMap.put("doc_type", "api_key"); sourceMap.put("api_key_hash", new String(hash)); sourceMap.put("role_descriptors", Collections.singletonMap("a role", Collections.singletonMap("cluster", "all"))); sourceMap.put("limited_by_role_descriptors", Collections.singletonMap("limited role", Collections.singletonMap("cluster", "all"))); @@ -356,7 +445,7 @@ public class ApiKeyServiceTests extends ESTestCase { ApiKeyService service = createApiKeyService(Settings.EMPTY); ApiKeyCredentials creds = new ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(apiKey.toCharArray())); PlainActionFuture future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); AuthenticationResult result = future.actionGet(); assertThat(result.isAuthenticated(), is(true)); CachedApiKeyHashResult cachedApiKeyHashResult = service.getFromCache(creds.getId()); @@ -365,7 +454,7 @@ public class ApiKeyServiceTests extends ESTestCase { creds = new ApiKeyCredentials(creds.getId(), new SecureString("foobar".toCharArray())); future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); result = future.actionGet(); assertThat(result.isAuthenticated(), is(false)); final CachedApiKeyHashResult shouldBeSame = service.getFromCache(creds.getId()); @@ -375,7 +464,7 @@ public class ApiKeyServiceTests extends ESTestCase { sourceMap.put("api_key_hash", new String(hasher.hash(new SecureString("foobar".toCharArray())))); creds = new ApiKeyCredentials(randomAlphaOfLength(12), new SecureString("foobar1".toCharArray())); future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); result = future.actionGet(); assertThat(result.isAuthenticated(), is(false)); cachedApiKeyHashResult = service.getFromCache(creds.getId()); @@ -384,7 +473,7 @@ public class ApiKeyServiceTests extends ESTestCase { creds = new ApiKeyCredentials(creds.getId(), new SecureString("foobar2".toCharArray())); future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); result = future.actionGet(); assertThat(result.isAuthenticated(), is(false)); assertThat(service.getFromCache(creds.getId()), not(sameInstance(cachedApiKeyHashResult))); @@ -392,7 +481,7 @@ public class ApiKeyServiceTests extends ESTestCase { creds = new ApiKeyCredentials(creds.getId(), new SecureString("foobar".toCharArray())); future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); result = future.actionGet(); assertThat(result.isAuthenticated(), is(true)); assertThat(service.getFromCache(creds.getId()), not(sameInstance(cachedApiKeyHashResult))); @@ -408,6 +497,7 @@ public class ApiKeyServiceTests extends ESTestCase { .build(); Map sourceMap = new HashMap<>(); + sourceMap.put("doc_type", "api_key"); sourceMap.put("api_key_hash", new String(hash)); sourceMap.put("role_descriptors", Collections.singletonMap("a role", Collections.singletonMap("cluster", "all"))); sourceMap.put("limited_by_role_descriptors", Collections.singletonMap("limited role", Collections.singletonMap("cluster", "all"))); @@ -420,7 +510,7 @@ public class ApiKeyServiceTests extends ESTestCase { ApiKeyService service = createApiKeyService(settings); ApiKeyCredentials creds = new ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(apiKey.toCharArray())); PlainActionFuture future = new PlainActionFuture<>(); - service.validateApiKeyCredentials(sourceMap, creds, Clock.systemUTC(), future); + service.validateApiKeyCredentials(creds.getId(), sourceMap, creds, Clock.systemUTC(), future); AuthenticationResult result = future.actionGet(); assertThat(result.isAuthenticated(), is(true)); CachedApiKeyHashResult cachedApiKeyHashResult = service.getFromCache(creds.getId()); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java index e914e5a416c..4224cbfd8c0 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java @@ -1305,7 +1305,6 @@ public class AuthenticationServiceTests extends ESTestCase { threadContext.putHeader("Authorization", headerValue); ElasticsearchSecurityException e = expectThrows(ElasticsearchSecurityException.class, () -> authenticateBlocking("_action", message, null)); - assertThat(e.getMessage(), containsString("api key is expired")); assertEquals(RestStatus.UNAUTHORIZED, e.status()); } }