Support keystore references in notification configs

[cwperks]

Summary

• register config-scoped secure settings under opensearch.notifications.keystore.<config-id>.<alias>
• allow notification URLs and webhook header values to contain ${keystore:<alias>} references
• resolve references only when constructing a destination, keeping secrets out of the system index and configuration APIs
• implement ReloadablePlugin so _nodes/reload_secure_settings refreshes the in-memory values
• clear prior character arrays on reload and shutdown

Example

Add a node-local value on every node that can send notifications:

bin/opensearch-keystore add opensearch.notifications.keystore.<config-id>.slack.path

Store the reference in the notification configuration:

https://hooks.slack.com/services/${keystore:slack.path}

After changing keystore files, reload them through the OpenSearch nodes reload secure settings API.

Design discussion

This is an exploratory alternative to #1218. Rather than encrypting secret material into .opensearch-notifications-config, it stores an explicit reference and obtains the value from each node OpenSearch keystore at send time. The system index remains searchable for non-secret structure, and the implementation does not require encryption-key rotation or ciphertext migration.

The current prototype intentionally scopes aliases by notification config ID so one configuration cannot reference another configuration secret alias.

Open questions and limitations

• Every node that can send a notification must have the referenced value in its local keystore.
• Auto-generated configuration IDs require provisioning the scoped setting after creation, followed by secure-settings reload.
• Existing common-utils URL validation rejects a URL whose entire value is ${keystore:...}. This prototype therefore demonstrates embedded URL references; a production design may need a common-utils change to recognize exact reference tokens.
• Embedded references in generic webhook URLs require additional threat-model review: a user allowed to edit the surrounding URL could redirect the resolved secret. Whole-field references or stronger validation/immutability constraints would reduce that risk.

This draft is intended to compare the keystore-reference approach with encrypted system-index storage before settling the API and authorization model.

Testing

