Support keystore references in dynamic security config

[cwperks]

Fixes #4004

Summary

• register a plugin-scoped plugins.security.dynamic_config.secrets.* secure-setting namespace
• allow dynamic authentication and authorization backend settings to reference aliases with ${keystore:<alias>}
• retain copied secret values in memory because the OpenSearch keystore is readable only during initialization and reload callbacks
• implement ReloadablePlugin so POST /_nodes/reload_secure_settings refreshes secrets and rebuilds the active security configuration
• restore the previous secret snapshot and configuration if rebuilding with reloaded values fails
• merge current main, retaining upstream setting upgraders and secure-settings reload support

Integration-test fix

The original integration failures were caused by DynamicConfigSecrets.resolve() rebuilding every setting as a scalar. That converted a list-valued JWT signing_key into one literal list string, which prevented authentication backends from initializing and caused broad 401 failures.

The resolver now preserves the original Settings representation and rewrites only exact keystore references. List-valued settings are read with getAsList() and written with putList(). New regression coverage verifies unchanged structured settings and keystore references inside lists.

Example

Add secrets to each node:

bin/opensearch-keystore add plugins.security.dynamic_config.secrets.ldap.bind_dn
bin/opensearch-keystore add plugins.security.dynamic_config.secrets.ldap.password

Reference them from config.yml or the corresponding security-index configuration:

bind_dn: ${keystore:ldap.bind_dn}
password: ${keystore:ldap.password}

After changing the values on disk, reload them across the cluster:

POST /_nodes/reload_secure_settings

Validation

• ./gradlew spotlessApply
• focused unit coverage: DynamicConfigSecretsTests and DynamicConfigModelV7Tests
• focused root integration coverage: the previously failing JWT test, full JwtAuthenticationTests, and LdapAuthenticationTest
• the complete root :integrationTest suite exceeded the 30-minute local command limit without producing a result; CI is the full cross-platform verification

Check List

