Add Views Security Model#141050
Conversation
773b37e to
3a9b417
Compare
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
034bde1 to
30b08f9
Compare
66887a4 to
18323bb
Compare
a9dfacf to
8604bd1
Compare
c4c9c1a to
748c0aa
Compare
n1v0lg
left a comment
There was a problem hiding this comment.
This is looking good! Sorry I ran out of time for the day. I just need to wrap up reviewing ViewResolver and the tests. Doing that first thing tomorrow morning.
| * processing. | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public void transformDown(BiConsumer<? super T, ActionListener<T>> rule, ActionListener<T> listener) { |
There was a problem hiding this comment.
Note: I'm deferring to the other reviewers here since it's ES|QL code. It looks complicated but I'm assuming there's not way around it.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/view/ViewResolutionService.java
Outdated
Show resolved
Hide resolved
| return false; | ||
| } | ||
|
|
||
| if (replaceable.getResolvedIndexExpressions() != null) { |
There was a problem hiding this comment.
Nit: It feels off that a boolean "check" style method would trip an assertion -- maybe we can combine shouldSetResolvedIndexExpressions and actually setting the expressions into one method and call it maybeSetResolvedIndexExpressions. WDYT?
There was a problem hiding this comment.
I was going back and forth on this and landed on this approach with the motivation that we will eventually remove shouldSetResolvedIndexExpressions and #135799 will be fixed, and when it is, this is kind of what it would look like IMO.
There was a problem hiding this comment.
I think #135799 may stick around for some time, unfortunately. It's one of those things that isn't painful enough to get prioritized but maybe I'm overly pessimistic.
| views.add(viewMetadata.getView(viewName)); | ||
| } | ||
| var patterns = Arrays.stream(unresolvedRelation.indexPattern().indexPattern().split(",")) | ||
| .filter(pattern -> Regex.isSimpleMatchPattern(pattern) == false || seenWildcards.add(pattern)) |
There was a problem hiding this comment.
There's an exclusion edge where this breaks I think: if do something like GET view*,-view-1,view* you should get back results for views* (because exclusions are sensitive to order). That sort of index expression is weird and I can't come up with a good reason to write it so perhaps it's okay to leave it unhandled but ideally we'd fix it somehow.
n1v0lg
left a comment
There was a problem hiding this comment.
The core logic minus exclusions LGTM. Nice test coverage, too 👍
For exclusions, see my inline comments. The main problem is that we don't preserve order and also drop the occurrence of the same pattern incorrectly which breaks some exclusion edge cases. In the interest of progress, I think it's okay to merge this PR but I do think we need exclusion handling follow ups.
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/view/ViewResolver.java
Outdated
Show resolved
Hide resolved
| views.add(viewMetadata.getView(viewName)); | ||
| } | ||
| var patterns = Arrays.stream(unresolvedRelation.indexPattern().indexPattern().split(",")) | ||
| .filter(pattern -> Regex.isSimpleMatchPattern(pattern) == false || seenWildcards.add(pattern)) |
There was a problem hiding this comment.
Hm it the edge case gets a little less far fetched once you write a view with an exclusion in it:
public void testViewBodyWithExclusionCombined() {
addView("safe-logs", "FROM logs*,-logs-secret");
addIndex("logs-public");
addIndex("logs-secret");
LogicalPlan plan = query("FROM safe-logs,logs-secret");
assertThat(replaceViews(plan), matchesPlan(query("FROM logs*,-logs-secret,logs-secret")));
}Is that I'd expect to happen. It would give us logs-secret.
Instead we get:
public void testViewBodyWithExclusionCombined() {
addView("safe-logs", "FROM logs*,-logs-secret");
addIndex("logs-public");
addIndex("logs-secret");
LogicalPlan plan = query("FROM safe-logs,logs-secret");
assertThat(replaceViews(plan), matchesPlan(query("FROM logs-secret,logs*,-logs-secret")));
}Meaning empty.
| views.add(viewMetadata.getView(viewName)); | ||
| } | ||
| var patterns = Arrays.stream(unresolvedRelation.indexPattern().indexPattern().split(",")) | ||
| .filter(pattern -> Regex.isSimpleMatchPattern(pattern) == false || seenWildcards.add(pattern)) |
There was a problem hiding this comment.
FROM safe-logs,logs-secret seems like a fairly natural query to write:
Maybe I started querying safe logs FROM safe-logs and now I'm interested in both safe logs and secret logs so I write:
FROM safe-logs,logs-secret
x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/view/ViewResolver.java
Outdated
Show resolved
Hide resolved
...ck/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/view/InMemoryViewServiceTests.java
Show resolved
Hide resolved
...ck/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/view/InMemoryViewServiceTests.java
Show resolved
Hide resolved
✅ Vale Linting ResultsNo issues found on modified lines! The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
4b46776 to
16d53ba
Compare
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This PR wires ES|QL views into index authorization and view resolution so view expansion preserves the same security/error semantics as direct index access. - Adds `EsqlResolveViewAction` plus `ViewResolutionService` to resolve views from project metadata using `ResolvedIndexExpressions`, including strict handling for unauthorized and missing concrete targets. - Refactors `ViewResolver` to resolve views asynchronously, recurse through nested/wildcard view references, detect circular references/max depth, and keep unresolved non-view targets in the rewritten plan rather than dropping them. - Updates `EsqlSession` to run view replacement asynchronously and carry `viewQueries` in `Configuration`, so source/view context survives later planning and serialization. - Updates view CRUD/read request handling to resolve views via indices options (`resolveViews=true`) and reuse the same resolution path for `GET view`. - Aligns security behavior by persisting resolved expressions for view-resolving requests and by handling view abstractions correctly in index-permission resource accounting (including selector/failure-store edge cases).
…locations * upstream/main: (153 commits) ES|QL: Update docs for TOP_SNIPPETS and DECAY (elastic#143739) Correctly include endpoint id in log msg in AuthorizationPoller (elastic#143743) Bar searching or sorting on _seq_no when disabled (elastic#143600) Generalize `testClientCancellation` test (elastic#143586) JSON_EXTRACT: zero-copy byte slicing for object, array, and number extraction (elastic#143702) Track recycler pages in circuit breaker (elastic#143738) [ESQL] Enable distributed pipeline breakers for external sources via FragmentExec (elastic#143696) Adding 'mode' and 'codec' fields to ES monitoring template (elastic#143673) [ESQL] Columnar I/O and vectorized block conversion for external sources (elastic#143703) Fix flaky MMR diversification YAML tests (elastic#143706) ES|QL codegen: check builder arguments for vector support (elastic#143724) Add Views Security Model (elastic#141050) ESQL: Prevent pushdown of unmapped fields in filters and sorts (elastic#143460) Don't run seq_no pruning tests in release CI (elastic#143725) ESQL: Support intra-row field references in ROW command (elastic#140217) ES|QL: Remove implicit limit in FORK branches in CSV tests (elastic#143601) IndexRoutingTests with and without synthetic id (elastic#143566) Synthetic id upgrade test in serverless (elastic#142471) Disable "Review skipped" comments for PRs without specified labels (elastic#143728) Cleanup ES|QL T-Digest code duplication, add memory accounting (elastic#143662) ...
This PR wires ES|QL views into index authorization and view resolution so view expansion preserves the same security/error semantics as direct index access. - Adds `EsqlResolveViewAction` plus `ViewResolutionService` to resolve views from project metadata using `ResolvedIndexExpressions`, including strict handling for unauthorized and missing concrete targets. - Refactors `ViewResolver` to resolve views asynchronously, recurse through nested/wildcard view references, detect circular references/max depth, and keep unresolved non-view targets in the rewritten plan rather than dropping them. - Updates `EsqlSession` to run view replacement asynchronously and carry `viewQueries` in `Configuration`, so source/view context survives later planning and serialization. - Updates view CRUD/read request handling to resolve views via indices options (`resolveViews=true`) and reuse the same resolution path for `GET view`. - Aligns security behavior by persisting resolved expressions for view-resolving requests and by handling view abstractions correctly in index-permission resource accounting (including selector/failure-store edge cases).
There has been several discussions on this topic since I opened #141050 and @idegtiarenko also mentioned this in #143561 (comment). The issue is that both `resolveViews` and `resolveAliases` are in `WildcardOptions` but are used when resolving both concrete index pattern targets and wildcard targets. This PR moves `resolveViews` from `IndicesOptions.WildcardOptions` into a new `IndicesOptions.IndexAbstractionOptions` record to address this confusing inconsistency. This is in preparation for sending `resolveViews` as a parameter to field caps for remote view resolving where more work will be done to serialize `resolveViews` and not having this in place would make for confusing code. See #143384 for more information on the upcoming serialization of `resolveViews`. **_Note_**: `resolveAliases` is moved in a follow up PR since it's decoupled from the resolveViews changes. #143953 **_Note_**: `IndicesRequest.includeDataStreams()` is the same type of flag as `resolveAliases` and `resolveViews`, it controls whether data streams participate in index resolution. It's a candidate for `IndexAbstractionOptions` but currently lives on `IndicesRequest` (not `IndicesOptions`) and is threaded separately through `IndexNameExpressionResolver.Context` as a constructor parameter. Moving it would touch 60+ files and change the `IndicesRequest` interface. Because of how big that change would be, I have not considered doing that in this PR (or at all).
This PR wires ES|QL views into index authorization and view resolution so view expansion preserves the same security/error semantics as direct index access.
EsqlResolveViewActionplusViewResolutionServiceto resolve views from project metadata usingResolvedIndexExpressions, including strict handling for unauthorized and missing concrete targets.ViewResolverto resolve views asynchronously, recurse through nested/wildcard view references, detect circular references/max depth, and keep unresolved non-view targets in the rewritten plan rather than dropping them.EsqlSessionto run view replacement asynchronously and carryviewQueriesinConfiguration, so source/view context survives later planning and serialization.resolveViews=true) and reuse the same resolution path forGET view.