Onboards Alerting Plugin to Centralized Resource Authz

[DarshitChanpura]

Description

Implements resource-access-control for monitor.

Note — SdkUtils.await behavior change: the CompletionStage.await() helper now unwraps CompletionException/ExecutionException before resuming, so the underlying exception type and REST status survive the async boundary (e.g. a 409 VersionConflictEngineException is no longer masked as a 500). This affects all existing await() call sites, not just the resource-sharing paths.

Related Issues

Resolves #2195

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.

[vikhy-aws]

How about alerts and comments which are stored in different indices?

Alerts: https://github.com/opensearch-project/alerting/blob/main/alerting/src/main/kotlin/org/opensearch/alerting/alerts/AlertIndices.kt#L116-L154

Comments: https://github.com/opensearch-project/alerting/blob/main/alerting/src/main/kotlin/org/opensearch/alerting/comments/CommentsIndices.kt#L66-L84

[DarshitChanpura]

Alerts and comments are treated as subordinate resources rather than registered resource types, so we don't register their indices with the sharing framework. Instead, access is inherited from the underlying monitor:

• TransportGetAlertsAction / TransportGetWorkflowAlertsAction call rsc.getAccessibleResourceIds("monitor", ...) and add a monitor_id terms filter to the alert search.
• TransportSearchAlertingCommentAction does the same via alert IDs derived from accessible monitors.
• TransportIndexAlertingCommentAction / TransportDeleteAlertingCommentAction rely on the ActionFilter blocking the underlying alert fetch (via DocRequest on the requests, which points at the monitor).

New tests bob cannot see alice's monitor alerts without share and bob can see alice's monitor alerts after share in SecureResourceSharingMonitorRestApiIT cover this.

[DarshitChanpura]

Heads up: CI on this branch will fail until common-utils#980 merges. That PR adds DocRequest to alerting request classes (GetAlertsRequest, AcknowledgeAlertRequest, IndexWorkflowRequest, GetWorkflowAlertsRequest, GetFindingsRequest, AcknowledgeChainedAlertRequest) which the transport actions here depend on for the security plugin's ActionFilter interception.

[riysaxen-amzn]

Review: opensearch-project/alerting PR #2180 — Onboards Alerting Plugin to Centralized Resource Authz

