Allow for secure LDAP password settings #5323
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| /* | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * The OpenSearch Contributors require contributions made to | ||
| * this file be licensed under the Apache-2.0 license or a | ||
| * compatible open source license. | ||
| * | ||
| * Modifications Copyright OpenSearch Contributors. See | ||
| * GitHub history for details. | ||
| */ | ||
|
|
||
| package org.opensearch.security.setting; | ||
|
|
||
| import java.util.Optional; | ||
|
|
||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
|
|
||
| import org.opensearch.common.settings.SecureSetting; | ||
| import org.opensearch.common.settings.Setting; | ||
| import org.opensearch.common.settings.Settings; | ||
| import org.opensearch.core.common.settings.SecureString; | ||
|
|
||
| /** | ||
| * Wrapper for legacy settings that support a secure variant located in the Keystore. | ||
| * <p> | ||
| * Secure name is the insecure name with "_secure" appended to it. | ||
| */ | ||
| public class SecurableLegacySetting { | ||
|
Comment on lines
+24
to
+29
There was a problem hiding this comment. I am a bit confused about the word "legacy" here. Could you please elaborate what "legacy settings" are? |
||
| private static final Logger LOG = LogManager.getLogger(SecurableLegacySetting.class); | ||
|
|
||
| public static final String SECURE_SUFFIX = "_secure"; | ||
|
|
||
| public final String insecurePropertyName; | ||
|
|
||
| public final String propertyName; | ||
|
|
||
| public final String defaultValue; | ||
|
|
||
| public SecurableLegacySetting(String insecurePropertyName) { | ||
| this(insecurePropertyName, null); | ||
| } | ||
|
|
||
| public SecurableLegacySetting(String insecurePropertyName, String defaultValue) { | ||
| this(insecurePropertyName, String.format("%s%s", insecurePropertyName, SECURE_SUFFIX), defaultValue); | ||
| } | ||
|
|
||
| public SecurableLegacySetting(String insecurePropertyName, String propertyName, String defaultValue) { | ||
| super(); | ||
| this.insecurePropertyName = insecurePropertyName; | ||
| this.propertyName = propertyName; | ||
| this.defaultValue = defaultValue; | ||
| } | ||
|
|
||
| public Setting<SecureString> asSetting() { | ||
| final Setting<SecureString> fallback = new InsecureFallbackStringSetting(this.insecurePropertyName, this.propertyName); | ||
| return SecureSetting.secureString(this.propertyName, fallback); | ||
| } | ||
|
|
||
| public Setting<SecureString> asInsecureSetting() { | ||
| return new InsecureFallbackStringSetting(this.insecurePropertyName, this.propertyName); | ||
| } | ||
|
|
||
| public String getSetting(Settings settings) { | ||
| return this.getSetting(settings, this.defaultValue); | ||
| } | ||
|
|
||
| public String getSetting(Settings settings, String defaultValue) { | ||
| return Optional.of(this.asSetting().get(settings)).filter(ss -> ss.length() > 0).map(SecureString::toString).orElse(defaultValue); | ||
| } | ||
|
|
||
| /** | ||
| * Alternative to InsecureStringSetting, which doesn't raise an exception if allow_insecure_settings is false, but | ||
| * instead log.WARNs the violation. This is to appease a potential cyclic dependency between commons-utils | ||
|
There was a problem hiding this comment. Interesting. For my curiosity, how did you discover this? There was a problem hiding this comment. I can't remember now, but setting If you look at the original review for #2296, @peternied and I were discussing the integration test issues related to this - hopefully that comment can help you here |
||
| */ | ||
| private static class InsecureFallbackStringSetting extends Setting<SecureString> { | ||
| private final String name; | ||
| private final String secureName; | ||
|
|
||
| private InsecureFallbackStringSetting(String name, String secureName) { | ||
| super(name, "", s -> new SecureString(s.toCharArray()), Property.Deprecated, Property.Filtered, Property.NodeScope); | ||
| this.name = name; | ||
| this.secureName = secureName; | ||
| } | ||
|
|
||
| public SecureString get(Settings settings) { | ||
| if (this.exists(settings)) { | ||
| LOG.warn( | ||
| "Setting [{}] has a secure counterpart [{}] which should be used instead. Allowing use of {} for legacy setups", | ||
| this.name, | ||
| this.secureName, | ||
| this.name | ||
| ); | ||
| } | ||
|
|
||
| return super.get(settings); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,115 +16,77 @@ | |
|
|
||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
| import java.util.stream.Stream; | ||
|
|
||
| import org.opensearch.common.settings.Setting; | ||
| import org.opensearch.security.setting.SecurableLegacySetting; | ||
|
|
||
| import static org.opensearch.security.ssl.util.SSLConfigConstants.DEFAULT_STORE_PASSWORD; | ||
|
|
||
| /** | ||
| * Container for secured settings (passwords for certs, keystores) and the now deprecated original settings | ||
| */ | ||
| public final class SecureSSLSettings { | ||
| private static final String PREFIX = "plugins.security.ssl"; | ||
| private static final String HTTP_PREFIX = PREFIX + ".http"; | ||
| private static final String TRANSPORT_PREFIX = PREFIX + ".transport"; | ||
|
|
||
| // http settings | ||
| public final static SecurableLegacySetting SECURITY_SSL_HTTP_PEMKEY_PASSWORD = new SecurableLegacySetting( | ||
| HTTP_PREFIX + ".pemkey_password" | ||
| ); | ||
| public final static SecurableLegacySetting SECURITY_SSL_HTTP_KEYSTORE_PASSWORD = new SecurableLegacySetting( | ||
| HTTP_PREFIX + ".keystore_password" | ||
| ); | ||
| public final static SecurableLegacySetting SECURITY_SSL_HTTP_KEYSTORE_KEYPASSWORD = new SecurableLegacySetting( | ||
| HTTP_PREFIX + ".keystore_keypassword" | ||
| ); | ||
| public final static SecurableLegacySetting SECURITY_SSL_HTTP_TRUSTSTORE_PASSWORD = new SecurableLegacySetting( | ||
| HTTP_PREFIX + ".truststore_password", | ||
| DEFAULT_STORE_PASSWORD | ||
| ); | ||
|
|
||
| // transport settings | ||
| public final static SecurableLegacySetting SECURITY_SSL_TRANSPORT_PEMKEY_PASSWORD = new SecurableLegacySetting( | ||
| TRANSPORT_PREFIX + ".pemkey_password" | ||
| ); | ||
| public final static SecurableLegacySetting SECURITY_SSL_TRANSPORT_SERVER_PEMKEY_PASSWORD = new SecurableLegacySetting( | ||
| TRANSPORT_PREFIX + ".server.pemkey_password" | ||
| ); | ||
| public final static SecurableLegacySetting SECURITY_SSL_TRANSPORT_CLIENT_PEMKEY_PASSWORD = new SecurableLegacySetting( | ||
| TRANSPORT_PREFIX + ".client.pemkey_password" | ||
| ); | ||
| public final static SecurableLegacySetting SECURITY_SSL_TRANSPORT_KEYSTORE_PASSWORD = new SecurableLegacySetting( | ||
| TRANSPORT_PREFIX + ".keystore_password" | ||
| ); | ||
| public final static SecurableLegacySetting SECURITY_SSL_TRANSPORT_KEYSTORE_KEYPASSWORD = new SecurableLegacySetting( | ||
| TRANSPORT_PREFIX + ".keystore_keypassword" | ||
| ); | ||
| public final static SecurableLegacySetting SECURITY_SSL_TRANSPORT_SERVER_KEYSTORE_KEYPASSWORD = new SecurableLegacySetting( | ||
| TRANSPORT_PREFIX + ".server.keystore_keypassword" | ||
| ); | ||
| public final static SecurableLegacySetting SECURITY_SSL_TRANSPORT_CLIENT_KEYSTORE_KEYPASSWORD = new SecurableLegacySetting( | ||
| TRANSPORT_PREFIX + ".client.keystore_keypassword" | ||
| ); | ||
| public final static SecurableLegacySetting SECURITY_SSL_TRANSPORT_TRUSTSTORE_PASSWORD = new SecurableLegacySetting( | ||
| TRANSPORT_PREFIX + ".truststore_password", | ||
| DEFAULT_STORE_PASSWORD | ||
| ); | ||
|
|
||
| private SecureSSLSettings() {} | ||
|
|
||
| public static List<Setting<?>> getSecureSettings() { | ||
| return Arrays.stream(SecureSSLSettings.class.getDeclaredFields()) | ||
| .filter(field -> SecurableLegacySetting.class.isAssignableFrom(field.getType())) | ||
| .map(field -> { | ||
| try { | ||
| return (SecurableLegacySetting) field.get(null); | ||
| } catch (IllegalAccessException e) { | ||
| throw new RuntimeException("Unable to access field: " + field.getName(), e); | ||
|
There was a problem hiding this comment. This should be SettingsException
Comment on lines
+80
to
+86
There was a problem hiding this comment. It took me a while to understand this until I figured out that you are using reflection to retrieve static members (as a replacement for enum.values()). As this is solution is maybe a bit non-obvious, I would recommend to add a comment somewhere to explain the approach. Another question: With the SecurityManager, this would needed to be wrapped in a doPrivileged() block in order to be able to do reflection. With the new security manager replacement, did this need go away? |
||
| } | ||
| }) | ||
| .flatMap(setting -> Stream.of(setting.asSetting(), setting.asInsecureSetting())) | ||
| .collect(Collectors.toList()); | ||
| } | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
What would be the complete name of the secure setting for LDAP passwords?
In the security plugin, you can configure more than one LDAP authentication domain. This can then also have different passwords. If I get this right, we won't be able to configure more than one LDAP domain with different passwords, correct?
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 AuthenticationBackend settings are created from a subset of the properties here:
So it should allow for each individual backend to have it's own secure value. But examining the above only the settings from the index are used - there are no secure settings subset created and used when configuring the settings object for the backend.
Also a misunderstanding on my part renders this whole PR void, as the settings are centralized in an index, not pulled from a yaml on each node. We could allow for pulling the secure settings from the keystore by amending the above to subset and inject secure settings when creating the Settings for each backend, but then you'd almost certainly run into issues where the admin would have to remember to update the keystore on every node when secure values are set, and when changed (also leading to confusion if those keystores are not in sync - where auth make work against one node, but fail against another).
I'm leaning towards withdrawing this PR in light of the above, unless anyone has any ideas on a way forward?
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.
@chriswhite199 I remember taking a look a few months ago when looking at #4004 and believe it will take some changes in OpenSearch core.
I'll take another look into what's required for the security config to support reading from the keystore and reply back ASAP. (It may take a few days).
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.
Maybe this is the direction the PR should take (in relation to #4004):
My original intent of this PR is to avoid having passwords in cleartext config files on the filesystem (currently an admin has to remember to remove the config.yaml or redact the passwords after syncing).
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.
Maybe it is a good idea to file an RFC about this.
We also have the trouble that there is not only securityadmin, but - optionally - the REST API can be also used to change/retrieve the config.yml configuration.