• New functionality includes testing
• 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.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit f272084)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible NPE on close close() calls dynamicConfigSecrets.close() without a null check. Although the field is assigned in the constructor, super.close() is called first, and if any subclass/superclass initialization path leaves dynamicConfigSecrets unset (or if close is invoked during a failed startup), this will NPE. More importantly, the ordering means super.close() runs before the secrets are cleared; if super.close throws, the secret material stays in memory. Consider defensively null-checking and clearing secrets even if super.close throws. public void close() throws IOException { super.close(); dynamicConfigSecrets.close(); if (auditLog != null) { auditLog.close(); } Secret material leaks to heap as String resolve(String) returns new String(secret), converting the keystore-sourced char[] into an immutable String that lives in the heap until GC. This defeats the purpose of storing secrets as char[] and using SecureString/keystore. The resolved Settings also stores the secret as a String value internally. If the goal is to keep secrets out of long-lived heap strings, the design of feeding secrets into Settings.put(key, String) inherently undermines it — worth documenting this limitation explicitly, or reconsidering the approach for password-like fields. private String resolve(String value) { Matcher matcher = REFERENCE_PATTERN.matcher(value); if (!matcher.matches()) { return value; } String alias = matcher.group(1); char[] secret = secrets.get(alias); if (secret == null) { throw new SettingsException("Keystore setting [" + SETTING_PREFIX + alias + "] referenced by dynamic configuration is missing"); } return new String(secret); } Reload rollback can double-clear In reload, if the initial rebuild.run() throws, the code restores previous into secrets, calls clear(replacement), then invokes rebuild.run() a second time to roll back the model. If that rollback rebuild also throws, only addSuppressed is called — but secrets still points at previous (correct) while the caller now has an inconsistent DynamicConfigFactory state. Also, if the second rebuild succeeds but a later close() is invoked, clear(secrets) will zero out previous's char[] arrays that are shared references — fine here, but note that replacement has already been cleared, so any code path still holding a reference to those char[] would see zeros. Worth verifying no listeners cached values from the transient new secrets before the failure. public synchronized void reload(Settings settings, Runnable rebuild) { Map<String, char[]> replacement = load(settings); Map<String, char[]> previous = secrets; secrets = replacement; try { rebuild.run(); clear(previous); } catch (RuntimeException | Error e) { secrets = previous; clear(replacement); try { rebuild.run(); } catch (RuntimeException | Error rollbackException) { e.addSuppressed(rollbackException); } throw e; } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f272084
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Ensure visibility of mutable secrets field The resolve method reads the secrets field without synchronization, but reload mutates it while holding the monitor. Since secrets is not volatile, a caller of the public synchronized resolve(Settings) will re-enter resolve(String) on the same thread (fine), but future refactors or concurrent validation from other threads could observe stale references. Make secrets volatile (or read it into a local within the synchronized public methods and pass down) to ensure visibility guarantees are explicit. src/main/java/org/opensearch/security/securityconf/DynamicConfigSecrets.java [36] -private String resolve(String value) { - Matcher matcher = REFERENCE_PATTERN.matcher(value); - if (!matcher.matches()) { - return value; - } +private volatile Map<String, char[]> secrets; - String alias = matcher.group(1); - char[] secret = secrets.get(alias); - if (secret == null) { - throw new SettingsException("Keystore setting [" + SETTING_PREFIX + alias + "] referenced by dynamic configuration is missing"); - } - return new String(secret); -} - Suggestion importance[1-10]: 5 __ Why: Making secrets volatile improves memory visibility guarantees for any non-synchronized reads. However, current public methods that mutate/read are all synchronized, so the practical impact is minor defensive hardening.	Low
	Clear replacement only after successful rollback On rollback the code calls clear(replacement) before invoking rebuild.run() a second time, but the rollback rebuild may itself invoke resolve which reads from secrets (now restored to previous) — that is fine, however if the rollback rebuild fails, the original exception is thrown with a suppressed one but previous is now the live map and is never cleared on subsequent close() if state is inconsistent. More importantly, if rebuild.run() (first call) partially consumed resolved secrets and then threw, callers rely on rollback succeeding; consider clearing replacement only after the rollback rebuild completes successfully, to avoid leaving stakeholders referencing zeroed char arrays. src/main/java/org/opensearch/security/securityconf/DynamicConfigSecrets.java [62-79] public synchronized void reload(Settings settings, Runnable rebuild) { Map<String, char[]> replacement = load(settings); Map<String, char[]> previous = secrets; secrets = replacement; try { rebuild.run(); clear(previous); } catch (RuntimeException | Error e) { secrets = previous; - clear(replacement); try { rebuild.run(); + clear(replacement); } catch (RuntimeException | Error rollbackException) { e.addSuppressed(rollbackException); } throw e; } } Suggestion importance[1-10]: 4 __ Why: The suggestion changes ordering of clear(replacement), but since secrets is already restored to previous before rollback, the rollback rebuild does not reference replacement. The reordering is a minor tweak with limited practical benefit.	Low
	Null-guard secrets close for safety dynamicConfigSecrets is initialized in the constructor, but close() is inherited and could theoretically be invoked in error paths before the field is set (e.g., if super(...) throws in subclasses). Additionally, close() is not idempotent — a second call after close() empties the map is safe, but null-guard the field to align with the defensive auditLog != null check pattern used immediately below. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [356-363] @Override public void close() throws IOException { super.close(); - dynamicConfigSecrets.close(); + if (dynamicConfigSecrets != null) { + dynamicConfigSecrets.close(); + } if (auditLog != null) { auditLog.close(); } } Suggestion importance[1-10]: 3 __ Why: dynamicConfigSecrets is final and initialized in the constructor, so it should never be null when close() is called normally. The null-guard aligns with local style but offers only minor defensive value.	Low
Security	Document secret materialization into String heap Converting the secret via new String(secret) copies the sensitive material into the interned/GC-managed String heap indefinitely, defeating the purpose of storing secrets as char[] and calling clear() on close/reload. Since Settings.put(String, String) requires a String, this leak is largely unavoidable at this boundary, but consider documenting this or using Settings.Builder.put(String, char[])/SecureString where the settings API supports it, to minimize exposure. src/main/java/org/opensearch/security/securityconf/DynamicConfigSecrets.java [87-92] String alias = matcher.group(1); char[] secret = secrets.get(alias); if (secret == null) { throw new SettingsException("Keystore setting [" + SETTING_PREFIX + alias + "] referenced by dynamic configuration is missing"); } +// Note: resolved secret is materialized as String because downstream Settings API requires it; +// this makes the secret live on the String heap until GC. return new String(secret); Suggestion importance[1-10]: 3 __ Why: The observation about secret exposure via new String(secret) is valid, but the suggestion only adds a comment without changing behavior, providing marginal value.	Low

---

Previous suggestions

Suggestions up to commit 6ff2165

Category	Suggestion	Impact
General	Fail fast on partial keystore references Using matcher.matches() requires the entire value to be a single reference, which silently ignores embedded references like prefix-${keystore:x} (as tested). This may surprise users who assume interpolation is supported. Consider either explicitly rejecting values that contain a ${keystore: substring but do not fully match the pattern, or supporting embedded substitution — otherwise misconfigurations pass through as literal strings and lead to authentication failures at runtime rather than config-load time. src/main/java/org/opensearch/security/securityconf/DynamicConfigSecrets.java [80-92] private String resolve(String value) { Matcher matcher = REFERENCE_PATTERN.matcher(value); if (!matcher.matches()) { + if (value.contains("${keystore:")) { + throw new SettingsException( + "Dynamic configuration value [" + value + "] contains a keystore reference that is not the entire value; " + + "embedded/partial keystore references are not supported" + ); + } return value; } String alias = matcher.group(1); char[] secret = secrets.get(alias); if (secret == null) { throw new SettingsException("Keystore setting [" + SETTING_PREFIX + alias + "] referenced by dynamic configuration is missing"); } return new String(secret); } Suggestion importance[1-10]: 6 __ Why: Valid observation: partial references like prefix-${keystore:x} are silently passed through as literals (as demonstrated by the test), which could lead to confusing runtime auth failures. Failing fast improves UX, though it's a design choice.	Low
	Clear old secrets only after successful rebuild The rollback path clears replacement before calling rebuild.run() again, but secrets was already reset to previous — this is fine, however if the rollback rebuild.run() itself throws, previous (now the live secrets) is never cleared and will leak on close only. More importantly, the finally-style clearing of previous on success occurs after rebuild.run(), so if any consumer retained a reference to the old char[] via resolve() it will be zeroed underneath them. Consider only clearing previous once you're certain no consumer holds the returned string-backed data, or document that resolve() returns copies (which it does via new String). src/main/java/org/opensearch/security/securityconf/DynamicConfigSecrets.java [61-78] public synchronized void reload(Settings settings, Runnable rebuild) { Map<String, char[]> replacement = load(settings); Map<String, char[]> previous = secrets; secrets = replacement; try { rebuild.run(); - clear(previous); } catch (RuntimeException | Error e) { secrets = previous; - clear(replacement); try { rebuild.run(); } catch (RuntimeException | Error rollbackException) { e.addSuppressed(rollbackException); } + clear(replacement); throw e; } + clear(previous); } Suggestion importance[1-10]: 3 __ Why: The existing code already clears previous only after successful rebuild.run() (on the try's last line). The suggested reordering is a minor restructuring with negligible functional impact, and the concern about consumers holding references is mitigated since resolve() returns new String(secret) copies.	Low
	Null-guard secrets close in plugin shutdown dynamicConfigSecrets is initialized in the constructor and should normally be non-null, but if the constructor ever throws after super() but before that assignment (or in subclasses/tests), close() will NPE and mask the real exception. Guard the call with a null check to make shutdown robust. src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java [352-359] @Override public void close() throws IOException { super.close(); - dynamicConfigSecrets.close(); + if (dynamicConfigSecrets != null) { + dynamicConfigSecrets.close(); + } if (auditLog != null) { auditLog.close(); } } Suggestion importance[1-10]: 3 __ Why: Since dynamicConfigSecrets is a final field initialized in the constructor, it cannot be null under normal circumstances. The guard is defensive but of minor value.	Low

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit f272084.

⛔ Hard block: Issues at High severity or above will block this PR from merging.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/securityconf/DynamicConfigSecrets.java	88	low	Resolved secrets are materialized as Java String objects (via `new String(secret)`), which are immutable and cannot be explicitly zeroed from memory. While the source char[] arrays are properly zeroed on close(), the String copies may persist in the JVM heap until GC. This is a known limitation of passing secrets through the Settings API but represents a minor residual exposure window. No malicious intent detected; this is an unavoidable trade-off given the OpenSearch Settings API design.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

Persistent review updated to latest commit f272084

[codecov]

Codecov Report

❌ Patch coverage is 85.39326% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.02%. Comparing base (ed1f745) to head (f272084).

Files with missing lines	Patch %	Lines
.../opensearch/security/OpenSearchSecurityPlugin.java	40.00%	6 Missing ⚠️
...ch/security/securityconf/DynamicConfigSecrets.java	90.90%	5 Missing ⚠️
...ch/security/securityconf/DynamicConfigFactory.java	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6475      +/-   ##
==========================================
+ Coverage   75.99%   76.02%   +0.03%     
==========================================
  Files         467      468       +1     
  Lines       31175    31241      +66     
  Branches     4692     4698       +6     
==========================================
+ Hits        23691    23751      +60     
- Misses       5306     5315       +9     
+ Partials     2178     2175       -3     

Files with missing lines	Coverage Δ
...ch/security/securityconf/DynamicConfigModelV7.java	70.45% <100.00%> (+0.79%)	⬆️
...ch/security/securityconf/DynamicConfigFactory.java	66.66% <60.00%> (-0.42%)	⬇️
...ch/security/securityconf/DynamicConfigSecrets.java	90.90% <90.90%> (ø)
.../opensearch/security/OpenSearchSecurityPlugin.java	83.75% <40.00%> (-0.43%)	⬇️

... and 9 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.