Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve ApiKey credentials for async verification #51244

Merged
merged 8 commits into from
Jan 24, 2020
Merged

Preserve ApiKey credentials for async verification #51244

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
45963e2
Preserve ApiKey credentials for async verification
tvernum Jan 21, 2020
0858ef2
Merge branch 'master' into fix/api-key-sync-close
tvernum Jan 23, 2020
b585ab8
Fix comment
tvernum Jan 23, 2020
4100f2c
Merge branch 'master' into fix/api-key-sync-close
tvernum Jan 23, 2020
dbf164a
Address feedback
tvernum Jan 24, 2020
cfe8e76
Merge branch 'master' into fix/api-key-sync-close
tvernum Jan 24, 2020
b59c67a
Fix import
tvernum Jan 24, 2020
a303bd4
Update x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/wa…
tvernum Jan 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 11 additions & 5 deletions ...k/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,10 +301,15 @@ void authenticateWithApiKeyIfPresent(ThreadContext ctx, ActionListener<Authentic
.request();
executeAsyncWithOrigin(ctx, SECURITY_ORIGIN, getRequest, ActionListener.<GetResponse>wrap(response -> {
if (response.isExists()) {
try (ApiKeyCredentials ignore = credentials) {
final Map<String, Object> source = response.getSource();
validateApiKeyCredentials(docId, source, credentials, clock, listener);
}
final Map<String, Object> source = response.getSource();
validateApiKeyCredentials(docId, source, credentials, clock, ActionListener.wrap(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A minor comment: Is it worthwhile to wrap the ActionListeners earlier on so that we don't have to call credentials.close() in multiple places. That is once we know credentials is not null, we can have:

var credentialsClosingListener = ActionListener.wrap(...);

Then just use the new listener in subsequent code. At this point, it may be worthwhile to extract this part of code into a new method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I'll see how it looks.

result -> {
credentials.close();
listener.onResponse(result);
}, e -> {
credentials.close();
listener.onFailure(e);
}));
} else {
credentials.close();
listener.onResponse(
Expand Down Expand Up @@ -535,7 +540,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());
Expand Down
117 changes: 96 additions & 21 deletions ...gin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
Expand Down Expand Up @@ -41,6 +42,7 @@
import org.elasticsearch.xpack.security.test.SecurityMocks;
import org.junit.After;
import org.junit.Before;
import org.mockito.Mockito;

import java.io.IOException;
import java.nio.charset.StandardCharsets;
Expand All @@ -54,6 +56,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
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;
Expand Down Expand Up @@ -431,16 +434,7 @@ public void testApiKeyCache() {
Hasher hasher = randomFrom(Hasher.PBKDF2, Hasher.BCRYPT4, Hasher.BCRYPT);
final char[] hash = hasher.hash(new SecureString(apiKey.toCharArray()));

Map<String, Object> 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<String, Object> creatorMap = new HashMap<>();
creatorMap.put("principal", "test_user");
creatorMap.put("metadata", Collections.emptyMap());
sourceMap.put("creator", creatorMap);
sourceMap.put("api_key_invalidated", false);
Map<String, Object> sourceMap = buildApiKeySourceDoc(hash);

ApiKeyService service = createApiKeyService(Settings.EMPTY);
ApiKeyCredentials creds = new ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(apiKey.toCharArray()));
Expand Down Expand Up @@ -488,6 +482,66 @@ 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<String, Object> sourceMap = buildApiKeySourceDoc(hash);

ApiKeyService realService = createApiKeyService(Settings.EMPTY);
ApiKeyService service = Mockito.spy(realService);

final Object hashWait = new Object();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trivial: Could probably replace this with a CompletableFuture object? If so, could save a couple of lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used a semaphore instead, but thanks for the prompt. When I wrote the test I planned to come back and replace the wait/notify with something else after it was all working, but I forgot.

final AtomicInteger hashCounter = new AtomicInteger(0);
doAnswer(invocationOnMock -> {
hashCounter.incrementAndGet();
synchronized (hashWait) {
hashWait.wait();
}
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<AuthenticationResult> future1 = new PlainActionFuture<>();

// Call the top level method because it has been know to be buggy in async situations
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Call the top level method because it has been know to be buggy in async situations
// Call the top level 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<AuthenticationResult> future2 = new PlainActionFuture<>();

service.authenticateWithApiKeyIfPresent(threadPool.getThreadContext(), future2);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the fix to not close the credentials, this line would fail with:

SecureString has already been closed


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());
}

synchronized (hashWait) {
hashWait.notify();
}

assertThat(future1.actionGet(TimeValue.timeValueSeconds(2)).isAuthenticated(), is(true));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly curious: it really takes 2 seconds for the notify to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but in these tests there's a delicate balance between not having the tests run too long and not getting noise.
This should be done in milliseconds. But if the VM that running CI get slowed down for some reason, or we get really long GC pause, and this takes 1.5 seconds, that shouldn't cause the test to fail.
We want a time that is long enough that the timeout means "this is never going to happen, the test failed" but not so long that you're sitting there wondering if the test is ever going to be done.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks that makes sense.

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);
Expand All @@ -496,16 +550,7 @@ public void testApiKeyCacheDisabled() {
.put(ApiKeyService.CACHE_TTL_SETTING.getKey(), "0s")
.build();

Map<String, Object> 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<String, Object> creatorMap = new HashMap<>();
creatorMap.put("principal", "test_user");
creatorMap.put("metadata", Collections.emptyMap());
sourceMap.put("creator", creatorMap);
sourceMap.put("api_key_invalidated", false);
Map<String, Object> sourceMap = buildApiKeySourceDoc(hash);

ApiKeyService service = createApiKeyService(settings);
ApiKeyCredentials creds = new ApiKeyCredentials(randomAlphaOfLength(12), new SecureString(apiKey.toCharArray()));
Expand All @@ -517,10 +562,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<String, Object> buildApiKeySourceDoc(char[] hash) {
Map<String, Object> 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<String, Object> 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<String, Object> sourceMap) throws IOException {
try (XContentBuilder builder = JsonXContent.contentBuilder()) {
builder.map(sourceMap);
SecurityMocks.mockGetRequest(client, id, BytesReference.bytes(builder));
}
}

}