Encrypted Channel Configurations

[TomasLongo]

Encrypted Channel Configurations

Currently, Webhook-Urls (and headers) like Chime, Slack, Teams, etc. are stored in plain text in OpenSearch. This PR addresses this limitation by introducing symmetrically encrypted webhook urls at rest.

URLs are opaque in the index and user facing APIs while beeing transparent at runtime.

Keys are provisioned manually via the opensearch-keystore tool and read inside the plugin using the SecureSetting pattern.

Architecture

Two new Classes are introduced:

FieldEncryptionService (notifications/util/FieldEncryptionService.kt)

• AES-256-GCM authenticated encryption with a fresh random 12-byte nonce per write
• Ciphertext wire format: enc:v1:<Base64(nonce || ciphertext || GCM-tag)> — the enc:v1: sentinel enables backward-compatible plaintext pass-through on read and future key-rotation versioning
• Passthrough mode: when no key is configured the service is a no-op, so the feature can be deployed before a key is provisioned without data loss
• close() zeroizes key material via the Destroyable interface

ConfigEncryptionTransformer (notifications/util/ConfigEncryptionTransformer.kt)

• Wraps FieldEncryptionService and applies encrypt/decrypt to the sensitive fields of a NotificationConfig object
• Covers: Slack.url, Chime.url, MicrosoftTeams.url, Mattermost.url, Webhook.url, and all Webhook.headerParams values

Changes in common-utils are necessary

common-utils PR

Channel objects (like Slack, Chime, ...) perform a strict url validation when beeing constructed. Handling ciphertexts makes this not feasible anymore. Validation of urls should be, and is already covered by the ui.

NOTE: Tests will fail, as long as common-utils is not updated

Point of Encryption

URLs are encrypted when creating or updating channel configurations. Encryption when updating also enables a lazy migration path.

Pass Through Mode

For backward compatibility, tbe process checks if the url is an encrypted value during decryption, by checking if the url is prefixed with enc:v1. Unencryped values are simply passed on as is and lazily encryped, when the channel configuration is updated.

Key-Rotation

Key-Rotation follows a simple procedure outlined in the included runbook

Limitations

• Search for terms in URLs won't work anymore
  • Tests covering the functionality have been removed
• GET Enpoints return the ciphertext for encrypted fields. Masking fields could be a hardenining follow-up
• The secret key is held in memory for the lifespan of the plugin. Re-reading the key at every request is not possible since the key store is closed after initialization and throws a 'key store closed' error.

Migration Paths

Lazy, by updating channel configs (currently working)

Passthrough-Mode enables lazy migration. Unencrypted channel configs are read with their legacy plain-text data and encrypted upon beeing updated.

Bulk, via admin endpoint (open for discussion)

An admin-only endpoint could be implemented that encrypts legacy channel configs in one pass.

Security Considerations

Lifespan of the decrypted config

The channel config is decrypted right before sending a message. Destroying the now plain-text fields of the config is at the mercy of the GC, since the backing field for the sensitive information is of type String which we can not be destroyed manually.

A possible approach would introduce dedicated data objects that read sensitive channel information into a ByteArray/char[] in order to zero it after a message has been sent. (Alternatively, the data objects in common-util are modified to support destruction of sensitive data)

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

• New functionality includes testing.
• New functionality has been documented.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff.
• Public documentation issue/PR created.

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.

[cwperks]

FYI Security repo also has had a feature request to better support using opensearch-keystore references from dynamic security configuration in the security index.

^ I would like to see better support for dynamic values in the keystore that can be updated from a rest handler to all nodes?

Alternatively, there is https://github.com/opensearch-project/opensearch-storage-encryption but idk how that would work for system indices.

[TomasLongo]

@KashKondaka or any other maintainer. Any Feedback for this change? Anything I can do to kick off a review?

Thanks and cheers, Tomas

[kanwaljeetd]

Reviewing Internally

[eirsep]

Thanks for adding this @TomasLongo

I will review the common utils and notifications this week.

Meanwhile plz sign-off the commits to make the DCO check succeed

[eirsep]

I verified the dependency tree and junit-platform-launcher already resolves to 1.7.2 via JUnit's platform version alignment (constrained by junit-jupiter-engine:${junit_version} where
junit_version = "5.7.2"). The explicit pin is redundant today, but creates a maintenance hazard — if junit_version is bumped in the future, the platform launcher would stay stuck at 1.7.2 and cause version
mismatches. The unpinned declaration lets Gradle's version alignment keep everything in sync automatically.

Please revert this change in core-spi/build.gradle, core/build.gradle, and notifications/build.gradle.

[eirsep]

