Skip to content

[7.17] Improve security-crypto threadpool overflow handling (#111369) #111567

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.

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

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

gwbrown
Copy link
Contributor

@gwbrown gwbrown commented Aug 2, 2024

Backports the following commits to 7.17:

Prior to this PR, when the security-crypto threadpool queue overflows and rejects API key hashing submissions, a toxic value (specifically, a future which will never be completed) is added to the API key auth cache. This toxic cache value causes future authentication attempts with that API key to fail by timeout, because they will attempt to wait for the toxic future, until that value is invalidated and removed from the cache. Additionally, this will hold on to memory for each request that waits on the toxic future, even after the request has timed out.

This PR adds a unit test to replicate this case, and adjusts the code which submits the key hashing task to the security-crypto threadpool to properly handle this point of failure by invalidating the cached future and notifying waiting handlers that the computation has failed.
@gwbrown gwbrown added >bug :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) backport auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) v7.17.24 labels Aug 2, 2024
@gwbrown gwbrown requested a review from jakelandis August 5, 2024 01:08
@gwbrown
Copy link
Contributor Author

gwbrown commented Aug 5, 2024

@jakelandis Any chance you could give me a +1 here to satisfy the branch protection rules? There should be no significant changes from the 8.x version, just compilation fixes.

Copy link
Contributor

@jakelandis jakelandis left a comment

Choose a reason for hiding this comment

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

LGTM

@gwbrown gwbrown merged commit 231b616 into elastic:7.17 Aug 5, 2024
11 checks passed
@gwbrown gwbrown deleted the backport/111369 branch August 5, 2024 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) backport >bug :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v7.17.24
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants