Provide a method to retrieve a closeable char[] from a SecureString #23389
Conversation
This change adds a new class, CloseableChars, that can be retrieved from an instance of a SecureString. This wrapper class contains a copy of the char[] that backs the SecureString and can also be closed. Closing the wrapper will wipe the char[] that backs the wrapper but the SecureString itself remains unaffected. Additionally, while making a separate change I found that SecureSettings will fail when diff is called on them and there is no fallback setting. Given the idea behind SecureSetting, I think that diff should just be a no-op and I have implemented this here as well.
jaymode
added
:Core/Infra/Settings
| * char[]. For example: | ||
| * | ||
| * <pre> | ||
| * try (CloseableChars closeableChars = secureString.getAsCloseableChars()) { |
There was a problem hiding this comment.
I'm confused on why you wouldn't just close the secureString? If we must have the ability get the the char[] so be it, and we add it to SecureString with sufficient warnings in the javadocs, but it seems this is adding an extra layer of indirection that would provide confusion, not utility.
There was a problem hiding this comment.
I'm confused on why you wouldn't just close the secureString?
I thought we were going to clear the value out of the in memory keystore at some point once we change the code to only read these during initialization (from Bootstrap.java):
/* TODO: close this once s3 repository doesn't try to read during repository construction
try {
// any secure settings must be read during node construction
IOUtils.close(keystore);
} catch (IOException e) {
throw new BootstrapException(e);
}*/The reasoning behind the additional class is that it provides a way to pass a copy of the chars to another consumer and prevents this consumer from manipulating the backing char[] of the secure string, while also providing an easy way to clear the array once it is finished being used. The secure string may need to be used in multiple places so I do not think it is safe to pass the only copy of the char[] to other code.
Maybe a simpler option would be to add a method to get the underlying chars and another method that returns a new copy of the secure string? Then the code would be something like:
try (SecureString copy = secureString.newCopy()) {
// execute code that uses the char[] one time and no longer needs it such as the keystore API
KeyStore store = KeyStore.getInstance("jks");
store.load(inputStream, copy.getChars());
}
| * ... | ||
| * } | ||
| * </pre> | ||
| */ | ||
| public synchronized CloseableChars getAsCloseableChars() { | ||
| public synchronized SecureString newCopy() { |
There was a problem hiding this comment.
Why wouldn't we just override clone()?
| * // execute code that uses the char[] one time and no longer needs it such as the keystore API | ||
| * KeyStore store = KeyStore.getInstance("jks"); | ||
| * store.load(inputStream, closeableChars.getUnderlyingChars()); | ||
| * store.load(inputStream, copy.getChars()); |
There was a problem hiding this comment.
I wonder if this new method/accessing the char[] is something we should just deprecate immediately? Long term I don't think eg we should not be using the keystore to store a password for another keystore; the contents of the other keystore should be moved into the elasticsearch keystore.
There was a problem hiding this comment.
I also don't think a copy should be made for this example. The secure string should be closed after using it to decrypt the keystore.
…23389) This change adds a new method that returns the underlying char[] of a SecureString and the ability to clone the SecureString so that the original SecureString is not vulnerable to modification. Closing the cloned SecureString will wipe the char[] that backs the clone but the original SecureString remains unaffected. Additionally, while making a separate change I found that SecureSettings will fail when diff is called on them and there is no fallback setting. Given the idea behind SecureSetting, I think that diff should just be a no-op and I have implemented this here as well.
This change adds a new class, CloseableChars, that can be retrieved from an instance of a SecureString. This wrapper class contains a copy of the char[] that backs the SecureString and can also be closed. Closing the wrapper will wipe the char[] that backs the wrapper but the SecureString itself remains unaffected.
Additionally, while making a separate change I found that SecureSettings will fail when diff is called on them and there is no fallback setting. Given the idea behind SecureSetting, I think that diff should just be a no-op and I have implemented this here as well.