-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Integrate secret store with Logstash core (part 4) #8905
Conversation
This change introduces the command line tooling and hooks needed to allow Logstash to use the secret store. This change hooks into the same logic that the does the environment variable substitution. The commnad line mirrors the Elasticsearch command line, and is implemented primarily in Java. Part of elastic#8353
To manually test follow the instructions at https://www.elastic.co/guide/en/elasticsearch/reference/current/secure-settings.html , just replace To use the secret use |
Jenkins doesn't run on this feature branch, so here's the
|
@jordansissel - The |
A fresh keystore contains one key. This may be confusing to users?
For my test, I added an entry
|
|
||
private void create(SecureConfig config) { | ||
if (System.getenv(SecretStoreFactory.ENVIRONMENT_PASS_KEY) == null) { | ||
terminal.write(String.format("WARNING: The keystore password is not set. Please set the environment variable `%s`. Failure to do so will result in" + |
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.
WARNING: The keystore password is not set. Please set the environment variable
LOGSTASH_KEYSTORE_PASS
. Failure to do so will result in reduced security. Continue anyway ? [y/N]
It's unclear what behavior the user should expect by pressing y
here. Maybe "Continue without password protection on the keystore?"
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.
Continue without password protection on the keystore?
+ 1 , will update
I struggled a bit on the wording of this.
I didn't remember what the precedence order is, and testing it shows that the keystore has priority over environment variables. Noting this so when we write the docs, this order is documented. Testing what happens when an invalid entry is given (no env, no keystore entry):
👍 |
Testing both env and keystore:
|
re : keystore.seed This mirrors Elasticsearch's keystore in both name and function. |
index = ARGV.find_index("--path.settings") | ||
# strip out any path.settings from the command line | ||
unless index.nil? | ||
path_settings_value = ARGV.slice!(index, 2)[1] |
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.
Feels weird modifying the value of a constant (ARGV) here. No change necessarily required, but something to think on.
include LogStash::Util::Loggable | ||
|
||
begin | ||
index = ARGV.find_index("--path.settings") |
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.
This flag would benefit from users being able to discover it. I didn't see it listed in the --help
output:
% bin/logstash-keystore --help
Commands
--------
create - Creates a new Logstash keystore
list - List entries in the keystore
add - Add a value to the keystore
remove - Remove a value from the keystore
And doing --help
on a subcommand results in possibly confusing behavior:
% bin/logstash-keystore add --help
Enter value for --help:
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.
I'll add --help as a sub command
@@ -74,6 +75,9 @@ module Environment | |||
# Compute the default dead_letter_queue path based on `path.data` | |||
default_dlq_file_path = ::File.join(SETTINGS.get("path.data"), "dead_letter_queue") | |||
SETTINGS.register Setting::WritableDirectory.new("path.dead_letter_queue", default_dlq_file_path) | |||
# Compute the default secret store path based on `path.data` | |||
default_secret_store_file_path = ::File.join(SETTINGS.get("path.data"), "logstash.keystore") |
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.
The Elasticsearch docs say:
To create the elasticsearch.keystore, use the create command: ...
The file elasticsearch.keystore will be created alongside elasticsearch.yml.
However, bin/logstash-keystore create
creates this file in the path.data
directory:
% bin/logstash-keystore create
...
Created Logstash keystore at /home/jls/projects/logstash/data/logstash.keystore
Should this be path.config
instead? (It's user-configuration, so my vote is config, not data directory)
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.
Will update.
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.
Updated to path.settings
(next to logstash.yml), not path.config
(next to my_input.conf) ...seems like a better fit, and assume that is what you meant.
ls config
jvm.options log4j2.properties logstash.keystore logstash.yml pipelines.yml startup.options
It's unclear what purpose The two questions I have are: Will a user ever set this manually? Will a user ever need to reference it in their logstash configuration? I think both answers are no, right? If it's not a thing users need to interact with, we should not show it to them. |
No.
No
You clearly said you two questions and this is three, so I can't answer this one.
Will update to no longer show it. I only added as visible to match ES and some minor testing benefits. |
Noted! Consistency is a good approach. Maybe we can convince ES to remove this from their listing also. |
@jordansissel - Could you take another look ? All changes have been made as requested (with just tiny bit extra) on 6600bb0 The command line help is quite a bit more verbose now, please let me know if you think we should trim it down any. Ninja Edit: updated diff hash link |
I like the new help output.
The first warning warning doesn't seem to have enough information to take action. I believe I get this warning because I specify path.settings of /tmp and there is no /tmp/logstash.yml ? |
That warning is actually not new, and comes from the core code that checks for this. I agree is kinda of confusing. Can we consider that one outside the scope of this review ? |
Agreed, will change. I think i like argument slightly better. |
Where is LS_SETTINGS_DIR referenced ? |
I ran the tests via gradle and many failures:
Most (all? I haven't evaluated) of the failures are basically this:
|
terminal.writeLine(""); | ||
terminal.writeLine("Options:"); | ||
terminal.writeLine("--------"); | ||
terminal.writeLine("--path.settings - Set the directory for the keystore. This is should be the same directory as the logstash.yml settings file. " + |
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.
This is where LS_SETTINGS_DIR is referenced.
I'm ok with this, but I'm certain the first customers who experience this will file an issue which will be escalated to us. I'd prefer to avoid that human cost. It's likely a user providing --path.settings is probably rare, do you think? If expected to be rare, we can defer to another improvement of the message to another PR. |
this is looking very good! I'm not sure why the (ruby) test suite fails for me. Does it pass for you? |
@jakelandis let me know what else you need. Conditional LGTM if:
If all 3 above are resolved, this is good to merge. |
@jordansissel - Changes : 135819a
done.
Logged #8934 to address |
merging to feature branch. |
grr.... just found another issue with the global state/registration settings mess... |
GH won't let me add more commits here... so I pushed directly to feature branch 8bb47d9 I will have one last PR before this hits master, so if changes are requested I can make them there. |
…ta store. Fixes elastic#8657 Part 1: API and JavaKeyStore implementation (elastic#8657) Introduces the API to read/write/delete sensitive data from a secure store and includes a Java KeyStore implementation. Note - this commit does NOT integrate with the Logstash configuration or settings. Part 2: Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support (elastic#8659) * Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support * Introduce a SecretStoreFactory to allow runtime definition of SecretStore implementation. * Introduce a SecureConfig to allow simple configuration of different SecretStore implementaiton. * Introduce random default password plus obfuscation. Best attempt at security through obscurity. * Corrections / better support for x-JVM modification. Part 3: Secret Store: SecretStore, SecretStoreFactory, JavaKeystore - refacactor (elastic#8745) * Adds more CRUD like operations for SecretStore API * SecretStoreFactory Mirror API's CRUD operations * Adds 'exists' to API to allow command line warning 'Overwrite ?' * Minor readabiliy Part 4: Integrate secret store with Logstash core (elastic#8905) This change introduces the command line tooling and hooks needed to allow Logstash to use the secret store. This change hooks into the same logic that the does the environment variable substitution. The commnad line mirrors the Elasticsearch command line, and is implemented primarily in Java.
…ta store. Fixes elastic#8657 Part 1: API and JavaKeyStore implementation (elastic#8657) Introduces the API to read/write/delete sensitive data from a secure store and includes a Java KeyStore implementation. Note - this commit does NOT integrate with the Logstash configuration or settings. Part 2: Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support (elastic#8659) * Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support * Introduce a SecretStoreFactory to allow runtime definition of SecretStore implementation. * Introduce a SecureConfig to allow simple configuration of different SecretStore implementaiton. * Introduce random default password plus obfuscation. Best attempt at security through obscurity. * Corrections / better support for x-JVM modification. Part 3: Secret Store: SecretStore, SecretStoreFactory, JavaKeystore - refacactor (elastic#8745) * Adds more CRUD like operations for SecretStore API * SecretStoreFactory Mirror API's CRUD operations * Adds 'exists' to API to allow command line warning 'Overwrite ?' * Minor readabiliy Part 4: Integrate secret store with Logstash core (elastic#8905) This change introduces the command line tooling and hooks needed to allow Logstash to use the secret store. This change hooks into the same logic that the does the environment variable substitution. The commnad line mirrors the Elasticsearch command line, and is implemented primarily in Java.
…ta store. Fixes elastic#8657 Part 1: API and JavaKeyStore implementation (elastic#8657) Introduces the API to read/write/delete sensitive data from a secure store and includes a Java KeyStore implementation. Note - this commit does NOT integrate with the Logstash configuration or settings. Part 2: Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support (elastic#8659) * Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support * Introduce a SecretStoreFactory to allow runtime definition of SecretStore implementation. * Introduce a SecureConfig to allow simple configuration of different SecretStore implementaiton. * Introduce random default password plus obfuscation. Best attempt at security through obscurity. * Corrections / better support for x-JVM modification. Part 3: Secret Store: SecretStore, SecretStoreFactory, JavaKeystore - refacactor (elastic#8745) * Adds more CRUD like operations for SecretStore API * SecretStoreFactory Mirror API's CRUD operations * Adds 'exists' to API to allow command line warning 'Overwrite ?' * Minor readabiliy Part 4: Integrate secret store with Logstash core (elastic#8905) This change introduces the command line tooling and hooks needed to allow Logstash to use the secret store. This change hooks into the same logic that the does the environment variable substitution. The commnad line mirrors the Elasticsearch command line, and is implemented primarily in Java.
…ta store. Fixes elastic#8657 Part 1: API and JavaKeyStore implementation (elastic#8657) Introduces the API to read/write/delete sensitive data from a secure store and includes a Java KeyStore implementation. Note - this commit does NOT integrate with the Logstash configuration or settings. Part 2: Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support (elastic#8659) * Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support * Introduce a SecretStoreFactory to allow runtime definition of SecretStore implementation. * Introduce a SecureConfig to allow simple configuration of different SecretStore implementaiton. * Introduce random default password plus obfuscation. Best attempt at security through obscurity. * Corrections / better support for x-JVM modification. Part 3: Secret Store: SecretStore, SecretStoreFactory, JavaKeystore - refacactor (elastic#8745) * Adds more CRUD like operations for SecretStore API * SecretStoreFactory Mirror API's CRUD operations * Adds 'exists' to API to allow command line warning 'Overwrite ?' * Minor readabiliy Part 4: Integrate secret store with Logstash core (elastic#8905) This change introduces the command line tooling and hooks needed to allow Logstash to use the secret store. This change hooks into the same logic that the does the environment variable substitution. The commnad line mirrors the Elasticsearch command line, and is implemented primarily in Java.
…ta store. Fixes elastic#8657 Part 1: API and JavaKeyStore implementation (elastic#8657) Introduces the API to read/write/delete sensitive data from a secure store and includes a Java KeyStore implementation. Note - this commit does NOT integrate with the Logstash configuration or settings. Part 2: Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support (elastic#8659) * Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support * Introduce a SecretStoreFactory to allow runtime definition of SecretStore implementation. * Introduce a SecureConfig to allow simple configuration of different SecretStore implementaiton. * Introduce random default password plus obfuscation. Best attempt at security through obscurity. * Corrections / better support for x-JVM modification. Part 3: Secret Store: SecretStore, SecretStoreFactory, JavaKeystore - refacactor (elastic#8745) * Adds more CRUD like operations for SecretStore API * SecretStoreFactory Mirror API's CRUD operations * Adds 'exists' to API to allow command line warning 'Overwrite ?' * Minor readabiliy Part 4: Integrate secret store with Logstash core (elastic#8905) This change introduces the command line tooling and hooks needed to allow Logstash to use the secret store. This change hooks into the same logic that the does the environment variable substitution. The commnad line mirrors the Elasticsearch command line, and is implemented primarily in Java. Part 5: Hardening and test fixes (this PR)
…ta store. Fixes #8657 Part 1: API and JavaKeyStore implementation (#8657) Introduces the API to read/write/delete sensitive data from a secure store and includes a Java KeyStore implementation. Note - this commit does NOT integrate with the Logstash configuration or settings. Part 2: Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support (#8659) * Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support * Introduce a SecretStoreFactory to allow runtime definition of SecretStore implementation. * Introduce a SecureConfig to allow simple configuration of different SecretStore implementaiton. * Introduce random default password plus obfuscation. Best attempt at security through obscurity. * Corrections / better support for x-JVM modification. Part 3: Secret Store: SecretStore, SecretStoreFactory, JavaKeystore - refacactor (#8745) * Adds more CRUD like operations for SecretStore API * SecretStoreFactory Mirror API's CRUD operations * Adds 'exists' to API to allow command line warning 'Overwrite ?' * Minor readabiliy Part 4: Integrate secret store with Logstash core (#8905) This change introduces the command line tooling and hooks needed to allow Logstash to use the secret store. This change hooks into the same logic that the does the environment variable substitution. The commnad line mirrors the Elasticsearch command line, and is implemented primarily in Java. Part 5: Hardening and test fixes (this PR) Fixes #8935
…ta store. Fixes #8657 Part 1: API and JavaKeyStore implementation (#8657) Introduces the API to read/write/delete sensitive data from a secure store and includes a Java KeyStore implementation. Note - this commit does NOT integrate with the Logstash configuration or settings. Part 2: Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support (#8659) * Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support * Introduce a SecretStoreFactory to allow runtime definition of SecretStore implementation. * Introduce a SecureConfig to allow simple configuration of different SecretStore implementaiton. * Introduce random default password plus obfuscation. Best attempt at security through obscurity. * Corrections / better support for x-JVM modification. Part 3: Secret Store: SecretStore, SecretStoreFactory, JavaKeystore - refacactor (#8745) * Adds more CRUD like operations for SecretStore API * SecretStoreFactory Mirror API's CRUD operations * Adds 'exists' to API to allow command line warning 'Overwrite ?' * Minor readabiliy Part 4: Integrate secret store with Logstash core (#8905) This change introduces the command line tooling and hooks needed to allow Logstash to use the secret store. This change hooks into the same logic that the does the environment variable substitution. The commnad line mirrors the Elasticsearch command line, and is implemented primarily in Java. Part 5: Hardening and test fixes (this PR) Fixes #8935
…ta store. Fixes elastic#8657 Part 1: API and JavaKeyStore implementation (elastic#8657) Introduces the API to read/write/delete sensitive data from a secure store and includes a Java KeyStore implementation. Note - this commit does NOT integrate with the Logstash configuration or settings. Part 2: Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support (elastic#8659) * Secret Store: SecretStoreFactory, SecureConfig, Obfuscation and X-JVM support * Introduce a SecretStoreFactory to allow runtime definition of SecretStore implementation. * Introduce a SecureConfig to allow simple configuration of different SecretStore implementaiton. * Introduce random default password plus obfuscation. Best attempt at security through obscurity. * Corrections / better support for x-JVM modification. Part 3: Secret Store: SecretStore, SecretStoreFactory, JavaKeystore - refacactor (elastic#8745) * Adds more CRUD like operations for SecretStore API * SecretStoreFactory Mirror API's CRUD operations * Adds 'exists' to API to allow command line warning 'Overwrite ?' * Minor readabiliy Part 4: Integrate secret store with Logstash core (elastic#8905) This change introduces the command line tooling and hooks needed to allow Logstash to use the secret store. This change hooks into the same logic that the does the environment variable substitution. The commnad line mirrors the Elasticsearch command line, and is implemented primarily in Java. Part 5: Hardening and test fixes (this PR) Fixes elastic#8935
This change introduces the command line tooling and hooks needed to allow Logstash to use the secret store. This change hooks into the same logic that the does the environment variable substitution. The command line mirrors the Elasticsearch command line, and is implemented primarily in Java.
Part of #8353
Documentation and full workflow integration tests are still coming ...but this should be the final change set to implement the secret store.
Once this review passes, I will squash this feature branch and create PR, ensure all tests pass, allow for one final review and merge this master. (doc and itests can come independently)