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

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 spotlessJavaCheck
• ./gradlew checkstyleMain checkstyleTest
• focused unit coverage for resolution, missing references, rotation, rollback, and dynamic backend construction
• relevant DynamicConfigSecretsTests, DynamicConfigModelV7Tests, and ConfigurationRepositoryTest suites pass

Note

• full precommit reaches unrelated existing forbidden-API failures in the sample resource plugin for URL.openStream()

[github-actions]

PR Reviewer Guide 🔍

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() unconditionally calls dynamicConfigSecrets.close(), but the field is only initialized in the constructor that takes (Settings, Path). If any other constructor path is used (or if close() is invoked before the plugin constructor completes in some scenarios), this will throw NPE. More concerning: the null-check pattern used for auditLog right below suggests defensive coding is expected here. Consider a null-check on dynamicConfigSecrets for consistency and safety. public void close() throws IOException { super.close(); dynamicConfigSecrets.close(); if (auditLog != null) { auditLog.close(); } Secret exposed as String resolve(String) returns new String(secret) and the resolved value is placed into a Settings.Builder via resolved.put(key, ...). This defeats the purpose of storing secrets as char[] and holding them via SecureString: the secret is now an immutable String in the heap (and in the resulting Settings object) that cannot be zeroed and will linger until GC. Downstream backends receiving this Settings will read a plain String anyway, but consumers that support SecureString lose that capability. Consider documenting this trade-off or routing values through SecureSetting-aware APIs where possible. 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); } Partial references silently ignored resolve(String) uses matcher.matches() (full-string match), so any value like prefix-${keystore:foo} or a value containing multiple references is returned as-is with no error. A user writing password: "pw-${keystore:ldap.password}" will get the literal string in the backend config with no warning. Either support matcher.find()/replacement semantics or throw a SettingsException when a partial ${keystore:...} token is detected so misconfigurations are not silent. 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); } Validation coverage gap validateSecretReferences() only validates domains where http_enabled is true, but buildAAA() builds backends without checking http_enabled here. If a domain has http_enabled=false but is still constructed (or transport-only), an unresolved ${keystore:...} reference will now surface as a SettingsException from dynamicSettings() during backend construction rather than as an upfront validation error. Confirm the filter matches the actual iteration path used in buildAAA(). private void validateSecretReferences() { config.dynamic.authz.getDomains() .values() .stream() .filter(domain -> domain.http_enabled) .forEach(domain -> dynamicConfigSecrets.validate(loadSettings(domain.authorization_backend.configAsJson()))); config.dynamic.authc.getDomains().values().stream().filter(domain -> domain.http_enabled).forEach(domain -> { dynamicConfigSecrets.validate(loadSettings(domain.authentication_backend.configAsJson())); if (domain.http_authenticator.type != null) { dynamicConfigSecrets.validate(loadSettings(domain.http_authenticator.configAsJson())); } }); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

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