Preserve ApiKey credentials for async verification #51244

tvernum · 2020-01-21T05:32:43Z

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.

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.

elasticmachine · 2020-01-21T05:32:45Z

Pinging @elastic/es-security (:Security/Authentication)

tvernum · 2020-01-21T05:41:14Z

...plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java

+        // The second authentication should pass (but not immediately, but will not block)
+        PlainActionFuture<AuthenticationResult> future2 = new PlainActionFuture<>();
+
+        service.authenticateWithApiKeyIfPresent(threadPool.getThreadContext(), future2);


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

SecureString has already been closed

ywangd · 2020-01-21T08:01:36Z

This is good catch for race conditions. The fix itself is easy to follow. The test code is more involved and took me sometime to understand it. But I don't have any suggestions on how it can be made simpler. Race condition testing is mostly tricky anyway. Other than the one comment about wrapping ActionListener earlier, everything else LGTM.

jkakavas

LGTM

jkakavas · 2020-01-21T10:36:41Z

...plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java

+        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


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

tvernum · 2020-01-24T00:36:15Z

Other than the one comment about wrapping ActionListener earlier, everything else LGTM.

I can't see any comment from you @ywangd, can you point me to it?

ywangd

This overall LGTM.

ywangd · 2020-01-21T07:55:15Z

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

-                            validateApiKeyCredentials(docId, source, credentials, clock, listener);
-                        }
+                        final Map<String, Object> source = response.getSource();
+                        validateApiKeyCredentials(docId, source, credentials, clock, ActionListener.wrap(


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.

Good idea, I'll see how it looks.

ywangd · 2020-01-21T08:07:46Z

...plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java

+            hashWait.notify();
+        }
+
+        assertThat(future1.actionGet(TimeValue.timeValueSeconds(2)).isAuthenticated(), is(true));


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

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.

Thanks that makes sense.

ywangd · 2020-01-21T08:09:40Z

...plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyServiceTests.java

+        ApiKeyService realService = createApiKeyService(Settings.EMPTY);
+        ApiKeyService service  = Mockito.spy(realService);
+
+        final Object hashWait = new Object();


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

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.

- Wrap the listener in 1 place - Use a semaphore instead of Object.wait

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpProxy.java

…tcher/common/http/HttpProxy.java

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: elastic#51244

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

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: elastic#51244

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

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: elastic#51244

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

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: elastic#51244

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

tvernum added >bug :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.5.2 v6.8.7 v7.7.0 v7.6.1 labels Jan 21, 2020

tvernum requested review from ywangd and jkakavas January 21, 2020 05:32

tvernum added v7.5.3 and removed v7.5.2 labels Jan 21, 2020

tvernum commented Jan 21, 2020

View reviewed changes

jkakavas approved these changes Jan 21, 2020

View reviewed changes

tvernum added 3 commits January 23, 2020 16:21

Merge branch 'master' into fix/api-key-sync-close

0858ef2

Fix comment

b585ab8

Merge branch 'master' into fix/api-key-sync-close

4100f2c

ywangd approved these changes Jan 24, 2020

View reviewed changes

tvernum added 3 commits January 24, 2020 14:54

Address feedback

dbf164a

- Wrap the listener in 1 place - Use a semaphore instead of Object.wait

Merge branch 'master' into fix/api-key-sync-close

cfe8e76

Fix import

b59c67a

tvernum commented Jan 24, 2020

View reviewed changes

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/common/http/HttpProxy.java Outdated Show resolved Hide resolved

Update x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/wa…

a303bd4

…tcher/common/http/HttpProxy.java

tvernum merged commit f68f513 into elastic:master Jan 24, 2020

tvernum added the backport pending label Jan 24, 2020

tvernum mentioned this pull request Jan 24, 2020

[Backport 7.x] Preserve ApiKey credentials for async verification #51389

Merged

tvernum mentioned this pull request Jan 24, 2020

[Backport 7.6] Preserve ApiKey credentials for async verification #51407

Merged

tvernum mentioned this pull request Feb 3, 2020

[Backport 7.5] Preserve ApiKey credentials for async verification #51797

Merged

tvernum mentioned this pull request Feb 4, 2020

Preserve ApiKey credentials for async verification #51845

Merged

tvernum removed the backport pending label Feb 4, 2020

$@polyfractal$ polyfractal added v7.6.0 and removed v7.6.1 labels Feb 7, 2020

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 3) elastic/elasticsearch-net#4534

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve ApiKey credentials for async verification #51244

Preserve ApiKey credentials for async verification #51244

tvernum commented Jan 21, 2020

elasticmachine commented Jan 21, 2020

tvernum Jan 21, 2020

ywangd commented Jan 21, 2020 •

edited

Loading

jkakavas left a comment •

edited

Loading

jkakavas Jan 21, 2020

tvernum commented Jan 24, 2020 •

edited

Loading

ywangd left a comment

ywangd Jan 21, 2020

tvernum Jan 24, 2020

ywangd Jan 21, 2020

tvernum Jan 24, 2020

ywangd Jan 24, 2020

ywangd Jan 21, 2020

tvernum Jan 24, 2020

	// 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

Preserve ApiKey credentials for async verification #51244

Preserve ApiKey credentials for async verification #51244

Conversation

tvernum commented Jan 21, 2020

elasticmachine commented Jan 21, 2020

Choose a reason for hiding this comment

ywangd commented Jan 21, 2020 • edited Loading

jkakavas left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum commented Jan 24, 2020 • edited Loading

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Jan 21, 2020 •

edited

Loading

jkakavas left a comment •

edited

Loading

tvernum commented Jan 24, 2020 •

edited

Loading