Support intersecting multi-sets of queries with DocumentPermissions #91151

ywangd · 2022-10-27T09:20:23Z

Since #81403, the Role class has been able to support multi-levels of limiting (intersections). However, it was an oversight that the underlying DocumentPermissions and FieldPermissions still do not support it. They are still hardcoded to support up to 2 levels of intersection. This PR now updates DocumentPermissions so it can support multi-level of intersections. The similar change for FieldPermissions will be done in a separate PR.

ywangd · 2022-10-27T09:23:28Z

...re/src/main/java/org/elasticsearch/xpack/core/termsenum/action/TransportTermsEnumAction.java

-                Set<BytesReference> queries = indexAccessControl.getDocumentPermissions().getQueries();
-                for (BytesReference querySource : queries) {
-                    QueryBuilder queryBuilder = DLSRoleQueryValidator.evaluateAndVerifyRoleQuery(
-                        querySource,
-                        scriptService,
-                        queryShardContext.getParserConfig().registry(),
-                        securityContext.getUser()
-                    );
-                    QueryBuilder rewrittenQueryBuilder = Rewriteable.rewrite(queryBuilder, queryShardContext);
-                    if (rewrittenQueryBuilder instanceof MatchAllQueryBuilder) {
-                        // One of the roles assigned has "all" permissions - allow unfettered access to termsDict
-                        return true;
+                final List<Set<BytesReference>> listOfRawQueries = indexAccessControl.getDocumentPermissions().getListOfQueries();
+
+                return listOfRawQueries.stream().allMatch(queries -> {
+                    for (BytesReference querySource : queries) {
+                        QueryBuilder queryBuilder = DLSRoleQueryValidator.evaluateAndVerifyRoleQuery(
+                            querySource,
+                            scriptService,
+                            queryShardContext.getParserConfig().registry(),
+                            securityContext.getUser()
+                        );
+                        QueryBuilder rewrittenQueryBuilder;
+                        try {
+                            rewrittenQueryBuilder = Rewriteable.rewrite(queryBuilder, queryShardContext);
+                        } catch (IOException e) {
+                            throw new UncheckedIOException(e);
+                        }
+                        if (rewrittenQueryBuilder instanceof MatchAllQueryBuilder) {
+                            // One of the roles assigned has "all" permissions - allow unfettered access to termsDict
+                            return true;
+                        }
                    }
-                }
-                return false;
+                    return false;
+                });


The change here is also a bug fix. The existing code only checks getQueries() but not getLimitedByQueries. So it can make wrong decisions if the authentication subject is an API key. It is possible to maitain the existing behaviour so that the PR is more of a pure refactoring. But with the changes to DocumentPermissions, maintaining the existing behaviour is rather odd because all queries are now return by a single method.

It is probably helpful to move this logic to DocumentPermissions because it is closely related and better for reuse. But I am leaving it as is now to avoid cascading changes.

Please let me if you prefer it otherwise.

I raised a separate PR for the bug fix #91170 to keep this PR a pure refactoring.

ywangd · 2022-10-27T09:26:45Z

...st/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissionsTests.java

-        final BytesStreamOutput out3 = new BytesStreamOutput();
-        final DocumentPermissions documentPermissions3 = new DocumentPermissions(
-            null,
-            Set.of(
-                new BytesArray("{\"term\":{\"q1\":\"v1\"}}"),
-                new BytesArray("{\"term\":{\"q2\":\"v2\"}}"),
-                new BytesArray("{\"term\":{\"q3\":\"v3\"}}")
+            List.of(
+                Set.of(new BytesArray("{\"term\":{\"q1\":\"v1\"}}")),
+                Set.of(new BytesArray("{\"term\":{\"q2\":\"v2\"}}"), new BytesArray("{\"term\":{\"q3\":\"v3\"}}"))
            )
        );
-        documentPermissions3.buildCacheKey(out3, BytesReference::utf8ToString);
+        documentPermissions2.buildCacheKey(out2, BytesReference::utf8ToString);

        assertThat(Arrays.equals(BytesReference.toBytes(out0.bytes()), BytesReference.toBytes(out1.bytes())), is(false));
        assertThat(Arrays.equals(BytesReference.toBytes(out0.bytes()), BytesReference.toBytes(out2.bytes())), is(false));
-        assertThat(Arrays.equals(BytesReference.toBytes(out0.bytes()), BytesReference.toBytes(out3.bytes())), is(false));


We used to differentiate between queries and limitedByQueries. Therefore two documentPermissions are considered different even if they have the exact same set of queries, but one has it as queries and the other has it as limitedByQueries. This difference is reflected in cacheKey computation. That is why the existing code assert non-equality between out0.bytes() and out3.bytes(). But this difference is not really necessary because intersection does not care about order or where the queries come from. Therefore the current PR removes this difference which is reflected in this test.

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

elasticsearchmachine · 2022-10-27T10:01:34Z

Pinging @elastic/es-security (Team:Security)

ywangd · 2022-10-27T11:55:56Z

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

+            BooleanQuery.Builder filter = new BooleanQuery.Builder();
+            for (int i = listOfEvaluatedQueries.size() - 1; i > 0; i--) {
+                final BooleanQuery.Builder scopedFilter = new BooleanQuery.Builder();
+                buildRoleQuery(shardId, searchExecutionContextProvider, listOfEvaluatedQueries.get(i), scopedFilter);
+                filter.add(scopedFilter.build(), FILTER);
            }
+            // TODO: All role queries should be filter since scoring is not needed
+            buildRoleQuery(shardId, searchExecutionContextProvider, listOfEvaluatedQueries.get(0), filter);


I think all role queries can be add as FILTER. But I am keeping it the same as the exsiting code just in case.

jakelandis

looking good, a few questions and minor comments

jakelandis · 2022-10-27T17:59:53Z

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

    }

    DocumentPermissions(Set<BytesReference> queries) {
-        this(queries, null);
+        if (queries == null || queries.isEmpty()) {
+            throw new IllegalArgumentException("null or empty queries not permitted");


should these (here and below) be assertions instead ? I tend to favor asserting over throwing if you are confident it will never happen without a programmer's bug.

I think so, i.e. it can only be programming bug. The throwing code is carried over from existing code (the filteredBy static method). In general, I also favor assertions. That said, null check is often done with Objects.requireNonNull which is a runtime logic. Empty check is closely related. So I don't mind them to be runtime check. I can also add assert false inside the if block so we can have both. What do you think?

Separately, I'll also further lock down the constructors by making them private. They are only used in tests and should be replaced by convenient methods.

We synced offline and agreed to go with assertions. I also made all constructors private so that instantiation is now only possible with the static methods of filteredBy and limitDocumentPermissions.

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

jakelandis · 2022-10-27T18:17:00Z

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

            }
+            // TODO: All role queries can be filters


not sure i understand this TODO

The existing logic is to have the limiting role (user) as filter. But the key's role as "should". So the query is something like:

{ "bool": { "should": [ _key_queries ], "filter": [ { "bool": { "should": [ _Limiting_queries_ ] } } ] } }

But the top level should clause is not necessary. Since intersection does not really care about order and does not treat any set of the queries specially, the TODO is proposing to change the above to:

{ "bool": { "filter": [ { "bool": { "should": [ _key_queries ] } } { "bool": { "should": [ _Limiting_queries_ ] } } ] } }

This also helps simplifying the query building logic, i.e. not treating the 1st set any specially..

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

jakelandis · 2022-10-27T18:28:03Z

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

        return new DocumentPermissions(queries);
    }

    /**
-     * Create {@link DocumentPermissions} with no restriction. The {@link #getQueries()}
+     * Create {@link DocumentPermissions} with no restriction. The {@link #getListOfQueries()}


not related to the change here ...but i think this javadoc is wrong, would you mind nuking it ?

Why do you think it is wrong? The word Create is not particularly accurate because the method always returns the same instance. But otherwise it is still accurate?

The javadoc is not really for this method since it describes how other methods work. It's not a big deal, just confusing.

IMO the javadoc should read

Returns a {@link DocumentPermissions} with no restrictions. This is equivalent to not having any document level permissions.

(or just not exist since it kinda self evident)
Also, outside the scope of this change, so feel free to disregard this comment.

Ah ok. Thanks for explaining this further. I see your point now. I removed the javadoc since it is indeed self evident.

...re/src/main/java/org/elasticsearch/xpack/core/termsenum/action/TransportTermsEnumAction.java

jakelandis · 2022-10-27T18:40:06Z

Can you provide a logical view of before and after (expressed as the query DSL or the like) ?

For example, what say your role (limited by) for queried index results in the following associated document level query

"query" : {
        "term" : { "a" : 1 }
      }

and

"query" : {
        "term" : { "b" : 2 }
      }

and then assume you are using an API key with the following document level query in it's role descriptor

"query" : {
        "term" : { "c" : 3 }
      }

What is the resultant query before and after this change ?

Does order matter between the two groups ?

ywangd · 2022-10-27T23:23:02Z

Can you provide a logical view of before and after (expressed as the query DSL or the like) ?

We sync'd offline on this topic. To summarize it here, the most fundamental change of this PR is replacing the following two instance variables:

Set<> queries
Set<> limitedByQueries

with a single instance variables

List<Set<>> listOfQueries

How these queries are combined intra or inter the Sets does not change at all. That is, the effective query at the end is still something like:

queries.reduce(or) && limitedByQueries.reduce(or)

the equivalent to the above after this PR will be:

listOfQueries[0].reduce(or) && listOfQueries[1].reduce(or)

Concretely, if the user's role has two queries Q1, Q2, and the key's role has one query Q3, the effective query is

(Q1 || Q2) && Q3

This combination logic is unchanged. When there are only two sets of queries, e.g. for API keys, the proposed change is rather superficial. What it really does is to prepare the class ready for multi-set of queries (more than 2), which is the intention of the PR and the reason for having the a List of Sets instead of two hard-coded Sets.

…s-fls

jakelandis

LGTM

jakelandis · 2022-10-28T15:09:41Z

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

        return new DocumentPermissions(queries);
    }

    /**
-     * Create {@link DocumentPermissions} with no restriction. The {@link #getQueries()}
+     * Create {@link DocumentPermissions} with no restriction. The {@link #getListOfQueries()}


The javadoc is not really for this method since it describes how other methods work. It's not a big deal, just confusing.

IMO the javadoc should read

Returns a {@link DocumentPermissions} with no restrictions. This is equivalent to not having any document level permissions.

(or just not exist since it kinda self evident)
Also, outside the scope of this change, so feel free to disregard this comment.

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

jakelandis · 2022-10-28T15:19:06Z

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java

-        } else {
-            out.writeBoolean(false);
+        if (false == hasDocumentLevelPermissions()) {
+            assert false;


as mentioned in person , probably more of style thing, but I not a fan of both assertions and throwing. If you keep both, can you add a comment to the assertion.

I changed it to just the assertion since the existing code was just an assertion. Thanks!

...re/src/main/java/org/elasticsearch/xpack/core/termsenum/action/TransportTermsEnumAction.java

…s-fls

ywangd · 2022-11-02T00:00:17Z

@elasticmachine update branch

…s-fls

ywangd · 2022-11-02T03:25:13Z

@elasticmachine run elasticsearch-ci/packaging-tests-windows-sample

This PR is the 2nd half of updating DocumentPermissions and FieldPermissions to support multi-level of limiting similar to LimitedRole (since #81403). Instead of hard coding fieldsDefinition and limitedByFieldsDefinition, this PR replaces them with a list of fieldsDefinitions which can accomodate multiple of them (more than 2). Relates: #91151

* main: (1300 commits) update c2id/c2id-server-demo docker image to support ARM (elastic#91144) Allow legacy index settings on legacy indices (elastic#90264) Skip prevoting if single-node discovery (elastic#91255) Chunked encoding for snapshot status API (elastic#90801) Allow different decay values depending on the score function (elastic#91195) Fix handling indexed envelopes crossing the dateline in mvt API (elastic#91105) Ensure cleanups succeed in JoinValidationService (elastic#90601) Add overflow behaviour test for RecyclerBytesStreamOutput (elastic#90638) More actionable error for ancient indices (elastic#91243) Fix APM configuration file delete (elastic#91058) Clean up handshake test class (elastic#90966) Improve H3#hexRing logic and add H3#areNeighborCells method (elastic#91140) Restrict direct use of `ApplicationPrivilege` constructor (elastic#91176) [ML] Allow NLP truncate option to be updated when span is set (elastic#91224) Support multi-intersection for FieldPermissions (elastic#91169) Support intersecting multi-sets of queries with DocumentPermissions (elastic#91151) Ensure TermsEnum action works correctly with API keys (elastic#91170) Fix NPE in auditing authenticationSuccess for non-existing run-as user (elastic#91171) Ensure PKI's delegated_by_realm metadata respect run-as (elastic#91173) [ML] Update API documentation for anomaly score explanation (elastic#91177) ... # Conflicts: # x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackClientPlugin.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/RollupShardIndexer.java # x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/downsample/TransportRollupIndexerAction.java # x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/v2/RollupActionSingleNodeTests.java

ywangd added 3 commits October 27, 2022 16:28

wip: production code

887241b

wip: fix test compliation, not running yet

8e56512

Fix tests

d3e4ba9

elasticsearchmachine added the v8.6.0 label Oct 27, 2022

ywangd commented Oct 27, 2022

View reviewed changes

...rc/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java Outdated Show resolved Hide resolved

ywangd marked this pull request as ready for review October 27, 2022 10:00

elasticsearchmachine added the needs:triage Requires assignment of a team area label label Oct 27, 2022

ywangd added >refactoring :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC and removed needs:triage Requires assignment of a team area label labels Oct 27, 2022

ywangd requested a review from jakelandis October 27, 2022 10:01

elasticsearchmachine added the Team:Security Meta label for security team label Oct 27, 2022

ywangd commented Oct 27, 2022

View reviewed changes

tweak

6b87003

jakelandis reviewed Oct 27, 2022

View reviewed changes

ywangd added 2 commits October 28, 2022 08:42

address feedback

42822c5

address feedback

8283578

ywangd requested a review from jakelandis October 27, 2022 23:23

Merge remote-tracking branch 'origin/main' into multi-intersection-dl…

641c5cd

…s-fls

ywangd mentioned this pull request Oct 28, 2022

Support multi-intersection for FieldPermissions #91169

Merged

jakelandis approved these changes Oct 28, 2022

View reviewed changes

ywangd added 3 commits October 29, 2022 09:08

address feedback

4db2f6e

Merge remote-tracking branch 'origin/main' into multi-intersection-dl…

6266edf

…s-fls

fix test

df16c1b

elasticmachine and others added 2 commits November 1, 2022 20:00

Merge branch 'main' into multi-intersection-dls-fls

4c5ce97

Merge remote-tracking branch 'origin/main' into multi-intersection-dl…

4c95ae7

…s-fls

ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Nov 2, 2022

elasticsearchmachine merged commit 1465b99 into elastic:main Nov 2, 2022

ywangd deleted the multi-intersection-dls-fls branch November 2, 2022 04:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support intersecting multi-sets of queries with DocumentPermissions #91151

Support intersecting multi-sets of queries with DocumentPermissions #91151

ywangd commented Oct 27, 2022

ywangd Oct 27, 2022 •

edited

Loading

ywangd Oct 28, 2022

ywangd Oct 27, 2022

elasticsearchmachine commented Oct 27, 2022

ywangd Oct 27, 2022

jakelandis left a comment

jakelandis Oct 27, 2022

ywangd Oct 27, 2022

ywangd Oct 27, 2022

jakelandis Oct 27, 2022

ywangd Oct 27, 2022

jakelandis Oct 27, 2022

ywangd Oct 27, 2022

jakelandis Oct 28, 2022

ywangd Oct 28, 2022

jakelandis commented Oct 27, 2022

ywangd commented Oct 27, 2022

jakelandis left a comment

jakelandis Oct 28, 2022

jakelandis Oct 28, 2022

ywangd Oct 28, 2022

ywangd commented Nov 2, 2022

ywangd commented Nov 2, 2022

Support intersecting multi-sets of queries with DocumentPermissions #91151

Support intersecting multi-sets of queries with DocumentPermissions #91151

Conversation

ywangd commented Oct 27, 2022

ywangd Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Oct 27, 2022

Choose a reason for hiding this comment

jakelandis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakelandis commented Oct 27, 2022

ywangd commented Oct 27, 2022

jakelandis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Nov 2, 2022

ywangd commented Nov 2, 2022

ywangd Oct 27, 2022 •

edited

Loading