Apply Index Monitor API input validation to the Execute Monitor API

[jmsusanto]

Description

Add validation to Execute Monitor API

Behavior change

The Execute Monitor API's pre-flight input check now fails the request on any search failure, not only security exceptions. Previously, an inline dry-run against a nonexistent (or otherwise unqueryable) index returned HTTP 200 with the error captured in input_results; it now returns an error response. API consumers relying on the old dry-run debugging behavior should be aware of this.

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.

[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 Incomplete DataSources Validation validateDataSources only checks queryIndex, findingsIndex, and alertsIndex, but DataSources also contains other fields (e.g., queryIndexMapping, findingsIndexPattern, alertsHistoryIndex, alertsHistoryIndexPattern, findingsEnabled, etc.) that can influence where data is written/read. A caller can still supply attacker-controlled values in those fields and bypass the intended restriction that "Custom Data Sources are not allowed". Consider either validating each field explicitly against defaults, or comparing against DataSources() defaults directly. private fun validateDataSources(monitor: Monitor) { // Data Sources are only supported at the transport layer for stored monitors. if (monitor.dataSources != null) { if ( monitor.dataSources.queryIndex != ScheduledJob.DOC_LEVEL_QUERIES_INDEX || monitor.dataSources.findingsIndex != AlertIndices.FINDING_HISTORY_WRITE_INDEX || monitor.dataSources.alertsIndex != AlertIndices.ALERT_INDEX ) { throw IllegalArgumentException("Custom Data Sources are not allowed.") } } } Alias/Data Stream Resolution Bypass checkIndicesAndExecute rewrites aliases/data streams to their backing write index before issuing the permission-check search. If the caller has permissions on the backing concrete index but not on the alias/data-stream pattern (or vice versa), the test search's result will not match what the actual monitor run performs (the runner uses the originally-configured names against the stashed/system context). This can either falsely allow execution or block legitimate execution. Consider searching against the originally supplied indices to match what the monitor will actually query. val updatedIndices = indices.map { index -> if (IndexUtils.isAlias(index, clusterService.state()) || IndexUtils.isDataStream(index, clusterService.state())) { val metadata = clusterService.state().metadata.indicesLookup[index]?.writeIndex metadata?.index?.name ?: index } else { index } } // Test search executed with the caller's security context (context not yet stashed). val searchRequest = SearchRequest().indices(*updatedIndices.toTypedArray()) .source(SearchSourceBuilder.searchSource().size(1).query(QueryBuilders.matchAllQuery())) Possible Issue In executeInlineMonitor, the scope.launch { ... } block runs the code asynchronously, so a synchronous try/catch around scope.launch(...) cannot catch exceptions thrown inside the coroutine (e.g., from initDocLevelQueryIndex, getOrCreateMetadata, or indexDocLevelQueries). Failures inside the coroutine will be lost and the listener will never be notified, causing the client to hang until timeout. Move the try/catch inside the coroutine body. try { scope.launch(TenantContext(tenantId)) { if (!docLevelMonitorQueries.docLevelQueryIndexExists(monitor.dataSources)) { docLevelMonitorQueries.initDocLevelQueryIndex(monitor.dataSources) log.info("Central Percolation index ${ScheduledJob.DOC_LEVEL_QUERIES_INDEX} created") } val (metadata, _) = MonitorMetadataService.getOrCreateMetadata(monitor, skipIndex = true) docLevelMonitorQueries.indexDocLevelQueries( monitor, monitor.id, metadata, WriteRequest.RefreshPolicy.IMMEDIATE, indexTimeout ) log.info("Queries inserted into Percolate index ${ScheduledJob.DOC_LEVEL_QUERIES_INDEX}") launchExecuteMonitor(monitor, execMonitorRequest, tenantId, actionListener) } } catch (t: Exception) { actionListener.onFailure(AlertingException.wrap(t)) }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Context stash exits before async callback executeExistingMonitor performs an async sdkClient.getDataObjectAsync(...) whose completion callback runs after the stashContext().use { } block exits, restoring the original context before the monitor actually executes. The stashing needs to wrap the async execution path (e.g., stash inside the whenComplete callback or before launchExecuteMonitor), otherwise the monitor runs with the caller's context rather than the plugin's. alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportExecuteMonitorAction.kt [104-111] if (execMonitorRequest.monitorId != null && execMonitorRequest.monitor == null) { val monitorId = execMonitorRequest.monitorId - // Existing monitor referenced by ID. Its inputs and data sources were already validated against - // the caller's permissions at creation time, and an explicit permission check is performed below, - // so it is safe to stash the context here. - client.threadPool().threadContext.stashContext().use { - executeExistingMonitor(execMonitorRequest, monitorId, user, tenantId, actionListener) - } + // Context stashing is performed inside executeExistingMonitor around the async callback, since + // stashContext().use { } would restore the context before the async response is handled. + executeExistingMonitor(execMonitorRequest, monitorId, user, tenantId, actionListener) } else { Suggestion importance[1-10]: 9 __ Why: Correct and significant issue: stashContext().use { } restores context on block exit, but sdkClient.getDataObjectAsync().whenComplete runs asynchronously after, so the monitor may execute with the caller's context instead of the stashed one. This can cause runtime permission/execution problems.	High
Security	Validate all DataSources fields, not three The validation ignores several other DataSources fields (e.g., queryIndexMappingsByType, findingsIndexPattern, alertsHistoryIndex, alertsHistoryIndexPattern, findingsEnabled) that a caller could still set to attacker-controlled or unexpected values, bypassing the intent of "no custom data sources". Consider comparing against a default DataSources() instance (or explicitly validating every field) rather than only three named indices. alerting/src/main/kotlin/org/opensearch/alerting/resthandler/RestExecuteMonitorAction.kt [90-100] private fun validateDataSources(monitor: Monitor) { // Data Sources are only supported at the transport layer for stored monitors. - if (monitor.dataSources != null) { - if ( - monitor.dataSources.queryIndex != ScheduledJob.DOC_LEVEL_QUERIES_INDEX || - monitor.dataSources.findingsIndex != AlertIndices.FINDING_HISTORY_WRITE_INDEX || - monitor.dataSources.alertsIndex != AlertIndices.ALERT_INDEX - ) { - throw IllegalArgumentException("Custom Data Sources are not allowed.") - } + if (monitor.dataSources != null && monitor.dataSources != DataSources()) { + throw IllegalArgumentException("Custom Data Sources are not allowed.") } } Suggestion importance[1-10]: 8 __ Why: Valid security concern: DataSources has additional fields (findings index patterns, alerts history index, etc.) that are not checked, allowing partial bypass of the intent to disallow custom data sources. Comparing to a default instance is a stronger, more comprehensive validation.	Medium
General	Check permissions on original index expressions Resolving alias/datastream inputs to only their write index and issuing the permission check against that concrete index can produce false positives/negatives: a caller may have read permissions on the alias but not the underlying write index (or vice versa), causing legitimate executions to be denied. Prefer issuing the search against the originally configured index expressions so the security plugin evaluates permissions the same way the actual monitor run will. alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportExecuteMonitorAction.kt [188-195] -val updatedIndices = indices.map { index -> - if (IndexUtils.isAlias(index, clusterService.state()) || IndexUtils.isDataStream(index, clusterService.state())) { - val metadata = clusterService.state().metadata.indicesLookup[index]?.writeIndex - metadata?.index?.name ?: index - } else { - index - } -} +val searchRequest = SearchRequest().indices(*indices.toTypedArray()) + .source(SearchSourceBuilder.searchSource().size(1).query(QueryBuilders.matchAllQuery())) Suggestion importance[1-10]: 6 __ Why: Reasonable concern: resolving aliases/data streams to only the write index can cause permission-check mismatches with actual monitor execution which uses the alias. However, the original code's rationale for resolution may be intentional, so impact is moderate.	Low

[riysaxen-amzn]

1. Behavior change for release notes: the pre-flight search fails the request for any search failure, not just security exceptions — an inline dry-run against a nonexistent index now returns an error response instead of a 200 with the error in input_results. Worth a line in the PR description since it changes dry-run debugging behavior for API consumers.

2. Follow-up question: validateDataSources checks queryIndex/findingsIndex/alertsIndex but not findingsIndexPattern/alertsHistoryIndex/alertsHistoryIndexPattern (consistent with the existing create-path check). If that was assessed as acceptable during verification, ignore; otherwise might deserve a tracking issue covering both handlers.

[jmsusanto]

1. Behavior change for release notes: the pre-flight search fails the request for any search failure, not just security exceptions — an inline dry-run against a nonexistent index now returns an error response instead of a 200 with the error in input_results. Worth a line in the PR description since it changes dry-run debugging behavior for API consumers.
2. Follow-up question: validateDataSources checks queryIndex/findingsIndex/alertsIndex but not findingsIndexPattern/alertsHistoryIndex/alertsHistoryIndexPattern (consistent with the existing create-path check). If that was assessed as acceptable during verification, ignore; otherwise might deserve a tracking issue covering both handlers.

1. Good catch, this is intentional. The pre-flight search now surfaces any search failure (not just security
   exceptions), so an inline dry-run against a nonexistent index returns an error response instead of a 200 with the error in input_results. I'veadded a note to the PR description calling this out for consumers.

2. This was intentional to stay consistent with the existing create-path check in RestIndexMonitorAction, which validates the same three indices (queryIndex/findingsIndex/alertsIndex). Extending both handlers to cover findingsIndexPattern/alertsHistoryIndex/alertsHistoryIndexPattern is worth doing separately

[opensearch-ci-bot]

The backport to 3.3 failed. Please backport manually. See failed workflow run: https://github.com/opensearch-project/alerting/actions/runs/34395276496

[opensearch-ci-bot]

The backport to 3.0 failed. Please backport manually. See failed workflow run: https://github.com/opensearch-project/alerting/actions/runs/34395276496

[opensearch-ci-bot]

The backport to 3.8 failed. Please backport manually. See failed workflow run: https://github.com/opensearch-project/alerting/actions/runs/34395276496

[opensearch-ci-bot]

The backport to 3.4 failed. Please backport manually. See failed workflow run: https://github.com/opensearch-project/alerting/actions/runs/34395276496

[opensearch-ci-bot]

The backport to 2.19 failed. Please backport manually. See failed workflow run: https://github.com/opensearch-project/alerting/actions/runs/34395276496

[opensearch-ci-bot]

The backport to 3.5 failed. Please backport manually. See failed workflow run: https://github.com/opensearch-project/alerting/actions/runs/34395276496

[opensearch-ci-bot]

The backport to 3.1 failed. Please backport manually. See failed workflow run: https://github.com/opensearch-project/alerting/actions/runs/34395276496

[opensearch-ci-bot]

The backport to 3.2 failed. Please backport manually. See failed workflow run: https://github.com/opensearch-project/alerting/actions/runs/34395276496