[8.11] Fix memory leak from JWT cache (and fix the usage of the JWT auth cache) (#101799) #101841
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…he) (elastic#101799) This commit fixes a memory leak and ensures that the JWT authentication cache is actually used to short circuit authenticate. When JWT authentication is successful we cache a hash(JWT) -> User. On subsequent authentication attempts we should short circuit some of the more expensive validation thanks to this cache. The existing cache key is a `BytesArray` constructed from `jwtAuthenticationToken.getUserCredentialsHash()` (the hash value of the JWT). However, `jwtAuthenticationToken` is comes from the security context which is effectively a request scoped object. When the security context goes out of scope the value of `jwtAuthenticationToken.getUserCredentialsHash()` is zero'ed out to help keep sensitive data out of the heap. It is arguable if zero'ing out that data is useful especially for a hashed value, but is inline with the internal contract/expectations. Since the cache key is derived from `jwtAuthenticationToken.getUserCredentialsHash()` when that get object is zero'ed out, so is the cache key. This results in a cache key that changes from a valid value to an empty byte array. This results in a junk cache entry. Subsequent authentication with the same JWT will result in a new cache entry which will then follow the same pattern of getting zero'ed out. This results in a useless cache with nothing but zero'ed out cache keys. This negates any benefits of having a cache at all in that a full authentication is preformed all the time which can be expensive for JWT (especially since JWT requires role mappings and role mappings are not cached). Fortunately the default cache size is 100k (by count) so the actual memory leak is technically capped but can vary depending on how large the cache values. This is an approximate cap of ~55MB where 3.125 MB (100k * 256 bits) for the sha256 cache keys + ~50MB (100k * ~50 bytes) cache values, however, it is possible for the cache to be larger if the values in the cache are larger. The fix here is to ensure the cache key used is a copy of the value that is zero'ed out (before it is zero'ed out). fixes: elastic#101752 (cherry picked from commit cef2b80) # Conflicts: # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/jwt/JwtRealm.java
jakelandis
added
auto-merge-without-approval
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport
This will backport the following commits from
mainto8.11:Questions ?
Please refer to the Backport tool documentation