[6.8 backport] Fix keystore thread safety (#12233) #12237
Merged
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
Clean backport of elastic#12233 This commit is intended to fix thread safety issues with the JavaKeystore implementation of the secret store. From reading the code, it appears that thread safety for the keystore was intended to be provided by a ReentrantReadWriteLock, a read lock for accessing secrets from the keystore, and a write lock for updating secrets in the keystore. In practice, this was insufficient, the act of accessing a secret from the keystore involved the mutation of a shared keyStore object - the keyStore is `load`ed every time a secret is retrieved from the store. Previous to elastic#10794, this did not matter, each pipeline held its own instance of the secret store, effectively meaning that only a single thread would ever access a key store at any one time. This PR moved to using a shared keystore instance for substitution variables, exposing the lack of thread safety in the JavaKeystore class. This commit is intended to be the simplest change to fix the underlying issue, and does not address whether we *need* to reload the secrets every time they are read. Relates elastic#12229
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.
Clean backport of #12233
This commit is intended to fix thread safety issues with the JavaKeystore implementation of the secret store.
From reading the code, it appears that thread safety for the keystore was intended to be provided by
a ReentrantReadWriteLock, a read lock for accessing secrets from the keystore, and a write lock for updating
secrets in the keystore.
In practice, this was insufficient, the act of accessing a secret from the keystore involved the mutation of
a shared keyStore object - the keyStore is
loaded every time a secret is retrieved from the store.Previous to #10794, this did not matter, each pipeline held its
own instance of the secret store, effectively meaning that only a single thread would ever access a key
store at any one time. This PR moved to using a shared keystore instance for substitution variables,
exposing the lack of thread safety in the JavaKeystore class.
This commit is intended to be the simplest change to fix the underlying issue, and does not address whether
we need to reload the secrets every time they are read.
Relates #12229