Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Jan 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8254efb
ES|QL: Fix queries with document level security on lookup indexes
luigidellaquila Jan 22, 2025
0427012
Merge branch 'main' into esql/fix_lookup_dls
luigidellaquila Jan 22, 2025
c40d800
Skip tests with alias lookup indexes
luigidellaquila Jan 22, 2025
edab3ec
Skip tests
luigidellaquila Jan 22, 2025
d0b5512
Update docs/changelog/120617.yaml
luigidellaquila Jan 22, 2025
1cdce58
Skip bwc tests
luigidellaquila Jan 22, 2025
c003b61
Merge remote-tracking branch 'luigidellaquila/esql/fix_lookup_dls' in…
luigidellaquila Jan 22, 2025
129133b
Merge branch 'main' into esql/fix_lookup_dls
luigidellaquila Jan 22, 2025
b3408fc
Mute tests (again)
luigidellaquila Jan 22, 2025
5d377a8
Use PostAnalysisVerificationAware
luigidellaquila Jan 22, 2025
f16494a
Implement review suggestions
luigidellaquila Jan 23, 2025
151e4ed
Merge branch 'main' into esql/fix_lookup_dls
luigidellaquila Jan 23, 2025
a0bf736
Validation cleanup and test on datastreams
luigidellaquila Jan 23, 2025
23743b9
Merge remote-tracking branch 'luigidellaquila/esql/fix_lookup_dls' in…
luigidellaquila Jan 23, 2025
8040428
Fix yaml tests
luigidellaquila Jan 23, 2025
6d5b21e
Merge branch 'main' into esql/fix_lookup_dls
luigidellaquila Jan 23, 2025
17b40f8
Simplify datastream tests
luigidellaquila Jan 23, 2025
ffa5fb9
More tests
luigidellaquila Jan 23, 2025
85b33ff
Merge branch 'main' into esql/fix_lookup_dls
luigidellaquila Jan 23, 2025
72ed0b5
More tests and comments
luigidellaquila Jan 23, 2025
d070ed7
Merge branch 'main' into esql/fix_lookup_dls
luigidellaquila Jan 23, 2025
35cbc91
Merge remote-tracking branch 'luigidellaquila/esql/fix_lookup_dls' in…
luigidellaquila Jan 23, 2025
72328a1
Validation message
luigidellaquila Jan 24, 2025
0b30657
Refactoring of enrich security and more tests
luigidellaquila Jan 24, 2025
43a2d07
Merge branch 'main' into esql/fix_lookup_dls
luigidellaquila Jan 24, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/120617.yaml
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]
6 changes: 6 additions & 0 deletions x-pack/plugin/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -96,5 +96,11 @@ tasks.named("yamlRestCompatTestTransform").configure({ task ->
task.skipTest("esql/180_match_operator/match with non text field", "Match operator can now be used on non-text fields")
task.skipTest("esql/180_match_operator/match with functions", "Error message changed")
task.skipTest("esql/40_unsupported_types/semantic_text declared in mapping", "The semantic text field format changed")
task.skipTest("esql/190_lookup_join/Alias as lookup index", "LOOKUP JOIN does not support index aliases for now")
task.skipTest("esql/190_lookup_join/alias-repeated-alias", "LOOKUP JOIN does not support index aliases for now")
task.skipTest("esql/190_lookup_join/alias-repeated-index", "LOOKUP JOIN does not support index aliases for now")
task.skipTest("esql/190_lookup_join/alias-pattern-multiple", "LOOKUP JOIN does not support index aliases for now")
task.skipTest("esql/190_lookup_join/alias-pattern-single", "LOOKUP JOIN does not support index aliases for now")

})

Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ 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("fls_user4_1", "x-pack-test-password", "fls_user4_1", 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)
Expand Down Expand Up @@ -92,7 +96,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"}}
"properties":{"value": {"type": "double"}, "org": {"type": "keyword"}, "other": {"type": "keyword"}}
""";

createIndex("index", Settings.EMPTY, mapping);
Expand Down Expand Up @@ -163,6 +167,32 @@ 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));

request = new Request("POST", "_security/user/fls_user4");
request.setJsonEntity("""
{
"password" : "x-pack-test-password",
"roles" : [ "fls_user4_1", "fls_user4_2" ],
"full_name" : "Test Role",
"email" : "[email protected]"
}
""");
assertOK(client().performRequest(request));
}

protected MapMatcher responseMatcher(Map<String, Object> result) {
Expand Down Expand Up @@ -553,25 +583,130 @@ public void testLookupJoinIndexAllowed() throws Exception {
);
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");
// 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_V12.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(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");
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(Arrays.asList(123.0, null))));
assertThat(respMap.get("values"), equalTo(List.of(List.of(32.0, "marketing"))));

}

@SuppressWarnings("unchecked")
public void testLookupJoinFieldLevelSecurity() throws Exception {
assumeTrue(
"Requires LOOKUP JOIN capability",
EsqlSpecTestCase.hasCapabilities(adminClient(), List.of(EsqlCapabilities.Cap.JOIN_LOOKUP_V12.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")
)
)

);

resp = runESQLCommand("fls_user4", "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")
)
)
);

ResponseException error = expectThrows(
ResponseException.class,
() -> runESQLCommand("fls_user4_1", "ROW x = 40.0 | EVAL value = x | LOOKUP JOIN lookup-user2 ON value")
);
assertThat(error.getMessage(), containsString("Unknown column [value] in right side of join"));
assertThat(error.getResponse().getStatusLine().getStatusCode(), equalTo(HttpStatus.SC_BAD_REQUEST));
}
Copy link
Contributor

@alex-spies alex-spies Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,53 @@ fls_user:
field_security:
grant: [ value ]

fls_user2:
cluster: []
indices:
- names: [ 'lookup-user2' ]
privileges: [ 'read' ]
field_security:
grant: [ "org", "value" ]

fls_user3:
cluster: []
indices:
- names: [ 'lookup-user2' ]
privileges: [ 'read' ]
field_security:
grant: [ "org", "value", "other" ]

fls_user4_1:
cluster: []
indices:
- names: [ 'lookup-user2' ]
privileges: [ 'read' ]
field_security:
grant: [ "org" ]

fls_user4_2:
cluster: []
indices:
- names: [ 'lookup-user2' ]
privileges: [ 'read' ]
field_security:
grant: [ "value" ]

dls_user:
cluster: []
indices:
- names: [ 'lookup-user2' ]
privileges: [ 'read' ]
query: '{"match": {"org": "marketing"}}'

dls_user2:
cluster: []
indices:
- names: [ 'lookup-user2' ]
privileges: [ 'read' ]
query: '{"match": {"org": "sales"}}'


logs_foo_all:
cluster: []
indices:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,12 @@ public enum Cap {
/**
* Support named argument for function in map format.
*/
OPTIONAL_NAMED_ARGUMENT_MAP_FOR_FUNCTION(Build.current().isSnapshot());
OPTIONAL_NAMED_ARGUMENT_MAP_FOR_FUNCTION(Build.current().isSnapshot()),

/**
* Disabled support for index aliases in lookup joins
*/
LOOKUP_JOIN_NO_ALIASES(JOIN_LOOKUP_V12.isEnabled());

private final boolean enabled;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -237,36 +237,6 @@ private LogicalPlan resolveIndex(UnresolvedRelation plan, IndexResolution indexR

EsIndex esIndex = indexResolution.get();

if (plan.indexMode().equals(IndexMode.LOOKUP)) {
String indexResolutionMessage = null;

var indexNameWithModes = esIndex.indexNameWithModes();
if (indexNameWithModes.size() != 1) {
indexResolutionMessage = "invalid ["
+ table
+ "] resolution in lookup mode to ["
+ indexNameWithModes.size()
+ "] indices";
} else if (indexNameWithModes.values().iterator().next() != IndexMode.LOOKUP) {
indexResolutionMessage = "invalid ["
+ table
+ "] resolution in lookup mode to an index in ["
+ indexNameWithModes.values().iterator().next()
+ "] mode";
}

if (indexResolutionMessage != null) {
return new UnresolvedRelation(
plan.source(),
plan.table(),
plan.frozen(),
plan.metadataFields(),
plan.indexMode(),
indexResolutionMessage,
plan.commandName()
);
}
}
var attributes = mappingAsAttributes(plan.source(), esIndex.mapping());
attributes.addAll(plan.metadataFields());
return new EsRelation(
Expand Down
Loading