DLS for pluggable data formats and query execution

[nibix]

Description

Adds logical-plan DLS enforcement for analytics queries according to RFC spec in #22756 .

This introduces the plugin interface AccessPolicyProviderPlugin and the injectable ReadAccessPolicyService.

The AccessPolicyProviderPlugin will be implemented by the security plugin in the separate PR opensearch-project/security#6569

The analytics plugins resolve the concrete indices referenced by a logical plan, obtains the effective policy from ReadAccessPolicyService., and rewrites protected table scans before planning/execution:

• indices with the same restriction remain in one scan with a filter;
• indices with different restrictions become separate filtered branches joined by UNION ALL;
• unrestricted indices remain unfiltered;
• scans inside Calcite RexSubQuery expressions are also rewritten.

dsl-query-executor provides the QueryBuilder-to-Calcite translation SPI used by the rewrite.

Notes

• When analytics selects the Lucene backend, DLS is enforced both at the logical-query level and at the existing Lucene level. This is intentional in order avoid weakening the established Lucene DLS protection for normal search execution.

• The implementation fails closed in case a DLS query uses a query type that cannot be translated to the target query language.

• FLS and field masking will follow in a separate PR.

Tests

• Unit tests cover single and grouped index restrictions, protected/unrestricted branches, missing policy coverage, match_none, missing translators, and RexSubQuery rewriting.
• Coordinator QA integration tests run PPL against Parquet-backed indices with a mock AccessPolicyProviderPlugin, covering both a restricted and an unrestricted user policy.

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 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 Security concerns Authorization bypass risk: See the resolveConcreteIndicesByTable finding — plan-embedded table expressions are resolved independently of the request's authorized index set, and only intersected with it. Any index literal appearing in the logical plan that intersects the authorized set at all is accepted; unauthorized indices are simply filtered out rather than causing rejection. If a caller can inject index names into a plan that differ from request.indices(), this permits silently querying indices the request did not name. This should be reviewed carefully and, if the concern is real, hardened with an explicit rejection.

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Introduce ReadAccessPolicy SPI and node-level service Relevant files: server/src/main/java/org/opensearch/action/support/ReadAccessContext.java server/src/main/java/org/opensearch/action/support/ReadAccessPolicy.java server/src/main/java/org/opensearch/action/support/ReadAccessPolicyProvider.java server/src/main/java/org/opensearch/action/support/ReadAccessPolicyService.java server/src/main/java/org/opensearch/plugins/AccessPolicyProviderPlugin.java server/src/main/java/org/opensearch/node/Node.java server/src/test/java/org/opensearch/action/support/ReadAccessPolicyServiceTests.java Sub-PR theme: Register DSL QueryBuilder translator provider Relevant files: sandbox/plugins/dsl-query-executor/src/main/java/org/opensearch/dsl/query/DslQueryBuilderTranslatorProvider.java sandbox/plugins/dsl-query-executor/src/main/resources/META-INF/services/org.opensearch.analytics.query.QueryBuilderTranslatorProvider sandbox/plugins/dsl-query-executor/src/test/java/org/opensearch/dsl/query/DslQueryBuilderTranslatorProviderTests.java

