Validate `query` field when creating roles #46275

bizybot · 2019-09-03T13:55:22Z

In the current implementation, the validation of the role query
occurs at runtime when the query is being executed.

This commit adds validation for the role query when creating a role
but not for the template query as we do not have the runtime
information required for evaluating the template query (eg. authenticated user's
information). This is similar to the scripts that we
store but do not evaluate or parse if they are valid queries or not.

For validation, the query is evaluated (if not a template), parsed to build the
QueryBuilder and verify if the query type is allowed.

Closes #34252

elasticmachine · 2019-09-03T13:55:24Z

Pinging @elastic/es-security

bizybot · 2019-09-05T04:40:37Z

unrelated failure
@elasticmachine run elasticsearch-ci/2

As of now the validation occurs at runtime when the query is being executed. I think the reason was due to the use of template queries which need runtime information as they need to be evaluated like user information. This commit adds validation for the role query but **not for the template query** as we do not have the runtime information required for evaluating the template query. This also corrects some tests and roles.yml files where the `query` field was not populated correctly. For validation, the query is evaluated (if not a template), parsed to build the `QueryBuilder` and verify if the query type is allowed. Closes elastic#34252

bizybot · 2019-09-05T09:37:24Z

@elasticmachine run elasticsearch-ci/2

jkakavas

Looks good @bizybot , left some comments

jkakavas · 2019-09-10T08:35:15Z

...src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java

+                }
+            }
+        } catch (XContentParseException | IOException e) {
+            throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e);


consider changing the error message here, we wouldn't trying parse the query. WDYT ?

changed the message here, Thank you.

...src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java

...src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java

jkakavas · 2019-09-10T09:14:14Z

...src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java

