Skip to content

Fix multi index resolution on analytics engine - #22314

Merged
mch2 merged 1 commit into
opensearch-project:mainfrom
finnegancarroll:fix-fgac-multi-index-bypass
Jun 25, 2026
Merged

Fix multi index resolution on analytics engine#22314
mch2 merged 1 commit into
opensearch-project:mainfrom
finnegancarroll:fix-fgac-multi-index-bypass

Conversation

@finnegancarroll

@finnegancarroll finnegancarroll commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Description

RelNodeUtils.extractIndices() returned comma-delimited table names as a single array element (e.g. ["idx1,idx2"]). The security filter's IndexNameExpressionResolver tried to resolve this as one index name, was unable to, and resolved no concrete indices.

Due to the default setting of strict expand open on analytics query request index resolution errors are not fatal.

Fix

Split comma-delimited table names in collectIndices() using Strings.splitStringByCommaToArray() (same utility used by IndexResolution in the planner). Each index is now a separate entry in the String[] array, so security evaluates permissions on each independently.

PPL multi-source queries (source=idx1,idx2) produce a single TableScan
with a comma-delimited table name. RelNodeUtils.extractIndices() was
returning this as one array element (e.g. ["idx1,idx2"]) causing the
security filter to see a single non-existent index name, resolve to
zero concrete indices, and skip permission evaluation entirely.

Fix: split comma-delimited table names using
Strings.splitStringByCommaToArray() — the same utility used by
IndexResolution in the planner — so each index is extracted as a
separate entry. The security filter then evaluates permissions on
each index independently and correctly denies unauthorized access.

Reproduction: with cluster.pluggable.dataformat=composite, a user with
logs-* permissions could query source=logs-2024-01,secrets-2024-01 and
bypass FGAC because the joined string passed through security unchecked.

Signed-off-by: Finnegan Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
@github-actions

Copy link
Copy Markdown
Contributor

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
⚡ No major issues detected

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Trim whitespace from split index tokens

Strings.splitStringByCommaToArray does not trim whitespace, so a table name like "a,
b" would yield " b" and bypass intended index matching in the security filter. Trim
each token before adding (and re-check for empty after trim) to ensure each index is
evaluated correctly.

sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/RelNodeUtils.java [234-242]

 String tableName = names.get(names.size() - 1);
 // PPL multi-source queries (source=a,b) produce a single TableScan with a
 // comma-delimited table name. Split so each index is evaluated independently
 // by the security filter — same logic as IndexResolution.
 for (String idx : Strings.splitStringByCommaToArray(tableName)) {
-    if (!idx.isEmpty()) {
-        indices.add(idx);
+    String trimmed = idx.trim();
+    if (!trimmed.isEmpty()) {
+        indices.add(trimmed);
     }
 }
Suggestion importance[1-10]: 4

__

Why: Trimming whitespace is a reasonable defensive measure, but PPL-generated table names from source=a,b syntax are unlikely to contain spaces, and the comment in the test (splitStringByCommaToArray trims and skips empty tokens) suggests the author believed trimming already happens. Minor robustness improvement.

Low

@finnegancarroll finnegancarroll changed the title Fix FGAC bypass on multi-index PPL queries Fix multi index resolution on analytics engine Jun 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

✅ Gradle check result for 0982986: SUCCESS

@codecov

codecov Bot commented Jun 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.32%. Comparing base (50accb8) to head (0982986).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main   #22314      +/-   ##
============================================
- Coverage     73.35%   73.32%   -0.03%     
+ Complexity    76086    76000      -86     
============================================
  Files          6075     6075              
  Lines        345371   345371              
  Branches      49718    49718              
============================================
- Hits         253348   253253      -95     
- Misses        71838    71850      +12     
- Partials      20185    20268      +83     

☔ 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.

@finnegancarroll
finnegancarroll marked this pull request as ready for review June 25, 2026 02:51
@finnegancarroll
finnegancarroll requested a review from a team as a code owner June 25, 2026 02:51
}

public void testDoubleCommaProducesEmptyStringFiltered() {
RelBuilder b = builderWithTable("index1,,index2");

@expani expani Jun 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, Vanilla allows incorrectly parsed names ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some of these might be rejected up front by sql/ppl before making it into AE. I'm unsure. Adding SQL plugin integ tests for this PR here opensearch-project/sql#5581 and will cover these cases.

@mch2
mch2 merged commit ac8d27d into opensearch-project:main Jun 25, 2026
20 checks passed
KhishorekumarBS pushed a commit to KhishorekumarBS/OpenSearch that referenced this pull request Jul 3, 2026
PPL multi-source queries (source=idx1,idx2) produce a single TableScan
with a comma-delimited table name. RelNodeUtils.extractIndices() was
returning this as one array element (e.g. ["idx1,idx2"]) causing the
security filter to see a single non-existent index name, resolve to
zero concrete indices, and skip permission evaluation entirely.

Fix: split comma-delimited table names using
Strings.splitStringByCommaToArray() — the same utility used by
IndexResolution in the planner — so each index is extracted as a
separate entry. The security filter then evaluates permissions on
each index independently and correctly denies unauthorized access.

Reproduction: with cluster.pluggable.dataformat=composite, a user with
logs-* permissions could query source=logs-2024-01,secrets-2024-01 and
bypass FGAC because the joined string passed through security unchecked.

Signed-off-by: Finnegan Carroll <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants