Skip to content

Commit 3abf88e

Browse files
Fix multi-index FGAC routing and add bypass regression tests
SQL plugin routing fix: - RestUnifiedQueryAction.isAnalyticsIndex() now splits comma-separated index names and checks each independently. Routes to analytics engine only if ALL indices are composite. Previously, the joined string 'idx1,idx2' was looked up as a single index in cluster metadata, causing multi-index queries to fall through to the legacy pipeline. Regression tests: - testPPLMultiIndexDeniedWhenSecondIndexUnauthorized - testPPLMultiIndexDeniedWithBackticksAuthorizedFirst - testPPLMultiIndexDeniedWithUnauthorizedFirst - testPPLMultiIndexAllowedWhenAllAuthorized Also fixes plugin install order (composite-engine before backends). Signed-off-by: Finnegan Carroll <carrofin@amazon.com> Signed-off-by: Finn Carroll <carrofin@amazon.com>
1 parent 1dc92d6 commit 3abf88e

3 files changed

Lines changed: 157 additions & 2 deletions

File tree

integ-test/build.gradle

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -502,11 +502,11 @@ testClusters.analyticsEngineSecurityIT {
502502
plugin(getJobSchedulerPlugin())
503503
plugin(getArrowBasePlugin())
504504
plugin(getArrowFlightRpcPlugin())
505+
plugin(getCompositeEnginePlugin())
505506
plugin(getAnalyticsEnginePlugin())
506507
plugin(getAnalyticsBackendLucenePlugin())
507508
plugin(getAnalyticsBackendDatafusionPlugin())
508509
plugin(getParquetDataFormatPlugin())
509-
plugin(getCompositeEnginePlugin())
510510
plugin ":opensearch-sql-plugin"
511511
// Arrow Flight / streaming transport requirements
512512
jvmArgs '--add-opens=java.base/java.nio=ALL-UNNAMED'

integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -393,6 +393,94 @@ public void testPPLQueryWithWildcardIndexPartialAccessDenied() throws IOExceptio
393393
assertEquals(403, e.getResponse().getStatusLine().getStatusCode());
394394
}
395395

