-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Add InMemoryClonedSecureSettings for keeping temporary secure settings loaded #134349
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
Add InMemoryClonedSecureSettings for keeping temporary secure settings loaded #134349
Conversation
|
Pinging @elastic/es-security (Team:Security) |
|
Hi @jfreden, I've created a changelog YAML for you. |
mosche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imho if turning this into a general util, this requires a bit more care in a few places. See the comments below.
I'm just wondering, are there no security concerns around providing such a simple util to work around the purpose of secure settings?
server/src/main/java/org/elasticsearch/common/settings/LoadedSecureSettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/settings/LoadedSecureSettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/settings/LoadedSecureSettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/settings/LoadedSecureSettings.java
Outdated
Show resolved
Hide resolved
I'm suspecting that's why this has not been added before and that was also my hesitation. This utility class works against the current intention of the KeyStoreWrapper - that the settings are only available during the reload to refresh some state and then dropped. The security implication is that the secure settings that are "always loaded" will be in memory and visible in a heap dump. The secure settings approach we're taking with the KeyStoreWrapper isn't immune to that, even if the settings are just loaded for a while they can be extracted and stored in some state past the reload. I think ideally the settings would be extracted and stored in an instance variable of the class that uses it. The problem this PR solves is that the actual Settings object is needed at some later point by the code we're using and having some intermediate representation of the settings becomes hacky. |
|
In practice, we need to retain some secure settings in memory. They're used in places that are dynamic - typically because the secure setting is paired with some other dynamic cluster settings - and we can't clear them from memory after the first use. We definitely don't want to retain all secure settings in memory, but making it hard to do the thing we have to do, doesn't help anyone. It just forces duplication of the same work-arounds, often with poorer security than we would have gotten if we'd just implemented something that filled the need. I think we can come up with a better name that |
|
Thanks for the feedback! I've changed the name to |
server/src/main/java/org/elasticsearch/common/settings/ClonedSecureSettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/settings/ClonedSecureSettings.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/common/settings/ClonedSecureSettings.java
Outdated
Show resolved
Hide resolved
How about
keeping it out of core was never my intention, if we highlight the compromise and make devs well aware (as done in the Javadocs now) I'm totally fine with this |
mosche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm provided tests pass, just a few optional suggestions
…ecureSettings.java Co-authored-by: Moritz Mack <[email protected]>
…ecureSettings.java Co-authored-by: Moritz Mack <[email protected]>
…ecureSettings.java Co-authored-by: Moritz Mack <[email protected]>
The pattern to load secure settings for later use during secure settings reload has been repeated across the code base and is now needed by #134137. This PR extracts the functionality to a
InMemoryClonedSecureSettingsutility class.