⚡ Recommended focus areas for review Table name collision on rewritten scans replaceTableName creates a ConcreteIndicesTable whose qualified name is the comma-joined concrete index expression (e.g. logs-2025,logs-2026). If the same TableScan appears more than once in the plan (e.g. self-join, repeated CTE), the shuttle rewrites each occurrence, and subsequent traversals or later planning stages that key on getQualifiedName().getLast() (as extractTableExpressions does) will see the derived expression rather than the original table. Also, if a user table literally named a,b legitimately existed, the derived name for a scan resolving to a and b would be indistinguishable. Consider carrying concrete indices via a distinct property rather than overloading the table name. private static TableScan replaceTableName(TableScan scan, String concreteIndexExpression) { RelOptTable table = new ConcreteIndicesTable(scan.getTable(), concreteIndexExpression); return new LogicalTableScan(scan.getCluster(), scan.getTraitSet(), scan.getHints(), table); } Possible authorization bypass via table expression mismatch resolveConcreteIndicesByTable resolves each tableExpression extracted from the plan against the cluster state independently and only filters the result against authorizedIndices (the concrete indices resolved from request.indices()). If a plan contains a TableScan whose table expression is not present in request.indices(), but the intersection with authorizedIndices is non-empty (e.g. request is logs-* and plan contains a hard-coded logs-2025), the scan is silently allowed. Confirm that plan-level tables are constrained to be a subset of request.indices(), or reject any table expression that resolves to indices outside the request-authorized set with an explicit error rather than filtering silently. private Map<String, List<String>> resolveConcreteIndicesByTable( RelNode logicalPlan, ClusterState clusterState, List<String> requestConcreteIndices ) { Set<String> authorizedIndices = new HashSet<>(requestConcreteIndices); Map<String, List<String>> concreteIndicesByTable = new LinkedHashMap<>(); for (String tableExpression : RelNodeUtils.extractTableExpressions(logicalPlan)) { List<String> concreteIndices = IndexResolution.resolve(tableExpression, clusterState, indexNameExpressionResolver) .concreteIndexNames() .stream() .sorted() .filter(authorizedIndices::contains) .toList(); if (concreteIndices.isEmpty()) { throw new OpenSearchException("No authorized concrete indices were resolved for table [" + tableExpression + "]"); } concreteIndicesByTable.put(tableExpression, concreteIndices); } return concreteIndicesByTable; } Coverage check may falsely pass with unrelated policy indices The final check associatedPolicyIndices.containsAll(policy.coveredConcreteIndices()) fails if any covered index in the policy was not touched by a scan. However, associatedPolicyIndices is only populated from indices that were both in a group AND in concreteIndicesByTable. If the policy covers indices unrelated to the query (a common case for tenant-wide policies), this will throw even though the plan is safely rewritten. Verify whether coverage should instead be checked against the union of concrete indices actually referenced by the plan, not against the whole policy. if (associatedPolicyIndices.containsAll(policy.coveredConcreteIndices()) == false) { Set<String> missing = new HashSet<>(policy.coveredConcreteIndices()); missing.removeAll(associatedPolicyIndices); throw new OpenSearchException("DLS policy indices could not be associated with table scans: " + missing); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Fail loudly on unauthorized indices Silently filtering out unauthorized indices via authorizedIndices::contains can mask an authorization bypass attempt or a bug where a table expression references indices outside the request scope. Since this is a security-critical path (DLS enforcement), consider throwing when the resolved set contains any index not present in authorizedIndices, rather than only failing when the intersection is empty. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [976-984] -List<String> concreteIndices = IndexResolution.resolve(tableExpression, clusterState, indexNameExpressionResolver) +List<String> resolved = IndexResolution.resolve(tableExpression, clusterState, indexNameExpressionResolver) .concreteIndexNames() .stream() .sorted() - .filter(authorizedIndices::contains) .toList(); -if (concreteIndices.isEmpty()) { +List<String> unauthorized = resolved.stream().filter(idx -> authorizedIndices.contains(idx) == false).toList(); +if (unauthorized.isEmpty() == false) { + throw new OpenSearchException("Table [" + tableExpression + "] resolves to unauthorized indices " + unauthorized); +} +if (resolved.isEmpty()) { throw new OpenSearchException("No authorized concrete indices were resolved for table [" + tableExpression + "]"); } +List<String> concreteIndices = resolved; Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid security consideration: silently filtering may mask authorization issues. However, in the current design authorizedIndices corresponds to the request-scoped indices, so filtering may be intentional. The suggestion adds defense-in-depth.	Low
	Avoid qualified-name collisions for policy keying Using only the last qualified-name segment as the key can collide when two scans reference tables with the same leaf name from different schemas. Since RelNodeUtils.extractTableExpressions similarly uses only the last name, both callers must stay in sync, but a same-leaf-name collision would silently apply the wrong policy set. Consider keying by the full qualified name (or asserting uniqueness of leaf names) to prevent a cross-schema DLS misapplication. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/LogicalPlanDlsRewriter.java [101-104] +@Override +public RelNode visit(TableScan scan) { + String tableName = scan.getTable().getQualifiedName().getLast(); + List<String> concreteIndices = concreteIndicesByTable.get(tableName); - Suggestion importance[1-10]: 5 __ Why: The improved_code is identical to existing_code, offering no concrete fix. The concern about cross-schema leaf-name collisions is valid but is not demonstrated with a code change.	Low
General	Ensure deterministic branch index ordering intersection preserves the ordering from concreteIndices but the resulting comma-joined expression could vary based on that input order. Since concreteIndicesByTable values are already sorted, this is fine, but the branchIndices list should be explicitly sorted here defensively to guarantee a stable, deterministic index expression regardless of caller ordering. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/planner/LogicalPlanDlsRewriter.java [111-116] -List<String> branchIndices = intersection(concreteIndices, group.concreteIndices()); +List<String> branchIndices = intersection(concreteIndices, group.concreteIndices()).stream().sorted().toList(); if (branchIndices.isEmpty()) { continue; } TableScan concreteScan = replaceTableName(scan, String.join(",", branchIndices)); Suggestion importance[1-10]: 3 __ Why: The concreteIndices passed in is already sorted upstream in resolveConcreteIndicesByTable, so intersection already produces deterministic ordering. The suggestion is defensive but has minimal impact.	Low
	Validate injected policy service at construction readAccessPolicyService is a new required constructor parameter but there is no null-check or defensive fallback. If wiring fails, a NullPointerException will surface as a generic internal error instead of a clear configuration issue. Consider validating it in the constructor with Objects.requireNonNull to fail fast at startup. sandbox/plugins/analytics-engine/src/main/java/org/opensearch/analytics/exec/DefaultPlanExecutor.java [921-925] +ClusterState clusterState = clusterService.state(); +List<String> requestConcreteIndices = resolveConcreteIndices(clusterState, request.indicesOptions(), request.indices()); +ReadAccessPolicy readAccessPolicy = readAccessPolicyService.getReadAccessPolicy( + ReadAccessContext.of(requestConcreteIndices) +); - Suggestion importance[1-10]: 2 __ Why: The improved_code is identical to the existing_code, so the suggestion does not actually demonstrate any change. Additionally, fail-fast null-checks provide only minor value with DI wiring.	Low

[nibix]

@cwperks @DarshitChanpura @Bukhtawar @niravpi

Please have a look at this.

It should already be the complete implementation. I left it in draft state, because we have to coordinate merging on the core and security side.

[github-actions]

❕ Gradle check result for 5a3ce6c: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.83%. Comparing base (83d2a81) to head (5a3ce6c).

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #23168      +/-   ##
============================================
+ Coverage     71.81%   71.83%   +0.02%     
- Complexity    77894    77910      +16     
============================================
  Files          6183     6183              
  Lines        361063   361066       +3     
  Branches      52555    52555              
============================================
+ Hits         259295   259371      +76     
+ Misses        81189    81134      -55     
+ Partials      20579    20561      -18     

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

[github-actions]

PR Code Analyzer ❗

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

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

Path	Line	Severity	Description
sandbox/qa/analytics-engine-coordinator/build.gradle	65	high	New dependency added: 'internalClusterTestImplementation project(':sandbox:plugins:dsl-query-executor')'. Per mandatory rule, all dependency/module additions must be flagged for maintainer verification regardless of apparent legitimacy. This appears to be an internal same-repo project reference (Gradle ':' syntax), but maintainers should confirm the dsl-query-executor module has not been tampered with and that pulling it into the test classpath is intentional.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 1 | Medium: 0 | Low: 0

---

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.