From 8dbb25df5aaa0bc4fc1ebbc3934bfa30ef360b92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Cea=20Fontenla?= Date: Tue, 24 Dec 2024 11:10:19 +0100 Subject: [PATCH] ESQL: LOOKUP JOIN security tests and privileges (#118447) - Remove Enrich privilege for LOOKUP JOIN - Add security tests to lookup, to ensure they work/fail depending on user roles and privileges --- .../xpack/esql/EsqlSecurityIT.java | 91 ++++++++++++++++++- .../src/javaRestTest/resources/roles.yml | 6 +- .../esql/enrich/AbstractLookupService.java | 17 +++- .../esql/enrich/EnrichLookupService.java | 16 ++-- .../esql/enrich/LookupFromIndexService.java | 17 ++-- 5 files changed, 118 insertions(+), 29 deletions(-) diff --git a/x-pack/plugin/esql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/esql/EsqlSecurityIT.java b/x-pack/plugin/esql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/esql/EsqlSecurityIT.java index 9566aeb8f28d..d9f211405e23 100644 --- a/x-pack/plugin/esql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/esql/EsqlSecurityIT.java +++ b/x-pack/plugin/esql/qa/security/src/javaRestTest/java/org/elasticsearch/xpack/esql/EsqlSecurityIT.java @@ -30,6 +30,7 @@ import org.junit.Before; import org.junit.ClassRule; import java.io.IOException; +import java.util.Arrays; import java.util.List; import java.util.Locale; import java.util.Map; @@ -87,9 +88,11 @@ public class EsqlSecurityIT extends ESRestTestCase { @Before public void indexDocuments() throws IOException { + Settings lookupSettings = Settings.builder().put("index.mode", "lookup").build(); String mapping = """ "properties":{"value": {"type": "double"}, "org": {"type": "keyword"}} """; + createIndex("index", Settings.EMPTY, mapping); indexDocument("index", 1, 10.0, "sales"); indexDocument("index", 2, 20.0, "engineering"); @@ -110,6 +113,16 @@ public class EsqlSecurityIT extends ESRestTestCase { indexDocument("indexpartial", 2, 40.0, "sales"); refresh("indexpartial"); + createIndex("lookup-user1", lookupSettings, mapping); + indexDocument("lookup-user1", 1, 12.0, "engineering"); + indexDocument("lookup-user1", 2, 31.0, "sales"); + refresh("lookup-user1"); + + createIndex("lookup-user2", lookupSettings, mapping); + indexDocument("lookup-user2", 1, 32.0, "marketing"); + indexDocument("lookup-user2", 2, 40.0, "sales"); + refresh("lookup-user2"); + if (aliasExists("second-alias") == false) { Request aliasRequest = new Request("POST", "_aliases"); aliasRequest.setJsonEntity(""" @@ -126,6 +139,17 @@ public class EsqlSecurityIT extends ESRestTestCase { } } }, + { + "add": { + "alias": "lookup-first-alias", + "index": "lookup-user1", + "filter": { + "term": { + "org": "sales" + } + } + } + }, { "add": { "alias": "second-alias", @@ -229,22 +253,30 @@ public class EsqlSecurityIT extends ESRestTestCase { public void testUnauthorizedIndices() throws IOException { ResponseException error; error = expectThrows(ResponseException.class, () -> runESQLCommand("user1", "from index-user2 | stats sum(value)")); + assertThat(error.getMessage(), containsString("Unknown index [index-user2]")); assertThat(error.getResponse().getStatusLine().getStatusCode(), equalTo(400)); error = expectThrows(ResponseException.class, () -> runESQLCommand("user2", "from index-user1 | stats sum(value)")); + assertThat(error.getMessage(), containsString("Unknown index [index-user1]")); assertThat(error.getResponse().getStatusLine().getStatusCode(), equalTo(400)); error = expectThrows(ResponseException.class, () -> runESQLCommand("alias_user2", "from index-user2 | stats sum(value)")); + assertThat(error.getMessage(), containsString("Unknown index [index-user2]")); assertThat(error.getResponse().getStatusLine().getStatusCode(), equalTo(400)); error = expectThrows(ResponseException.class, () -> runESQLCommand("metadata1_read2", "from index-user1 | stats sum(value)")); + assertThat(error.getMessage(), containsString("Unknown index [index-user1]")); assertThat(error.getResponse().getStatusLine().getStatusCode(), equalTo(400)); } public void testInsufficientPrivilege() { - Exception error = expectThrows(Exception.class, () -> runESQLCommand("metadata1_read2", "FROM index-user1 | STATS sum=sum(value)")); + ResponseException error = expectThrows( + ResponseException.class, + () -> runESQLCommand("metadata1_read2", "FROM index-user1 | STATS sum=sum(value)") + ); logger.info("error", error); assertThat(error.getMessage(), containsString("Unknown index [index-user1]")); + assertThat(error.getResponse().getStatusLine().getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST)); } public void testIndexPatternErrorMessageComparison_ESQL_SearchDSL() throws Exception { @@ -511,6 +543,63 @@ public class EsqlSecurityIT extends ESRestTestCase { } } + public void testLookupJoinIndexAllowed() throws Exception { + Response resp = runESQLCommand( + "metadata1_read2", + "ROW x = 40.0 | EVAL value = x | LOOKUP JOIN `lookup-user2` ON value | KEEP x, org" + ); + assertOK(resp); + Map 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(40.0, "sales")))); + + // Alias, should find the index and the row + resp = runESQLCommand("alias_user1", "ROW x = 31.0 | EVAL value = x | LOOKUP JOIN `lookup-first-alias` 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(31.0, "sales")))); + + // Alias, for a row that's filtered out + resp = runESQLCommand("alias_user1", "ROW x = 123.0 | EVAL value = x | LOOKUP JOIN `lookup-first-alias` 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(123.0, null)))); + } + + public void testLookupJoinIndexForbidden() { + var resp = expectThrows( + ResponseException.class, + () -> runESQLCommand("metadata1_read2", "FROM lookup-user2 | EVAL value = 10.0 | LOOKUP JOIN `lookup-user1` ON value | KEEP x") + ); + assertThat(resp.getMessage(), containsString("Unknown index [lookup-user1]")); + assertThat(resp.getResponse().getStatusLine().getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST)); + + resp = expectThrows( + ResponseException.class, + () -> runESQLCommand("metadata1_read2", "ROW x = 10.0 | EVAL value = x | LOOKUP JOIN `lookup-user1` ON value | KEEP x") + ); + assertThat(resp.getMessage(), containsString("Unknown index [lookup-user1]")); + assertThat(resp.getResponse().getStatusLine().getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST)); + + resp = expectThrows( + ResponseException.class, + () -> runESQLCommand("alias_user1", "ROW x = 10.0 | EVAL value = x | LOOKUP JOIN `lookup-user1` ON value | KEEP x") + ); + assertThat(resp.getMessage(), containsString("Unknown index [lookup-user1]")); + assertThat(resp.getResponse().getStatusLine().getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST)); + } + private void createEnrichPolicy() throws Exception { createIndex("songs", Settings.EMPTY, """ "properties":{"song_id": {"type": "keyword"}, "title": {"type": "keyword"}, "artist": {"type": "keyword"} } diff --git a/x-pack/plugin/esql/qa/security/src/javaRestTest/resources/roles.yml b/x-pack/plugin/esql/qa/security/src/javaRestTest/resources/roles.yml index 365a072edb74..f46e7ef56f3a 100644 --- a/x-pack/plugin/esql/qa/security/src/javaRestTest/resources/roles.yml +++ b/x-pack/plugin/esql/qa/security/src/javaRestTest/resources/roles.yml @@ -35,15 +35,15 @@ user2: metadata1_read2: cluster: [] indices: - - names: [ 'index-user1' ] + - names: [ 'index-user1', 'lookup-user1' ] privileges: [ 'view_index_metadata' ] - - names: [ 'index-user2' ] + - names: [ 'index-user2', 'lookup-user2' ] privileges: [ 'read' ] alias_user1: cluster: [] indices: - - names: [ 'first-alias' ] + - names: [ 'first-alias', 'lookup-first-alias' ] privileges: - read diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/AbstractLookupService.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/AbstractLookupService.java index e52e9ae989a9..74c66c0d1b33 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/AbstractLookupService.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/AbstractLookupService.java @@ -132,7 +132,6 @@ import java.util.stream.IntStream; */ abstract class AbstractLookupService { private final String actionName; - private final String privilegeName; private final ClusterService clusterService; private final SearchService searchService; private final TransportService transportService; @@ -143,7 +142,6 @@ abstract class AbstractLookupService readRequest ) { this.actionName = actionName; - this.privilegeName = privilegeName; this.clusterService = clusterService; this.searchService = searchService; this.transportService = transportService; @@ -237,9 +234,21 @@ abstract class AbstractLookupService + * If null is returned, no privilege check will be performed. + *

+ */ + @Nullable + protected abstract String getRequiredPrivilege(); + private void hasPrivilege(ActionListener outListener) { final Settings settings = clusterService.getSettings(); - if (settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey()) == false || XPackSettings.SECURITY_ENABLED.get(settings) == false) { + String privilegeName = getRequiredPrivilege(); + if (privilegeName == null + || settings.hasValue(XPackSettings.SECURITY_ENABLED.getKey()) == false + || XPackSettings.SECURITY_ENABLED.get(settings) == false) { outListener.onResponse(null); return; } diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichLookupService.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichLookupService.java index 50a1ffce4841..7057b586871e 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichLookupService.java +++ b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/EnrichLookupService.java @@ -52,16 +52,7 @@ public class EnrichLookupService extends AbstractLookupService