Overall this is a well-executed onboarding. The ThreadContext choreography (capture-before-stash → restore for the shard-level ResourceIndexListener → per-call stash for internal-index writes via putDataObjectStashed/indexStashed) is handled carefully with clear comments explaining the why at each site. The alerting-workflow type naming (avoiding collision with flow-framework's workflow), the fail-closed shouldUseResourceAuthz (requires plugin present AND feature enabled for the type), and the E2E migration test are all solid.

A few items below, roughly in priority order.

---

1. Bug: getAccessibleAlertIDs caps results at 10 alerts

File: alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportSearchAlertingCommentAction.kt (new getAccessibleAlertIDs)

The SearchSourceBuilder in getAccessibleAlertIDs never sets .size(), so the alerts search returns the default 10 hits:

val searchSourceBuilder = SearchSourceBuilder()
    .version(true)
    .seqNoAndPrimaryTerm(true)
    .query(queryBuilder)   // <-- no .size(), defaults to 10

Under RSC, comment search will silently drop comments for any alerts beyond the first 10 on accessible monitors. Suggest .size(MAX_SEARCH_SIZE) (mirroring the existing non-RSC getAlertIDs path) or scroll/PIT.

---

2. Question: does super-admin retain visibility under RSC?

Files: TransportGetAlertsAction.kt, TransportGetWorkflowAlertsAction.kt

The RSC branch is evaluated before the user == null (super-admin) branch:

if (ResourceSharingUtils.shouldUseResourceAuthz(...)) {
    // filter by accessible monitor IDs
} else if (user == null) {
    // super-admin path — never reached under RSC

Unless getAccessibleResourceIds special-cases admin/super-admin inside the security plugin, cluster admins will see zero alerts/monitors they don't own or aren't shared. The tests hint this is real (createMonitorAs notes the base helper's admin-client follow-up GET "fails under resource sharing because admin has no share on the newly-created monitor"). Is admin bypass handled framework-side? If not, this is a significant operational regression worth addressing or documenting.

---

3. Security: stale rbac_roles can be planted while RSC is on, then gate access when RSC is toggled off

Files: TransportIndexMonitorAction.kt, TransportIndexWorkflowAction.kt

Under RSC the legacy rbac_roles validation is skipped entirely:

if (
    !useRsc &&
    user != null &&
    !isAdmin(user) &&
    transformedRequest.rbacRoles != null

Since plugins.security.experimental.resource_sharing.enabled is dynamic (the E2E test toggles it at runtime), a non-admin user could persist arbitrary rbac_roles in a monitor doc while RSC is enabled. If RSC is later disabled, those backend roles become the access gate on the legacy path. Suggest still validating (or stripping) rbac_roles when RSC is on, so the doc can't carry roles the creator never had.

---

4. Production race: async postIndex sharing-entry write vs. immediate read

File: AlertingRestTestCase.kt (waitForResourceSharingEntry) — but the concern is production behavior

The tests poll .opendistro-alerting-config-sharing because create-then-immediately-get races the security plugin's async shard-level postIndex listener and gets a spurious 403 ("No sharing info found"). That same race exists in production: Dashboards creates a monitor and immediately GETs it on the detail page. This is framework-level rather than alerting-level, but it will surface as user-visible flaky 403s — worth an explicit tracking issue against the security plugin if one doesn't exist.

---

5. Scale: unbounded termsQuery on accessible resource IDs

Files: TransportGetAlertsAction.kt, TransportGetWorkflowAlertsAction.kt, TransportSearchAlertingCommentAction.kt

termsQuery("monitor_id", accessibleMonitorIds) will fail past index.max_terms_count (default 65,536) for tenants with very large monitor counts and broad shares. Fine for now, but a code comment or follow-up issue would be prudent.

---

6. Legacy destinations under RSC

File: TransportGetDestinationsAction.kt

Under RSC the destinations search routes through PluginClient and relies on the security plugin's DLS over the shared scheduled-jobs index. Destination docs are not a registered resource type and have no sharing entries / all_shared_principals — please confirm DLS doesn't hide legacy destinations entirely for non-admin users.

---

7. Nits

• Hardcoded sharing-index name: test helpers hardcode .opendistro-alerting-config-sharing. If the security plugin can expose the naming convention (constant or SPI helper), prefer that to avoid silent breakage if the convention changes.
• @Ignored FIXME tests: the two ignored subordinate-resource tests (comments-flow hang in CommentsIndices.createOrUpdateInitialCommentsHistoryIndex, alerts read-only inheritance DLS gap) have good in-code explanations — please link tracking issues so they don't get lost.
• SdkUtils.await unwrapping: unwrapping CompletionException/ExecutionException is a nice fix (preserves e.g. 409 from VersionConflictEngineException), but it changes behavior for all existing await() call sites, not just RSC paths. Deserves a line in the PR description.
• CI matrix: the RSC-enabled leg only running the two RSC suites is reasonable and well-commented; consider a periodic (nightly) full-suite RSC run later to catch interactions the filtered leg misses.

[github-actions]

PR Code Analyzer ❗

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

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

Path	Line	Severity	Description
alerting/build.gradle	177	high	New dependency added: 'opensearch-security-spi' compileOnly artifact. Per mandatory rule, all new dependency additions must be flagged regardless of apparent legitimacy. Maintainers must verify artifact authenticity and that this SPI version matches the expected security plugin contract.
alerting/build.gradle	167	high	opensearch-security plugin dependency changed from conditional (securityEnabled guard removed) to unconditional. It is now always resolved and installed. This expands the dependency surface and changes the deployment contract for non-security clusters.
alerting/build.gradle	57	high	extendedPlugins now includes 'opensearch-security;optional=true' as a new plugin dependency declaration. Per mandatory rule, any build plugin or plugin-extension change must be flagged for maintainer verification.
build.gradle	25	high	common_utils_version default changed from pinned '3.7.0.0-SNAPSHOT' to the dynamic 'opensearch_build' variable. The resolved artifact will now vary with build context, which can silently pull a different artifact version than previously pinned.
alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportAcknowledgeAlertAction.kt	96	medium	validateUserBackendRoles and checkUserPermissionsWithResource are both short-circuited when shouldUseResourceAuthz returns true. This pattern repeats across ~10 transport actions. If ResourceSharingClientAccessor.setResourceSharingClient can be called by untrusted code via the SPI extension mechanism, an injected client whose isFeatureEnabledForType always returns true would bypass all legacy authorization checks across the plugin.
alerting/src/main/kotlin/org/opensearch/alerting/util/PluginClient.kt	24	medium	PluginClient explicitly executes transport actions as the plugin system subject to bypass user-level DLS. The subject is assigned via assignSubject called by the security plugin, but the client instance is stored in a nullable field and used across all resource-sharing search paths. A null subject causes an unguarded error() throw rather than a graceful denial, and the DLS bypass is broad — any caller that routes through PluginClient reads resources regardless of the caller's own permissions.
core/src/main/resources/mappings/scheduled-jobs.json	7	low	New 'all_shared_principals' keyword field added to the scheduled-jobs index mapping. This field stores all principals with access to each resource and will be indexed and searchable. If the security plugin DLS implementation relies on this field being accurate, any process that can write to the scheduled-jobs index directly could manipulate access control by overwriting this field.

The table above displays the top 10 most important findings.

Total: 7 | Critical: 0 | High: 4 | Medium: 2 | 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.

[DarshitChanpura]

Thanks @riysaxen-amzn — addressed in ad0fbb9:

• 1 (alert-search size): real bug — getAccessibleAlertIDs (and the legacy getFilteredAlertIDs, which had the same latent cap) now set .size(MAX_SEARCH_SIZE).

• 2 (super-admin): super-admin now sees everything — moved the user == null check ahead of the resource-sharing filter branch in get-alerts, get-workflow-alerts, comment-search, and get-destinations.

• 3 (rbac_roles): now validated even under RSC, so a non-admin can't persist roles they don't hold while the (dynamic) flag is on and have them gate access if RSC is later disabled.

• 5 (max_terms_count): documented the bound at the monitor_id/workflow_id term filters.

• 6 (destinations): super-admin now runs a direct (non-DLS) search so it isn't filtered.

• Nits: the sharing-index name is derived from the config-index constant instead of hardcoded; the comments-history bootstrap now runs on the plugin subject (fixes the non-admin hang); the SdkUtils.await unwrap is called out in the PR description.

• 4 (async postIndex race): I don't think this is a practical concern. The sharing entry is written with RefreshPolicy.IMMEDIATE and lands within milliseconds; the tests only hit the race because they fire the read programmatically microseconds after create (hence waitForResourceSharingEntry). No real client — including the Dashboards create→detail flow, which is a human/network round-trip — issues a sub-millisecond create-then-read, so it won't surface in practice. Leaving as-is.

The subordinate-resource alert/comment access tests stay @Ignored pending the child-resource (parent-linked) sharing model in opensearch-project/security#6373; the FIXMEs now reference it.

Verified SecureResourceSharingMonitorRestApiIT under the resource-sharing variant: 31 tests, 3 skipped, 0 failures.

[riysaxen-amzn]

Thanks @DarshitChanpura — verified ad0fbb9 against each item and everything checks out:

• Migrate Alerting to OpenSearch #1 .size(MAX_SEARCH_SIZE) on both getAccessibleAlertIDs and the legacy getFilteredAlertIDs — nice catch extending it to the pre-existing latent cap.
• Change Kibana UserAgent to OpenSearchDashboards #2 user == null branch ordering confirmed in all four actions.
• Change Kibana UserAgent to OpenSearchDashboards #3 rbac_roles validation restored under RSC with the toggle-off rationale documented inline.
• Update licenses #5/FIx Test Workflow #6/nits — all confirmed in the diff.

On #4: agreed, accepting your reasoning. RefreshPolicy.IMMEDIATE plus a human/network round-trip means the create→read gap is orders of magnitude larger than the sharing-entry write; only the tests' programmatic back-to-back calls can race it, and waitForResourceSharingEntry covers those. No change needed.

Good to see the @Ignored subordinate-resource tests now tracked against opensearch-project/security#6373.

The remaining Code-Diff-Analyzer High findings look like inherent-to-onboarding dependency flags (security-spi compile dep, unconditional security zip, common_utils tracking opensearch_build) rather than defects — presumably a maintainer skip-diff-analyzer review. With that and the common-utils#980 CI dependency resolved, no further concerns from my side.

[riysaxen-amzn]

1. RSC-variant security job (the real failure): 24 of 32 tests failed in SecureResourceSharingMonitorRestApiIT + RscMigrateE2ERestApiIT — the suites this PR introduced. Two distinct signatures:

Dominant (~most failures): ParsingException: expecting token of type [END_OBJECT] but found [FIELD_NAME] → 500 on monitor GET/PUT. This is the security plugin's injected top-level field (all_shared_principals — the PR itself added it to the scheduled-jobs mapping) appearing in the doc _source, and ScheduledJob.parse choking on the unexpected extra field. The PR hardened JobSweeper.isSweepableJobType for exactly this, but not the transport GET/update parse path — that's the gap.
Secondary: RscMigrateE2ERestApiIT fails earlier — "After enabling RSC without migration, legacy monitor GET must fail" but the GET succeeded, suggesting the security snapshot's enforcement behavior changed. Plus cascading 404s in delete tests.

[DarshitChanpura]

Root-caused both signatures to the same thing: alerting CI resolves common-utils from the published 3.8 snapshot, and two commits the RSC path needs were only on main. Both are backported and merged; snapshot build 16 has them, and the security workflow is green on both legs (RSC-on and RSC-off).

• Parse 500 on GET/PUT — ScheduledJob.parse didn't tolerate the injected top-level all_shared_principals on the transport path (only JobSweeper did). Order-independent parse backported to 3.8: [Backport 3.8] Order-independent ScheduledJob.parse tolerating ancillary top-level fields (#984) common-utils#998
• RscMigrateE2E un-migrated GET returning 200, and the cascading 404s — the DocRequest.type() overrides were also main-only. Without them type() defaults to "indices", so ResourceAccessEvaluator.shouldEvaluate skips every alerting request and the ActionFilter doesn't gate it. Backported separately (the parse backport's squash dropped this commit): [Backport 3.8] Override DocRequest.type() on alerting request classes (#981) common-utils#1000. WORKFLOW_RESOURCE_TYPE is "alerting-workflow" on 3.8 to match the ResourceProvider; main still has "workflow", which I'll fix there.

Verified locally against build 16 under -Dresource_sharing.enabled=true: RscMigrateE2ERestApiIT and SecureResourceSharingMonitorRestApiIT pass (31/0/0).

Subordinate resources: with opensearch-project/security#6373 merged, I enabled the three previously-ignored tests. Two gaps fixed here:

• Alert GET — GetAlertsRequest is typed monitor/id=monitorId, so the evaluator gates it, but TransportGetAlertsAction ran the alert-index search as the caller; a read-only-shared non-owner has no direct perms on the system alerts index, so it 500'd. Now runs on the plugin subject, with the monitor_id filter (accessible monitors) as the boundary.
• Comment create — the request targets the comments index, not the monitor, so the evaluator doesn't gate it. Now gated via ResourceSharingClient.verifyAccess(monitorId, "monitor", <comment-write-action>) (read-write/full-access only). Comment delete is already author/admin-restricted.

SdkUtils.await unwrapping noted in the PR description.