Reloadable SecureSettings

[albertzaharovits]

This is my current work in progress, which is imperfect and I need advice, please:

I have changed the SecretSettings interface to have unlock(char[] pass) and lock() methods, that will guard calls to getString and getFile. The unlock(char[] pass) call (usable in a try-with-resource) will read the keystore contents from disk and cache the password, so that subsequent get* calls retrieve and decrypt the latest entries (from the last unlock). The KeyStoreWrapper implements this interface; I have experimented with another wrapper (knowing that the keystore implementation will change) but it turned messy andKeyStoreWrapper required rewriting anyway to remove the set* methods (which would complicate unlock and multithreading, and are only required when building keystores, therefore have been moved to a Builder class); I might change my mind on this, but I digress...
The bigger picture is that the reload action will call unlock (from a try-with-resource) on the SecretSettings instance from the node's Settings and then iterate over AbstractComponents implementing some interface that will reinitialize the AbstractComponent (maybe a new Lifecycle transition); the point is, that everything relies on the existing Settings behavior.

The stuff that I need advice on:
I don't like the (soft) requirement that secure setting names are fixed but values are dynamic. If we are ok with this, then should all the settings be reloadable? Otherwise I think SecureSettings should move out of Settings, in their own component (service, maybe)?

Notes:
Does not compile, all relevant code is in KeyStoreWrapper.java and SecureSettings.java.

[tvernum]

With respect to the AbstractComponent part, I wonder whether we ought to take a path more like the way we currently handle dynamic non-secure settings, with listeners rather than lifecycles.

That is:

1. When defining a secure setting, you would mark it reloadable
2. You would register a callback to be called when secure settings are changed.

However, (1) is tricky.

• On the one hand, I don't think we can assume that every secure setting can be reloaded into a running node. We don't do that for non-secure settings, why should it be true for all secure settings?
• But we can't (and don't want to) say that the non-reloadable secure settings cannot be modified (within the keystore) while node is running, so what would we do if a non-reloadable setting were changed, and how would we even know? (we could keep a hash the old value, but that's getting into quite a degree of compexity)

For (2) I think a single consumer across the whole set of secure settings is more useful than a consumer for each setting (like the existing AbstractScopedSettings.addSettingsUpdateConsumer). We don't want a component to reinitialise itself multiple times if several secure settings are changed in a single reload.
But having a "I want to subscribe to SecureSetting" updates seems like a nicer approach than forcing in an additional lifecycle stage.

[rjernst]

I agree with everything @tvernum said.

[albertzaharovits]

Thank you @rjernst and @tvernum .
I think I might have strayed the discussion in a place I was least prepared, but in the end that's for the better :)
My first sub-task was to have a reloadable SecureSettings implementation and neglecting the context where it will be used. I think there is no better alternative than to re-read the keystore from disk, and my implementation does exactly that. I was trying to contrast the reloadability with the immutability of the Settings object and the necessity to decouple them.

Nevertheless, starting from what @tvernum proposed I am thinking to have it like this:

Have secure settings stored in the cluster state with the value being a pointer indirection to the actual value inside the keystore (or other repository).
This way, Setting<T>.Property#Dynamic and ClusterSettings#addSettingsUpdateConsumer will assure reloadability in the usual way, the same way plaintext settings are dynamic right now.
There would be another property marker, eg Setting<T>.Property#Secure to label such settings, and the AbstractScopedSettings will handle the indirection; it will require a SecureSettings instance (decoupled from the usual Settings object) and the password.

Not sure how the password and the SecureSettings will meet when the indirection has to be resolved, but what'd you say about this?

[rjernst]

I think that would be complicated to try to reuse the dynamic cluster settings infrastructure. As @tvernum pointed out, it doesn't make sense to reload these settings one by one. Anything using secure settings will likely want to reload their state completely based on the new SecureSettings. I think a simple callback for secure settings updates that take the new SecureSettings (plus Settings, which will be needed to rebuild eg aws s3 config objects).

Also, as a side note, please try to narrow the PR to only the changes necessary. I noticed there are a number of rewrites of existing javadocs and method names (eg decrypt -> unlock). If you think those changes are worthwhile (I disagree with the method rename and not sure about the javadoc changes), please do so in separate PRs.

[jasontedor]

Also, as a side note, please try to narrow the PR to only the changes necessary.

+1; every change should be as small and as coherent (a single unit) as possible, and no smaller.

[elasticmachine]

Pinging @elastic/es-core-infra

[jaymode]

@albertzaharovits can we close this?

[albertzaharovits]

yes, sorry for letting it linger.
The progress on this is tracked by #29135 .