[analytics engine] Execute analytics engine plan as a local transport action to provide ActionFilter authorization

[finnegancarroll]

Description

The analytics engine's query execution path (PPL, SQL) bypasses the security plugin's index-level privilege evaluation. The FragmentExecutionRequest on the data node is executed with dispatchFragmentStreaming and never evaluated for index permissions by the security plugin SecurityFilter (wraps the ActionFilter).

SQL / PPL queries with analytics engine

In a typical SQL / PPL query authorization is handled at the node-to-node transport layer on the coordinator as the request is being dispatched to shards. The security plugin provides an ActionFilter which every TransportAction child holds a reference to, and executes on new requests.

The overall flow for a regular DSL search query is then:

NodeClient.execute(SearchAction.INSTANCE, searchRequest)
    → NodeClient.executeLocally(action, request, listener)
      → transportAction(action).execute(request, listener)
        → TransportAction.execute(task, request, listener)
          → RequestFilterChain.proceed()

The path for an analytics engine SQL / PPL query avoids the RequestFilterChain entirely. The ShardTaskRunner is responsible for dispatching a FragmentExecutionRequest on the transport layer.

    @Override
    public void run(ShardStageTask task, ActionListener<Void> listener) {
        ShardExecutionTarget target = (ShardExecutionTarget) task.target();
        FragmentExecutionRequest request = requestBuilder.apply(target);
        PendingExecutions pending = pendingFor(target);
        transport.dispatchFragmentStreaming(request, target.node(), stage.responseListenerFor(listener), config.parentTask(), pending);
    }

The FragmentExecutionRequest fetches a new connection from the transport service, and directly executes the request itself. Skipping the TransportAction framework which typically holds and applies the ActionFilter.

        pending.tryRun(() -> {
            try {
                Transport.Connection connection = getConnection(null, targetNode.getId());
                transportService.sendChildRequest(connection, FragmentExecutionAction.NAME, request, parentTask, options, handler);
            } catch (Exception e) {
                try {
                    listener.onFailure(e);
                } finally {
                    pending.finishAndRunNext();
                }
            }
        });

DSL queries with analytics engine

DSL permissions are actually evaluated correctly already. This comes from how DSL is routed into the analytics plugin. DSL queries construct a regular SearchRequest which is handled by the TransportSearchAction and are executed on the transport layer with the local node client as usual.

Handling of DSL queries is done by a new SearchActionFilter which is also injected into the RequestFilterChain and redirects these SearchRequests to the DslExecuteAction.INSTANCE handler instead of executing them as normal search requests. Since this SearchActionFilter executes AFTER the security plugin ActionFilter (which has highest priority), privileges are evaluated correctly.

Path	Unauthorized Index	Status
DSL (POST /{index}/_search)	✅ Denied	Secure
PPL (POST /_plugins/_ppl)	❌ Allowed	Gap
SQL (POST /_plugins/_sql)	❌ Allowed	Gap

Proposed fix

These changes somewhat mimic the functionality of DSL queries for SQL and PPL by adding a pre-planning authorization check which passes a no-op AnalyticsAuthRequest through the ActionFilter to determine if permissions are valid for the equivalent set of FragmentExecutionRequests.

No actual request is executed on the transport layer, the AnalyticsAuthRequest is passed through the full ActionFilter chain to invoke security plugin privilege evaluation, and proceeds with the analytics engine planning + query only if this probing AnalyticsAuthRequest is authorized for the same indices.

Testing

These changes route all analytics engine query execution through the existing AnalyticsQueryAction and TransportAction via NodeClient.execute() during DefaultPlanExecutor.execute(). This triggers the ActionFilter chain (including SecurityFilter) before even planning begins.

Test implementation is pending. Currently changes are validated with local single node testing.

Related Issues

N/A

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

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 🔍

