Reach the report indices through the plugin subject

[DarshitChanpura]

Description

Replaces ThreadContext.stashContext with the plugin subject assigned through IdentityAwarePlugin, so reporting reaches its own system indices as its plugin subject rather than from an unrestricted stashed context. IdentityAwarePlugin and util.PluginClient are already on main; this finishes the job by moving the remaining call sites onto them.

30 of the 32 stash calls being removed were inside SecureIndexClient, a Client-by-delegation wrapper that stashed in every override, wired at one site each in ReportDefinitionsIndex and ReportInstancesIndex. That class is deleted and both index classes now hold the PluginClient. The other two were the explicit stash around index creation in each of those classes.

Both index classes touch exactly one index each, .opendistro-reports-definitions and .opendistro-reports-instances, and both are already registered through getSystemIndexDescriptors.

Because the plugin client is now the client the index classes hold, the per-request client choice in getAllReportDefinitions and getAllReportInstances resolves to the same client on both sides of the resource-sharing branch, so the parameter threaded in from the two list actions is gone. Which access list the search carries is still decided by the resource-sharing check, unchanged.

Two behaviour changes worth review:

• SecureIndexClient delegated admin() to the raw client, so admin().indices().create() ran as the caller inside a hand written stash. PluginClient extends FilterClient, whose admin() routes back through doExecute, so index creation now runs as the plugin subject as well.
• PluginClient.doExecute had no catch, so a failure raised while switching to the subject left doExecute as a throw and the listener was never completed, leaving a listener only caller waiting. It is now reported through listener.onFailure. The same commit lowers the line naming the subject's principal from info to debug, so an identity is not written on every transport action at a level that is on by default, and marks the subject field volatile.

The stash in PluginBaseAction is deliberately kept. It is not system index access: the action captures the caller's context on the transport thread, then on the coroutine's pooled thread it stashes whatever the previous task left there, restores the caller's context for the length of the request, and lets the stash put the pooled thread back as it was found. Removing it would both run the request with a stale context and leak the caller's context to the next task on that thread. A comment now records why it is there.

Also adds plugin-additional-permissions.yml with an empty cluster_permissions list. For index actions a plugin subject is already granted everything on its own registered system indices, and the migrated call sites issue only indices:admin/create, get, index, update, delete and search. None of the operations that security classifies as cluster permissions despite acting on indices (mget, msearch, scroll, multi term vectors, reindex, template admin) are used anywhere in the plugin, so there is nothing to grant. The key is present rather than the file omitted because security appends bulk to the list it parses.

Testing

compileKotlin, compileTestKotlin, test, ktlint and detekt pass on JDK 21. 109 unit tests, no skips and no failures, against a baseline of 105 on main; the four added tests are the whole delta.

Both of the added tests that guard a behaviour were confirmed to fail without it: the synchronous failure test fails against the current doExecute with the exception escaping, and the context restore test fails if the ActionListener.runBefore wiring is dropped. The fake subject in those tests stashes and restores around its body, the way both the security plugin's subject and core's noop subject do, so the tests do not pass for the wrong reason.

Not verified by execution: the permissions file and the behaviour of the migrated call sites against a cluster with security installed. Unit tests do not go through the security plugin, so the reasoning about which actions need granting comes from reading security's privileges evaluator, not from a run. This repository does have a with security workflow, including a resource sharing enabled matrix leg, so CI will exercise it here; it was not run locally.

Related

Tracking issue: opensearch-project/opensearch-plugins#238
RFC: opensearch-project/security#4439

Check List

• New functionality includes testing.
• Commits are signed per the DCO using --signoff.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

PR Code Analyzer ❗

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

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

Path	Line	Severity	Description
src/main/kotlin/org/opensearch/reportsscheduler/index/ReportDefinitionsIndex.kt	67	medium	Removal of explicit stashContext() for index admin (create-index) operations. PluginClient extends FilterClient and only overrides doExecute(); calls through client.admin().indices() bypass the runAs subject wrapper entirely. Index creation now runs with whatever security context is on the calling thread rather than a stashed/system context. The same pattern is repeated in ReportInstancesIndex.kt. This may allow index admin operations to execute under an unexpected caller identity or fail under restricted user contexts, depending on where createIndex() is triggered from.
src/main/kotlin/org/opensearch/reportsscheduler/util/PluginClient.kt	51	low	Logging of the acting subject was downgraded from INFO to DEBUG. Subject/principal logging at a higher level is useful as an audit trail for privilege context switches. Silencing it by default reduces observability of which identity is being used for transport actions, though this could also be a legitimate noise-reduction decision.

The table above displays the top 10 most important findings.

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

