-
Notifications
You must be signed in to change notification settings - Fork 25.2k
Validate query field when creating roles
#46275
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
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
6e1c8b6
Validate `query` field when creating roles
3a5915a
address review comments
8e849ff
address review comments
5649a69
Merge branch 'master' into 34252-fix
8cc234e
address review comments
98edc47
precommit error
93d0930
oh precommits
a1112a5
Address review comments
f4b92af
Merge branch 'master' into 34252-fix
145338e
Address review comments
1bcdf18
throw error when has privileges request contains index privilege with…
dc5afd4
add missing license header
bc901ce
add better message to help users and is machine readable
340f91b
Merge branch 'master' into 34252-fix
cafa9e9
Merge branch 'master' into 34252-fix
0708e44
Address review comments
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
182 changes: 182 additions & 0 deletions
182
.../main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,182 @@ | ||
| /* | ||
| * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
| * or more contributor license agreements. Licensed under the Elastic License; | ||
| * you may not use this file except in compliance with the Elastic License. | ||
| */ | ||
| package org.elasticsearch.xpack.core.security.authz.support; | ||
|
|
||
| import org.elasticsearch.ElasticsearchParseException; | ||
| import org.elasticsearch.common.Nullable; | ||
| import org.elasticsearch.common.ParsingException; | ||
| import org.elasticsearch.common.Strings; | ||
| import org.elasticsearch.common.bytes.BytesReference; | ||
| import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; | ||
| import org.elasticsearch.common.xcontent.NamedXContentRegistry; | ||
| import org.elasticsearch.common.xcontent.XContentFactory; | ||
| import org.elasticsearch.common.xcontent.XContentParseException; | ||
| import org.elasticsearch.common.xcontent.XContentParser; | ||
| import org.elasticsearch.common.xcontent.XContentType; | ||
| import org.elasticsearch.index.query.AbstractQueryBuilder; | ||
| import org.elasticsearch.index.query.BoolQueryBuilder; | ||
| import org.elasticsearch.index.query.BoostingQueryBuilder; | ||
| import org.elasticsearch.index.query.ConstantScoreQueryBuilder; | ||
| import org.elasticsearch.index.query.GeoShapeQueryBuilder; | ||
| import org.elasticsearch.index.query.QueryBuilder; | ||
| import org.elasticsearch.index.query.TermsQueryBuilder; | ||
| import org.elasticsearch.index.query.functionscore.FunctionScoreQueryBuilder; | ||
| import org.elasticsearch.script.ScriptService; | ||
| import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; | ||
| import org.elasticsearch.xpack.core.security.user.User; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| /** | ||
| * This class helps in evaluating the query field if it is template, | ||
| * validating the query and checking if the query type is allowed to be used in DLS role query. | ||
| */ | ||
| public final class DLSRoleQueryValidator { | ||
|
|
||
| private DLSRoleQueryValidator() { | ||
| } | ||
|
|
||
| /** | ||
bizybot marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * Validates the query field in the {@link RoleDescriptor.IndicesPrivileges} only if it is not a template query.<br> | ||
| * It parses the query and builds the {@link QueryBuilder}, also checks if the query type is supported in DLS role query. | ||
| * | ||
| * @param indicesPrivileges {@link RoleDescriptor.IndicesPrivileges} | ||
| * @param xContentRegistry {@link NamedXContentRegistry} for finding named queries | ||
| */ | ||
| public static void validateQueryField(RoleDescriptor.IndicesPrivileges[] indicesPrivileges, | ||
| NamedXContentRegistry xContentRegistry) { | ||
| if (indicesPrivileges != null) { | ||
| for (int i = 0; i < indicesPrivileges.length; i++) { | ||
| BytesReference query = indicesPrivileges[i].getQuery(); | ||
| try { | ||
| if (query != null) { | ||
| if (isTemplateQuery(query, xContentRegistry)) { | ||
| // skip template query, this requires runtime information like 'User' information. | ||
| continue; | ||
| } | ||
|
|
||
| evaluateAndVerifyRoleQuery(query.utf8ToString(), xContentRegistry); | ||
| } | ||
| } catch (ParsingException | IllegalArgumentException | IOException e) { | ||
| throw new ElasticsearchParseException("failed to parse field 'query' for indices [" + | ||
| Strings.arrayToCommaDelimitedString(indicesPrivileges[i].getIndices()) + | ||
| "] at index privilege [" + i + "] of role descriptor", e); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static boolean isTemplateQuery(BytesReference query, NamedXContentRegistry xContentRegistry) throws IOException { | ||
| try (XContentParser parser = XContentType.JSON.xContent().createParser(xContentRegistry, | ||
| LoggingDeprecationHandler.INSTANCE, query.utf8ToString())) { | ||
| XContentParser.Token token = parser.nextToken(); | ||
| if (token != XContentParser.Token.START_OBJECT) { | ||
| throw new XContentParseException(parser.getTokenLocation(), "expected [" + XContentParser.Token.START_OBJECT + "] but " + | ||
| "found [" + token + "] instead"); | ||
| } | ||
| token = parser.nextToken(); | ||
| if (token != XContentParser.Token.FIELD_NAME) { | ||
| throw new XContentParseException(parser.getTokenLocation(), "expected [" + XContentParser.Token.FIELD_NAME + "] with " + | ||
| "value a query name or 'template' but found [" + token + "] instead"); | ||
| } | ||
| String fieldName = parser.currentName(); | ||
| if ("template".equals(fieldName)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| /** | ||
| * Evaluates the query if it is a template and then validates the query by parsing | ||
| * and building the {@link QueryBuilder}. It also checks if the query type is | ||
| * supported in DLS role query. | ||
| * | ||
| * @param query {@link BytesReference} query field from the role | ||
| * @param scriptService {@link ScriptService} used for evaluation of a template query | ||
| * @param xContentRegistry {@link NamedXContentRegistry} for finding named queries | ||
| * @param user {@link User} used when evaluation a template query | ||
| * @return {@link QueryBuilder} if the query is valid and allowed, in case {@link RoleDescriptor.IndicesPrivileges} | ||
| * * does not have a query field then it returns {@code null}. | ||
| */ | ||
| @Nullable | ||
| public static QueryBuilder evaluateAndVerifyRoleQuery(BytesReference query, ScriptService scriptService, | ||
| NamedXContentRegistry xContentRegistry, User user) { | ||
| if (query != null) { | ||
| String templateResult = SecurityQueryTemplateEvaluator.evaluateTemplate(query.utf8ToString(), scriptService, | ||
| user); | ||
| try { | ||
| return evaluateAndVerifyRoleQuery(templateResult, xContentRegistry); | ||
| } catch (ElasticsearchParseException | ParsingException | XContentParseException | IOException e) { | ||
| throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e); | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Nullable | ||
| private static QueryBuilder evaluateAndVerifyRoleQuery(String query, NamedXContentRegistry xContentRegistry) throws IOException { | ||
| if (query != null) { | ||
| try (XContentParser parser = XContentFactory.xContent(query).createParser(xContentRegistry, | ||
| LoggingDeprecationHandler.INSTANCE, query)) { | ||
| QueryBuilder queryBuilder = AbstractQueryBuilder.parseInnerQueryBuilder(parser); | ||
| verifyRoleQuery(queryBuilder); | ||
| return queryBuilder; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * Checks whether the role query contains queries we know can't be used as DLS role query. | ||
| * | ||
| * @param queryBuilder {@link QueryBuilder} for given query | ||
| */ | ||
| // pkg protected for testing | ||
| static void verifyRoleQuery(QueryBuilder queryBuilder) { | ||
| if (queryBuilder instanceof TermsQueryBuilder) { | ||
| TermsQueryBuilder termsQueryBuilder = (TermsQueryBuilder) queryBuilder; | ||
| if (termsQueryBuilder.termsLookup() != null) { | ||
| throw new IllegalArgumentException("terms query with terms lookup isn't supported as part of a role query"); | ||
| } | ||
| } else if (queryBuilder instanceof GeoShapeQueryBuilder) { | ||
| GeoShapeQueryBuilder geoShapeQueryBuilder = (GeoShapeQueryBuilder) queryBuilder; | ||
| if (geoShapeQueryBuilder.shape() == null) { | ||
| throw new IllegalArgumentException("geoshape query referring to indexed shapes isn't supported as part of a role query"); | ||
| } | ||
| } else if (queryBuilder.getName().equals("percolate")) { | ||
| // actually only if percolate query is referring to an existing document then this is problematic, | ||
| // a normal percolate query does work. However we can't check that here as this query builder is inside | ||
| // another module. So we don't allow the entire percolate query. I don't think users would ever use | ||
| // a percolate query as role query, so this restriction shouldn't prohibit anyone from using dls. | ||
| throw new IllegalArgumentException("percolate query isn't supported as part of a role query"); | ||
| } else if (queryBuilder.getName().equals("has_child")) { | ||
| throw new IllegalArgumentException("has_child query isn't supported as part of a role query"); | ||
| } else if (queryBuilder.getName().equals("has_parent")) { | ||
| throw new IllegalArgumentException("has_parent query isn't supported as part of a role query"); | ||
| } else if (queryBuilder instanceof BoolQueryBuilder) { | ||
| BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) queryBuilder; | ||
| List<QueryBuilder> clauses = new ArrayList<>(); | ||
| clauses.addAll(boolQueryBuilder.filter()); | ||
| clauses.addAll(boolQueryBuilder.must()); | ||
| clauses.addAll(boolQueryBuilder.mustNot()); | ||
| clauses.addAll(boolQueryBuilder.should()); | ||
| for (QueryBuilder clause : clauses) { | ||
| verifyRoleQuery(clause); | ||
| } | ||
| } else if (queryBuilder instanceof ConstantScoreQueryBuilder) { | ||
| verifyRoleQuery(((ConstantScoreQueryBuilder) queryBuilder).innerQuery()); | ||
| } else if (queryBuilder instanceof FunctionScoreQueryBuilder) { | ||
| verifyRoleQuery(((FunctionScoreQueryBuilder) queryBuilder).query()); | ||
| } else if (queryBuilder instanceof BoostingQueryBuilder) { | ||
| verifyRoleQuery(((BoostingQueryBuilder) queryBuilder).negativeQuery()); | ||
| verifyRoleQuery(((BoostingQueryBuilder) queryBuilder).positiveQuery()); | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we really want this to be a random role then may this would be better as
But maybe that's overkill, I didn't really look at where we use this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
createNewRandomis 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.