(Review updated until commit ae0e73b)

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 AssertionError handling Catching AssertionError and converting it to IllegalStateException can mask programming errors that should fail fast. AssertionErrors typically indicate bugs in the code (e.g., violated invariants) and should not be caught in production code. If the analytics engine throws AssertionErrors for user-facing validation failures, those should be proper exceptions instead. } catch (AssertionError e) { convertingListener.onFailure( new IllegalStateException("Analytics-engine executor rejected the plan: " + e.getMessage(), e) ); } Silent depth limit The depth guard returns false when MAX_EXTRACT_INDICES_DEPTH is exceeded, but the caller only checks this boolean after the entire traversal completes. If a deeply nested plan has multiple branches, some branches may be fully traversed (collecting indices) before the depth limit is hit in another branch, leading to incomplete index extraction without an error. The exception is only thrown if the root call returns false, but partial results may have been collected. private static boolean collectIndices(RelNode node, java.util.Set<String> indices, int depth) { if (depth >= MAX_EXTRACT_INDICES_DEPTH) { return false; } if (node instanceof org.apache.calcite.rel.core.TableScan scan) { java.util.List<String> names = scan.getTable().getQualifiedName(); indices.add(names.get(names.size() - 1)); } for (RelNode input : node.getInputs()) { if (!collectIndices(input, indices, depth + 1)) { return false; } } return true; }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to ae0e73b

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Add null check for node parameter Add a null check for node at the beginning of the method to prevent potential NullPointerException when traversing the RelNode tree. This guards against malformed plans or unexpected null inputs during recursion. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/RelNodeUtils.java [173-187] private static boolean collectIndices(RelNode node, java.util.Set<String> indices, int depth) { + if (node == null) { + return true; + } if (depth >= MAX_EXTRACT_INDICES_DEPTH) { return false; } if (node instanceof org.apache.calcite.rel.core.TableScan scan) { java.util.List<String> names = scan.getTable().getQualifiedName(); indices.add(names.get(names.size() - 1)); } for (RelNode input : node.getInputs()) { if (!collectIndices(input, indices, depth + 1)) { return false; } } return true; } Suggestion importance[1-10]: 5 __ Why: Adding a null check for node is a defensive programming practice that prevents potential NullPointerException. However, the method is private and called from extractIndices which doesn't pass null, making this a moderate improvement for robustness rather than fixing an actual bug.	Low
	Remove unnecessary superclass constructor call Remove the super(in) call before throwing the exception. Calling the superclass constructor is unnecessary when immediately throwing, and may trigger unwanted side effects or state initialization in the parent class. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/action/AnalyticsQueryRequest.java [42-45] public AnalyticsQueryRequest(StreamInput in) throws IOException { - super(in); throw new UnsupportedOperationException("AnalyticsQueryRequest is local-dispatch only"); } Suggestion importance[1-10]: 3 __ Why: While removing super(in) before throwing is technically cleaner, the impact is minimal since the exception is thrown immediately. The superclass constructor call doesn't cause significant harm in this context.	Low

---

Previous suggestions

Suggestions up to commit f653ac9

Category	Suggestion	Impact
General	Handle index extraction exceptions consistently The extractIndices call can throw IllegalStateException if the plan depth exceeds the limit, but this exception is not caught. This would bypass the exception conversion logic in doExecute that wraps backend exceptions. Wrap the index extraction in a try-catch to ensure consistent error handling. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [124-130] -String[] indices = RelNodeUtils.extractIndices(logicalFragment); -AnalyticsQueryRequest request = new AnalyticsQueryRequest(logicalFragment, context, indices); -client.execute( - AnalyticsQueryAction.INSTANCE, - request, - ActionListener.wrap(resp -> listener.onResponse(resp.getRows()), listener::onFailure) -); +try { + String[] indices = RelNodeUtils.extractIndices(logicalFragment); + AnalyticsQueryRequest request = new AnalyticsQueryRequest(logicalFragment, context, indices); + client.execute( + AnalyticsQueryAction.INSTANCE, + request, + ActionListener.wrap(resp -> listener.onResponse(resp.getRows()), listener::onFailure) + ); +} catch (Exception e) { + listener.onFailure(engineContext.convertException(e)); +} Suggestion importance[1-10]: 8 __ Why: The extractIndices method can throw IllegalStateException for excessively deep plans, but this exception is not caught in the execute method. Wrapping the call in try-catch ensures consistent error handling through engineContext.convertException(), matching the pattern used in doExecute. This addresses a real gap in exception handling.	Medium
Possible issue	Add null check for plan parameter The extractIndices method should validate that the plan parameter is not null before processing. Without this check, a null plan will cause a NullPointerException in collectIndices when attempting to call node.getInputs(). sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/RelNodeUtils.java [167-173] public static String[] extractIndices(RelNode plan) { + if (plan == null) { + throw new IllegalArgumentException("Query plan cannot be null"); + } Set<String> indices = new LinkedHashSet<>(); if (!collectIndices(plan, indices, 0)) { throw new IllegalStateException("Query plan exceeds maximum depth (" + MAX_EXTRACT_INDICES_DEPTH + ") for index extraction"); } return indices.toArray(String[]::new); } Suggestion importance[1-10]: 7 __ Why: Adding a null check for the plan parameter prevents potential NullPointerException when collectIndices is called. This is a valid defensive programming practice that improves robustness, though the calling code in DefaultPlanExecutor.execute() already passes a non-null logicalFragment.	Medium
	Validate required constructor parameters The constructor should validate that critical parameters (plan and indices) are not null, as these are required for security authorization and query execution. Null values would cause failures downstream in doExecute and security filter evaluation. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/action/AnalyticsQueryRequest.java [36-40] public AnalyticsQueryRequest(RelNode plan, Object context, String[] indices) { + if (plan == null) { + throw new IllegalArgumentException("Query plan cannot be null"); + } + if (indices == null) { + throw new IllegalArgumentException("Indices array cannot be null"); + } this.plan = plan; this.context = context; this.indices = indices; } Suggestion importance[1-10]: 7 __ Why: Validating that plan and indices are not null in the constructor prevents downstream failures and provides clearer error messages. This is good defensive programming, though the calling code in DefaultPlanExecutor.execute() already ensures these parameters are non-null before constructing the request.	Medium