396+
// --- Multi-index comma-separated source tests (FGAC bypass regression) ---
397+
398+
@Test
399+
public void testPPLMultiIndexDeniedWhenSecondIndexUnauthorized() throws IOException {
400+
ResponseException e =
401+
assertThrows(
402+
ResponseException.class,
403+
() ->
404+
executePPLAsUser(
405+
"source = " + TEST_INDEX + ", " + FORBIDDEN_INDEX + " | fields name, age",
406+
ALLOWED_USER));
407+
assertEquals(403, e.getResponse().getStatusLine().getStatusCode());
408+
}
409+
410+
@Test
411+
public void testPPLMultiIndexDeniedWithBackticksAuthorizedFirst() throws IOException {
412+
ResponseException e =
413+
assertThrows(
414+
ResponseException.class,
415+
() ->
416+
executePPLAsUser(
417+
"source = `" + TEST_INDEX + "`, `" + FORBIDDEN_INDEX + "` | fields name, age",
418+
ALLOWED_USER));
419+
assertEquals(403, e.getResponse().getStatusLine().getStatusCode());
420+
}
421+
422+
@Test
423+
public void testPPLMultiIndexDeniedWithUnauthorizedFirst() throws IOException {
424+
ResponseException e =
425+
assertThrows(
426+
ResponseException.class,
427+
() ->
428+
executePPLAsUser(
429+
"source = " + FORBIDDEN_INDEX + ", " + TEST_INDEX + " | fields name, age",
430+
ALLOWED_USER));
431+
assertEquals(403, e.getResponse().getStatusLine().getStatusCode());
432+
}
433+
434+
@Test
435+
public void testPPLMultiIndexAllowedWhenAllAuthorized() throws IOException {
436+
try {
437+
JSONObject result =
438+
executePPLAsUser(
439+
"source = " + TEST_INDEX + ", " + TEST_INDEX_2 + " | fields name, age",
440+
WILDCARD_USER);
441+
assertTrue("Expected datarows in response", result.has("datarows"));
442+
} catch (ResponseException e) {
443+
assertNotEquals(
444+
"Expected auth to pass (not 403) when all indices are authorized",
445+
403,
446+
e.getResponse().getStatusLine().getStatusCode());
447+
}
448+
}
449+
450+
// --- Edge cases: malformed comma-separated source patterns ---
451+
452+
@Test
453+
public void testPPLDoubleCommaRejected() throws IOException {
454+
ResponseException e =
455+
assertThrows(
456+
ResponseException.class,
457+
() ->
458+
executePPLAsUser(
459+
"source = " + TEST_INDEX + ",," + FORBIDDEN_INDEX + " | fields name, age",
460+
ALLOWED_USER));
461+
assertEquals(400, e.getResponse().getStatusLine().getStatusCode());
462+
}
463+
464+
@Test
465+
public void testPPLLeadingCommaRejected() throws IOException {
466+
ResponseException e =
467+
assertThrows(
468+
ResponseException.class,
469+
() ->
470+
executePPLAsUser("source = ," + TEST_INDEX + " | fields name, age", ALLOWED_USER));
471+
assertEquals(400, e.getResponse().getStatusLine().getStatusCode());
472+
}
473+
474+
@Test
475+
public void testPPLTrailingCommaRejected() throws IOException {
476+
ResponseException e =
477+
assertThrows(
478+
ResponseException.class,
479+
() ->
480+
executePPLAsUser("source = " + TEST_INDEX + ", | fields name, age", ALLOWED_USER));
481+
assertEquals(400, e.getResponse().getStatusLine().getStatusCode());
482+
}
483+
396484
@Test
397485
public void testSQLQueryAllowedForAuthorizedUser() throws IOException {
398486
try {
@@ -441,6 +529,51 @@ public void testSQLQueryDeniedWithSearchPermissionOnly() throws IOException {
441529
assertEquals(403, e.getResponse().getStatusLine().getStatusCode());
442530
}
443531

532+
// --- SQL multi-index syntax validation ---
533+
534+
@Test
535+
public void testSQLMultiIndexCommaInFromRejected() throws IOException {
536+
// SQL FROM "idx1,idx2" — comma inside identifier, should be rejected as syntax error
537+
ResponseException e =
538+
assertThrows(
539+
ResponseException.class,
540+
() ->
541+
executeSQLAsUser(
542+
"SELECT name, age FROM " + TEST_INDEX + "," + FORBIDDEN_INDEX + " LIMIT 3",
543+
ALLOWED_USER));
544+
assertEquals(400, e.getResponse().getStatusLine().getStatusCode());
545+
}
546+
547+
@Test
548+
public void testSQLMultiIndexCrossJoinRejected() throws IOException {
549+
// SQL cross join syntax — should not bypass FGAC
550+
ResponseException e =
551+
assertThrows(
552+
ResponseException.class,
553+
() ->
554+
executeSQLAsUser(
555+
"SELECT a.name FROM " + TEST_INDEX + " a, " + FORBIDDEN_INDEX + " b LIMIT 3",
556+
ALLOWED_USER));
557+
assertEquals(400, e.getResponse().getStatusLine().getStatusCode());
558+
}
559+
560+
@Test
561+
public void testSQLMultiIndexJoinRejected() throws IOException {
562+
// Explicit JOIN — should not bypass FGAC
563+
ResponseException e =
564+
assertThrows(
565+
ResponseException.class,
566+
() ->
567+
executeSQLAsUser(
568+
"SELECT a.name FROM "
569+
+ TEST_INDEX
570+
+ " a JOIN "
571+
+ FORBIDDEN_INDEX
572+
+ " b ON a.name = b.name LIMIT 3",
573+
ALLOWED_USER));
574+
assertEquals(400, e.getResponse().getStatusLine().getStatusCode());
575+
}
576+
444577
/** Executes a PPL query via the production SQL plugin endpoint (/_plugins/_ppl). */
445578
private JSONObject executePPLAsUser(String query, String username) throws IOException {
446579
Request request = new Request("POST", "/_plugins/_ppl");

plugin/src/main/java/org/opensearch/sql/plugin/rest/RestUnifiedQueryAction.java

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,35 @@ public boolean isAnalyticsIndex(String query, QueryType queryType) {
122122
try (UnifiedQueryContext context = buildParsingContext(queryType)) {
123123
return extractIndexName(query, queryType, context)
124124
.map(this::stripSchemaPrefix)
125-
.map(this::isPluggableDataformatIndex)
125+
.map(this::allIndicesArePluggableDataformat)
126126
.orElse(false);
127127
} catch (Exception e) {
128128
return false;
129129
}
130130
}
131131

132+
/**
133+
* Checks if all indices in a (possibly comma-separated) index expression are pluggable
134+
* dataformat. For multi-index queries (source=idx1,idx2), each index is checked independently.
135+
* Returns true only if every index is composite — mixed or all-lucene returns false.
136+
*/
137+
private boolean allIndicesArePluggableDataformat(String indexExpression) {
138+
String[] indices = indexExpression.split(",");
139+
if (indices.length == 0) {
140+
return false;
141+
}
142+
for (String idx : indices) {
143+
String trimmed = idx.trim();
144+
if (trimmed.isEmpty()) {
145+
continue;
146+
}
147+
if (!isPluggableDataformatIndex(trimmed)) {
148+
return false;
149+
}
150+
}
151+
return true;
152+
}
153+
132154
private static boolean isSystemCatalog(String name) {
133155
return SystemIndexUtils.isSystemIndex(name)
134156
|| SystemIndexUtils.DATASOURCES_TABLE_NAME.equals(name);

0 commit comments

Comments
 (0)