Skip to content

Commit 03fbc35

Browse files
ywangdywangd
authored
Move hashing on API key creation to crypto thread pool (#74165) (#74417)
The changes in #74106 make API keys cached on creation time. It helps avoid the expensive hashing operation on initial authentication when a request using the key hits the same node that creates the key. Since the more expensive hashing on authentication time is handled by a dedicated "crypto" thread pool (#58090), it is expected that usage of the "crypto" thread pool to be reduced. This PR moves the hashing on creation time to the "crypto" thread pool so that a similar (before #74106) usage level of "crypto" thread pool is maintained. It also has the benefit to avoid costly operations in the transport_worker thread, which is generally preferred. Relates: #74106
1 parent 80a6f56 commit 03fbc35

File tree

3 files changed

+69
-35
lines changed

3 files changed

+69
-35
lines changed

‎x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -998,7 +998,7 @@ public void testDerivedKeys() throws ExecutionException, InterruptedException {
998998
assertApiKeyNotCreated(client,"key-5");
999999
}
10001000

1001-
public void testAuthenticationReturns429WhenThreadPoolIsSaturated() throws IOException, InterruptedException, ExecutionException {
1001+
public void testCreationAndAuthenticationReturns429WhenThreadPoolIsSaturated() throws Exception {
10021002
final String nodeName = randomFrom(internalCluster().getNodeNames());
10031003
final Settings settings = internalCluster().getInstance(Settings.class, nodeName);
10041004
final int allocatedProcessors = EsExecutors.allocatedProcessors(settings);
@@ -1059,9 +1059,17 @@ public void testAuthenticationReturns429WhenThreadPoolIsSaturated() throws IOExc
10591059
final Request authRequest = new Request("GET", "_security/_authenticate");
10601060
authRequest.setOptions(RequestOptions.DEFAULT.toBuilder().addHeader(
10611061
"Authorization", "ApiKey " + base64ApiKeyKeyValue).build());
1062-
final ResponseException responseException = expectThrows(ResponseException.class, () -> restClient.performRequest(authRequest));
1063-
assertThat(responseException.getMessage(), containsString("429 Too Many Requests"));
1064-
assertThat(responseException.getResponse().getStatusLine().getStatusCode(), is(429));
1062+
final ResponseException e1 = expectThrows(ResponseException.class, () -> restClient.performRequest(authRequest));
1063+
assertThat(e1.getMessage(), containsString("429 Too Many Requests"));
1064+
assertThat(e1.getResponse().getStatusLine().getStatusCode(), is(429));
1065+
1066+
final Request createApiKeyRequest = new Request("POST", "_security/api_key");
1067+
createApiKeyRequest.setJsonEntity("{\"name\":\"key\"}");
1068+
createApiKeyRequest.setOptions(createApiKeyRequest.getOptions().toBuilder()
1069+
.addHeader("Authorization", basicAuthHeaderValue(TEST_SUPERUSER, TEST_PASSWORD_SECURE_STRING)));
1070+
final ResponseException e2 = expectThrows(ResponseException.class, () -> restClient.performRequest(createApiKeyRequest));
1071+
assertThat(e2.getMessage(), containsString("429 Too Many Requests"));
1072+
assertThat(e2.getResponse().getStatusLine().getStatusCode(), is(429));
10651073
} finally {
10661074
blockingLatch.countDown();
10671075
if (lastTaskFuture != null) {

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

Lines changed: 35 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -286,37 +286,41 @@ private void createApiKeyAndIndexIt(Authentication authentication, CreateApiKeyR
286286
Version.V_6_7_0);
287287
}
288288

289-
try (XContentBuilder builder = newDocument(apiKey, request.getName(), authentication,
290-
roleDescriptorSet, created, expiration,
291-
request.getRoleDescriptors(), version, request.getMetadata())) {
292-
293-
final IndexRequest indexRequest =
294-
client.prepareIndex(SECURITY_MAIN_ALIAS, SINGLE_MAPPING_NAME)
295-
.setSource(builder)
296-
.setRefreshPolicy(request.getRefreshPolicy())
297-
.request();
298-
final BulkRequest bulkRequest = toSingleItemBulkRequest(indexRequest);
299-
300-
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
301-
executeAsyncWithOrigin(client, SECURITY_ORIGIN, BulkAction.INSTANCE, bulkRequest,
302-
TransportSingleItemBulkWriteAction.<IndexResponse>wrapBulkResponse(ActionListener.wrap(
303-
indexResponse -> {
304-
final ListenableFuture<CachedApiKeyHashResult> listenableFuture = new ListenableFuture<>();
305-
listenableFuture.onResponse(new CachedApiKeyHashResult(true, apiKey));
306-
apiKeyAuthCache.put(indexResponse.getId(), listenableFuture);
307-
listener.onResponse(
308-
new CreateApiKeyResponse(request.getName(), indexResponse.getId(), apiKey, expiration));
309-
},
310-
listener::onFailure))));
311-
} catch (IOException e) {
312-
listener.onFailure(e);
313-
}
289+
computeHashForApiKey(apiKey, listener.delegateFailure((l, apiKeyHashChars) -> {
290+
try (XContentBuilder builder = newDocument(apiKeyHashChars, request.getName(), authentication,
291+
roleDescriptorSet, created, expiration,
292+
request.getRoleDescriptors(), version, request.getMetadata())) {
293+
294+
final IndexRequest indexRequest =
295+
client.prepareIndex(SECURITY_MAIN_ALIAS, SINGLE_MAPPING_NAME)
296+
.setSource(builder)
297+
.setRefreshPolicy(request.getRefreshPolicy())
298+
.request();
299+
final BulkRequest bulkRequest = toSingleItemBulkRequest(indexRequest);
300+
301+
securityIndex.prepareIndexIfNeededThenExecute(listener::onFailure, () ->
302+
executeAsyncWithOrigin(client, SECURITY_ORIGIN, BulkAction.INSTANCE, bulkRequest,
303+
TransportSingleItemBulkWriteAction.<IndexResponse>wrapBulkResponse(ActionListener.wrap(
304+
indexResponse -> {
305+
final ListenableFuture<CachedApiKeyHashResult> listenableFuture = new ListenableFuture<>();
306+
listenableFuture.onResponse(new CachedApiKeyHashResult(true, apiKey));
307+
apiKeyAuthCache.put(indexResponse.getId(), listenableFuture);
308+
listener.onResponse(
309+
new CreateApiKeyResponse(request.getName(), indexResponse.getId(), apiKey, expiration));
310+
},
311+
listener::onFailure))));
312+
} catch (IOException e) {
313+
listener.onFailure(e);
314+
} finally {
315+
Arrays.fill(apiKeyHashChars, (char) 0);
316+
}
317+
}));
314318
}
315319

316320
/**
317321
* package-private for testing
318322
*/
319-
XContentBuilder newDocument(SecureString apiKey, String name, Authentication authentication, Set<RoleDescriptor> userRoles,
323+
XContentBuilder newDocument(char[] apiKeyHashChars, String name, Authentication authentication, Set<RoleDescriptor> userRoles,
320324
Instant created, Instant expiration, List<RoleDescriptor> keyRoles,
321325
Version version, @Nullable Map<String, Object> metadata) throws IOException {
322326
XContentBuilder builder = XContentFactory.jsonBuilder();
@@ -328,15 +332,13 @@ XContentBuilder newDocument(SecureString apiKey, String name, Authentication aut
328332

329333

330334
byte[] utf8Bytes = null;
331-
final char[] keyHash = hasher.hash(apiKey);
332335
try {
333-
utf8Bytes = CharArrays.toUtf8Bytes(keyHash);
336+
utf8Bytes = CharArrays.toUtf8Bytes(apiKeyHashChars);
334337
builder.field("api_key_hash").utf8Value(utf8Bytes, 0, utf8Bytes.length);
335338
} finally {
336339
if (utf8Bytes != null) {
337340
Arrays.fill(utf8Bytes, (byte) 0);
338341
}
339-
Arrays.fill(keyHash, (char) 0);
340342
}
341343

342344
// Save role_descriptors
@@ -772,6 +774,10 @@ public static boolean isApiKeyAuthentication(Authentication authentication) {
772774
}
773775
}
774776

777+
void computeHashForApiKey(SecureString apiKey, ActionListener<char[]> listener) {
778+
threadPool.executor(SECURITY_CRYPTO_THREAD_POOL_NAME).execute(ActionRunnable.supply(listener, () -> hasher.hash(apiKey)));
779+
}
780+
775781
// Protected instance method so this can be mocked
776782
protected void verifyKeyAgainstHash(String apiKeyHash, ApiKeyCredentials credentials, ActionListener<Boolean> listener) {
777783
threadPool.executor(SECURITY_CRYPTO_THREAD_POOL_NAME).execute(ActionRunnable.supply(listener, () -> {

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -399,7 +399,8 @@ Version.CURRENT, randomFrom(AuthenticationType.REALM, AuthenticationType.TOKEN,
399399
AuthenticationType.ANONYMOUS), Collections.emptyMap());
400400
}
401401
final Map<String, Object> metadata = ApiKeyTests.randomMetadata();
402-
XContentBuilder docSource = service.newDocument(new SecureString(key.toCharArray()), "test", authentication,
402+
XContentBuilder docSource = service.newDocument(
403+
getFastStoredHashAlgoForTests().hash(new SecureString(key.toCharArray())),"test", authentication,
403404
Collections.singleton(SUPERUSER_ROLE_DESCRIPTOR), Instant.now(), Instant.now().plus(expiry), keyRoles,
404405
Version.CURRENT, metadata);
405406
if (invalidated) {
@@ -976,6 +977,25 @@ public void testAuthWillTerminateIfHashingThreadPoolIsSaturated() throws IOExcep
976977
assertThat(authenticationResult.getMessage(), containsString("server is too busy to respond"));
977978
}
978979

980+
public void testCreationWillFailIfHashingThreadPoolIsSaturated() {
981+
final EsRejectedExecutionException rejectedExecutionException = new EsRejectedExecutionException("rejected");
982+
final ExecutorService mockExecutorService = mock(ExecutorService.class);
983+
when(threadPool.executor(SECURITY_CRYPTO_THREAD_POOL_NAME)).thenReturn(mockExecutorService);
984+
Mockito.doAnswer(invocationOnMock -> {
985+
final AbstractRunnable actionRunnable = (AbstractRunnable) invocationOnMock.getArguments()[0];
986+
actionRunnable.onRejection(rejectedExecutionException);
987+
return null;
988+
}).when(mockExecutorService).execute(any(Runnable.class));
989+
990+
final Authentication authentication = mock(Authentication.class);
991+
final CreateApiKeyRequest createApiKeyRequest = new CreateApiKeyRequest(randomAlphaOfLengthBetween(3, 8), null, null);
992+
ApiKeyService service = createApiKeyService(Settings.EMPTY);
993+
final PlainActionFuture<CreateApiKeyResponse> future = new PlainActionFuture<>();
994+
service.createApiKey(authentication, createApiKeyRequest, org.elasticsearch.core.Set.of(), future);
995+
final EsRejectedExecutionException e = expectThrows(EsRejectedExecutionException.class, future::actionGet);
996+
assertThat(e, is(rejectedExecutionException));
997+
}
998+
979999
public void testCachedApiKeyValidationWillNotBeBlockedByUnCachedApiKey() throws IOException, ExecutionException, InterruptedException {
9801000
final String apiKey1 = randomAlphaOfLength(16);
9811001
final ApiKeyCredentials creds = new ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(apiKey1.toCharArray()));
@@ -1130,7 +1150,7 @@ public static Authentication createApiKeyAuthentication(ApiKeyService apiKeyServ
11301150
List<RoleDescriptor> keyRoles,
11311151
Version version) throws Exception {
11321152
XContentBuilder keyDocSource = apiKeyService.newDocument(
1133-
new SecureString(randomAlphaOfLength(16).toCharArray()), "test", authentication,
1153+
getFastStoredHashAlgoForTests().hash(new SecureString(randomAlphaOfLength(16).toCharArray())), "test", authentication,
11341154
userRoles, Instant.now(), Instant.now().plus(Duration.ofSeconds(3600)), keyRoles, Version.CURRENT,
11351155
randomBoolean() ? null : org.elasticsearch.core.Map.of(
11361156
randomAlphaOfLengthBetween(3, 8), randomAlphaOfLengthBetween(3, 8)));

0 commit comments

Comments
 (0)