Suggestions up to commit a31a6b0

Category	Suggestion	Impact
Possible issue	Guard against empty qualified name list The code assumes getQualifiedName() returns a non-empty list and directly accesses the last element. If the list is empty (which could occur with malformed table metadata), this will throw an IndexOutOfBoundsException. Add a guard to verify the list is not empty. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/RelNodeUtils.java [179-182] if (node instanceof TableScan scan) { List<String> names = scan.getTable().getQualifiedName(); - indices.add(names.get(names.size() - 1)); + if (!names.isEmpty()) { + indices.add(names.get(names.size() - 1)); + } } Suggestion importance[1-10]: 8 __ Why: This is a valid defensive check that prevents IndexOutOfBoundsException if getQualifiedName() returns an empty list. While this scenario may be rare with well-formed Calcite table metadata, adding this guard improves robustness against malformed or edge-case table configurations.	Medium
	Add null check for plan parameter The extractIndices method should validate that the plan parameter is not null before attempting to traverse it. Without this check, a null plan will cause a NullPointerException in collectIndices, making error diagnosis harder. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/RelNodeUtils.java [167-173] public static String[] extractIndices(RelNode plan) { + if (plan == null) { + throw new IllegalArgumentException("Query plan cannot be null"); + } Set<String> indices = new LinkedHashSet<>(); if (!collectIndices(plan, indices, 0)) { throw new IllegalStateException("Query plan exceeds maximum depth (" + MAX_EXTRACT_INDICES_DEPTH + ") for index extraction"); } return indices.toArray(String[]::new); } Suggestion importance[1-10]: 7 __ Why: Adding a null check for the plan parameter improves error handling by providing a clearer error message instead of allowing a NullPointerException to propagate from collectIndices. This is a defensive programming practice that enhances code robustness.	Medium
General	Handle index extraction exceptions consistently If extractIndices throws an IllegalStateException for excessively deep plans, the exception will propagate to the caller without being wrapped in the engine-specific exception handling. Consider catching and converting this exception using engineContext.convertException for consistency. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [124-125] -String[] indices = RelNodeUtils.extractIndices(logicalFragment); +String[] indices; +try { + indices = RelNodeUtils.extractIndices(logicalFragment); +} catch (IllegalStateException e) { + listener.onFailure(engineContext.convertException(e)); + return; +} AnalyticsQueryRequest request = new AnalyticsQueryRequest(logicalFragment, context, indices); Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that extractIndices can throw IllegalStateException for deep plans, which should be handled consistently with other exceptions using engineContext.convertException. However, the exception is already caught by the outer try-catch block at line 155, so this is more of a style improvement than a critical fix.	Low

Suggestions up to commit d2296be

