Keystore improvements. #17404
Keystore improvements. #17404
Conversation
…nfig, safely resolve keystore file and classname from the settings if available. If they both not available, it might be that user intentionally turned off the keystore otherwise require both.
|
This pull request does not have a backport label. Could you fix it @mashhurs? 🙏
|
mashhurs
added
backport-active-9
|
💚 Build Succeeded
|
There was a problem hiding this comment.
Could you share more context on the purpose of this change? What specific problem does it address from the user's perspective?
keystore.file and keystore.classname are not user facing setting and are not listed in document. Both have assigned with default values. I am not sure how user will end up setting these value to empty string.
I am trying to understand how users interact with the setting and what other value can be set to keystore.classname other than the default. In order to load a custom class, the class needs to implement SecretStore. This is pretty advance use case. Does any user config this setting?
That is the reason why this PR is draft! :)
If settings are not documented, does it mean LS is not expected to accept them through configs?
I myself also not sure about the expectation. Let's get some input from @jsvd!
|
There was a problem hiding this comment.
I'm unclear on the value in allowing a user to provide null for either keystore.file or keystore.classname; if a user doesn't want to use the keystore, they can simply invoke Logstash without having created a keystore.
In Logstash 8.17.4, setting either or both of these to null results in a TypeError that prevents pipelines from starting:
-
only
keystore.fileis null:printf "%s\n" 'keystore.file: null' > config/logstash.yml bin/logstash -e 'input { generator { count => 1} }'Click me
[2025-04-08T19:53:25,171][ERROR][logstash.agent ] Failed to execute action {:action=>LogStash::PipelineAction::Create/pipeline_id:main, :exception=>"TypeError", :message=>"nil is not a string", :backtrace=>["org/logstash/execution/AbstractPipelineExt.java:293:in `initialize'", "org/logstash/execution/AbstractPipelineExt.java:227:in `initialize'", "/Users/rye/src/elastic/sdh-logstash/1634-elasticsearch-filter-missing/logstash-8.17.4/logstash-core/lib/logstash/java_pipeline.rb:47:in `initialize'", "org/jruby/RubyClass.java:949:in `new'", "/Users/rye/src/elastic/sdh-logstash/1634-elasticsearch-filter-missing/logstash-8.17.4/logstash-core/lib/logstash/pipeline_action/create.rb:50:in `execute'", "/Users/rye/src/elastic/sdh-logstash/1634-elasticsearch-filter-missing/logstash-8.17.4/logstash-core/lib/logstash/agent.rb:420:in `block in converge_state'"]} -
when only
keystore.classnameis null:printf "%s\n" 'keystore.classname: null' > config/logstash.yml bin/logstash -e 'input { generator { count => 1} }'Click me
[2025-04-08T19:54:06,183][ERROR][logstash.agent ] Failed to execute action {:action=>LogStash::PipelineAction::Create/pipeline_id:main, :exception=>"TypeError", :message=>"nil is not a string", :backtrace=>["org/logstash/execution/AbstractPipelineExt.java:293:in `initialize'", "org/logstash/execution/AbstractPipelineExt.java:227:in `initialize'", "/Users/rye/src/elastic/sdh-logstash/1634-elasticsearch-filter-missing/logstash-8.17.4/logstash-core/lib/logstash/java_pipeline.rb:47:in `initialize'", "org/jruby/RubyClass.java:949:in `new'", "/Users/rye/src/elastic/sdh-logstash/1634-elasticsearch-filter-missing/logstash-8.17.4/logstash-core/lib/logstash/pipeline_action/create.rb:50:in `execute'", "/Users/rye/src/elastic/sdh-logstash/1634-elasticsearch-filter-missing/logstash-8.17.4/logstash-core/lib/logstash/agent.rb:420:in `block in converge_state'"]} -
when both are null:
printf "%s\n" 'keystore.file: null' 'keystore.classname: null' > config/logstash.yml bin/logstash -e 'input { generator { count => 1} }'Click me
[2025-04-08T19:55:20,126][ERROR][logstash.agent ] Failed to execute action {:action=>LogStash::PipelineAction::Create/pipeline_id:main, :exception=>"TypeError", :message=>"nil is not a string", :backtrace=>["org/logstash/execution/AbstractPipelineExt.java:293:in `initialize'", "org/logstash/execution/AbstractPipelineExt.java:227:in `initialize'", "/Users/rye/src/elastic/sdh-logstash/1634-elasticsearch-filter-missing/logstash-8.17.4/logstash-core/lib/logstash/java_pipeline.rb:47:in `initialize'", "org/jruby/RubyClass.java:949:in `new'", "/Users/rye/src/elastic/sdh-logstash/1634-elasticsearch-filter-missing/logstash-8.17.4/logstash-core/lib/logstash/pipeline_action/create.rb:50:in `execute'", "/Users/rye/src/elastic/sdh-logstash/1634-elasticsearch-filter-missing/logstash-8.17.4/logstash-core/lib/logstash/agent.rb:420:in `block in converge_state'"]}
I believe this can be addressed more simply by just erroring helpfully if either setting is null. It appears that there was a regression in a recent settings refactor in which the supposedly-non-nullable SettingString incorrectly accepted null values. Fixing this in #17522 makes this PR a non-issue.
| if (keystoreFile == null || keystoreClassname == null) { | ||
| throw new IllegalArgumentException("`keystore.file` and `keystore.classname` cannot be null"); | ||
| } |
There was a problem hiding this comment.
nit: while we know that the current path to these variables are the settings keystore.file and keystore.classname, I'm wary of making the exception reference those settings here -- we don't know the future uses of the secretstore.
| if (keystoreFile == null || keystoreClassname == null) { | |
| throw new IllegalArgumentException("`keystore.file` and `keystore.classname` cannot be null"); | |
| } | |
| Objects.requireNonNull(keystoreFile, "keystoreFile"); | |
| Objects.requireNonNull(keystoreClassname, "keystoreClassname"); |
| if (keystoreFile == null && keystoreClassname == null) { | ||
| // explicitly set keystore and classname null | ||
| return null; | ||
| } | ||
|
|
||
| if (keystoreFile == null | keystoreClassname == null) { | ||
| throw new IllegalStateException("Setting `keystore.file` requires `keystore.classname`, or vice versa"); | ||
| } |
There was a problem hiding this comment.
nit: the mix of bitwise logic and conditional logic works, but is surprising to me.
I would also rather have an else-if chain to provide specific error messages instead of grouping them up.
| if (keystoreFile == null && keystoreClassname == null) { | |
| // explicitly set keystore and classname null | |
| return null; | |
| } | |
| if (keystoreFile == null | keystoreClassname == null) { | |
| throw new IllegalStateException("Setting `keystore.file` requires `keystore.classname`, or vice versa"); | |
| } | |
| if (keystoreFile == null && keystoreClassname == null) { | |
| // explicitly set keystore and classname null | |
| return null; | |
| } else if (keystoreFile == null) { | |
| throw new IllegalStateException("Setting `keystore.file` is required when `keystore.classname` is provided"); | |
| } else if (keystoreClassname == null) { | |
| throw new IllegalStateException("Setting `keystore.classname` is required when `keystore.file` is provided"); | |
| } |
But separately and possibly more importantly, both keystore.file and keystore.classname are registered settings, so #hasSetting() will always return true for either of them, and the existing code will have already thrown a TypeError if either of them are null.
| @@ -842,15 +843,28 @@ protected final boolean hasSetting(final ThreadContext context, final String nam | |||
| } | |||
|
|
|||
| protected SecretStore getSecretStore(final ThreadContext context) { | |||
| String keystoreFile = hasSetting(context, "keystore.file") | |||
There was a problem hiding this comment.
It would be an illegal state for hasSetting(context, "keystore.file") to return false -- it would mean that the setting hasn't been registered (not simply that a value was not provided by the user).
| @@ -842,15 +843,28 @@ protected final boolean hasSetting(final ThreadContext context, final String nam | |||
| } | |||
|
|
|||
| protected SecretStore getSecretStore(final ThreadContext context) { | |||
| String keystoreFile = hasSetting(context, "keystore.file") | |||
| ? getSetting(context, "keystore.file").asJavaString() | |||
There was a problem hiding this comment.
When we send IRubyObject#asJavaString() to nil, we get a TypeError: nil is not a string. This effectively already safeguards against a nil/null value (although it could be done in a more clear way).
| String keystoreClassname = hasSetting(context, "keystore.classname") | ||
| ? getSetting(context, "keystore.classname").asJavaString() | ||
| : null; | ||
| return (keystoreFile != null && keystoreClassname != null) |
There was a problem hiding this comment.
Neither keystoreFile nor keystoreClassname can actually be null here.
|
Addressed by #17522 |
Release notes
[rn:skip]
What does this PR do?
keystore.fileandkeystore.classnamewhen generating a valid secure config (SecureConfiginSecretStoreExt).SecretStoreFactory#existsvalidates file existency and loads.Why is it important/What is the impact to the user?
Improves user experience
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files (and/or docker env variables)Author's Checklist
How to test this PR locally
keystore.fileandkeystore.classnamebin/logstash-keystore create/add/listkeystore.fileandkeystore.classnameinlogstash.yml)keystore.filerequireskeystore.classnameifkeystore.classnameis null, or vise-versaMY_VARto keystore is available in pipeline configs with"${MY_VAR}"Related issues
Use cases
Screenshots
Logs