ES|QL: Fix queries with document level security on lookup indexes #120617

luigidellaquila · 2025-01-22T11:57:50Z

Fixes: #120509

Make ES|QL respect document level permissions on lookup indexes.

This fix exposes a problem with aliases as lookup indexes: if a user has permissions on an alias, but not on the underlying index, the query fails with an authorization error. This needs to be investigated further and fixed. For now we'll just disable support for aliases in LOOKUP JOIN command

elasticsearchmachine · 2025-01-22T13:45:29Z

Hi @luigidellaquila, I've created a changelog YAML for you.

elasticsearchmachine · 2025-01-22T13:45:53Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

…to esql/fix_lookup_dls

luigidellaquila · 2025-01-22T14:38:52Z

Failures due to #120637
Wait for it to be merged both in main and 8.x and then change the bwc mute

bpintea · 2025-01-22T15:44:51Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/analysis/Verifier.java

@@ -109,6 +112,16 @@ Collection<Failure> verify(LogicalPlan plan, BitSet partialMetrics) {
        return failures.failures();
    }

+    private void checkLookupJoin(LogicalPlan p, Failures failures) {


Drive by comment: LookupJoin could implement PostAnalysisVerificationAware and perform the check there.

Thanks @bpintea, good point, doing it now

Muting a yaml bwc test called "alias" apparently is not possible: ``` task.skipTest("esql/190_lookup_join/alias", "LOOKUP JOIN does not support index aliases for now") ``` ``` * What went wrong: Execution failed for task ':x-pack:plugin:yamlRestCompatTestTransform'. > class com.fasterxml.jackson.databind.node.TextNode cannot be cast to class com.fasterxml.jackson.databind.node.ArrayNode (com.fasterxml.jackson.databind.node.TextNode and com.fasterxml.jackson.databind.node.ArrayNode are in unnamed module of loader org.gradle.internal.classloader.VisitableURLClassLoader$InstrumentingVisitableURLClassLoader @725f7fdd) ``` I need to mute this test in #120617

Muting a yaml bwc test called "alias" apparently is not possible: ``` task.skipTest("esql/190_lookup_join/alias", "LOOKUP JOIN does not support index aliases for now") ``` ``` * What went wrong: Execution failed for task ':x-pack:plugin:yamlRestCompatTestTransform'. > class com.fasterxml.jackson.databind.node.TextNode cannot be cast to class com.fasterxml.jackson.databind.node.ArrayNode (com.fasterxml.jackson.databind.node.TextNode and com.fasterxml.jackson.databind.node.ArrayNode are in unnamed module of loader org.gradle.internal.classloader.VisitableURLClassLoader$InstrumentingVisitableURLClassLoader @725f7fdd) ``` I need to mute this test in elastic#120617

dnhatn

The LookupService looks good to me. Thanks @luigidellaquila

Muting a yaml bwc test called "alias" apparently is not possible: ``` task.skipTest("esql/190_lookup_join/alias", "LOOKUP JOIN does not support index aliases for now") ``` ``` * What went wrong: Execution failed for task ':x-pack:plugin:yamlRestCompatTestTransform'. > class com.fasterxml.jackson.databind.node.TextNode cannot be cast to class com.fasterxml.jackson.databind.node.ArrayNode (com.fasterxml.jackson.databind.node.TextNode and com.fasterxml.jackson.databind.node.ArrayNode are in unnamed module of loader org.gradle.internal.classloader.VisitableURLClassLoader$InstrumentingVisitableURLClassLoader @725f7fdd) ``` I need to mute this test in #120617

alex-spies

Had a first look, will continue tomorrow. Looking good so far, thanks @luigidellaquila !

docs/changelog/120617.yaml

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/esql/190_lookup_join.yml

...ugin/esql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/esql/EsqlSecurityIT.java

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/LookupJoin.java

alex-spies · 2025-01-22T18:21:21Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/LookupJoin.java

+    public void postAnalysisVerification(Failures failures) {
+        super.postAnalysisVerification(failures);
+        if (right() instanceof EsRelation esr) {
+            if (esr.concreteIndices().contains(esr.indexPattern()) == false) {


Also: Could there be weird edge cases where the name of the index contains e.g. an escaped wildcard character, so that an index pattern is being used, but coincidentally, it also matches a concrete index? There's no escaping in index names, right? Per the docs it doesn't seem so: https://www.elastic.co/guide/en/elasticsearch/reference/current/indices-create-index.html

I understand this is as edge case-y as it gets; but since avoiding aliases is required to ensure permissions are respected correctly, we better try to think about every possible scenario.

to make them easier to run on Serverless

alex-spies

Thanks a lot for the iterations @luigidellaquila !

I have a final test suggestion, but otherwise LGTM.

x-pack/plugin/esql/qa/security/src/javaRestTest/resources/roles.yml

alex-spies · 2025-01-23T17:18:09Z

...ugin/esql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/esql/EsqlSecurityIT.java

+        assertThat(respMap.get("values"), equalTo(List.of(Arrays.asList(40.0, "sales"))));
+
+        resp = runESQLCommand("dls_user2", "ROW x = 32.0 | EVAL value = x | LOOKUP JOIN `lookup-user2` ON value | KEEP x, org");
+        assertOK(resp);
+        respMap = entityAsMap(resp);
+        assertThat(
+            respMap.get("columns"),
+            equalTo(List.of(Map.of("name", "x", "type", "double"), Map.of("name", "org", "type", "keyword")))
+        );
+        assertThat(respMap.get("values"), equalTo(List.of(List.of(32.0, "marketing"))));
+


Nice, these are very helpful. Thanks for adding!

…to esql/fix_lookup_dls

idegtiarenko · 2025-01-24T07:20:04Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/join/LookupJoin.java

+                failures.add(
+                    fail(
+                        esr,
+                        "invalid [{}] resolution in lookup mode to an index in [{}] indices",


Suggested change

"invalid [{}] resolution in lookup mode to an index in [{}] indices",

"invalid [{}] resolution in lookup mode to [{}] indices",

Previously it was a slightly different message, not sure if it was changed intentionally.

alex-spies

Thanks a lot for the iterations @luigidellaquila ! I'm feeling much more confident about the security of LOOKUP JOIN now :)

alex-spies · 2025-01-24T09:43:09Z

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichLookupService.java

+        TransportRequest transportRequest
+    ) {
+        ThreadContext threadContext = transportService.getThreadPool().getThreadContext();
+        try (ThreadContext.StoredContext unused = threadContext.stashWithOrigin(ClientHelper.ENRICH_ORIGIN)) {


Agree, with @dnhatn 's suggestion the whole situation is more localized and more clear.

Personally, I'd still leave a comment in place because for people unfamiliar with this code, it can be unexpected that the call to ThreadContext.stashWithOrigin will abandon the current user's authorization. (It's explained in stashWithOrigin's javadoc, but that requires an extra hop.)

alex-spies · 2025-01-24T09:51:43Z

...ugin/esql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/esql/EsqlSecurityIT.java

+                    Map.of("name", "org", "type", "keyword")
+                )
+            )
+        );
    }


Sorry, one last test suggestion - we can also add this in a separate PR if you're already about to make this ready to merge.

I just noticed: all of the fls test cases have permission to read the value field, which is the join key.

I think it'd be interesting to test if forbidding this field for a user makes it so all look ups are null. And, if we wanna be extra paranoid, consider the case where the lookup index has duplicates for a given join key, so the LOOKUP JOIN would lead to extra rows - but without permissions granted on the join key, no extra rows should be emitted. (Otherwise, users could exfiltrate statistics about fields they have no permissions on).

Thanks Alex, I still have a couple of things to push, I'll add this one as well.
I think the whole query will fail because the user doesn't see the join key in the schema, but I'll double-check

The new tests confirm the above: if a role cannot see the join key in the lookup index, the query will fail with "Unknown column"

luigidellaquila · 2025-01-24T10:17:02Z

Thanks for the reviews folks!
I addressed all the comments, if there are no further observations I'll merge it as soon as the CI is happy.

elasticsearchmachine · 2025-01-24T12:00:32Z

💔 Backport failed

Status	Branch	Result
❌	8.x	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 120617

…astic#120617)

…20617) (#120798)

There are some analyser tests for this, but I thought it useful to also have a yaml test for more coverage, since there was temporarily an issue elastic#120189 with this, later fixed in elastic#120617.

There are some analyser tests for this, but I thought it useful to also have a yaml test for more coverage, since there was temporarily an issue #120189 with this, later fixed in #120617.

There are some analyser tests for this, but I thought it useful to also have a yaml test for more coverage, since there was temporarily an issue elastic#120189 with this, later fixed in elastic#120617.

… (#128015) * Additional yaml test for missing join key on right side (#127906) There are some analyser tests for this, but I thought it useful to also have a yaml test for more coverage, since there was temporarily an issue #120189 with this, later fixed in #120617. * Fix mistake in merge

There are some analyser tests for this, but I thought it useful to also have a yaml test for more coverage, since there was temporarily an issue elastic#120189 with this, later fixed in elastic#120617.

luigidellaquila added 2 commits January 22, 2025 12:53

ES|QL: Fix queries with document level security on lookup indexes

8254efb

Merge branch 'main' into esql/fix_lookup_dls

0427012

luigidellaquila added the >bug label Jan 22, 2025

elasticsearchmachine added the v9.0.0 label Jan 22, 2025

Skip tests with alias lookup indexes

c40d800

luigidellaquila added v8.18.0 auto-backport Automatically create backport pull requests when merged labels Jan 22, 2025

Skip tests

edab3ec

luigidellaquila added the :Analytics/ES|QL AKA ESQL label Jan 22, 2025

Update docs/changelog/120617.yaml

d0b5512

luigidellaquila marked this pull request as ready for review January 22, 2025 13:45

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jan 22, 2025

luigidellaquila requested review from nik9000, dnhatn and alex-spies January 22, 2025 13:46

luigidellaquila added 2 commits January 22, 2025 15:15

Skip bwc tests

1cdce58

Merge remote-tracking branch 'luigidellaquila/esql/fix_lookup_dls' in…

c003b61

…to esql/fix_lookup_dls

luigidellaquila mentioned this pull request Jan 22, 2025

Rename ES|QL LOOKUP JOIN test #120637

Merged

bpintea reviewed Jan 22, 2025

View reviewed changes

dnhatn approved these changes Jan 22, 2025

View reviewed changes

luigidellaquila added 3 commits January 22, 2025 17:06

Merge branch 'main' into esql/fix_lookup_dls

129133b

Mute tests (again)

b3408fc

Use PostAnalysisVerificationAware

5d377a8

alex-spies reviewed Jan 22, 2025

View reviewed changes

luigidellaquila added 4 commits January 23, 2025 13:54

Merge branch 'main' into esql/fix_lookup_dls

6d5b21e

Simplify datastream tests

17b40f8

to make them easier to run on Serverless

More tests

ffa5fb9

Merge branch 'main' into esql/fix_lookup_dls

85b33ff

alex-spies approved these changes Jan 23, 2025

View reviewed changes

luigidellaquila added 3 commits January 23, 2025 18:48

More tests and comments

72ed0b5

Merge branch 'main' into esql/fix_lookup_dls

d070ed7

Merge remote-tracking branch 'luigidellaquila/esql/fix_lookup_dls' in…

35cbc91

…to esql/fix_lookup_dls

idegtiarenko reviewed Jan 24, 2025

View reviewed changes

Validation message

72328a1

alex-spies approved these changes Jan 24, 2025

View reviewed changes

Refactoring of enrich security and more tests

0b30657

luigidellaquila enabled auto-merge (squash) January 24, 2025 10:31

luigidellaquila disabled auto-merge January 24, 2025 10:46

Merge branch 'main' into esql/fix_lookup_dls

43a2d07

luigidellaquila enabled auto-merge (squash) January 24, 2025 10:55

luigidellaquila merged commit dc195f4 into elastic:main Jan 24, 2025
15 of 16 checks passed

elasticsearchmachine added the backport pending label Jan 24, 2025

luigidellaquila added a commit to luigidellaquila/elasticsearch that referenced this pull request Jan 24, 2025

ES|QL: Fix queries with document level security on lookup indexes (el…

7d0de1e

…astic#120617)

luigidellaquila mentioned this pull request Jan 24, 2025

ES|QL: Fix queries with document level security on lookup indexes (#120617) #120798

Merged

elasticsearchmachine pushed a commit that referenced this pull request Jan 24, 2025

ES|QL: Fix queries with document level security on lookup indexes (#1…

e33a1c3

…20617) (#120798)

This was referenced May 8, 2025

Additional yaml test for missing join key on right side #127906

Merged

Lookup index validity error message for ES|QL could be improved #120189

Closed

	"invalid [{}] resolution in lookup mode to an index in [{}] indices",
	"invalid [{}] resolution in lookup mode to [{}] indices",

ES|QL: Fix queries with document level security on lookup indexes #120617

ES|QL: Fix queries with document level security on lookup indexes #120617

Uh oh!

Conversation

luigidellaquila commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 22, 2025

Uh oh!

elasticsearchmachine commented Jan 22, 2025

Uh oh!

luigidellaquila commented Jan 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpintea Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

luigidellaquila Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alex-spies Jan 22, 2025

Choose a reason for hiding this comment

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alex-spies Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

idegtiarenko Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alex-spies left a comment

Choose a reason for hiding this comment

Uh oh!

alex-spies Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

alex-spies Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luigidellaquila Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

luigidellaquila Jan 24, 2025

Choose a reason for hiding this comment

Uh oh!

luigidellaquila commented Jan 24, 2025

Uh oh!

Uh oh!

elasticsearchmachine commented Jan 24, 2025

💔 Backport failed

Uh oh!

Uh oh!

luigidellaquila commented Jan 22, 2025 •

edited

Loading

luigidellaquila commented Jan 22, 2025 •

edited

Loading

idegtiarenko Jan 24, 2025 •

edited

Loading

alex-spies Jan 24, 2025 •

edited

Loading