Category	Suggestion	Impact
Security	Validate extracted indices exist Validate that extractIndices() returns a non-empty array before creating the request. If no indices are extracted from the plan, the security filter may not properly evaluate permissions, potentially allowing unauthorized access or causing unexpected behavior. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [125-132] String[] indices = extractIndices(logicalFragment); +if (indices == null || indices.length == 0) { + listener.onFailure(new IllegalArgumentException("Query plan must reference at least one index")); + return; +} AnalyticsQueryRequest request = new AnalyticsQueryRequest(logicalFragment, context, indices); client.execute( AnalyticsQueryAction.INSTANCE, request, ActionListener.wrap(resp -> listener.onResponse(resp.getRows()), listener::onFailure) ); Suggestion importance[1-10]: 8 __ Why: This is a security-relevant suggestion. If extractIndices() returns an empty array, the SecurityFilter may not properly evaluate index-level permissions, potentially allowing unauthorized access. Validating that at least one index is present before dispatching the request is important for security.	Medium
Possible issue	Add null safety checks Add null safety check for scan.getTable() and getQualifiedName() before accessing the list. If either returns null or the list is empty, the names.get(names.size() - 1) call will throw an exception. This could occur with malformed or synthetic RelNode trees. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [276-287] private static void collectIndices(RelNode node, java.util.Set<String> indices, int depth) { if (depth >= MAX_COLLECT_INDICES_DEPTH) { return; } if (node instanceof org.apache.calcite.rel.core.TableScan scan) { - List<String> names = scan.getTable().getQualifiedName(); - indices.add(names.get(names.size() - 1)); + List<String> names = scan.getTable() != null ? scan.getTable().getQualifiedName() : null; + if (names != null && !names.isEmpty()) { + indices.add(names.get(names.size() - 1)); + } } for (RelNode input : node.getInputs()) { collectIndices(input, indices, depth + 1); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullPointerException or IndexOutOfBoundsException when accessing scan.getTable().getQualifiedName() and names.get(names.size() - 1). Adding null and empty checks improves robustness against malformed RelNode trees.	Medium

Suggestions up to commit 31474ea

Category	Suggestion	Impact
Possible issue	Prevent index out of bounds error Add bounds checking before accessing the qualified name list to prevent IndexOutOfBoundsException when the list is empty. The current code assumes names is non-empty, but getQualifiedName() could return an empty list in edge cases. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [276-287] private static void collectIndices(RelNode node, java.util.Set<String> indices, int depth) { if (depth >= MAX_COLLECT_INDICES_DEPTH) { return; } if (node instanceof org.apache.calcite.rel.core.TableScan scan) { List<String> names = scan.getTable().getQualifiedName(); - indices.add(names.get(names.size() - 1)); + if (!names.isEmpty()) { + indices.add(names.get(names.size() - 1)); + } } for (RelNode input : node.getInputs()) { collectIndices(input, indices, depth + 1); } } Suggestion importance[1-10]: 7 __ Why: Valid defensive programming suggestion. While getQualifiedName() typically returns non-empty lists in Calcite, adding the isEmpty() check prevents potential IndexOutOfBoundsException in edge cases and improves code robustness.	Medium
Security	Validate non-empty indices array Validate that extractIndices() returns a non-empty array before creating the request. An empty indices array could bypass security checks or cause unexpected behavior in the authorization filter chain. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [125-132] String[] indices = extractIndices(logicalFragment); +if (indices.length == 0) { + listener.onFailure(new IllegalArgumentException("Query must reference at least one index")); + return; +} AnalyticsQueryRequest request = new AnalyticsQueryRequest(logicalFragment, context, indices); client.execute( AnalyticsQueryAction.INSTANCE, request, ActionListener.wrap(resp -> listener.onResponse(resp.getRows()), listener::onFailure) ); Suggestion importance[1-10]: 6 __ Why: Reasonable validation suggestion that could prevent queries without table references from bypassing security checks. However, the score is moderate because the security filter may already handle empty indices arrays appropriately, and the actual security impact depends on downstream filter behavior.	Low

Suggestions up to commit 106fe3a

Category	Suggestion	Impact
Possible issue	Prevent IndexOutOfBoundsException on empty list The method accesses names.get(names.size() - 1) without checking if the list is empty, which will throw an IndexOutOfBoundsException if getQualifiedName() returns an empty list. Add a null/empty check before accessing the list element to prevent runtime failures during index extraction. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [273-284] private static void collectIndices(RelNode node, java.util.Set<String> indices, int depth) { if (depth >= MAX_COLLECT_INDICES_DEPTH) { return; } if (node instanceof org.apache.calcite.rel.core.TableScan scan) { List<String> names = scan.getTable().getQualifiedName(); - indices.add(names.get(names.size() - 1)); + if (names != null && !names.isEmpty()) { + indices.add(names.get(names.size() - 1)); + } } for (RelNode input : node.getInputs()) { collectIndices(input, indices, depth + 1); } } Suggestion importance[1-10]: 7 __ Why: Valid defensive programming suggestion. While getQualifiedName() typically returns a non-empty list for valid TableScan nodes, adding a null/empty check prevents potential IndexOutOfBoundsException at runtime and makes the code more robust against edge cases.	Medium
General	Move index extraction off calling thread The extractIndices() call happens on the calling thread before dispatching to the action filter chain. For complex query plans, this recursive traversal could block transport threads. Move the index extraction inside doExecute() where it runs on the SEARCH executor, consistent with the existing pattern of offloading planning work. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [120-132] @Override public void execute(RelNode logicalFragment, Object context, ActionListener<Iterable<Object[]>> listener) { - String[] indices = extractIndices(logicalFragment); - AnalyticsQueryRequest request = new AnalyticsQueryRequest(logicalFragment, context, indices); + AnalyticsQueryRequest request = new AnalyticsQueryRequest(logicalFragment, context, new String[0]); client.execute( AnalyticsQueryAction.INSTANCE, request, ActionListener.wrap(resp -> listener.onResponse(resp.getRows()), listener::onFailure) ); } Suggestion importance[1-10]: 3 __ Why: The suggestion misunderstands the security architecture. Index extraction must happen before client.execute() so the SecurityFilter can evaluate permissions on the target indices. Moving it to doExecute() would bypass security checks, defeating the PR's core purpose. The suggested code also passes empty indices array, which would break authorization.	Low

[github-actions]

PR Code Analyzer ❗

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

Path	Line	Severity	Description
sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/action/AnalyticsQueryRequest.java	74	medium	validate() unconditionally returns null, performing no validation on the request. While this is an internal request constructed from a RelNode plan, skipping validation entirely means any caller can pass null indices or a null plan without rejection. If the construction path is ever widened to accept external input, this becomes a direct injection vector.
sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/action/AnalyticsQueryRequest.java	62	low	indices() returns the internal String[] array by direct reference rather than a defensive copy. Because the class implements IndicesRequest.Replaceable, the security plugin's IndexResolverReplacer may call indices(String...) to replace the array, but external code holding the old reference could still observe or mutate stale state. Low risk in current local-dispatch-only usage but worth noting.

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]

Persistent review updated to latest commit 106fe3a

[github-actions]

✅ Gradle check result for 106fe3a: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.50%. Comparing base (9ffd031) to head (f653ac9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21789      +/-   ##
============================================
+ Coverage     73.46%   73.50%   +0.03%     
- Complexity    75205    75219      +14     
============================================
  Files          6023     6023              
  Lines        341475   341475              
  Branches      49141    49141              
============================================
+ Hits         250878   251006     +128     
+ Misses        70627    70526     -101     
+ Partials      19970    19943      -27     

☔ View full report in Codecov by Sentry.
📢 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.

[github-actions]

Persistent review updated to latest commit 31474ea

[github-actions]

Persistent review updated to latest commit d2296be

[github-actions]

✅ Gradle check result for d2296be: SUCCESS

[github-actions]

Persistent review updated to latest commit a31a6b0

[github-actions]

Persistent review updated to latest commit f653ac9

[github-actions]

✅ Gradle check result for f653ac9: SUCCESS

[finnegancarroll]

Associated initial IT suite in SQL plugin:
opensearch-project/sql#5462

Currently still running these locally and working on having them run correctly in CI with all analytics plugin + security dependencies.

[cwperks]

This PR LGTM! Looks like there's now a merge conflict that needs to be resolved. TY for fixing this @finnegancarroll !

[github-actions]

Persistent review updated to latest commit ae0e73b