[@kbn/es] Add a predefined list of Elasticsearch secure settings to be added into keystore. Re-enable OIDC tests.#42239
Conversation
…e added into keystore. Re-enable OIDC tests.
azasypkin
added
test
dev
Team:Operations
|
Pinging @elastic/kibana-operations |
|
Pinging @elastic/kibana-security |
| log: this._log, | ||
| }).reduce((acc, cur) => acc.concat(['-E', cur]), []); | ||
| const args = parseSettings( | ||
| extractConfigFiles(options.esArgs || [], installPath, { log: this._log }), |
There was a problem hiding this comment.
note: I'm using parseSettngs here to not touch extractConfigFiles.
| await configureKeystore(installPath, password, log, bundledJDK); | ||
| await configureKeystore( | ||
| installPath, | ||
| password, |
There was a problem hiding this comment.
note: decided to keep password separate from secureSettings even though the code that adds secure settings could be reused for password as well since are kind of different things. What do you think?
There was a problem hiding this comment.
I actually really like the idea of combing them and just manually passing [['bootstrap.plassword', password], ...parseSettings(esArgs, { filter: SettingsFilter.SecureOnly })] to configureKeystore
| }); | ||
|
|
||
| for (const [secureSettingName, secureSettingValue] of secureSettings) { | ||
| log.info(`setting secure setting [${secureSettingName}] to %s`, chalk.bold(secureSettingValue)); |
There was a problem hiding this comment.
note: setting .... setting not sure how that sounds 🤷♂️ Couldn't find a better verb.
There was a problem hiding this comment.
nit, but we could keep this consistent with the previous output:
| log.info(`setting secure setting [${secureSettingName}] to %s`, chalk.bold(secureSettingValue)); | |
| log.info(`setting secure setting %s to %s`, chalk.bold(secureSettingName), chalk.bold(secureSettingValue)); |
| * List of the patterns for the settings names that are supposed to be secure and stored in the keystore. | ||
| */ | ||
| const SECURE_SETTINGS_LIST = [ | ||
| /^xpack\.security\.authc\.realms\.oidc\.[a-zA-Z0-9_]+\.rp\.client_secret$/, |
There was a problem hiding this comment.
note: didn't realize it before, but realm name is a custom string, so had to resort to reqular expression here.
There was a problem hiding this comment.
That's fine - I was thinking we could also just match rp.client_secret but this is better.
| export enum SettingsFilter { | ||
| All = 'all', | ||
| SecureOnly = 'secure-only', | ||
| NonSecureOnly = 'non-secure-only', |
There was a problem hiding this comment.
note: NonSecureOnly is kind of misnomer, also was considering ExcludeSecure instead, but don't have a strong opinion.
| for (const rawSettingNameValuePair of rawSettingNameValuePairs) { | ||
| const [settingName, settingValue] = rawSettingNameValuePair.split('='); | ||
|
|
||
| const includeSetting = |
There was a problem hiding this comment.
note: if I understand JS correctly isSecureSetting should never be called twice, so there shouldn't be overhead (unless a filter is undefined/arbitrary string that shouldn't be allowed in TS, but can be set in JS though).
| * under the License. | ||
| */ | ||
|
|
||
| require('../src/setup_node_env'); |
There was a problem hiding this comment.
note: not sure if it's the correct solution - but I wanted to use TypeScript in kbn/es. Let me know if there is a better solution or if you prefer me to switch to JavaScript instead.
There was a problem hiding this comment.
That's fine, as long as it doesn't have to be transpiled.
| @@ -0,0 +1,63 @@ | |||
| /* | |||
There was a problem hiding this comment.
note: I see we call these settings as esArgs, but I picked the word settings as it's used in Secure Settings.
This comment has been minimized.
This comment has been minimized.
💚 Build Succeeded |
tylersmalley
left a comment
tylersmalley
left a comment
There was a problem hiding this comment.
LGTM, just a couple of small nits I could live without.
…e setting, improve log message.
💚 Build Succeeded |
…e added into keystore. Re-enable OIDC tests. (elastic#42239)
3 participants
This PR adds a predefined list of Elasticsearch settings that should be added into keystore instead of config or set of command line arguments.
Additionally I tweaked @kbn/test a bit to pass
esArgsintoinstallSource/Snapshotand re-enabled OIDC tests that rely on this functionality (and hence implicitly test it).See that OIDC tests are run on the CI now
Fixes: #36959