• ./gradlew :notifications:test
• ktlint (run by the Gradle build)

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit edec2a7)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Secret material lifetime: Resolved secrets are concatenated into Java String instances in NotificationConfigSecrets.resolve() and passed to destination constructors (Slack/Chime/Webhook URLs, header values). Strings are immutable and cannot be zeroed, so the "clear char arrays on reload/close" guarantee only applies to the cached copy, not to resolved values in use. Additionally, as noted in the PR description, embedded ${keystore:...} references inside user-editable webhook URLs allow a user with edit permission on the URL to redirect the resolved secret to an attacker-controlled host; this threat model is acknowledged but unmitigated in this prototype.

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Alias key mismatch In load(), SETTING.getAsMap(settings) returns the alias portion of the affix setting as keys (e.g. config-1.slack.path), but the code stores them directly into secrets. Then resolve() looks up secrets["$configId.$alias"]. Confirm that getAsMap for a prefixKeySetting returns the suffix (concatenation matches) and not the full setting name; if it returns the full name including the opensearch.notifications.keystore. prefix, all lookups will miss and every reference will throw SettingsException. Worth verifying with an integration test that actually configures a real KeyStoreWrapper, since the unit test uses a custom SecureSettings that may not reproduce the framework behavior. private fun load(settings: Settings): Map<String, CharArray> { val loaded = mutableMapOf<String, CharArray>() try { SETTING.getAsMap(settings).forEach { (alias, secureString) -> secureString.use { loaded[alias] = it.chars.clone() } } return Collections.unmodifiableMap(loaded) } catch (exception: RuntimeException) { clear(loaded) throw exception } } Secret leaked into String resolve() builds a String via REFERENCE_PATTERN.replace and String(secret), which places the secret material into immutable String objects that cannot be cleared. This defeats the purpose of storing secrets in CharArray and clearing on reload/close, since the resolved URL/header value will live in the JVM string pool/heap until GC. Given the PR explicitly advertises clearing prior character arrays on reload and shutdown, callers should be aware secrets still end up as long-lived Strings once passed to destination constructors. fun resolve(configId: String, value: String): String { return REFERENCE_PATTERN.replace(value) { matchResult -> val alias = matchResult.groupValues[1] val settingName = "$configId.$alias" val secret = secrets[settingName] ?: throw SettingsException("Keystore setting [$SETTING_PREFIX$settingName] referenced by notification configuration is missing") String(secret) } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to edec2a7
Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use robust API for reading secure affix settings SETTING.getAsMap(settings) returns concrete namespaces from the affix setting; the key passed to the lambda is the concrete setting suffix (e.g., config-1.slack.path), not just the alias. The variable name alias is misleading but the value is what resolve() looks up (configId.alias), so this works. However, closing the SecureString inside use {} and then only holding a cloned CharArray is fine—but note that the map returned by getAsMap may already close SecureStrings; verify the SecureString remains usable inside use. Consider using SETTING.getNamespaces(settings) and SETTING.getConcreteSetting(...).get(settings) for a more robust API contract. notifications/notifications/src/main/kotlin/org/opensearch/notifications/util/NotificationConfigSecrets.kt [53-66] private fun load(settings: Settings): Map<String, CharArray> { val loaded = mutableMapOf<String, CharArray>() try { - SETTING.getAsMap(settings).forEach { (alias, secureString) -> - secureString.use { - loaded[alias] = it.chars.clone() + SETTING.getNamespaces(settings).forEach { namespace -> + SETTING.getConcreteSettingForNamespace(namespace).get(settings).use { secureString -> + loaded[namespace] = secureString.chars.clone() } } return Collections.unmodifiableMap(loaded) } catch (exception: RuntimeException) { clear(loaded) throw exception } } Suggestion importance[1-10]: 5 __ Why: Suggests a more idiomatic API for affix settings, which is reasonable but the existing code appears functional. Moderate maintainability improvement.	Low
Possible issue	Avoid non-null assertion on document id Using channel.docInfo.id!! will throw a NullPointerException at runtime if id is ever null, resulting in an unclear error for the caller. Guard against a null id explicitly and fail the send with a descriptive error, or fall back to a safe default (e.g., empty string) so secret resolution simply won't match and returns a clean SettingsException. notifications/notifications/src/main/kotlin/org/opensearch/notifications/send/SendMessageActionHelper.kt [234-243] -ConfigType.SLACK -> sendSlackMessage(channel.docInfo.id!!, configData as Slack, message, eventStatus, eventSource.referenceId) -ConfigType.MATTERMOST -> sendSlackMessage(channel.docInfo.id!!, configData as Slack, message, eventStatus, eventSource.referenceId) -ConfigType.CHIME -> sendChimeMessage(channel.docInfo.id!!, configData as Chime, message, eventStatus, eventSource.referenceId) +val configId = channel.docInfo.id + ?: throw IllegalStateException("Notification config document id is missing; cannot resolve keystore references") +ConfigType.SLACK -> sendSlackMessage(configId, configData as Slack, message, eventStatus, eventSource.referenceId) +ConfigType.MATTERMOST -> sendSlackMessage(configId, configData as Slack, message, eventStatus, eventSource.referenceId) +ConfigType.CHIME -> sendChimeMessage(configId, configData as Chime, message, eventStatus, eventSource.referenceId) Suggestion importance[1-10]: 4 __ Why: Replacing !! with an explicit null check produces clearer errors, but docInfo.id is likely always populated for retrieved configs, making this a minor robustness improvement.	Low
Security	Resolved secrets leak into immutable strings The resolve method converts the stored CharArray secret to an immutable String, which defeats the purpose of using CharArray/SecureString for sensitive material—the plaintext credential ends up in the string pool and any URL/header logs. Consider whether returning String here is acceptable; if the destination APIs require String, at minimum document this leak and ensure the resolved value is never logged. notifications/notifications/src/main/kotlin/org/opensearch/notifications/util/NotificationConfigSecrets.kt [29-37] @Synchronized fun resolve(configId: String, value: String): String { + // NOTE: destination SPI requires String; resolved secret material will exist as an immutable String + // for the duration of the send. Callers must never log the returned value. return REFERENCE_PATTERN.replace(value) { matchResult -> val alias = matchResult.groupValues[1] val settingName = "$configId.$alias" val secret = secrets[settingName] ?: throw SettingsException("Keystore setting [$SETTING_PREFIX$settingName] referenced by notification configuration is missing") String(secret) } } Suggestion importance[1-10]: 4 __ Why: Valid security observation about secrets ending up in immutable String objects, but since destination SPIs require String, the suggestion mainly adds documentation rather than a substantive fix.	Low

---

Previous suggestions

Suggestions up to commit 5d0bd60

Category	Suggestion	Impact
Possible issue	Fix affix setting enumeration API usage Setting.AffixSetting.getAsMap does not exist on a plain affix setting and would fail at runtime; you need to iterate over concrete namespaces. Use SETTING.getNamespaces(settings) (or iterate settings.keySet() filtered by prefix) and then call SETTING.getConcreteSetting(fullKey).get(settings) to load each secret. Otherwise plugin startup and reload() will throw. notifications/notifications/src/main/kotlin/org/opensearch/notifications/util/NotificationConfigSecrets.kt [53-66] private fun load(settings: Settings): Map<String, CharArray> { val loaded = mutableMapOf<String, CharArray>() try { - SETTING.getAsMap(settings).forEach { (alias, secureString) -> - secureString.use { - loaded[alias] = it.chars.clone() + SETTING.getNamespaces(settings).forEach { namespace -> + val concrete = SETTING.getConcreteSettingForNamespace(namespace) + concrete.get(settings).use { secureString -> + loaded[namespace] = secureString.chars.clone() } } return Collections.unmodifiableMap(loaded) } catch (exception: RuntimeException) { clear(loaded) throw exception } } Suggestion importance[1-10]: 8 __ Why: Valid concern - Setting.AffixSetting.getAsMap is not a standard API on affix settings in OpenSearch. Using getNamespaces + getConcreteSettingForNamespace is the correct pattern and this could cause runtime failures on plugin startup.	Medium
	Avoid NPE on missing config id Using channel.docInfo.id!! will throw an NPE and abort delivery if the config document has no id populated, which was previously tolerated. Consider falling back to a safe default (e.g., empty string or eventSource.referenceId) so a missing id doesn't break sends for configs without keystore references. notifications/notifications/src/main/kotlin/org/opensearch/notifications/send/SendMessageActionHelper.kt [234-245] -ConfigType.SLACK -> sendSlackMessage(channel.docInfo.id!!, configData as Slack, message, eventStatus, eventSource.referenceId) -ConfigType.MATTERMOST -> sendSlackMessage(channel.docInfo.id!!, configData as Slack, message, eventStatus, eventSource.referenceId) -ConfigType.CHIME -> sendChimeMessage(channel.docInfo.id!!, configData as Chime, message, eventStatus, eventSource.referenceId) +val configId = channel.docInfo.id ?: "" +val response = when (configType) { + ConfigType.NONE -> null + ConfigType.SLACK -> sendSlackMessage(configId, configData as Slack, message, eventStatus, eventSource.referenceId) + ConfigType.MATTERMOST -> sendSlackMessage(configId, configData as Slack, message, eventStatus, eventSource.referenceId) + ConfigType.CHIME -> sendChimeMessage(configId, configData as Chime, message, eventStatus, eventSource.referenceId) Suggestion importance[1-10]: 5 __ Why: Reasonable defensive programming suggestion, but in practice docInfo.id is likely always populated for retrieved configs. The impact is moderate.	Low
Security	Minimize secret exposure during resolution Building a String from the secret's CharArray places the secret into the JVM string pool where it cannot be zeroed out, defeating the purpose of SecureString. Additionally, the returned resolved string is passed to destinations as a plain String. At minimum, avoid keeping resolved strings longer than needed; consider documenting this trade-off, and ensure resolveSecureValue result is not logged. notifications/notifications/src/main/kotlin/org/opensearch/notifications/util/NotificationConfigSecrets.kt [29-37] @Synchronized fun resolve(configId: String, value: String): String { + if (!REFERENCE_PATTERN.containsMatchIn(value)) return value return REFERENCE_PATTERN.replace(value) { matchResult -> val alias = matchResult.groupValues[1] val settingName = "$configId.$alias" val secret = secrets[settingName] ?: throw SettingsException("Keystore setting [$SETTING_PREFIX$settingName] referenced by notification configuration is missing") String(secret) } } Suggestion importance[1-10]: 3 __ Why: The improved code only adds a short-circuit check but doesn't actually solve the underlying string interning concern raised in the description. The suggestion is more advisory than an actual code fix.	Low

[github-actions]

PR Code Analyzer ❗

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

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

Path	Line	Severity	Description
notifications/notifications/build.gradle	148	high	New dependency forced: software.amazon.awssdk:sts. Per mandatory rule, all dependency changes must be flagged regardless of apparent legitimacy — maintainers must verify artifact authenticity.
notifications/notifications/build.gradle	149	high	New dependency forced: software.amazon.awssdk:netty-nio-client. Per mandatory rule, all dependency changes must be flagged regardless of apparent legitimacy — maintainers must verify artifact authenticity.
notifications/core/build.gradle	149	high	New dependency added: com.fasterxml.jackson.core:jackson-core. Per mandatory rule, all dependency changes must be flagged regardless of apparent legitimacy — maintainers must verify artifact authenticity.

The table above displays the top 10 most important findings.

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

---

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 edec2a7