+import java.util.List;
+
+/**
+ * This class helps in evaluating the query field if it is template


it evaluates non template queries also, right ?

So there are two validation methods one that validates query (validateQueryField) and another one that evaluates a query if it is a template and then performs validation(validateAndVerifyRoleQuery).
validateQueryField cannot evaluate the template query so skips the validation for it as the runtime information is not available.
I have corrected the documentation for validateQueryField which was incorrect. Thank you.

- add index privileg index to the error message to denote query of which index privilege failed to validate. - add more information to the error message where we know the expected values

tvernum · 2019-09-12T06:18:09Z

client/rest-high-level/src/test/java/org/elasticsearch/client/SecurityIT.java

@@ -190,7 +190,7 @@ private static Role randomRole(String roleName) {
                .name(roleName)
                .clusterPrivileges(randomSubsetOf(randomInt(3), Role.ClusterPrivilegeName.ALL_ARRAY))
                .indicesPrivileges(
-                        randomArray(3, IndicesPrivileges[]::new, () -> IndicesPrivilegesTests.createNewRandom(randomAlphaOfLength(3))))
+                        randomArray(3, IndicesPrivileges[]::new, () -> IndicesPrivilegesTests.createNewRandom("{\"match_all\": {}}")))


If we really want this to be a random role then may this would be better as

Suggested change

randomArray(3, IndicesPrivileges[]::new, () -> IndicesPrivilegesTests.createNewRandom("{\"match_all\": {}}")))

randomArray(3, IndicesPrivileges[]::new, () -> IndicesPrivilegesTests.createNewRandom(

"{\"term\":{ \"" + randomAlphaOfLength(5) + "\" : \"" + randomAlphaOfLength(3) + "\"}}")))

But maybe that's overkill, I didn't really look at where we use this method.

Since this was an IT testing put role, we were failing due to validation, earlier the query was invalid.
I don't think we need to create a random query here, the createNewRandom is being used in unit tests mostly. But if you feel that we should have a random query string generated then we can do something here. Thank you.

...src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java

...java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java

...src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java

legrego · 2019-09-12T11:26:34Z

I think adding a new API only for validation is something we might not want to do as it is very specific to the presentation layer and we have avoided doing that till now.

Yep, I assumed that was the case, but there's no harm in asking 😄

Few things UI could do more:
validate if the query JSON is a valid JSON. For example, if the query is { malformed }

Yeah, this is a quick win for us, good suggestion.

another suggestion would be to use the query completion which is present on the dev tools? I think this could be useful, though the query completion may allow query names that are not supported for DLS but you will get an error from the backend.

I think this would be ideal, but that would be quite a bit of work for us, and not something I could take on in the near future.

- public to package protected method - return immediately for simpler methods - correct the logic in file roles and add tests

… DLS query

...src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java

In the current implementation, the validation of the role query occurs at runtime when the query is being executed. This commit adds validation for the role query when creating a role but not for the template query as we do not have the runtime information required for evaluating the template query (eg. authenticated user's information). This is similar to the scripts that we store but do not evaluate or parse if they are valid queries or not. For validation, the query is evaluated (if not a template), parsed to build the QueryBuilder and verify if the query type is allowed. Closes elastic#34252

In the current implementation, the validation of the role query occurs at runtime when the query is being executed. This commit adds validation for the role query when creating a role but not for the template query as we do not have the runtime information required for evaluating the template query (eg. authenticated user's information). This is similar to the scripts that we store but do not evaluate or parse if they are valid queries or not. For validation, the query is evaluated (if not a template), parsed to build the QueryBuilder and verify if the query type is allowed. Closes #34252

When creating a role, we do not check if the exceptions for the field permissions are a subset of granted fields. If such a role is assigned to a user then that user's authentication fails for this reason. We added a check to validate role query in #46275 and on the same lines, this commit adds check if the exceptions for the field permissions is a subset of granted fields when parsing the index privileges from the role descriptor. Co-authored-by: Yogesh Gaikwad <bizybot@users.noreply.github.com>

When creating a role, we do not check if the exceptions for the field permissions are a subset of granted fields. If such a role is assigned to a user then that user's authentication fails for this reason. We added a check to validate role query in elastic#46275 and on the same lines, this commit adds check if the exceptions for the field permissions is a subset of granted fields when parsing the index privileges from the role descriptor. Co-authored-by: Yogesh Gaikwad <bizybot@users.noreply.github.com> Backport of: elastic#50212

When creating a role, we do not check if the exceptions for the field permissions are a subset of granted fields. If such a role is assigned to a user then that user's authentication fails for this reason. We added a check to validate role query in #46275 and on the same lines, this commit adds check if the exceptions for the field permissions is a subset of granted fields when parsing the index privileges from the role descriptor. Backport of: #50212 Co-authored-by: Yogesh Gaikwad <bizybot@users.noreply.github.com>

When creating a role, we do not check if the exceptions for the field permissions are a subset of granted fields. If such a role is assigned to a user then that user's authentication fails for this reason. We added a check to validate role query in elastic#46275 and on the same lines, this commit adds check if the exceptions for the field permissions is a subset of granted fields when parsing the index privileges from the role descriptor. Co-authored-by: Yogesh Gaikwad <bizybot@users.noreply.github.com>

bizybot added >bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.5.0 labels Sep 3, 2019

bizybot force-pushed the 34252-fix branch 10 times, most recently from 406f3a1 to 6efafea Compare September 5, 2019 03:15

bizybot force-pushed the 34252-fix branch from 6efafea to 6e1c8b6 Compare September 5, 2019 09:20

bizybot requested review from jkakavas and tvernum September 5, 2019 09:47

bizybot marked this pull request as ready for review September 5, 2019 09:47

jkakavas reviewed Sep 10, 2019

View reviewed changes

Yogesh Gaikwad added 6 commits September 10, 2019 19:40

address review comments

3a5915a

address review comments

8e849ff

Merge branch 'master' into 34252-fix

5649a69

address review comments

8cc234e

precommit error

98edc47

oh precommits

93d0930

bizybot requested a review from jkakavas September 10, 2019 23:34

Yogesh Gaikwad added 2 commits September 12, 2019 13:15

Address review comments

a1112a5

- add index privileg index to the error message to denote query of which index privilege failed to validate. - add more information to the error message where we know the expected values

Merge branch 'master' into 34252-fix

f4b92af

tvernum reviewed Sep 12, 2019

View reviewed changes

Yogesh Gaikwad added 6 commits September 13, 2019 00:10

Address review comments

145338e

- public to package protected method - return immediately for simpler methods - correct the logic in file roles and add tests

throw error when has privileges request contains index privilege with…

1bcdf18

… DLS query

add missing license header

dc5afd4

add better message to help users and is machine readable

bc901ce

Merge branch 'master' into 34252-fix

340f91b

Merge branch 'master' into 34252-fix

cafa9e9

bizybot requested a review from tvernum September 25, 2019 02:41

tvernum requested changes Sep 25, 2019

View reviewed changes

...src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java Outdated Show resolved Hide resolved

Address review comments

0708e44

bizybot requested a review from tvernum September 25, 2019 05:22

tvernum approved these changes Sep 25, 2019

View reviewed changes

bizybot merged commit 777f4b3 into elastic:master Sep 25, 2019

bizybot mentioned this pull request Oct 20, 2019

Validate field permissions when creating a role #48108

Closed

tvernum mentioned this pull request Dec 16, 2019

Validate field permissions when creating a role #50212

Merged

phillbaker mentioned this pull request Dec 19, 2019

Xpack roles:elastic: Error 400 (Bad Request): request body is required [type=parse_exception] phillbaker/terraform-provider-elasticsearch#49

Closed

tvernum mentioned this pull request Jan 13, 2020

[Backport 7.x] Validate field permissions when creating a role #50917

Merged

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate `query` field when creating roles #46275

Validate `query` field when creating roles #46275

bizybot commented Sep 3, 2019 •

edited

elasticmachine commented Sep 3, 2019

bizybot commented Sep 5, 2019

bizybot commented Sep 5, 2019

jkakavas left a comment

jkakavas Sep 10, 2019

bizybot Sep 10, 2019

jkakavas Sep 10, 2019

bizybot Sep 10, 2019

tvernum Sep 12, 2019

bizybot Sep 12, 2019

legrego commented Sep 12, 2019

	randomArray(3, IndicesPrivileges[]::new, () -> IndicesPrivilegesTests.createNewRandom("{\"match_all\": {}}")))
	randomArray(3, IndicesPrivileges[]::new, () -> IndicesPrivilegesTests.createNewRandom(
	"{\"term\":{ \"" + randomAlphaOfLength(5) + "\" : \"" + randomAlphaOfLength(3) + "\"}}")))

Validate query field when creating roles #46275

Validate query field when creating roles #46275

Conversation

bizybot commented Sep 3, 2019 • edited

elasticmachine commented Sep 3, 2019

bizybot commented Sep 5, 2019

bizybot commented Sep 5, 2019

jkakavas left a comment

Choose a reason for hiding this comment

jkakavas Sep 10, 2019

Choose a reason for hiding this comment

bizybot Sep 10, 2019

Choose a reason for hiding this comment

jkakavas Sep 10, 2019

Choose a reason for hiding this comment

bizybot Sep 10, 2019

Choose a reason for hiding this comment

tvernum Sep 12, 2019

Choose a reason for hiding this comment

bizybot Sep 12, 2019

Choose a reason for hiding this comment

legrego commented Sep 12, 2019

Validate `query` field when creating roles #46275

Validate `query` field when creating roles #46275

bizybot commented Sep 3, 2019 •

edited