Fail if reading from closed KeyStoreWrapper #30394
Conversation
In elastic#28255 the implementation of the elasticsearch.keystore was changed to no longer be built on top of a PKCS#12 keystore. A side effect of that change was that calling getString or getFile on a closed KeyStoreWrapper ceased to throw an exception, and would instead return a value consisting of all 0 bytes. This change restores the previous behaviour as closely as possible. It is possible to retrieve the _keys_ from a closed keystore, but any attempt to get or set the entries will throw an IllegalStateException. The only divergence from the previous behaviour is that "isLoaded" will now return false if the keystore is closed, in keeping with the "loaded and retrievable" description in the parent javadoc.
tvernum
added
>bug
review
:Core/Infra/Settings
|
Pinging @elastic/es-core-infra |
|
I've (re-)labelled this If for some reason we decide not to put this into 6.3.0 then we can re-label and I'll update the release-notes accordingly. |
|
pinging @jkakavas since you were involved in the design of keystore v3 and might be interested |
There was a problem hiding this comment.
This looks mostly ok for fixing the immediate bug, but I think we need some followup work here to make this actually thread safe. A separate thread (eg a service in x-pack) could be in the middle of async startup, and have the keystore closed underneath it, while in the middle of reading a value (after the closed check).
I think we should fix this here unless there is some immediate need to get this fix in. Making the methods synchronized should be enough to accomplish this |
|
I agree @jaymode. At first I thought we might want something that would allow parallel reading, but these are only read during startup, and there aren't that many. Let's start with synchronized and revisit later if necessary. |
- synchronize methods - ensureOpen before encrypt or save
Save needs to be sychronized (so that the data doesn't get zero'd out while being written to disk) Encrypt is only called from save, so it can stay as an assert isLoaded rather than ensureOpen
|
I've made three changes to this PR:
The last one is a bit of an issue. At the moment there is no method that allows a caller to check that it is safe to read from a I think we may need a new |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
run gradle build tests (again) 😿 |
|
@rjernst I've updated this since you reviewed - are you good with the changes? |
In #28255 the implementation of the elasticsearch.keystore was changed to no longer be built on top of a PKCS#12 keystore. A side effect of that change was that calling getString or getFile on a closed KeyStoreWrapper ceased to throw an exception, and would instead return a value consisting of all 0 bytes. This change restores the previous behaviour as closely as possible. It is possible to retrieve the _keys_ from a closed keystore, but any attempt to get or set the entries will throw an IllegalStateException.
In #28255 the implementation of the elasticsearch.keystore was changed to no longer be built on top of a PKCS#12 keystore. A side effect of that change was that calling getString or getFile on a closed KeyStoreWrapper ceased to throw an exception, and would instead return a value consisting of all 0 bytes. This change restores the previous behaviour as closely as possible. It is possible to retrieve the _keys_ from a closed keystore, but any attempt to get or set the entries will throw an IllegalStateException.
* 6.x: Revert "Silence IndexUpgradeIT test failures. (#30430)" [DOCS] Remove references to changelog and to highlights Revert "Mute ML upgrade test (#30458)" [ML] Fix BWC version for backport of #30125 [Docs] Improve section detailing translog usage (#30573) [Tests] Relax allowed delta in extended_stats aggregation (#30569) Fail if reading from closed KeyStoreWrapper (#30394) [ML] Reverse engineer Grok patterns from categorization results (#30125) Derive max composite buffers from max content len Update build file due to doc file rename SQL: Extract SQL request and response classes (#30457) Remove the changelog (#30593) Revert "Add deprecation warning for default shards (#30587)" Silence IndexUpgradeIT test failures. (#30430) Add deprecation warning for default shards (#30587) [DOCS] Adds 6.4.0 release highlight pages [DOCS] Adds release highlight pages (#30590) Docs: Document how to rebuild analyzers (#30498) [DOCS] Fixes title capitalization in security content LLRest: Add equals and hashcode tests for Request (#30584) [DOCS] Fix realm setting names (#30499) [DOCS] Fix path info for various security files (#30502) Docs: document precision limitations of geo_bounding_box (#30540) Fix non existing javadocs link in RestClientTests Auto-expand replicas only after failing nodes (#30553)
…ngs-to-true * elastic/master: [DOCS] Restores 7.0.0 release notes and highlights Remove assert statements from field caps documentation. (elastic#30601) Repository GCS plugin new client library (elastic#30168) HLRestClient: Follow-up for put index template api (elastic#30592) Unmute IndexUpgradeIT tests [DOCS] Remove references to changelog and to highlights Side-step pending deletes check (elastic#30571) [DOCS] Remove references to removed changelog Revert "Mute ML upgrade test (elastic#30458)" [ML] Adjust BWC version following backport of elastic#30125 [Docs] Improve section detailing translog usage (elastic#30573) [Tests] Relax allowed delta in extended_stats aggregation (elastic#30569) [ML] Reverse engineer Grok patterns from categorization results (elastic#30125) Update build file due to doc file rename Remove the changelog (elastic#30593) Fix issue with finishing handshake in ssl driver (elastic#30580) Fail if reading from closed KeyStoreWrapper (elastic#30394) Silence IndexUpgradeIT test failures. (elastic#30430)
In #28255 the implementation of the elasticsearch.keystore was changed
to no longer be built on top of a PKCS#12 keystore. A side effect of
that change was that calling getString or getFile on a closed
KeyStoreWrapper ceased to throw an exception, and would instead return
a value consisting of all 0 bytes.
This change restores the previous behaviour as closely as possible.
It is possible to retrieve the keys from a closed keystore, but any
attempt to get or set the entries will throw an IllegalStateException.
The only divergence from the previous behaviour is that "isLoaded"
will now return false if the keystore is closed, in keeping with the
"loaded and retrievable" description in the parent javadoc.