ES|QL: Fix queries with document level security on lookup indexes #120617
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| pr: 120617 | ||
| summary: Fix queries with document level security on lookup indexes | ||
| area: ES|QL | ||
| type: bug | ||
| issues: [120509] |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alex-spies marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,6 +57,9 @@ public class EsqlSecurityIT extends ESRestTestCase { | |
| .user("user4", "x-pack-test-password", "user4", false) | ||
| .user("user5", "x-pack-test-password", "user5", false) | ||
| .user("fls_user", "x-pack-test-password", "fls_user", false) | ||
| .user("fls_user2", "x-pack-test-password", "fls_user2", false) | ||
| .user("fls_user3", "x-pack-test-password", "fls_user3", false) | ||
| .user("dls_user", "x-pack-test-password", "dls_user", false) | ||
| .user("metadata1_read2", "x-pack-test-password", "metadata1_read2", false) | ||
| .user("alias_user1", "x-pack-test-password", "alias_user1", false) | ||
| .user("alias_user2", "x-pack-test-password", "alias_user2", false) | ||
|
|
@@ -92,7 +95,7 @@ private void indexDocument(String index, int id, double value, String org) throw | |
| public void indexDocuments() throws IOException { | ||
| Settings lookupSettings = Settings.builder().put("index.mode", "lookup").build(); | ||
| String mapping = """ | ||
| "properties":{"value": {"type": "double"}, "org": {"type": "keyword"}, "other": {"type": "keyword"}} | ||
| """; | ||
|
|
||
| createIndex("index", Settings.EMPTY, mapping); | ||
|
|
@@ -163,6 +166,21 @@ public void indexDocuments() throws IOException { | |
| """); | ||
| assertOK(client().performRequest(aliasRequest)); | ||
| } | ||
|
|
||
| createMultiRoleUsers(); | ||
| } | ||
|
|
||
| private void createMultiRoleUsers() throws IOException { | ||
| Request request = new Request("POST", "_security/user/dls_user2"); | ||
| request.setJsonEntity(""" | ||
| { | ||
| "password" : "x-pack-test-password", | ||
| "roles" : [ "dls_user", "dls_user2" ], | ||
| "full_name" : "Test Role", | ||
| "email" : "[email protected]" | ||
| } | ||
| """); | ||
| assertOK(client().performRequest(request)); | ||
| } | ||
|
|
||
| protected MapMatcher responseMatcher() { | ||
|
|
@@ -563,25 +581,109 @@ public void testLookupJoinIndexAllowed() throws Exception { | |
| ); | ||
| assertThat(respMap.get("values"), equalTo(List.of(List.of(40.0, "sales")))); | ||
|
|
||
| // Aliases are not allowed in LOOKUP JOIN | ||
| var resp2 = expectThrows( | ||
| ResponseException.class, | ||
| () -> runESQLCommand("alias_user1", "ROW x = 31.0 | EVAL value = x | LOOKUP JOIN `lookup-first-alias` ON value | KEEP x, org") | ||
| ); | ||
|
|
||
| assertThat(resp2.getMessage(), containsString("Aliases and index patterns are not allowed for LOOKUP JOIN [lookup-first-alias]")); | ||
| assertThat(resp2.getResponse().getStatusLine().getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST)); | ||
|
|
||
| // Aliases are not allowed in LOOKUP JOIN, regardless of alias filters | ||
| resp2 = expectThrows( | ||
| ResponseException.class, | ||
| () -> runESQLCommand("alias_user1", "ROW x = 123.0 | EVAL value = x | LOOKUP JOIN `lookup-first-alias` ON value | KEEP x, org") | ||
| ); | ||
| assertThat(resp2.getMessage(), containsString("Aliases and index patterns are not allowed for LOOKUP JOIN [lookup-first-alias]")); | ||
| assertThat(resp2.getResponse().getStatusLine().getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST)); | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public void testLookupJoinDocLevelSecurity() throws Exception { | ||
| assumeTrue( | ||
| "Requires LOOKUP JOIN capability", | ||
| EsqlSpecTestCase.hasCapabilities(adminClient(), List.of(EsqlCapabilities.Cap.JOIN_LOOKUP_V11.capabilityName())) | ||
| ); | ||
|
|
||
| Response resp = runESQLCommand("dls_user", "ROW x = 40.0 | EVAL value = x | LOOKUP JOIN `lookup-user2` ON value | KEEP x, org"); | ||
| assertOK(resp); | ||
| Map<String, Object> 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(Arrays.asList(40.0, null)))); | ||
|
|
||
| resp = runESQLCommand("dls_user", "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")))); | ||
|
|
||
| // same, but with a user that has two dls roles that allow him more visibility | ||
|
|
||
| resp = runESQLCommand("dls_user2", "ROW x = 40.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(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")))); | ||
|
|
||
|
There was a problem hiding this comment. Nice, these are very helpful. Thanks for adding! |
||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| public void testLookupJoinFieldLevelSecurity() throws Exception { | ||
| assumeTrue( | ||
| "Requires LOOKUP JOIN capability", | ||
| EsqlSpecTestCase.hasCapabilities(adminClient(), List.of(EsqlCapabilities.Cap.JOIN_LOOKUP_V11.capabilityName())) | ||
| ); | ||
|
|
||
| Response resp = runESQLCommand("fls_user2", "ROW x = 40.0 | EVAL value = x | LOOKUP JOIN `lookup-user2` ON value"); | ||
| assertOK(resp); | ||
| Map<String, Object> respMap = entityAsMap(resp); | ||
| assertThat( | ||
| respMap.get("columns"), | ||
| equalTo( | ||
| List.of( | ||
| Map.of("name", "x", "type", "double"), | ||
| Map.of("name", "value", "type", "double"), | ||
| Map.of("name", "org", "type", "keyword") | ||
| ) | ||
| ) | ||
| ); | ||
|
|
||
| resp = runESQLCommand("fls_user3", "ROW x = 40.0 | EVAL value = x | LOOKUP JOIN `lookup-user2` ON value"); | ||
| assertOK(resp); | ||
| respMap = entityAsMap(resp); | ||
| assertThat( | ||
| respMap.get("columns"), | ||
| equalTo( | ||
| List.of( | ||
| Map.of("name", "x", "type", "double"), | ||
| Map.of("name", "value", "type", "double"), | ||
| Map.of("name", "org", "type", "keyword"), | ||
| Map.of("name", "other", "type", "keyword") | ||
| ) | ||
| ) | ||
|
|
||
| ); | ||
| } | ||
|
There was a problem hiding this comment. 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 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 There was a problem hiding this comment. Thanks Alex, I still have a couple of things to push, I'll add this one as well. There was a problem hiding this comment. 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" |
||
|
|
||
| public void testLookupJoinIndexForbidden() throws Exception { | ||
|
|
||
alex-spies marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,13 @@ | |
| package org.elasticsearch.xpack.esql.enrich; | ||
|
|
||
| import org.elasticsearch.TransportVersions; | ||
| import org.elasticsearch.action.ActionListener; | ||
| import org.elasticsearch.cluster.node.DiscoveryNode; | ||
| import org.elasticsearch.cluster.service.ClusterService; | ||
| import org.elasticsearch.common.io.stream.StreamInput; | ||
| import org.elasticsearch.common.io.stream.StreamOutput; | ||
| import org.elasticsearch.common.util.BigArrays; | ||
| import org.elasticsearch.common.util.concurrent.ThreadContext; | ||
| import org.elasticsearch.compute.data.Block; | ||
| import org.elasticsearch.compute.data.BlockFactory; | ||
| import org.elasticsearch.compute.data.BlockStreamInput; | ||
|
|
@@ -23,8 +26,10 @@ | |
| import org.elasticsearch.index.mapper.RangeType; | ||
| import org.elasticsearch.index.query.SearchExecutionContext; | ||
| import org.elasticsearch.index.shard.ShardId; | ||
| import org.elasticsearch.tasks.CancellableTask; | ||
| import org.elasticsearch.tasks.TaskId; | ||
| import org.elasticsearch.transport.TransportService; | ||
| import org.elasticsearch.xpack.core.ClientHelper; | ||
| import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver; | ||
| import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException; | ||
| import org.elasticsearch.xpack.esql.action.EsqlQueryAction; | ||
|
|
@@ -270,4 +275,17 @@ protected void innerRelease() { | |
| } | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| protected void sendChildRequest( | ||
| CancellableTask parentTask, | ||
| ActionListener<List<Page>> delegate, | ||
| DiscoveryNode targetNode, | ||
| TransportRequest transportRequest | ||
| ) { | ||
| ThreadContext threadContext = transportService.getThreadPool().getThreadContext(); | ||
| try (ThreadContext.StoredContext unused = threadContext.stashWithOrigin(ClientHelper.ENRICH_ORIGIN)) { | ||
|
There was a problem hiding this comment. Can we try and add a comment here with the best understanding that we can gather on what's happening here? I think it's crucial and extremely important for how permissions are handled for ENRICH. My understanding:
@dnhatn , could you maybe confirm that this is happening, or correct me if I'm wrong? There was a problem hiding this comment. @luigidellaquila @alex-spies I thought more about the fix. Can we move AbstractLookupService.lookupAsync public final void lookupAsync(R request, CancellableTask parentTask, ActionListener<List<Page>> listener) {
ClusterState clusterState = clusterService.state();
GroupShardsIterator<ShardIterator> shardIterators = clusterService.operationRouting()
.searchShards(clusterState, new String[] { request.index }, Map.of(), "_local");
if (shardIterators.size() != 1) {
listener.onFailure(new EsqlIllegalArgumentException("target index {} has more than one shard", request.index));
return;
}
ShardIterator shardIt = shardIterators.get(0);
ShardRouting shardRouting = shardIt.nextOrNull();
ShardId shardId = shardIt.shardId();
if (shardRouting == null) {
listener.onFailure(new UnavailableShardsException(shardId, "target index is not available"));
return;
}
DiscoveryNode targetNode = clusterState.nodes().get(shardRouting.currentNodeId());
T transportRequest = transportRequest(request, shardId);
// TODO: handle retry and avoid forking for the local lookup
sendChildRequest(parentTask, listener, targetNode, transportRequest);
}EnrichLookupService#sendChildRequest ThreadContext threadContext = transportService.getThreadPool().getThreadContext();
ActionListener<List<Page>> listener = ContextPreservingActionListener.wrapPreservingContext(outListener, threadContext);
hasEnrichPrivilege(listener.delegateFailureAndWrap((delegate, ignored) -> {
try (ThreadContext.StoredContext unused = threadContext.stashWithOrigin(ClientHelper.ENRICH_ORIGIN)) {
super.sendChildRequest(parentTask, delegate, targetNode, transportRequest);
}
}));There was a problem hiding this comment. Thanks @dnhatn, I agree, it will make things much more clear. Doing it now. There was a problem hiding this comment. 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 |
||
| super.sendChildRequest(parentTask, delegate, targetNode, transportRequest); | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.