Allow for secure LDAP password settings #5323

chriswhite199 · 2025-05-10T18:47:36Z

Description

Category: Enhancement
Why these changes are required: To allow for secure storage of LDAP password settings
What is the old behavior before changes and new behavior after changes
- Before - settings for ldap password and pem_password were only configurable in an insecure manner (plaintext in yml or via cli / env variables)
- After - Settings can now be acquired from the keystore (or still provided in an insecure manner, for which a warning will be shown in the logs, prompting the administrator to migrate to the secure versions)

If this PR is approved, i plan to submit similar PRs for the other secure settings using the SecurableLegacySetting class:

OIDC: pemkey_password, client_secret, passphrase (https://docs.opensearch.org/docs/latest/security/authentication-backends/openid-connect/)
JWT: signing_key (https://docs.opensearch.org/docs/latest/security/authentication-backends/jwt/)
SAML: signature_private_key_password, exchange_key, pemkey_password (https://docs.opensearch.org/docs/latest/security/authentication-backends/saml/)

Issues Resolved

Related to (extension and refactor of) Added secure settings for ssl related passwords: #2296 (for which I'm the original author)
Draft Documentation PR: Added documentation for secure LDAP settings documentation-website#9884
Is this a backport: no

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end: no

Testing

Unit tests updated for LDAP password (with insecure legacy setting and secure setting)
There is currently no existing test for pemkey_password

Check List

New functionality includes testing
New functionality has been documented
- Draft PR: Added documentation for secure LDAP settings documentation-website#9884
New Roles/Permissions have a corresponding security dashboards plugin PR - N/A
API changes companion pull request created - N/A
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Chris White <chriswhite199@gmail.com>

codecov · 2025-05-10T19:25:01Z

Codecov Report

Attention: Patch coverage is 95.74468% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71.75%. Comparing base (6e78dd9) to head (d33afd6).
Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
...org/opensearch/security/ssl/SecureSSLSettings.java	88.88%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5323      +/-   ##
==========================================
+ Coverage   71.68%   71.75%   +0.07%     
==========================================
  Files         384      386       +2     
  Lines       23869    23877       +8     
  Branches     3647     3647              
==========================================
+ Hits        17111    17134      +23     
+ Misses       4928     4912      -16     
- Partials     1830     1831       +1

Files with missing lines	Coverage Δ
...security/auditlog/sink/ExternalOpenSearchSink.java	`62.36% <ø> (ø)`
...opensearch/security/auditlog/sink/WebhookSink.java	`76.30% <ø> (ø)`
...ty/auth/ldap/backend/LDAPAuthorizationBackend.java	`68.54% <100.00%> (ø)`
...earch/security/auth/ldap/util/ConfigConstants.java	`100.00% <100.00%> (ø)`
...urity/auth/ldap2/LDAPConnectionFactoryFactory.java	`60.43% <100.00%> (ø)`
...earch/security/setting/SecurableLegacySetting.java	`100.00% <100.00%> (ø)`
...ensearch/security/ssl/DefaultSecurityKeyStore.java	`30.63% <ø> (ø)`
...rch/security/ssl/config/SslCertificatesLoader.java	`87.03% <100.00%> (ø)`
...opensearch/security/ssl/util/SSLRequestHelper.java	`66.98% <ø> (ø)`
...ch/security/util/SettingsBasedSSLConfigurator.java	`62.75% <ø> (ø)`
... and 2 more

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

See associated PR for implementation: opensearch-project/security#5323 Signed-off-by: Chris White <chriswhite199@gmail.com>

cwperks · 2025-05-12T15:46:34Z

Thank you for this PR @chriswhite199 ! Its been on my agenda to support the keystore here for some time now :)

DarshitChanpura · 2025-05-12T18:34:27Z

src/test/java/org/opensearch/security/ssl/SecureSSLSettingsTest.java

-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.is;
-import static org.opensearch.security.ssl.SecureSSLSettings.SSLSetting.SECURITY_SSL_HTTP_PEMKEY_PASSWORD;
-
 public class SecureSSLSettingsTest {


Should this class be renamed. It seems to only have 1 test now. It doesn't make sense to have this test class file.

DarshitChanpura · 2025-05-12T18:36:02Z

src/test/java/org/opensearch/security/setting/SecurableLegacySettingTest.java

+    }
+
+    @Test
+    public void testShouldFavorSecureOverInsecureSetting() {


DarshitChanpura · 2025-05-12T18:37:29Z

src/main/java/org/opensearch/security/ssl/SecureSSLSettings.java

+                try {
+                    return (SecurableLegacySetting) field.get(null);
+                } catch (IllegalAccessException e) {
+                    throw new RuntimeException("Unable to access field: " + field.getName(), e);


This should be SettingsException

DarshitChanpura · 2025-05-12T18:38:56Z

src/main/java/org/opensearch/security/setting/SecurableLegacySetting.java

+
+    /**
+     * 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


Interesting. For my curiosity, how did you discover this?

I can't remember now, but setting allow_insecure_settings and defining the property as secure would raise an exception - hence the original implementation crafted two properties - password and password_secure to not break existing installations.

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

cwperks · 2025-05-12T19:35:00Z

@chriswhite199 have you tested this outside of the unit tests? I think there may be larger structural changes required for settings in security yaml files to be able to fetch values from the keystore. For settings that reside in opensearch.yml this approach works, but I don't think that the security plugin currently has the ability to read from the keystore (unless I'm mistaken).

chriswhite199 · 2025-05-13T01:21:13Z

@chriswhite199 have you tested this outside of the unit tests? I think there may be larger structural changes required for settings in security yaml files to be able to fetch values from the keystore. For settings that reside in opensearch.yml this approach works, but I don't think that the security plugin currently has the ability to read from the keystore (unless I'm mistaken).

I have not yet (for authn / authz plugins, I know it works for tls certs for https / transport as this was tested in the original PR for that scope). I didn't think it would be any different for authNZ plugins but I will test and get back to you.

On a side not the DEVELOPING_WITH_DOCKER.md instructions didn't work for me under 3.0, something about a missing plugin.properties file (which could be related to the base image being opensearchstaging/opensearch, maybe that image doesn't have the security plugin unpacked correctly)

nibix

Thank you for this. Secure settings for the security plugin are certainly an important topic.

Still, it feels to me that we have kind of a concept mismatch between the normal security settings (stored in an index) and the secure settings (stored in the opensearch-keystore file on each node).

It is unclear how the config names in security settings config.yml map to to the setting in the keystore, especially if we have lists of auth domains.
The processes of config hot reloading (without starting the node) seem to unaligned for security settings and secure settings (I think it is possible, but it will involve quite a few steps).

See below for more elaboration on that.

nibix · 2025-05-13T05:04:15Z

src/main/java/org/opensearch/security/auth/ldap2/LDAPConnectionFactoryFactory.java

@@ -184,7 +184,7 @@ private ConnectionInitializer getConnectionInitializer() {
        BindConnectionInitializer result = new BindConnectionInitializer();

        String bindDn = settings.get(ConfigConstants.LDAP_BIND_DN, null);
-        String password = settings.get(ConfigConstants.LDAP_PASSWORD, null);
+        String password = ConfigConstants.LDAP_PASSWORD.getSetting(settings);


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?

The AuthenticationBackend settings are created from a subset of the properties here:

https://github.com/opensearch-project/security/blob/3.0/src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java#L267:L276

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?

@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).

Maybe this is the direction the PR should take (in relation to #4004):

Amend the securityadmin script to allow for the passwords / secret settings to be sourced from the opensearch keystore (or config.yaml for legacy reasons)

securityadmin still stores the secrets in the .opendistro-security index

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).

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.

nibix · 2025-05-13T05:07:44Z

src/test/java/org/opensearch/security/auth/ldap/LdapBackendTest.java

+            .put(ConfigConstants.LDAP_AUTHC_USERSEARCH, "(uid={0})")
+            .put(ConfigConstants.LDAP_AUTHC_USERBASE, "ou=people,o=TEST")
+            .put(ConfigConstants.LDAP_BIND_DN, "cn=Captain Spock,ou=people,o=TEST")
+            .setSecureSettings(mockSecureSettings)


I think we need one integration test for secure settings. You can find integration tests for LDAP here:

https://github.com/opensearch-project/security/blob/main/src/integrationTest/java/org/opensearch/security/http/LdapAuthenticationTest.java

nibix · 2025-05-13T05:13:24Z

src/main/java/org/opensearch/security/setting/SecurableLegacySetting.java

+/**
+ * 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 {


I am a bit confused about the word "legacy" here. Could you please elaborate what "legacy settings" are?

nibix · 2025-05-13T05:20:36Z

src/main/java/org/opensearch/security/ssl/SecureSSLSettings.java

+        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);


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?

DarshitChanpura · 2025-06-23T16:28:25Z

@chriswhite199 Are you still working on this PR?

Allow for secure LDAP password settings

d33afd6

Signed-off-by: Chris White <chriswhite199@gmail.com>

chriswhite199 requested review from cwperks, DarshitChanpura, derek-ho, nibix, peternied, RyanL1997, reta, shikharj05 and willyborankin as code owners May 10, 2025 18:47

chriswhite199 added a commit to chriswhite199/documentation-website that referenced this pull request May 10, 2025

Added documentation for secure LDAP settings

c4e013c

See associated PR for implementation: opensearch-project/security#5323 Signed-off-by: Chris White <chriswhite199@gmail.com>

chriswhite199 mentioned this pull request May 10, 2025

Added documentation for secure LDAP settings opensearch-project/documentation-website#9884

Draft

1 task

DarshitChanpura reviewed May 12, 2025

View reviewed changes

nibix reviewed May 13, 2025

View reviewed changes

Allow for secure LDAP password settings #5323

Are you sure you want to change the base?

Allow for secure LDAP password settings #5323

Uh oh!

Conversation

chriswhite199 commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Testing

Check List

Uh oh!

codecov bot commented May 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

cwperks commented May 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks commented May 12, 2025

Uh oh!

chriswhite199 commented May 13, 2025

Uh oh!

nibix left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura commented Jun 23, 2025

Uh oh!

Uh oh!

chriswhite199 commented May 10, 2025 •

edited

Loading

codecov bot commented May 10, 2025 •

edited

Loading