[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 context leak In doExecute, when currentSubject.runAs returns normally, the storedContext is never closed on the success path — only the catch branch calls storedContext.close(). The listener's runBefore restores the context, but a StoredContext acquired via newStoredContext(false) should still be closed to release its resources. In the previous code this was handled by a finally block. If super.doExecute completes synchronously (or the listener never fires), the context is leaked. val storedContext = threadPool().threadContext.newStoredContext(false) try { currentSubject.runAs<Exception> { LOGGER.debug("Running transport action with subject: {}", currentSubject.principal.name) super.doExecute(action, request, ActionListener.runBefore(listener) { storedContext.restore() }) } } catch (exception: Exception) { // Reported through the listener rather than thrown, so a caller that only waits on the // listener is not left waiting forever. storedContext.close() listener.onFailure(exception) } Behavior change Index creation (client.admin().indices().create(request)) now runs through PluginClient/FilterClient as the plugin subject rather than as the caller via a direct stash. If the plugin subject lacks the cluster-level indices:admin/create permission (not currently listed in plugin-additional-permissions.yml), lazy index creation triggered by read/write operations will fail. Verify the plugin subject has permission to create the registered system indices, or ensure index creation only occurs at plugin startup where the thread already has adequate privileges. try { val actionFuture = client.admin().indices().create(request) val response = actionFuture.actionGet(PluginSettings.operationTimeoutMs) if (response.isAcknowledged) { log.info("$LOG_PREFIX:Index $REPORT_DEFINITIONS_INDEX_NAME creation Acknowledged") } else { Metrics.REPORT_DEFINITION_CREATE_SYSTEM_ERROR.counter.increment() error("$LOG_PREFIX:Index $REPORT_DEFINITIONS_INDEX_NAME creation not Acknowledged") } Access-check semantics change Previously, when shouldUseResourceAuthz was true and a pluginClient was provided, the search ran as the plugin subject (bypassing document-level security on the system index) while the access list was still populated from the user. Now the search always runs as the plugin subject regardless of the resource-authz branch. Confirm this matches the intended authorization model for the non-resource-authz branch, where previously the search ran as the caller and relied on DLS/user permissions on .opendistro-reports-definitions. The same applies to ReportInstanceActions.getAll. fun getAll(request: GetAllReportDefinitionsRequest, user: User?): GetAllReportDefinitionsResponse { log.info("$LOG_PREFIX:ReportDefinition-getAll fromIndex:${request.fromIndex} maxItems:${request.maxItems}") // only use backend_role path if resource-sharing is disabled if (!shouldUseResourceAuthz(Utils.REPORT_DEFINITION_TYPE)) { UserAccessManager.validateUser(user) } // if resource-sharing is enabled, search result will automatically be filtered within security plugin val access = if (shouldUseResourceAuthz(Utils.REPORT_DEFINITION_TYPE)) { emptyList() } else { UserAccessManager.getSearchAccessInfo(user) } val reportDefinitionsList = ReportDefinitionsIndex.getAllReportDefinitions( UserAccessManager.getUserTenant(user), access, request.fromIndex, request.maxItems ) return GetAllReportDefinitionsResponse(reportDefinitionsList, true)

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Confirm admin calls use plugin subject Removing the stashContext wrapper means admin index-creation calls now run with whatever thread context the caller carried. If PluginClient's doExecute is not on the path for client.admin().indices().create(...) (admin clients often bypass filter clients for cluster/admin actions), the request may execute with the user's context instead of the plugin subject's, causing authorization failures. Verify admin calls route through the plugin subject, or re-add stashContext here. src/main/kotlin/org/opensearch/reportsscheduler/index/ReportDefinitionsIndex.kt [70-77] try { - val actionFuture = client.admin().indices().create(request) - val response = actionFuture.actionGet(PluginSettings.operationTimeoutMs) + client.threadPool().threadContext.stashContext().use { + val actionFuture = client.admin().indices().create(request) + val response = actionFuture.actionGet(PluginSettings.operationTimeoutMs) + if (response.isAcknowledged) { + log.info("$LOG_PREFIX:Index $REPORT_DEFINITIONS_INDEX_NAME creation Acknowledged") + } else { + Metrics.REPORT_DEFINITION_CREATE_SYSTEM_ERROR.counter.increment() + error("$LOG_PREFIX:Index $REPORT_DEFINITIONS_INDEX_NAME creation not Acknowledged") + } + } Suggestion importance[1-10]: 6 __ Why: This is a valid concern worth verifying: the PluginClient is a FilterClient and admin calls should route through doExecute, but if they don't, the index creation would run under the caller's context. The suggestion is essentially asking for verification rather than definitively identifying a bug.	Low
Possible issue	Avoid leaking stored thread context The success path never closes storedContext. ActionListener.runBefore restores the context but does not close the StoredContext, so the stored reference leaks on every successful execution. Close it after the subject invocation completes, or rely on runBefore's callback to also close it. src/main/kotlin/org/opensearch/reportsscheduler/util/PluginClient.kt [62-66] } catch (exception: Exception) { // Reported through the listener rather than thrown, so a caller that only waits on the // listener is not left waiting forever. storedContext.close() listener.onFailure(exception) } +// Note: storedContext is closed by the wrappedListener's runBefore callback via restore(); +// ensure restore() releases resources, otherwise close it explicitly here. Suggestion importance[1-10]: 5 __ Why: The concern about StoredContext leakage on the success path is plausible, since ActionListener.runBefore only invokes restore() and does not close the context. However, the suggestion's improved_code is essentially identical to the existing code with only a comment added, making it not actionable.	Low

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.65%. Comparing base (85ea83b) to head (019a6cd).

Files with missing lines	Patch %	Lines
...h/reportsscheduler/index/ReportDefinitionsIndex.kt	62.50%	2 Missing and 1 partial ⚠️
...rch/reportsscheduler/index/ReportInstancesIndex.kt	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1232      +/-   ##
============================================
+ Coverage     73.12%   74.65%   +1.53%     
+ Complexity      406      403       -3     
============================================
  Files            68       67       -1     
  Lines          2221     2166      -55     
  Branches        236      233       -3     
============================================
- Hits           1624     1617       -7     
+ Misses          463      418      -45     
+ Partials        134      131       -3     

Flag	Coverage Δ
reports-scheduler	74.65% <80.00%> (+1.53%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.