Secure password for monitoring HTTP exporter

[danhermann]

Adds reloadable SECURE_AUTH_PASSWORD to allow keystore entries in the form xpack.monitoring.exporters.*.secure_auth_password to securely supply passwords for monitoring exporters. Deprecates the insecure AUTH_PASSWORD setting.

It would have been ideal to include validation that only one of either the AUTH_PASSWORD or SECURE_AUTH_PASSWORD were set but the settings validation framework does not support secure settings. Additionally, the validation on the AUTH_USERNAME setting that required a password to be set if the username was set had to be removed because the password can now be set via a secure setting that the validation framework cannot see. It might be possible to extend the validation framework to handle secure settings, but if so, that would definitely be a separate work effort.

Making the SECURE_AUTH_PASSWORD setting reloadable meant that the Monitoring plugin had to be able to trigger a reload of any HTTP exporters when the cluster received a _nodes/reload_secure_settings request. This necessitated (unless I missed a cleaner approach) the addition of a reference to Exporters within the Monitoring class.

Fixes #50197.

[elasticmachine]

Pinging @elastic/es-core-features (:Core/Features/Monitoring)

[danhermann]

In its draft state, the password is securely read and supplied to the exporter, but any attempt to dynamically change any other monitoring settings such as xpack.monitoring.exporters.*.enabled results in an error setting [xpack.monitoring.exporters.*.secure_auth_password] is not dynamic though it is neither designated nor intended to be dynamic.

[albertzaharovits]

@danhermann The reason for the error, is that there's an assumption that all HTTP exporter settings are dynamic, but you've just added one that isn't. Take a look at:

elasticsearch/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporters.java

Line 61 in a6a3d2b

clusterService.getClusterSettings().addSettingsUpdateConsumer(this::setExportersSetting, getSettings());

You'll have to exclude the reloadable Secure setting from the list that an updater is registered, as well as have a way to furnish the cached keystore values to the initExporters method

elasticsearch/x-pack/plugin/monitoring/src/main/java/org/elasticsearch/xpack/monitoring/exporter/Exporters.java

Line 113 in a6a3d2b

Map<String, Exporter> initExporters(Settings settings) {

[danhermann]

@elasticmachine update branch

[danhermann]

@elasticmachine update branch

[danhermann]

@elasticmachine update branch

[jakelandis]

Passwords as String should be avoided where possible due to help avoid leaking to heap since gc happens when ever it feel like it. I think you can use SecureString instead and then close it out to ensure it zero'd out when you are done.

Also since it looks like you are using this for equality, can you just hash it and store the hash in the map ?

[danhermann]

I could change it to a Map<String, SecureString> but I couldn't close any of the SecureString instances because the HTTP exporter has to be recreated (so I need the full value, not just an equality check) any time any of its settings, whether secure or not, change. Unfortunately, this is unavoidable for settings that are both secure and reloadable since the keystore itself is not accessible outside of node startup and reload events. The security folks directed me to

elasticsearch/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/NotificationService.java

Lines 167 to 225 in c3b5f71

	* Extracts the {@link SecureSettings}` out of the passed in {@link Settings} object. The {@code Setting} argument has to have the
	* {@code SecureSettings} open/available. Normally {@code SecureSettings} are available only under specific callstacks (eg. during node
	* initialization or during a `reload` call). The returned copy can be reused freely as it will never be closed (this is a bit of
	* cheating, but it is necessary in this specific circumstance). Only works for secure settings of type string (not file).
	*
	* @param source
	* A {@code Settings} object with its {@code SecureSettings} open/available.
	* @param securePluginSettings
	* The list of settings to copy.
	* @return A copy of the {@code SecureSettings} of the passed in {@code Settings} argument.
	*/
	private static SecureSettings extractSecureSettings(Settings source, List<Setting<?>> securePluginSettings)
	throws GeneralSecurityException {
	// get the secure settings out
	final SecureSettings sourceSecureSettings = Settings.builder().put(source, true).getSecureSettings();
	// filter and cache them...
	final Map<String, Tuple<SecureString, byte[]>> cache = new HashMap<>();
	if (sourceSecureSettings != null && securePluginSettings != null) {
	for (final String settingKey : sourceSecureSettings.getSettingNames()) {
	for (final Setting<?> secureSetting : securePluginSettings) {
	if (secureSetting.match(settingKey)) {
	cache.put(settingKey,
	new Tuple<>(sourceSecureSettings.getString(settingKey), sourceSecureSettings.getSHA256Digest(settingKey)));
	}
	}
	}
	}
	return new SecureSettings() {
	@Override
	public boolean isLoaded() {
	return true;
	}
	@Override
	public SecureString getString(String setting) {
	return cache.get(setting).v1();
	}
	@Override
	public Set<String> getSettingNames() {
	return cache.keySet();
	}
	@Override
	public InputStream getFile(String setting) {
	throw new IllegalStateException("A NotificationService setting cannot be File.");
	}
	@Override
	public byte[] getSHA256Digest(String setting) {
	return cache.get(setting).v2();
	}
	@Override
	public void close() throws IOException {
	}
	};
	}

where they did essentially the same thing for the secure settings in watcher notifications.

[jakelandis]

ditto about SecureString

[jakelandis]

Could you add some unit tests for changing the secure setting and ensure it is changed (or no-op) as well as trying to set this for a non-http type setting ? See org.elasticsearch.xpack.watcher.notification.NotificationService for some examples.

Ideally, we would also have Integration tests, but I didn't see any framework already setup to read from the keystore for a Integration tests (and an almost complete lack of Rest tests) :(

[danhermann]

@elasticmachine update branch

[danhermann]

@elasticmachine run elasticsearch-ci/docs

[danhermann]

@jakelandis, I think this one is ready for another look.

[jakelandis]

LGTM, thanks for the updates and test

[danhermann]

@jakelandis, sorry for changing this after your review -- it occurred to me that because we cannot validate that the user supplied only one password, we should at least log a warning if that happens so we are not silently ignoring the value of auth.password if auth.secure_password is also specified. I added that logging in a7974e7. If you concur, I will merge this. There were also a couple doc fix commits that the doc folks reviewed.

[jakelandis]

@danhermann - no problem, additions look good.

LGTM2