ESQL: LOOKUP JOIN security tests and privileges (#118447)

ivancea · web-flow · commit 8dbb25df5aaa · 2024-12-24T11:10:19.000+01:00
- Remove Enrich privilege for LOOKUP JOIN
- Add security tests to lookup, to ensure they work/fail depending on user roles and privileges
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
@@ -30,6 +30,7 @@
 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 @@ private void indexDocument(String index, int id, double value, String org) throw
 
     @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 void indexDocuments() throws IOException {
         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 void indexDocuments() throws IOException {
                             }
                           }
                         },
+                        {
+                          "add": {
+                            "alias": "lookup-first-alias",
+                            "index": "lookup-user1",
+                            "filter": {
+                                "term": {
+                                    "org": "sales"
+                                }
+                            }
+                          }
+                        },
                         {
                           "add": {
                             "alias": "second-alias",
@@ -229,22 +253,30 @@ public void testAliasFilter() throws Exception {
     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 @@ record Listen(long timestamp, String songId, double duration) {
         }
     }
 
+    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<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(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
@@ -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
@@ -132,7 +132,6 @@
  */
 abstract class AbstractLookupService<R extends AbstractLookupService.Request, T extends AbstractLookupService.TransportRequest> {
     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<R extends AbstractLookupService.Request, T
 
     AbstractLookupService(
         String actionName,
-        String privilegeName,
         ClusterService clusterService,
         SearchService searchService,
         TransportService transportService,
@@ -152,7 +150,6 @@ abstract class AbstractLookupService<R extends AbstractLookupService.Request, T
         CheckedBiFunction<StreamInput, BlockFactory, T, IOException> readRequest
     ) {
         this.actionName = actionName;
-        this.privilegeName = privilegeName;
         this.clusterService = clusterService;
         this.searchService = searchService;
         this.transportService = transportService;
@@ -237,9 +234,21 @@ public final void lookupAsync(R request, CancellableTask parentTask, ActionListe
         }));
     }
 
+    /**
+     * Get the privilege required to perform the lookup.
+     * <p>
+     *     If null is returned, no privilege check will be performed.
+     * </p>
+     */
+    @Nullable
+    protected abstract String getRequiredPrivilege();
+
     private void hasPrivilege(ActionListener<Void> 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
@@ -52,16 +52,7 @@ public EnrichLookupService(
         BigArrays bigArrays,
         BlockFactory blockFactory
     ) {
-        super(
-            LOOKUP_ACTION_NAME,
-            ClusterPrivilegeResolver.MONITOR_ENRICH.name(),
-            clusterService,
-            searchService,
-            transportService,
-            bigArrays,
-            blockFactory,
-            TransportRequest::readFrom
-        );
+        super(LOOKUP_ACTION_NAME, clusterService, searchService, transportService, bigArrays, blockFactory, TransportRequest::readFrom);
     }
 
     @Override
@@ -90,6 +81,11 @@ protected QueryList queryList(TransportRequest request, SearchExecutionContext c
         };
     }
 
+    @Override
+    protected String getRequiredPrivilege() {
+        return ClusterPrivilegeResolver.MONITOR_ENRICH.name();
+    }
+
     private static void validateTypes(DataType inputDataType, MappedFieldType fieldType) {
         if (fieldType instanceof RangeFieldMapper.RangeFieldType rangeType) {
             // For range policy types, the ENRICH index field type will be one of a list of supported range types,
diff --git a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexService.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/enrich/LookupFromIndexService.java
@@ -23,7 +23,6 @@
 import org.elasticsearch.search.SearchService;
 import org.elasticsearch.tasks.TaskId;
 import org.elasticsearch.transport.TransportService;
-import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver;
 import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
 import org.elasticsearch.xpack.esql.action.EsqlQueryAction;
 import org.elasticsearch.xpack.esql.core.expression.NamedExpression;
@@ -50,16 +49,7 @@ public LookupFromIndexService(
         BigArrays bigArrays,
         BlockFactory blockFactory
     ) {
-        super(
-            LOOKUP_ACTION_NAME,
-            ClusterPrivilegeResolver.MONITOR_ENRICH.name(), // TODO some other privilege
-            clusterService,
-            searchService,
-            transportService,
-            bigArrays,
-            blockFactory,
-            TransportRequest::readFrom
-        );
+        super(LOOKUP_ACTION_NAME, clusterService, searchService, transportService, bigArrays, blockFactory, TransportRequest::readFrom);
     }
 
     @Override
@@ -83,6 +73,11 @@ protected QueryList queryList(TransportRequest request, SearchExecutionContext c
         return termQueryList(fieldType, context, inputBlock, inputDataType);
     }
 
+    @Override
+    protected String getRequiredPrivilege() {
+        return null;
+    }
+
     private static void validateTypes(DataType inputDataType, MappedFieldType fieldType) {
         // TODO: consider supporting implicit type conversion as done in ENRICH for some types
         if (fieldType.typeName().equals(inputDataType.typeName()) == false) {