same as https://github.com/opensearch-project/notifications/pull/1218/changes#r3539136039

[eirsep]

Before I review I also wanted to call out a few things:

1. Plz check security plugin https://github.com/opensearch-project/security/blob/main/config/roles.yml#L337-L350 for adding mapping new API actions to appropriate roles: https://github.com/opensearch-project/security/blob/main/config/roles.yml#L337-L350
2. Kindly fix DCO failure by amending all commits with -s
3. Plz try revert the version pinning on the junit dep

[TomasLongo]

Thanks for the hints.

Point 2. and 3. are fixed. Though I'm not able to run any tests (neither via IDE nor terminal) if I don't pin the junit version number. Am I missing some local settings?

As for the role mapping, should I place the new action cluster:admin/opensearch/notifications/configs/reencrypt under the notifications_read_access section?

[TomasLongo]

@eirsep Please note, that I had to reset some commits in order to fix the DCO issue. A new pull is needed.

[eirsep]

@TomasLongo

Thanks for the contribution — protecting channel secrets at rest addresses a real gap, and the implementation shows care (authenticated encryption, sentinel prefix, rotation runbook).

That said, I'm requesting changes at two levels, and I'd suggest holding off on code changes until the first one is settled — no point reworking details if the direction shifts.

1. The approach itself needs maintainer alignment first.

Before we commit to plugin-local field encryption, I'd like consensus on whether this is the right layer to solve this:

• The key lives in each node's local keystore, while the encrypted data lives in the replicated index. These have different lifecycles, and the design currently has no answer for node replacement,
  autoscaling/containerized deployments, or fleet-replacement version upgrades — every new node needs the key injected before serving traffic, effectively making an external key-distribution system a
  prerequisite for the feature. Mixed-state clusters (some nodes keyed, some not) mean the send notifications API succeeds or fails depending on which node executes the request — routing-dependent intermittent
  failures.
• As @cwperks noted, the security plugin has the same plaintext-secrets-in-a-system-index problem. Solving this per-plugin means every plugin independently inherits the crypto code, rotation machinery, and
  key-distribution burden. A core-level capability (keystore-referenced settings or storage-layer encryption) would solve it once.
• Once merged, the enc:v1: wire format, the _reencrypt endpoint, and the rotation procedure all become API surface we maintain indefinitely.

I'll follow up to drive that discussion with maintainers. Please hold off on rework until there's a green signal on the direction.

2. Independent of the approach, there are functional bugs in the current implementation.

I've left these as individual comments on the relevant lines so they're actionable whenever the direction is confirmed. Summary: a read-modify-write data-corruption bug (double encryption), a silently-unsafe
passthrough mode, GET returning ciphertext (breaking API change), a missing decrypt on the Mattermost path, key material being logged, and a couple of build hygiene items.

[eirsep]

this method,encrypt(), has no isEncrypted() guard. Combined with GET returning ciphertext and updateNotificationConfig() encrypting unconditionally, any read-modify-write flow (e.g., the Dashboards edit screen — user changes only the name) double-encrypts the URL. The "send notification API" path decrypts exactly once, so delivery then posts to enc:v1:... as if it were a hostname — the channel is permanently broken, and the update returned 200. Webhook headerParams hit the same path. I verified this with a unit test against this branch (happy to share). Minimum fix: if (isEncrypted(plaintext)) return plaintext. but i still have reservations on why

[eirsep]

The passthrough failure mode is dangerously silent. A node without the key :
(a) returns ciphertext as-is on decrypt → "send notification API" fails confusingly,
(b) no-ops on encrypt → new configs are written in plaintext, silently voiding the security guarantee — both with only a WARN log. If encrypted values exist in the index, a keyless node should fail closed for config operations rather than degrade quietly. Please add a strict/fail-fast mode.

[eirsep]

GET returning ciphertext is a breaking API change, not a hardening follow-up. Dashboards edit screens, automation, and cross-plugin consumers via NotificationsPluginInterface all receive unusable values —
and writing them back triggers the corruption in my other comment. GET semantics (decrypt-on-read vs. masked *** with a documented contract) need to be decided in this PR.

[eirsep]

The MATTERMOST case uses the raw configData without calling configEncryptionTransformer.decryptConfig(...), unlike the Slack/Chime/Teams/Webhook branches. The send notifications API will use the ciphertext
as the URL for Mattermost channels and fail.

[TomasLongo]

Thanks for pointing that out. I simply didn't see that Mattermost used the sendSlack function.

[eirsep]

Please remove — never log key material or key objects, even at INFO. The stray "tlongo" debug string above it should also go.

[TomasLongo]

d#$% it. This is a debug log that should have been removed. Will remove it.