From 66b68051a505e5929ee56daffa9be62064b264ae Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Tue, 4 Feb 2020 13:14:05 +1100 Subject: [PATCH] Preserve ApiKey credentials for async verification The ApiKeyService would aggressively "close" ApiKeyCredentials objects during processing. However, under rare circumstances, the verfication of the secret key would be performed asychronously and may need access to the SecureString after it had been closed by the caller. The trigger for this would be if the cache already held a Future for that ApiKey, but the future was not yet complete. In this case the verification of the secret key would take place asynchronously on the generic thread pool. This commit moves the "close" of the credentials to the body of the listener so that it only occurs after key verification is complete. Backport of: #51244 --- .../xpack/security/authc/ApiKeyService.java | 49 +++++--- .../security/authc/ApiKeyServiceTests.java | 114 ++++++++++++++---- 2 files changed, 124 insertions(+), 39 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 17eff60c3e08b..a666d6ac6e950 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 @@ -334,25 +334,16 @@ void authenticateWithApiKeyIfPresent(ThreadContext ctx, ActionListenerwrap(response -> { - if (response.isExists()) { - try (ApiKeyCredentials ignore = credentials) { - final Map source = response.getSource(); - validateApiKeyCredentials(docId, source, credentials, clock, listener); - } - } else { + loadApiKeyAndValidateCredentials(ctx, credentials, ActionListener.wrap( + response -> { + credentials.close(); + listener.onResponse(response); + }, + e -> { credentials.close(); - listener.onResponse( - AuthenticationResult.unsuccessful("unable to find apikey with id " + credentials.getId(), null)); + listener.onFailure(e); } - }, e -> { - credentials.close(); - listener.onResponse(AuthenticationResult.unsuccessful("apikey authentication for id " + credentials.getId() + - " encountered a failure", e)); - }), client::get); + )); } else { listener.onResponse(AuthenticationResult.notHandled()); } @@ -361,6 +352,27 @@ void authenticateWithApiKeyIfPresent(ThreadContext ctx, ActionListener listener) { + final String docId = credentials.getId(); + final GetRequest getRequest = client + .prepareGet(SecurityIndexManager.SECURITY_INDEX_NAME, TYPE, docId) + .setFetchSource(true) + .request(); + executeAsyncWithOrigin(ctx, SECURITY_ORIGIN, getRequest, ActionListener.wrap(response -> { + if (response.isExists()) { + final Map source = response.getSource(); + validateApiKeyCredentials(docId, source, credentials, clock, listener); + } else { + listener.onResponse( + AuthenticationResult.unsuccessful("unable to find apikey with id " + credentials.getId(), null)); + } + }, + e -> listener.onResponse(AuthenticationResult.unsuccessful( + "apikey authentication for id " + credentials.getId() + " encountered a failure", e))), + client::get); + } + /** * The current request has been authenticated by an API key and this method enables the * retrieval of role descriptors that are associated with the api key @@ -572,7 +584,8 @@ static ApiKeyCredentials getCredentialsFromHeader(ThreadContext threadContext) { return null; } - private static boolean verifyKeyAgainstHash(String apiKeyHash, ApiKeyCredentials credentials) { + // Protected instance method so this can be mocked + protected boolean verifyKeyAgainstHash(String apiKeyHash, ApiKeyCredentials credentials) { final char[] apiKeyHashChars = apiKeyHash.toCharArray(); try { Hasher hasher = Hasher.resolveFromHash(apiKeyHash.toCharArray()); 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 04c6aa900b1ad..d1f96dbcd436f 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 @@ -70,6 +70,8 @@ import java.util.HashMap; import java.util.Map; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Semaphore; +import java.util.concurrent.atomic.AtomicInteger; import static org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR; import static org.hamcrest.Matchers.arrayContaining; @@ -508,16 +510,7 @@ public void testApiKeyCache() { Hasher hasher = randomFrom(Hasher.PBKDF2, Hasher.BCRYPT4, Hasher.BCRYPT); 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"))); - Map creatorMap = new HashMap<>(); - creatorMap.put("principal", "test_user"); - creatorMap.put("metadata", Collections.emptyMap()); - sourceMap.put("creator", creatorMap); - sourceMap.put("api_key_invalidated", false); + Map sourceMap = buildApiKeySourceDoc(hash); ApiKeyService service = createApiKeyService(Settings.EMPTY); ApiKeyCredentials creds = new ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(apiKey.toCharArray())); @@ -565,6 +558,64 @@ public void testApiKeyCache() { assertThat(service.getFromCache(creds.getId()).success, is(true)); } + public void testAuthenticateWhileCacheBeingPopulated() throws Exception { + final String apiKey = randomAlphaOfLength(16); + Hasher hasher = randomFrom(Hasher.PBKDF2, Hasher.BCRYPT4, Hasher.BCRYPT); + final char[] hash = hasher.hash(new SecureString(apiKey.toCharArray())); + + Map sourceMap = buildApiKeySourceDoc(hash); + + ApiKeyService realService = createApiKeyService(Settings.EMPTY); + ApiKeyService service = Mockito.spy(realService); + + // Used to block the hashing of the first api-key secret so that we can guarantee + // that a second api key authentication takes place while hashing is "in progress". + final Semaphore hashWait = new Semaphore(0); + final AtomicInteger hashCounter = new AtomicInteger(0); + doAnswer(invocationOnMock -> { + hashCounter.incrementAndGet(); + hashWait.acquire(); + return invocationOnMock.callRealMethod(); + }).when(service).verifyKeyAgainstHash(any(String.class), any(ApiKeyCredentials.class)); + + final ApiKeyCredentials creds = new ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(apiKey.toCharArray())); + final PlainActionFuture future1 = new PlainActionFuture<>(); + + // Call the top level authenticate... method because it has been known to be buggy in async situations + writeCredentialsToThreadContext(creds); + mockSourceDocument(creds.getId(), sourceMap); + + // This needs to be done in another thread, because we need it to not complete until we say so, but it should not block this test + this.threadPool.generic().execute(() -> service.authenticateWithApiKeyIfPresent(threadPool.getThreadContext(), future1)); + + // Wait for the first credential validation to get to the blocked state + assertBusy(() -> assertThat(hashCounter.get(), equalTo(1))); + if (future1.isDone()) { + // We do this [ rather than assertFalse(isDone) ] so we can get a reasonable failure message + fail("Expected authentication to be blocked, but was " + future1.actionGet()); + } + + // The second authentication should pass (but not immediately, but will not block) + PlainActionFuture future2 = new PlainActionFuture<>(); + + service.authenticateWithApiKeyIfPresent(threadPool.getThreadContext(), future2); + + assertThat(hashCounter.get(), equalTo(1)); + if (future2.isDone()) { + // We do this [ rather than assertFalse(isDone) ] so we can get a reasonable failure message + fail("Expected authentication to be blocked, but was " + future2.actionGet()); + } + + hashWait.release(); + + assertThat(future1.actionGet(TimeValue.timeValueSeconds(2)).isAuthenticated(), is(true)); + assertThat(future2.actionGet(TimeValue.timeValueMillis(100)).isAuthenticated(), is(true)); + + CachedApiKeyHashResult cachedApiKeyHashResult = service.getFromCache(creds.getId()); + assertNotNull(cachedApiKeyHashResult); + assertThat(cachedApiKeyHashResult.success, is(true)); + } + public void testApiKeyCacheDisabled() { final String apiKey = randomAlphaOfLength(16); Hasher hasher = randomFrom(Hasher.PBKDF2, Hasher.BCRYPT4, Hasher.BCRYPT); @@ -573,16 +624,7 @@ public void testApiKeyCacheDisabled() { .put(ApiKeyService.CACHE_TTL_SETTING.getKey(), "0s") .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"))); - Map creatorMap = new HashMap<>(); - creatorMap.put("principal", "test_user"); - creatorMap.put("metadata", Collections.emptyMap()); - sourceMap.put("creator", creatorMap); - sourceMap.put("api_key_invalidated", false); + Map sourceMap = buildApiKeySourceDoc(hash); ApiKeyService service = createApiKeyService(settings); ApiKeyCredentials creds = new ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(apiKey.toCharArray())); @@ -594,10 +636,40 @@ public void testApiKeyCacheDisabled() { assertNull(cachedApiKeyHashResult); } - private ApiKeyService createApiKeyService(Settings settings) { + private ApiKeyService createApiKeyService(Settings baseSettings) { + final Settings settings = Settings.builder() + .put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true) + .put(baseSettings) + .build(); return new ApiKeyService(settings, Clock.systemUTC(), client, licenseState, securityIndex, ClusterServiceUtils.createClusterService(threadPool), threadPool); } + private Map buildApiKeySourceDoc(char[] hash) { + 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"))); + Map creatorMap = new HashMap<>(); + creatorMap.put("principal", "test_user"); + creatorMap.put("metadata", Collections.emptyMap()); + sourceMap.put("creator", creatorMap); + sourceMap.put("api_key_invalidated", false); + return sourceMap; + } + + private void writeCredentialsToThreadContext(ApiKeyCredentials creds) { + final String credentialString = creds.getId() + ":" + creds.getKey(); + this.threadPool.getThreadContext().putHeader("Authorization", + "ApiKey " + Base64.getEncoder().encodeToString(credentialString.getBytes(StandardCharsets.US_ASCII))); + } + + private void mockSourceDocument(String id, Map sourceMap) throws IOException { + try (XContentBuilder builder = JsonXContent.contentBuilder()) { + builder.map(sourceMap); + SecurityMocks.mockGetRequest(client, id, BytesReference.bytes(builder)); + } + } }