From 6e1c8b649b7f9f5178a92c14f45d09ed239e9b7f Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Mon, 2 Sep 2019 14:46:39 +1000 Subject: [PATCH 01/12] Validate `query` field when creating roles 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 #34252 --- .../org/elasticsearch/client/SecurityIT.java | 2 +- .../authz/permission/DocumentPermissions.java | 69 +------ .../authz/support/DLSRoleQueryValidator.java | 177 ++++++++++++++++++ ...ityIndexReaderWrapperIntegrationTests.java | 32 ++-- .../permission/DocumentPermissionsTests.java | 19 +- .../xpack/security/Security.java | 8 +- .../action/TransportCreateApiKeyAction.java | 20 +- .../action/role/TransportPutRoleAction.java | 15 +- .../user/TransportHasPrivilegesAction.java | 20 +- .../security/authz/store/FileRolesStore.java | 63 ++++--- .../xpack/security/SecurityTests.java | 3 +- .../action/role/PutRoleBuilderTests.java | 2 +- .../role/TransportPutRoleActionTests.java | 101 +++++++++- .../HasPrivilegesRequestBuilderTests.java | 2 +- .../authc/esnative/NativeRealmIntegTests.java | 6 +- .../security/authz/RoleDescriptorTests.java | 21 ++- .../authz/store/FileRolesStoreTests.java | 41 ++-- .../xpack/security/authz/store/roles.yml | 2 +- .../security/authz/store/roles2xformat.yml | 2 +- .../test/roles/30_prohibited_role_query.yml | 50 +---- 20 files changed, 463 insertions(+), 192 deletions(-) create mode 100644 x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/SecurityIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/SecurityIT.java index abf65d19df3b7..8122ff17648b1 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/SecurityIT.java +++ b/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\": {}}"))) .applicationResourcePrivileges(randomArray(3, ApplicationResourcePrivileges[]::new, () -> ApplicationResourcePrivilegesTests.createNewRandom(randomAlphaOfLength(3).toLowerCase(Locale.ROOT)))) .runAsPrivilege(randomArray(3, String[]::new, () -> randomAlphaOfLength(3))); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java index bde94b116c982..6aed0d1213816 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java @@ -12,29 +12,18 @@ import org.apache.lucene.search.join.ToChildBlockJoinQuery; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.lucene.search.Queries; -import org.elasticsearch.common.xcontent.LoggingDeprecationHandler; -import org.elasticsearch.common.xcontent.XContentFactory; -import org.elasticsearch.common.xcontent.XContentParser; -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.QueryRewriteContext; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.index.query.Rewriteable; -import org.elasticsearch.index.query.TermsQueryBuilder; -import org.elasticsearch.index.query.functionscore.FunctionScoreQueryBuilder; import org.elasticsearch.index.search.NestedHelper; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.script.ScriptService; -import org.elasticsearch.xpack.core.security.authz.support.SecurityQueryTemplateEvaluator; +import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator; import org.elasticsearch.xpack.core.security.user.User; import java.io.IOException; -import java.util.ArrayList; import java.util.Collections; -import java.util.List; import java.util.Set; import java.util.function.Function; @@ -127,11 +116,9 @@ private static void buildRoleQuery(User user, ScriptService scriptService, Shard BooleanQuery.Builder filter) throws IOException { for (BytesReference bytesReference : queries) { QueryShardContext queryShardContext = queryShardContextProvider.apply(shardId); - String templateResult = SecurityQueryTemplateEvaluator.evaluateTemplate(bytesReference.utf8ToString(), scriptService, user); - try (XContentParser parser = XContentFactory.xContent(templateResult).createParser(queryShardContext.getXContentRegistry(), - LoggingDeprecationHandler.INSTANCE, templateResult)) { - QueryBuilder queryBuilder = queryShardContext.parseInnerQueryBuilder(parser); - verifyRoleQuery(queryBuilder); + QueryBuilder queryBuilder = DLSRoleQueryValidator.validateAndVerifyRoleQuery(bytesReference, scriptService, + queryShardContext.getXContentRegistry(), user); + if (queryBuilder != null) { failIfQueryUsesClient(queryBuilder, queryShardContext); Query roleQuery = queryShardContext.toQuery(queryBuilder).query(); filter.add(roleQuery, SHOULD); @@ -139,11 +126,11 @@ private static void buildRoleQuery(User user, ScriptService scriptService, Shard NestedHelper nestedHelper = new NestedHelper(queryShardContext.getMapperService()); if (nestedHelper.mightMatchNestedDocs(roleQuery)) { roleQuery = new BooleanQuery.Builder().add(roleQuery, FILTER) - .add(Queries.newNonNestedFilter(), FILTER).build(); + .add(Queries.newNonNestedFilter(), FILTER).build(); } // If access is allowed on root doc then also access is allowed on all nested docs of that root document: BitSetProducer rootDocs = queryShardContext - .bitsetFilter(Queries.newNonNestedFilter()); + .bitsetFilter(Queries.newNonNestedFilter()); ToChildBlockJoinQuery includeNestedDocs = new ToChildBlockJoinQuery(roleQuery, rootDocs); filter.add(includeNestedDocs, SHOULD); } @@ -153,50 +140,6 @@ private static void buildRoleQuery(User user, ScriptService scriptService, Shard filter.setMinimumNumberShouldMatch(1); } - /** - * Checks whether the role query contains queries we know can't be used as DLS role query. - */ - static void verifyRoleQuery(QueryBuilder queryBuilder) throws IOException { - 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 support 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 support as part of a role query"); - } else if (queryBuilder.getName().equals("has_child")) { - throw new IllegalArgumentException("has_child query isn't support as part of a role query"); - } else if (queryBuilder.getName().equals("has_parent")) { - throw new IllegalArgumentException("has_parent query isn't support as part of a role query"); - } else if (queryBuilder instanceof BoolQueryBuilder) { - BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) queryBuilder; - List 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()); - } - } - /** * Fall back validation that verifies that queries during rewrite don't use * the client to make remote calls. In the case of DLS this can cause a dead diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java new file mode 100644 index 0000000000000..9e714095d6dc3 --- /dev/null +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java @@ -0,0 +1,177 @@ +/* + * 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.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.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 + * and checking if the query type is allowed to be used in DLS role query. + */ +public class DLSRoleQueryValidator { + + /** + * Evaluates the query field in the {@link RoleDescriptor.IndicesPrivileges} only if it is not a template + * query.
+ * 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 (RoleDescriptor.IndicesPrivileges idp : indicesPrivileges) { + BytesReference query = idp.getQuery(); + if (query != null) { + if (isTemplateQuery(query, xContentRegistry)) { + // skip template query, this requires runtime information like 'User' information. + continue; + } + + validateAndVerifyRoleQuery(query.utf8ToString(), xContentRegistry); + } + } + } + } + + private static boolean isTemplateQuery(BytesReference query, NamedXContentRegistry xContentRegistry) { + try { + try (XContentParser parser = XContentFactory.xContent(query.utf8ToString()).createParser(xContentRegistry, + LoggingDeprecationHandler.INSTANCE, query.utf8ToString())) { + expectedToken(parser.nextToken(), parser, XContentParser.Token.START_OBJECT); + expectedToken(parser.nextToken(), parser, XContentParser.Token.FIELD_NAME); + String fieldName = parser.currentName(); + if ("template".equals(fieldName)) { + return true; + } + } + } catch (XContentParseException | IOException e) { + throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e); + } + return false; + } + + private static void expectedToken(XContentParser.Token read, XContentParser parser, XContentParser.Token expected) { + if (read != expected) { + throw new XContentParseException(parser.getTokenLocation(), + "expected [" + expected + "] but found [" + read + "] instead"); + } + } + + /** + * 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 validateAndVerifyRoleQuery(BytesReference query, ScriptService scriptService, + NamedXContentRegistry xContentRegistry, User user) { + QueryBuilder queryBuilder = null; + if (query != null) { + try { + String templateResult = SecurityQueryTemplateEvaluator.evaluateTemplate(query.utf8ToString(), scriptService, + user); + queryBuilder = validateAndVerifyRoleQuery(templateResult, xContentRegistry); + } catch (ElasticsearchParseException | ParsingException | XContentParseException | IOException e) { + throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e); + } + } + return queryBuilder; + } + + private static QueryBuilder validateAndVerifyRoleQuery(String query, NamedXContentRegistry xContentRegistry) { + QueryBuilder queryBuilder = null; + if (query != null) { + try { + try (XContentParser parser = XContentFactory.xContent(query).createParser(xContentRegistry, + LoggingDeprecationHandler.INSTANCE, query)) { + queryBuilder = AbstractQueryBuilder.parseInnerQueryBuilder(parser); + verifyRoleQuery(queryBuilder); + } + } catch (ElasticsearchParseException | ParsingException | XContentParseException | IOException e) { + throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e); + } + } + return queryBuilder; + } + + /** + * Checks whether the role query contains queries we know can't be used as DLS role query. + * + * @param queryBuilder {@link QueryBuilder} for given query + */ + public 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 support 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 support as part of a role query"); + } else if (queryBuilder.getName().equals("has_child")) { + throw new IllegalArgumentException("has_child query isn't support as part of a role query"); + } else if (queryBuilder.getName().equals("has_parent")) { + throw new IllegalArgumentException("has_parent query isn't support as part of a role query"); + } else if (queryBuilder instanceof BoolQueryBuilder) { + BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) queryBuilder; + List 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()); + } + } +} diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java index 3be46a031a0b2..699fda4a249df 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/accesscontrol/SecurityIndexReaderWrapperIntegrationTests.java @@ -27,12 +27,10 @@ import org.elasticsearch.common.lucene.index.ElasticsearchDirectoryReader; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ThreadContext; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.IndexSettings; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.query.ParsedQuery; import org.elasticsearch.index.query.QueryShardContext; -import org.elasticsearch.index.query.TermQueryBuilder; import org.elasticsearch.index.query.TermsQueryBuilder; import org.elasticsearch.index.shard.ShardId; import org.elasticsearch.license.XPackLicenseState; @@ -54,9 +52,7 @@ import static java.util.Collections.singletonMap; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.equalTo; -import static org.mockito.Matchers.any; import static org.mockito.Matchers.anyString; -import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; import static org.mockito.Mockito.when; @@ -75,9 +71,6 @@ public void testDLS() throws Exception { final Authentication authentication = mock(Authentication.class); when(authentication.getUser()).thenReturn(mock(User.class)); threadContext.putTransient(AuthenticationField.AUTHENTICATION_KEY, authentication); - IndicesAccessControl.IndexAccessControl indexAccessControl = new IndicesAccessControl.IndexAccessControl(true, new - FieldPermissions(), - DocumentPermissions.filteredBy(singleton(new BytesArray("{\"match_all\" : {}}")))); IndexSettings indexSettings = IndexSettingsModule.newIndexSettings(shardId.getIndex(), Settings.EMPTY); Client client = mock(Client.class); when(client.settings()).thenReturn(Settings.EMPTY); @@ -88,14 +81,6 @@ public void testDLS() throws Exception { DocumentSubsetBitsetCache bitsetCache = new DocumentSubsetBitsetCache(Settings.EMPTY); XPackLicenseState licenseState = mock(XPackLicenseState.class); when(licenseState.isDocumentAndFieldLevelSecurityAllowed()).thenReturn(true); - SecurityIndexReaderWrapper wrapper = new SecurityIndexReaderWrapper(s -> queryShardContext, - bitsetCache, threadContext, licenseState, scriptService) { - - @Override - protected IndicesAccessControl getIndicesAccessControl() { - return new IndicesAccessControl(true, singletonMap("_index", indexAccessControl)); - } - }; Directory directory = newDirectory(); IndexWriter iw = new IndexWriter( @@ -142,17 +127,32 @@ protected IndicesAccessControl getIndicesAccessControl() { DirectoryReader directoryReader = ElasticsearchDirectoryReader.wrap(DirectoryReader.open(directory), shardId); for (int i = 0; i < numValues; i++) { + String termQuery = "{\"term\": {\"field\": \""+ values[i] + "\"} }"; + IndicesAccessControl.IndexAccessControl indexAccessControl = new IndicesAccessControl.IndexAccessControl(true, new + FieldPermissions(), + DocumentPermissions.filteredBy(singleton(new BytesArray(termQuery)))); + SecurityIndexReaderWrapper wrapper = new SecurityIndexReaderWrapper(s -> queryShardContext, + bitsetCache, threadContext, licenseState, scriptService) { + + @Override + protected IndicesAccessControl getIndicesAccessControl() { + return new IndicesAccessControl(true, singletonMap("_index", indexAccessControl)); + } + }; + ParsedQuery parsedQuery = new ParsedQuery(new TermQuery(new Term("field", values[i]))); - doReturn(new TermQueryBuilder("field", values[i])).when(queryShardContext).parseInnerQueryBuilder(any(XContentParser.class)); when(queryShardContext.toQuery(new TermsQueryBuilder("field", values[i]))).thenReturn(parsedQuery); + DirectoryReader wrappedDirectoryReader = wrapper.apply(directoryReader); IndexSearcher indexSearcher = new ContextIndexSearcher(wrappedDirectoryReader, IndexSearcher.getDefaultSimilarity(), IndexSearcher.getDefaultQueryCache(), IndexSearcher.getDefaultQueryCachingPolicy()); int expectedHitCount = valuesHitCount[i]; logger.info("Going to verify hit count with query [{}] with expected total hits [{}]", parsedQuery.query(), expectedHitCount); + TotalHitCountCollector countCollector = new TotalHitCountCollector(); indexSearcher.search(new MatchAllDocsQuery(), countCollector); + assertThat(countCollector.getTotalHits(), equalTo(expectedHitCount)); assertThat(wrappedDirectoryReader.numDocs(), equalTo(expectedHitCount)); } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissionsTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissionsTests.java index f8d1334df7e4e..799b0cec0d38d 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissionsTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissionsTests.java @@ -24,6 +24,7 @@ import org.elasticsearch.join.query.HasChildQueryBuilder; import org.elasticsearch.join.query.HasParentQueryBuilder; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator; import java.io.IOException; import java.util.Collections; @@ -70,39 +71,39 @@ public void testHasDocumentPermissions() throws IOException { public void testVerifyRoleQuery() throws Exception { QueryBuilder queryBuilder1 = new TermsQueryBuilder("field", "val1", "val2"); - DocumentPermissions.verifyRoleQuery(queryBuilder1); + DLSRoleQueryValidator.verifyRoleQuery(queryBuilder1); QueryBuilder queryBuilder2 = new TermsQueryBuilder("field", new TermsLookup("_index", "_type", "_id", "_path")); - Exception e = expectThrows(IllegalArgumentException.class, () -> DocumentPermissions.verifyRoleQuery(queryBuilder2)); + Exception e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder2)); assertThat(e.getMessage(), equalTo("terms query with terms lookup isn't supported as part of a role query")); QueryBuilder queryBuilder3 = new GeoShapeQueryBuilder("field", "_id", "_type"); - e = expectThrows(IllegalArgumentException.class, () -> DocumentPermissions.verifyRoleQuery(queryBuilder3)); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder3)); assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't support as part of a role query")); QueryBuilder queryBuilder4 = new HasChildQueryBuilder("_type", new MatchAllQueryBuilder(), ScoreMode.None); - e = expectThrows(IllegalArgumentException.class, () -> DocumentPermissions.verifyRoleQuery(queryBuilder4)); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder4)); assertThat(e.getMessage(), equalTo("has_child query isn't support as part of a role query")); QueryBuilder queryBuilder5 = new HasParentQueryBuilder("_type", new MatchAllQueryBuilder(), false); - e = expectThrows(IllegalArgumentException.class, () -> DocumentPermissions.verifyRoleQuery(queryBuilder5)); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder5)); assertThat(e.getMessage(), equalTo("has_parent query isn't support as part of a role query")); QueryBuilder queryBuilder6 = new BoolQueryBuilder().must(new GeoShapeQueryBuilder("field", "_id", "_type")); - e = expectThrows(IllegalArgumentException.class, () -> DocumentPermissions.verifyRoleQuery(queryBuilder6)); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder6)); assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't support as part of a role query")); QueryBuilder queryBuilder7 = new ConstantScoreQueryBuilder(new GeoShapeQueryBuilder("field", "_id", "_type")); - e = expectThrows(IllegalArgumentException.class, () -> DocumentPermissions.verifyRoleQuery(queryBuilder7)); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder7)); assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't support as part of a role query")); QueryBuilder queryBuilder8 = new FunctionScoreQueryBuilder(new GeoShapeQueryBuilder("field", "_id", "_type")); - e = expectThrows(IllegalArgumentException.class, () -> DocumentPermissions.verifyRoleQuery(queryBuilder8)); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder8)); assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't support as part of a role query")); QueryBuilder queryBuilder9 = new BoostingQueryBuilder(new GeoShapeQueryBuilder("field", "_id", "_type"), new MatchAllQueryBuilder()); - e = expectThrows(IllegalArgumentException.class, () -> DocumentPermissions.verifyRoleQuery(queryBuilder9)); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder9)); assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't support as part of a role query")); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index 533897bac4466..b2ebf9733c25b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -343,7 +343,7 @@ public Collection createComponents(Client client, ClusterService cluster NamedXContentRegistry xContentRegistry, Environment environment, NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) { try { - return createComponents(client, threadPool, clusterService, resourceWatcherService, scriptService); + return createComponents(client, threadPool, clusterService, resourceWatcherService, scriptService, xContentRegistry); } catch (final Exception e) { throw new IllegalStateException("security initialization failed", e); } @@ -351,7 +351,8 @@ public Collection createComponents(Client client, ClusterService cluster // pkg private for testing - tests want to pass in their set of extensions hence we are not using the extension service directly Collection createComponents(Client client, ThreadPool threadPool, ClusterService clusterService, - ResourceWatcherService resourceWatcherService, ScriptService scriptService) throws Exception { + ResourceWatcherService resourceWatcherService, ScriptService scriptService, + NamedXContentRegistry xContentRegistry) throws Exception { if (enabled == false) { return Collections.singletonList(new SecurityUsageServices(null, null, null, null)); } @@ -406,7 +407,8 @@ Collection createComponents(Client client, ThreadPool threadPool, Cluste dlsBitsetCache.set(new DocumentSubsetBitsetCache(settings)); final FieldPermissionsCache fieldPermissionsCache = new FieldPermissionsCache(settings); - final FileRolesStore fileRolesStore = new FileRolesStore(settings, env, resourceWatcherService, getLicenseState()); + final FileRolesStore fileRolesStore = new FileRolesStore(settings, env, resourceWatcherService, getLicenseState(), + xContentRegistry); final NativeRolesStore nativeRolesStore = new NativeRolesStore(settings, client, getLicenseState(), securityIndex.get()); final ReservedRolesStore reservedRolesStore = new ReservedRolesStore(); List, ActionListener>> rolesProviders = new ArrayList<>(); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java index 09612c5e01f0f..72a92516e59ed 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java @@ -6,11 +6,13 @@ package org.elasticsearch.xpack.security.action; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.HandledTransportAction; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.tasks.Task; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.core.security.SecurityContext; @@ -18,6 +20,8 @@ import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest; import org.elasticsearch.xpack.core.security.action.CreateApiKeyResponse; import org.elasticsearch.xpack.core.security.authc.Authentication; +import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; +import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator; import org.elasticsearch.xpack.security.authc.ApiKeyService; import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore; @@ -32,14 +36,16 @@ public final class TransportCreateApiKeyAction extends HandledTransportAction) CreateApiKeyRequest::new); this.apiKeyService = apiKeyService; this.securityContext = context; this.rolesStore = rolesStore; + this.xContentRegistry = xContentRegistry; } @Override @@ -49,7 +55,17 @@ protected void doExecute(Task task, CreateApiKeyRequest request, ActionListener< listener.onFailure(new IllegalStateException("authentication is required")); } else { rolesStore.getRoleDescriptors(new HashSet<>(Arrays.asList(authentication.getUser().roles())), - ActionListener.wrap(roleDescriptors -> apiKeyService.createApiKey(authentication, request, roleDescriptors, listener), + ActionListener.wrap(roleDescriptors -> { + for (RoleDescriptor rd : roleDescriptors) { + try { + DLSRoleQueryValidator.validateQueryField(rd.getIndicesPrivileges(), xContentRegistry); + } catch (ElasticsearchException | IllegalArgumentException e) { + listener.onFailure(e); + return; + } + } + apiKeyService.createApiKey(authentication, request, roleDescriptors, listener); + }, listener::onFailure)); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleAction.java index c0a91bcdb021e..300c8c835ffc6 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleAction.java @@ -5,26 +5,32 @@ */ package org.elasticsearch.xpack.security.action.role; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.HandledTransportAction; import org.elasticsearch.common.inject.Inject; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.tasks.Task; import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.core.security.action.role.PutRoleAction; import org.elasticsearch.xpack.core.security.action.role.PutRoleRequest; import org.elasticsearch.xpack.core.security.action.role.PutRoleResponse; import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; +import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator; import org.elasticsearch.xpack.security.authz.store.NativeRolesStore; public class TransportPutRoleAction extends HandledTransportAction { private final NativeRolesStore rolesStore; + private final NamedXContentRegistry xContentRegistry; @Inject - public TransportPutRoleAction(ActionFilters actionFilters, NativeRolesStore rolesStore, TransportService transportService) { + public TransportPutRoleAction(ActionFilters actionFilters, NativeRolesStore rolesStore, TransportService transportService, + NamedXContentRegistry xContentRegistry) { super(PutRoleAction.NAME, transportService, actionFilters, PutRoleRequest::new); this.rolesStore = rolesStore; + this.xContentRegistry = xContentRegistry; } @Override @@ -35,6 +41,13 @@ protected void doExecute(Task task, final PutRoleRequest request, final ActionLi return; } + try { + DLSRoleQueryValidator.validateQueryField(request.roleDescriptor().getIndicesPrivileges(), xContentRegistry); + } catch (ElasticsearchException | IllegalArgumentException e) { + listener.onFailure(e); + return; + } + rolesStore.putRole(request, request.roleDescriptor(), new ActionListener() { @Override public void onResponse(Boolean created) { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java index ae400172bf110..e8d8b6c4db03a 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java @@ -5,19 +5,23 @@ */ package org.elasticsearch.xpack.security.action.user; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.HandledTransportAction; import org.elasticsearch.common.inject.Inject; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.tasks.Task; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; +import org.elasticsearch.xpack.core.security.SecurityContext; import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesAction; import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesRequest; import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesResponse; import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; +import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator; import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.security.authz.AuthorizationService; import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore; @@ -36,15 +40,20 @@ public class TransportHasPrivilegesAction extends HandledTransportAction authorizationService.checkPrivileges(authentication, request, applicationPrivilegeDescriptors, listener), listener::onFailure)); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java index a2be72dc6d631..dd0f07e6d15b5 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java @@ -9,6 +9,7 @@ import org.apache.logging.log4j.Logger; import org.apache.logging.log4j.message.ParameterizedMessage; import org.apache.logging.log4j.util.Supplier; +import org.elasticsearch.ElasticsearchException; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.common.Nullable; @@ -20,6 +21,7 @@ import org.elasticsearch.common.xcontent.yaml.YamlXContent; import org.elasticsearch.env.Environment; import org.elasticsearch.license.XPackLicenseState; +import org.elasticsearch.script.ScriptService; import org.elasticsearch.watcher.FileChangesListener; import org.elasticsearch.watcher.FileWatcher; import org.elasticsearch.watcher.ResourceWatcherService; @@ -29,6 +31,7 @@ import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.IndicesPrivileges; import org.elasticsearch.xpack.core.security.authz.store.ReservedRolesStore; import org.elasticsearch.xpack.core.security.authz.store.RoleRetrievalResult; +import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator; import org.elasticsearch.xpack.core.security.support.NoOpLogger; import org.elasticsearch.xpack.core.security.support.Validation; @@ -61,27 +64,30 @@ public class FileRolesStore implements BiConsumer, ActionListener>> listeners = new ArrayList<>(); private volatile Map permissions; - public FileRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, XPackLicenseState licenseState) + public FileRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, XPackLicenseState licenseState, + NamedXContentRegistry xContentRegistry) throws IOException { - this(settings, env, watcherService, null, licenseState); + this(settings, env, watcherService, null, licenseState, xContentRegistry); } FileRolesStore(Settings settings, Environment env, ResourceWatcherService watcherService, Consumer> listener, - XPackLicenseState licenseState) throws IOException { + XPackLicenseState licenseState, NamedXContentRegistry xContentRegistry) throws IOException { this.settings = settings; this.file = resolveFile(env); if (listener != null) { listeners.add(listener); } this.licenseState = licenseState; + this.xContentRegistry = xContentRegistry; FileWatcher watcher = new FileWatcher(file.getParent()); watcher.addListener(new FileListener()); watcherService.add(watcher, ResourceWatcherService.Frequency.HIGH); - permissions = parseFile(file, logger, settings, licenseState); + permissions = parseFile(file, logger, settings, licenseState, xContentRegistry); } @@ -150,15 +156,17 @@ public static Path resolveFile(Environment env) { } public static Set parseFileForRoleNames(Path path, Logger logger) { - return parseRoleDescriptors(path, logger, false, Settings.EMPTY).keySet(); + // EMPTY is safe here because we never use namedObject as we are just parsing role names + return parseRoleDescriptors(path, logger, false, Settings.EMPTY, null, NamedXContentRegistry.EMPTY).keySet(); } - public static Map parseFile(Path path, Logger logger, Settings settings, XPackLicenseState licenseState) { - return parseFile(path, logger, true, settings, licenseState); + public static Map parseFile(Path path, Logger logger, Settings settings, XPackLicenseState licenseState, + NamedXContentRegistry xContentRegistry) { + return parseFile(path, logger, true, settings, licenseState, xContentRegistry); } - public static Map parseFile(Path path, Logger logger, boolean resolvePermission, - Settings settings, XPackLicenseState licenseState) { + public static Map parseFile(Path path, Logger logger, boolean resolvePermission, Settings settings, + XPackLicenseState licenseState, NamedXContentRegistry xContentRegistry) { if (logger == null) { logger = NoOpLogger.INSTANCE; } @@ -170,7 +178,7 @@ public static Map parseFile(Path path, Logger logger, bo List roleSegments = roleSegments(path); final boolean flsDlsLicensed = licenseState.isDocumentAndFieldLevelSecurityAllowed(); for (String segment : roleSegments) { - RoleDescriptor descriptor = parseRoleDescriptor(segment, path, logger, resolvePermission, settings); + RoleDescriptor descriptor = parseRoleDescriptor(segment, path, logger, resolvePermission, settings, xContentRegistry); if (descriptor != null) { if (ReservedRolesStore.isReserved(descriptor.getName())) { logger.warn("role [{}] is reserved. the relevant role definition in the mapping file will be ignored", @@ -202,7 +210,8 @@ public static Map parseFile(Path path, Logger logger, bo return unmodifiableMap(roles); } - public static Map parseRoleDescriptors(Path path, Logger logger, boolean resolvePermission, Settings settings) { + public static Map parseRoleDescriptors(Path path, Logger logger, boolean resolvePermission, Settings settings, + ScriptService scriptService, NamedXContentRegistry xContentRegistry) { if (logger == null) { logger = NoOpLogger.INSTANCE; } @@ -213,7 +222,7 @@ public static Map parseRoleDescriptors(Path path, Logger try { List roleSegments = roleSegments(path); for (String segment : roleSegments) { - RoleDescriptor rd = parseRoleDescriptor(segment, path, logger, resolvePermission, settings); + RoleDescriptor rd = parseRoleDescriptor(segment, path, logger, resolvePermission, settings, xContentRegistry); if (rd != null) { roles.put(rd.getName(), rd); } @@ -231,12 +240,12 @@ public static Map parseRoleDescriptors(Path path, Logger } @Nullable - static RoleDescriptor parseRoleDescriptor(String segment, Path path, Logger logger, boolean resolvePermissions, Settings settings) { + static RoleDescriptor parseRoleDescriptor(String segment, Path path, Logger logger, boolean resolvePermissions, Settings settings, + NamedXContentRegistry xContentRegistry) { String roleName = null; try { - // EMPTY is safe here because we never use namedObject XContentParser parser = YamlXContent.yamlXContent - .createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, segment); + .createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, segment); XContentParser.Token token = parser.nextToken(); if (token == XContentParser.Token.START_OBJECT) { token = parser.nextToken(); @@ -258,7 +267,7 @@ static RoleDescriptor parseRoleDescriptor(String segment, Path path, Logger logg // we pass true as last parameter because we do not want to reject files if field permissions // are given in 2.x syntax RoleDescriptor descriptor = RoleDescriptor.parse(roleName, parser, true); - return checkDescriptor(descriptor, path, logger, settings); + return checkDescriptor(descriptor, path, logger, settings, xContentRegistry); } else { logger.error("invalid role definition [{}] in roles file [{}]. skipping role...", roleName, path.toAbsolutePath()); return null; @@ -295,16 +304,24 @@ static RoleDescriptor parseRoleDescriptor(String segment, Path path, Logger logg } @Nullable - private static RoleDescriptor checkDescriptor(RoleDescriptor descriptor, Path path, Logger logger, Settings settings) { + private static RoleDescriptor checkDescriptor(RoleDescriptor descriptor, Path path, Logger logger, Settings settings, + NamedXContentRegistry xContentRegistry) { String roleName = descriptor.getName(); // first check if FLS/DLS is enabled on the role... - for (RoleDescriptor.IndicesPrivileges privilege : descriptor.getIndicesPrivileges()) { - if ((privilege.getQuery() != null || privilege.getGrantedFields() != null || privilege.getDeniedFields() != null) - && XPackSettings.DLS_FLS_ENABLED.get(settings) == false) { + if (descriptor.isUsingDocumentOrFieldLevelSecurity()) { + if (XPackSettings.DLS_FLS_ENABLED.get(settings) == false) { logger.error("invalid role definition [{}] in roles file [{}]. document and field level security is not " + - "enabled. set [{}] to [true] in the configuration file. skipping role...", roleName, path - .toAbsolutePath(), XPackSettings.DLS_FLS_ENABLED.getKey()); + "enabled. set [{}] to [true] in the configuration file. skipping role...", roleName, path + .toAbsolutePath(), XPackSettings.DLS_FLS_ENABLED.getKey()); return null; + } else { + try { + DLSRoleQueryValidator.validateQueryField(descriptor.getIndicesPrivileges(), xContentRegistry); + } catch (ElasticsearchException | IllegalArgumentException e) { + logger.error((Supplier) () -> new ParameterizedMessage( + "invalid role definition [{}] in roles file [{}]. failed to validate query field. skipping role...", roleName, + path.toAbsolutePath()), e); + } } } return descriptor; @@ -350,7 +367,7 @@ public synchronized void onFileChanged(Path file) { if (file.equals(FileRolesStore.this.file)) { final Map previousPermissions = permissions; try { - permissions = parseFile(file, logger, settings, licenseState); + permissions = parseFile(file, logger, settings, licenseState, xContentRegistry); logger.info("updated roles (roles file [{}] {})", file.toAbsolutePath(), Files.exists(file) ? "changed" : "removed"); } catch (Exception e) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java index 4d8dca8e095a8..364b32f18b356 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java @@ -128,7 +128,8 @@ protected SSLService getSslService() { Client client = mock(Client.class); when(client.threadPool()).thenReturn(threadPool); when(client.settings()).thenReturn(settings); - return security.createComponents(client, threadPool, clusterService, mock(ResourceWatcherService.class), mock(ScriptService.class)); + return security.createComponents(client, threadPool, clusterService, mock(ResourceWatcherService.class), mock(ScriptService.class), + xContentRegistry()); } private static T findComponent(Class type, Collection components) { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/PutRoleBuilderTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/PutRoleBuilderTests.java index ba305e15ed768..33fe3259b3cb1 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/PutRoleBuilderTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/PutRoleBuilderTests.java @@ -32,4 +32,4 @@ public void testBWCFieldPermissions() throws Exception { "[role1], use [\"field_security\": {\"grant\":[...],\"except\":[...]}] instead")); } } -} \ No newline at end of file +} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java index 3cbb7782688e6..9126e4feecff6 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java @@ -5,10 +5,20 @@ */ package org.elasticsearch.xpack.security.action.role; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.bytes.BytesArray; +import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; +import org.elasticsearch.index.query.MatchAllQueryBuilder; +import org.elasticsearch.index.query.QueryBuilder; +import org.elasticsearch.index.query.TermQueryBuilder; +import org.elasticsearch.join.query.HasChildQueryBuilder; +import org.elasticsearch.join.query.HasParentQueryBuilder; import org.elasticsearch.tasks.Task; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.transport.Transport; @@ -23,6 +33,7 @@ import java.util.ArrayList; import java.util.Collections; +import java.util.List; import java.util.concurrent.atomic.AtomicReference; import static org.hamcrest.Matchers.containsString; @@ -41,12 +52,26 @@ public class TransportPutRoleActionTests extends ESTestCase { + @Override + protected NamedXContentRegistry xContentRegistry() { + return new NamedXContentRegistry(List.of( + new NamedXContentRegistry.Entry(QueryBuilder.class, new ParseField(MatchAllQueryBuilder.NAME), + (p, c) -> MatchAllQueryBuilder.fromXContent(p)), + new NamedXContentRegistry.Entry(QueryBuilder.class, new ParseField(HasChildQueryBuilder.NAME), + (p, c) -> HasChildQueryBuilder.fromXContent(p)), + new NamedXContentRegistry.Entry(QueryBuilder.class, new ParseField(HasParentQueryBuilder.NAME), + (p, c) -> HasParentQueryBuilder.fromXContent(p)), + new NamedXContentRegistry.Entry(QueryBuilder.class, new ParseField(TermQueryBuilder.NAME), + (p, c) -> TermQueryBuilder.fromXContent(p)))); + } + public void testReservedRole() { final String roleName = randomFrom(new ArrayList<>(ReservedRolesStore.names())); NativeRolesStore rolesStore = mock(NativeRolesStore.class); TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null, TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet()); - TransportPutRoleAction action = new TransportPutRoleAction(mock(ActionFilters.class), rolesStore, transportService); + TransportPutRoleAction action = new TransportPutRoleAction(mock(ActionFilters.class), rolesStore, transportService, + xContentRegistry()); PutRoleRequest request = new PutRoleRequest(); request.name(roleName); @@ -76,7 +101,8 @@ public void testValidRole() { NativeRolesStore rolesStore = mock(NativeRolesStore.class); TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null, TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet()); - TransportPutRoleAction action = new TransportPutRoleAction(mock(ActionFilters.class), rolesStore, transportService); + TransportPutRoleAction action = new TransportPutRoleAction(mock(ActionFilters.class), rolesStore, transportService, + xContentRegistry()); final boolean created = randomBoolean(); PutRoleRequest request = new PutRoleRequest(); @@ -119,7 +145,8 @@ public void testException() { NativeRolesStore rolesStore = mock(NativeRolesStore.class); TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null, TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet()); - TransportPutRoleAction action = new TransportPutRoleAction(mock(ActionFilters.class), rolesStore, transportService); + TransportPutRoleAction action = new TransportPutRoleAction(mock(ActionFilters.class), rolesStore, transportService, + xContentRegistry()); PutRoleRequest request = new PutRoleRequest(); request.name(roleName); @@ -154,4 +181,72 @@ public void onFailure(Exception e) { assertThat(throwableRef.get(), is(sameInstance(e))); verify(rolesStore, times(1)).putRole(eq(request), any(RoleDescriptor.class), any(ActionListener.class)); } + + public void testCreationOfRoleWithMalformedQueryJsonFails() { + NativeRolesStore rolesStore = mock(NativeRolesStore.class); + TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null, + TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet()); + TransportPutRoleAction action = new TransportPutRoleAction(mock(ActionFilters.class), rolesStore, transportService, + xContentRegistry()); + PutRoleRequest request = new PutRoleRequest(); + request.name("test"); + String[] malformedQueryJson = new String[]{"{ \"match_all\": { \"unknown_field\": \"\" } }", + "{ malformed JSON }", + "{ \"unknown\": {\"\"} }", + "{}"}; + BytesReference query = new BytesArray(randomFrom(malformedQueryJson)); + request.addIndex(new String[]{"idx1"}, new String[]{"read"}, null, null, query, randomBoolean()); + + final AtomicReference throwableRef = new AtomicReference<>(); + final AtomicReference responseRef = new AtomicReference<>(); + action.doExecute(mock(Task.class), request, new ActionListener() { + @Override + public void onResponse(PutRoleResponse response) { + responseRef.set(response); + } + + @Override + public void onFailure(Exception e) { + throwableRef.set(e); + } + }); + + assertThat(responseRef.get(), is(nullValue())); + assertThat(throwableRef.get(), is(notNullValue())); + Throwable t = throwableRef.get(); + assertThat(t, instanceOf(ElasticsearchParseException.class)); + } + + public void testCreationOfRoleWithUnsupportedQueryFails() throws Exception { + NativeRolesStore rolesStore = mock(NativeRolesStore.class); + TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null, + TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet()); + TransportPutRoleAction action = new TransportPutRoleAction(mock(ActionFilters.class), rolesStore, transportService, + xContentRegistry()); + PutRoleRequest request = new PutRoleRequest(); + request.name("test"); + String hasChildQuery = "{ \"has_child\": { \"type\": \"child\", \"query\": { \"match_all\": {} } } }"; + String hasParentQuery = "{ \"has_parent\": { \"parent_type\": \"parent\", \"query\": { \"match_all\": {} } } }"; + BytesReference query = new BytesArray(randomFrom(hasChildQuery, hasParentQuery)); + request.addIndex(new String[]{"idx1"}, new String[]{"read"}, null, null, query, randomBoolean()); + + final AtomicReference throwableRef = new AtomicReference<>(); + final AtomicReference responseRef = new AtomicReference<>(); + action.doExecute(mock(Task.class), request, new ActionListener() { + @Override + public void onResponse(PutRoleResponse response) { + responseRef.set(response); + } + + @Override + public void onFailure(Exception e) { + throwableRef.set(e); + } + }); + + assertThat(responseRef.get(), is(nullValue())); + assertThat(throwableRef.get(), is(notNullValue())); + Throwable t = throwableRef.get(); + assertThat(t, instanceOf(IllegalArgumentException.class)); + } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/HasPrivilegesRequestBuilderTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/HasPrivilegesRequestBuilderTests.java index 0b9de2da33288..612437a4b992c 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/HasPrivilegesRequestBuilderTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/HasPrivilegesRequestBuilderTests.java @@ -116,4 +116,4 @@ public void testMissingPrivilegesThrowsException() throws Exception { ); assertThat(parseException.getMessage(), containsString("[cluster,index,applications] are missing")); } -} \ No newline at end of file +} diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java index 64d6cfd938f81..1975c9c12ac63 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java @@ -202,7 +202,7 @@ public void testAddAndGetRole() throws Exception { .cluster("all", "none") .runAs("root", "nobody") .addIndices(new String[]{"index"}, new String[]{"read"}, new String[]{"body", "title"}, null, - new BytesArray("{\"query\": {\"match_all\": {}}}"), randomBoolean()) + new BytesArray("{\"match_all\": {}}"), randomBoolean()) .metadata(metadata) .get(); logger.error("--> waiting for .security index"); @@ -219,13 +219,13 @@ public void testAddAndGetRole() throws Exception { .cluster("all", "none") .runAs("root", "nobody") .addIndices(new String[]{"index"}, new String[]{"read"}, new String[]{"body", "title"}, null, - new BytesArray("{\"query\": {\"match_all\": {}}}"), randomBoolean()) + new BytesArray("{\"match_all\": {}}"), randomBoolean()) .get(); preparePutRole("test_role3") .cluster("all", "none") .runAs("root", "nobody") .addIndices(new String[]{"index"}, new String[]{"read"}, new String[]{"body", "title"}, null, - new BytesArray("{\"query\": {\"match_all\": {}}}"), randomBoolean()) + new BytesArray("{\"match_all\": {}}"), randomBoolean()) .get(); logger.info("--> retrieving all roles"); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RoleDescriptorTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RoleDescriptorTests.java index cab4e660512c3..0c20a7c20d09c 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RoleDescriptorTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RoleDescriptorTests.java @@ -61,7 +61,7 @@ public void testToString() { .indices("i1", "i2") .privileges("read") .grantedFields("body", "title") - .query("{\"query\": {\"match_all\": {}}}") + .query("{\"match_all\": {}}") .build() }; final RoleDescriptor.ApplicationResourcePrivileges[] applicationPrivileges = { @@ -82,7 +82,7 @@ public void testToString() { assertThat(descriptor.toString(), is("Role[name=test, cluster=[all,none]" + ", global=[{APPLICATION:manage:applications=app01,app02}]" + ", indicesPrivileges=[IndicesPrivileges[indices=[i1,i2], allowRestrictedIndices=[false], privileges=[read]" + - ", field_security=[grant=[body,title], except=null], query={\"query\": {\"match_all\": {}}}],]" + + ", field_security=[grant=[body,title], except=null], query={\"match_all\": {}}],]" + ", applicationPrivileges=[ApplicationResourcePrivileges[application=my_app, privileges=[read,write], resources=[*]],]" + ", runAs=[sudo], metadata=[{}]]")); } @@ -94,7 +94,7 @@ public void testToXContent() throws Exception { .privileges("read") .grantedFields("body", "title") .allowRestrictedIndices(randomBoolean()) - .query("{\"query\": {\"match_all\": {}}}") + .query("{\"match_all\": {}}") .build() }; final RoleDescriptor.ApplicationResourcePrivileges[] applicationPrivileges = { @@ -136,7 +136,7 @@ public void testParse() throws Exception { "\"p2\"]}, {\"names\": \"idx2\", \"allow_restricted_indices\": true, \"privileges\": [\"p3\"], \"field_security\": " + "{\"grant\": [\"f1\", \"f2\"]}}, {\"names\": " + "\"idx2\", \"allow_restricted_indices\": false," + - "\"privileges\": [\"p3\"], \"field_security\": {\"grant\": [\"f1\", \"f2\"]}, \"query\": \"{\\\"match_all\\\": {}}\"}]}"; + "\"privileges\": [\"p3\"], \"field_security\": {\"grant\": [\"f1\", \"f2\"]}, \"query\": {\"match_all\": {}} }]}"; rd = RoleDescriptor.parse("test", new BytesArray(q), false, XContentType.JSON); assertEquals("test", rd.getName()); assertArrayEquals(new String[] { "a", "b" }, rd.getClusterPrivileges()); @@ -261,6 +261,18 @@ public void testParseEmptyQuery() throws Exception { assertNull(rd.getIndicesPrivileges()[0].getQuery()); } + public void testParseNullQuery() throws Exception { + String json = "{\"cluster\":[\"a\", \"b\"], \"run_as\": [\"m\", \"n\"], \"index\": [{\"names\": [\"idx1\",\"idx2\"], " + + "\"privileges\": [\"p1\", \"p2\"], \"query\": null}]}"; + RoleDescriptor rd = RoleDescriptor.parse("test", new BytesArray(json), false, XContentType.JSON); + assertEquals("test", rd.getName()); + assertArrayEquals(new String[] { "a", "b" }, rd.getClusterPrivileges()); + assertEquals(1, rd.getIndicesPrivileges().length); + assertArrayEquals(new String[] { "idx1", "idx2" }, rd.getIndicesPrivileges()[0].getIndices()); + assertArrayEquals(new String[] { "m", "n" }, rd.getRunAs()); + assertNull(rd.getIndicesPrivileges()[0].getQuery()); + } + public void testParseEmptyQueryUsingDeprecatedIndicesField() throws Exception { String json = "{\"cluster\":[\"a\", \"b\"], \"run_as\": [\"m\", \"n\"], \"indices\": [{\"names\": [\"idx1\",\"idx2\"], " + "\"privileges\": [\"p1\", \"p2\"], \"query\": \"\"}]}"; @@ -283,4 +295,5 @@ public void testParseIgnoresTransientMetadata() throws Exception { assertEquals(1, parsed.getTransientMetadata().size()); assertEquals(true, parsed.getTransientMetadata().get("enabled")); } + } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java index 3a2c30891008e..9265c1fe5e454 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java @@ -9,9 +9,13 @@ import org.apache.logging.log4j.Logger; import org.apache.lucene.util.automaton.MinimizationOperations; import org.apache.lucene.util.automaton.Operations; +import org.elasticsearch.common.ParseField; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.env.Environment; import org.elasticsearch.env.TestEnvironment; +import org.elasticsearch.index.query.MatchAllQueryBuilder; +import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.threadpool.TestThreadPool; @@ -45,6 +49,7 @@ import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; +import static java.util.Collections.singletonList; import static org.hamcrest.Matchers.arrayContaining; import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; @@ -61,11 +66,17 @@ public class FileRolesStoreTests extends ESTestCase { + @Override + protected NamedXContentRegistry xContentRegistry() { + return new NamedXContentRegistry(singletonList(new NamedXContentRegistry.Entry(QueryBuilder.class, + new ParseField(MatchAllQueryBuilder.NAME), (p, c) -> MatchAllQueryBuilder.fromXContent(p)))); + } + public void testParseFile() throws Exception { Path path = getDataPath("roles.yml"); Map roles = FileRolesStore.parseFile(path, logger, Settings.builder() .put(XPackSettings.DLS_FLS_ENABLED.getKey(), true) - .build(), new XPackLicenseState(Settings.EMPTY)); + .build(), new XPackLicenseState(Settings.EMPTY), xContentRegistry()); assertThat(roles, notNullValue()); assertThat(roles.size(), is(9)); @@ -244,7 +255,7 @@ public void testParseFileWithFLSAndDLSDisabled() throws Exception { events.clear(); Map roles = FileRolesStore.parseFile(path, logger, Settings.builder() .put(XPackSettings.DLS_FLS_ENABLED.getKey(), false) - .build(), new XPackLicenseState(Settings.EMPTY)); + .build(), new XPackLicenseState(Settings.EMPTY), xContentRegistry()); assertThat(roles, notNullValue()); assertThat(roles.size(), is(6)); assertThat(roles.get("role_fields"), nullValue()); @@ -271,7 +282,7 @@ public void testParseFileWithFLSAndDLSUnlicensed() throws Exception { events.clear(); XPackLicenseState licenseState = mock(XPackLicenseState.class); when(licenseState.isDocumentAndFieldLevelSecurityAllowed()).thenReturn(false); - Map roles = FileRolesStore.parseFile(path, logger, Settings.EMPTY, licenseState); + Map roles = FileRolesStore.parseFile(path, logger, Settings.EMPTY, licenseState, xContentRegistry()); assertThat(roles, notNullValue()); assertThat(roles.size(), is(9)); assertNotNull(roles.get("role_fields")); @@ -295,7 +306,8 @@ public void testParseFileWithFLSAndDLSUnlicensed() throws Exception { public void testDefaultRolesFile() throws Exception { // TODO we should add the config dir to the resources so we don't copy this stuff around... Path path = getDataPath("default_roles.yml"); - Map roles = FileRolesStore.parseFile(path, logger, Settings.EMPTY, new XPackLicenseState(Settings.EMPTY)); + Map roles = FileRolesStore.parseFile(path, logger, Settings.EMPTY, new XPackLicenseState(Settings.EMPTY), + xContentRegistry()); assertThat(roles, notNullValue()); assertThat(roles.size(), is(0)); } @@ -325,7 +337,7 @@ public void testAutoReload() throws Exception { FileRolesStore store = new FileRolesStore(settings, env, watcherService, roleSet -> { modifiedRoles.addAll(roleSet); latch.countDown(); - }, new XPackLicenseState(Settings.EMPTY)); + }, new XPackLicenseState(Settings.EMPTY), xContentRegistry()); Set descriptors = store.roleDescriptors(Collections.singleton("role1")); assertThat(descriptors, notNullValue()); @@ -368,7 +380,7 @@ public void testAutoReload() throws Exception { store = new FileRolesStore(settings, env, watcherService, roleSet -> { truncatedFileRolesModified.addAll(roleSet); truncateLatch.countDown(); - }, new XPackLicenseState(Settings.EMPTY)); + }, new XPackLicenseState(Settings.EMPTY), xContentRegistry()); final Set allRolesPreTruncate = store.getAllRoleNames(); try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.TRUNCATE_EXISTING)) { @@ -391,7 +403,7 @@ public void testAutoReload() throws Exception { store = new FileRolesStore(settings, env, watcherService, roleSet -> { modifiedFileRolesModified.addAll(roleSet); modifyLatch.countDown(); - }, new XPackLicenseState(Settings.EMPTY)); + }, new XPackLicenseState(Settings.EMPTY), xContentRegistry()); try (BufferedWriter writer = Files.newBufferedWriter(tmp, StandardCharsets.UTF_8, StandardOpenOption.TRUNCATE_EXISTING)) { writer.append("role5:").append(System.lineSeparator()); @@ -416,7 +428,8 @@ public void testAutoReload() throws Exception { public void testThatEmptyFileDoesNotResultInLoop() throws Exception { Path file = createTempFile(); Files.write(file, Collections.singletonList("#"), StandardCharsets.UTF_8); - Map roles = FileRolesStore.parseFile(file, logger, Settings.EMPTY, new XPackLicenseState(Settings.EMPTY)); + Map roles = FileRolesStore.parseFile(file, logger, Settings.EMPTY, new XPackLicenseState(Settings.EMPTY), + xContentRegistry()); assertThat(roles.keySet(), is(empty())); } @@ -425,7 +438,8 @@ public void testThatInvalidRoleDefinitions() throws Exception { Logger logger = CapturingLogger.newCapturingLogger(Level.ERROR, null); List entries = CapturingLogger.output(logger.getName(), Level.ERROR); entries.clear(); - Map roles = FileRolesStore.parseFile(path, logger, Settings.EMPTY, new XPackLicenseState(Settings.EMPTY)); + Map roles = FileRolesStore.parseFile(path, logger, Settings.EMPTY, new XPackLicenseState(Settings.EMPTY), + xContentRegistry()); assertThat(roles.size(), is(1)); assertThat(roles, hasKey("valid_role")); RoleDescriptor descriptor = roles.get("valid_role"); @@ -467,7 +481,8 @@ public void testReservedRoles() throws Exception { List events = CapturingLogger.output(logger.getName(), Level.ERROR); events.clear(); Path path = getDataPath("reserved_roles.yml"); - Map roles = FileRolesStore.parseFile(path, logger, Settings.EMPTY, new XPackLicenseState(Settings.EMPTY)); + Map roles = FileRolesStore.parseFile(path, logger, Settings.EMPTY, new XPackLicenseState(Settings.EMPTY), + xContentRegistry()); assertThat(roles, notNullValue()); assertThat(roles.size(), is(1)); @@ -498,7 +513,8 @@ public void testUsageStats() throws Exception { .put(XPackSettings.DLS_FLS_ENABLED.getKey(), flsDlsEnabled) .build(); Environment env = TestEnvironment.newEnvironment(settings); - FileRolesStore store = new FileRolesStore(settings, env, mock(ResourceWatcherService.class), new XPackLicenseState(Settings.EMPTY)); + FileRolesStore store = new FileRolesStore(settings, env, mock(ResourceWatcherService.class), new XPackLicenseState(Settings.EMPTY), + xContentRegistry()); Map usageStats = store.usageStats(); @@ -512,9 +528,10 @@ public void testBWCFieldPermissions() throws IOException { Path path = getDataPath("roles2xformat.yml"); byte[] bytes = Files.readAllBytes(path); String roleString = new String(bytes, Charset.defaultCharset()); - RoleDescriptor role = FileRolesStore.parseRoleDescriptor(roleString, path, logger, true, Settings.EMPTY); + RoleDescriptor role = FileRolesStore.parseRoleDescriptor(roleString, path, logger, true, Settings.EMPTY, xContentRegistry()); RoleDescriptor.IndicesPrivileges indicesPrivileges = role.getIndicesPrivileges()[0]; assertThat(indicesPrivileges.getGrantedFields(), arrayContaining("foo", "boo")); assertNull(indicesPrivileges.getDeniedFields()); } + } diff --git a/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles.yml b/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles.yml index 99459c5f5ec26..0d262addafb07 100644 --- a/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles.yml +++ b/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles.yml @@ -65,7 +65,7 @@ role_query_fields: privileges: - READ query: - match_all: + match_all: {} field_security: grant: - foo diff --git a/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles2xformat.yml b/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles2xformat.yml index ebfdce617a013..d0eb7ba4922bb 100644 --- a/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles2xformat.yml +++ b/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles2xformat.yml @@ -5,7 +5,7 @@ role1: privileges: - READ query: - match_all: + match_all: {} fields: - foo - boo \ No newline at end of file diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/roles/30_prohibited_role_query.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/roles/30_prohibited_role_query.yml index abddcdc6dda65..d3b3245632449 100644 --- a/x-pack/plugin/src/test/resources/rest-api-spec/test/roles/30_prohibited_role_query.yml +++ b/x-pack/plugin/src/test/resources/rest-api-spec/test/roles/30_prohibited_role_query.yml @@ -7,7 +7,11 @@ setup: cluster.health: wait_for_status: yellow +--- +"Test use prohibited query inside role query": + - do: + catch: /terms query with terms lookup isn't supported as part of a role query/ security.put_role: name: "role" body: > @@ -24,49 +28,3 @@ setup: ] } - - do: - security.put_user: - username: "joe" - body: > - { - "password": "x-pack-test-password", - "roles" : [ "role" ] - } - ---- -teardown: - - do: - security.delete_user: - username: "joe" - ignore: 404 - - do: - security.delete_role: - name: "role" - ignore: 404 - - ---- -"Test use prohibited query inside role query": - - - do: - headers: - Authorization: "Basic am9lOngtcGFjay10ZXN0LXBhc3N3b3Jk" - index: - index: index - type: type - id: 1 - body: > - { - "foo": "bar" - } - - - - do: - catch: /terms query with terms lookup isn't supported as part of a role query/ - headers: - Authorization: "Basic am9lOngtcGFjay10ZXN0LXBhc3N3b3Jk" - search: - rest_total_hits_as_int: true - index: index - body: { "query" : { "match_all" : {} } } - From 3a5915a602deac3b822e532c2c511b5039d3696e Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 10 Sep 2019 19:40:29 +1000 Subject: [PATCH 02/12] address review comments --- .../authz/support/DLSRoleQueryValidator.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java index 9e714095d6dc3..6b9c7f13e12a1 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java @@ -14,6 +14,7 @@ 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; @@ -31,14 +32,16 @@ import java.util.List; /** - * This class helps in evaluating the query field if it is template - * and checking if the query type is allowed to be used in DLS role query. + * 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 class DLSRoleQueryValidator { +public final class DLSRoleQueryValidator { + + private DLSRoleQueryValidator() { + } /** - * Evaluates the query field in the {@link RoleDescriptor.IndicesPrivileges} only if it is not a template - * query.
+ * Validates the query field in the {@link RoleDescriptor.IndicesPrivileges} only if it is not a template query.
* 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} @@ -63,7 +66,7 @@ public static void validateQueryField(RoleDescriptor.IndicesPrivileges[] indices private static boolean isTemplateQuery(BytesReference query, NamedXContentRegistry xContentRegistry) { try { - try (XContentParser parser = XContentFactory.xContent(query.utf8ToString()).createParser(xContentRegistry, + try (XContentParser parser = XContentType.JSON.xContent().createParser(xContentRegistry, LoggingDeprecationHandler.INSTANCE, query.utf8ToString())) { expectedToken(parser.nextToken(), parser, XContentParser.Token.START_OBJECT); expectedToken(parser.nextToken(), parser, XContentParser.Token.FIELD_NAME); From 8e849ff889bd0a2c04434b25a6a8ac8e5ab41360 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 10 Sep 2019 20:50:26 +1000 Subject: [PATCH 03/12] address review comments --- .../xpack/security/authz/store/FileRolesStore.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java index dd0f07e6d15b5..5b804de8fd274 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java @@ -211,7 +211,7 @@ public static Map parseFile(Path path, Logger logger, bo } public static Map parseRoleDescriptors(Path path, Logger logger, boolean resolvePermission, Settings settings, - ScriptService scriptService, NamedXContentRegistry xContentRegistry) { + NamedXContentRegistry xContentRegistry) { if (logger == null) { logger = NoOpLogger.INSTANCE; } From 8cc234e16381b5ee5320cfeb6c811526412be39d Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 10 Sep 2019 21:39:18 +1000 Subject: [PATCH 04/12] address review comments --- .../authz/support/DLSRoleQueryValidator.java | 13 +++++-------- .../support/SecurityQueryTemplateEvaluator.java | 4 +++- .../xpack/security/authz/store/FileRolesStore.java | 3 +-- 3 files changed, 9 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java index 6b9c7f13e12a1..a89e2d46c5182 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.core.security.authz.support; import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesReference; @@ -76,7 +77,7 @@ private static boolean isTemplateQuery(BytesReference query, NamedXContentRegist } } } catch (XContentParseException | IOException e) { - throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e); + throw new ElasticsearchParseException("failed to determine if the query is a template query", e); } return false; } @@ -105,13 +106,9 @@ public static QueryBuilder validateAndVerifyRoleQuery(BytesReference query, Scri NamedXContentRegistry xContentRegistry, User user) { QueryBuilder queryBuilder = null; if (query != null) { - try { - String templateResult = SecurityQueryTemplateEvaluator.evaluateTemplate(query.utf8ToString(), scriptService, - user); - queryBuilder = validateAndVerifyRoleQuery(templateResult, xContentRegistry); - } catch (ElasticsearchParseException | ParsingException | XContentParseException | IOException e) { - throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e); - } + String templateResult = SecurityQueryTemplateEvaluator.evaluateTemplate(query.utf8ToString(), scriptService, + user); + queryBuilder = validateAndVerifyRoleQuery(templateResult, xContentRegistry); } return queryBuilder; } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java index 73a1d7fcde509..76bc4265f6f96 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java @@ -47,7 +47,7 @@ private SecurityQueryTemplateEvaluator() { * @throws IOException thrown when there is any error parsing the query * string. */ - public static String evaluateTemplate(final String querySource, final ScriptService scriptService, final User user) throws IOException { + public static String evaluateTemplate(final String querySource, final ScriptService scriptService, final User user) { // EMPTY is safe here because we never use namedObject try (XContentParser parser = XContentFactory.xContent(querySource).createParser(NamedXContentRegistry.EMPTY, LoggingDeprecationHandler.INSTANCE, querySource)) { @@ -76,6 +76,8 @@ public static String evaluateTemplate(final String querySource, final ScriptServ } else { return querySource; } + } catch (IOException ioe) { + throw new ElasticsearchParseException("failed to parse query"); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java index 5b804de8fd274..6a95e299e56ab 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java @@ -21,7 +21,6 @@ import org.elasticsearch.common.xcontent.yaml.YamlXContent; import org.elasticsearch.env.Environment; import org.elasticsearch.license.XPackLicenseState; -import org.elasticsearch.script.ScriptService; import org.elasticsearch.watcher.FileChangesListener; import org.elasticsearch.watcher.FileWatcher; import org.elasticsearch.watcher.ResourceWatcherService; @@ -157,7 +156,7 @@ public static Path resolveFile(Environment env) { public static Set parseFileForRoleNames(Path path, Logger logger) { // EMPTY is safe here because we never use namedObject as we are just parsing role names - return parseRoleDescriptors(path, logger, false, Settings.EMPTY, null, NamedXContentRegistry.EMPTY).keySet(); + return parseRoleDescriptors(path, logger, false, Settings.EMPTY, NamedXContentRegistry.EMPTY).keySet(); } public static Map parseFile(Path path, Logger logger, Settings settings, XPackLicenseState licenseState, From 98edc47677192a857ad79c372fb9f1cb57e8d7d4 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 10 Sep 2019 21:43:07 +1000 Subject: [PATCH 05/12] precommit error --- .../security/authz/support/SecurityQueryTemplateEvaluator.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java index 76bc4265f6f96..d08d73457c4ce 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java @@ -44,8 +44,6 @@ private SecurityQueryTemplateEvaluator() { * @return resultant query string after compiling and executing the script. * If the source does not contain template then it will return the query * source without any modifications. - * @throws IOException thrown when there is any error parsing the query - * string. */ public static String evaluateTemplate(final String querySource, final ScriptService scriptService, final User user) { // EMPTY is safe here because we never use namedObject From 93d09306fe16087246b720df0eb6de4986c76859 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 10 Sep 2019 21:59:59 +1000 Subject: [PATCH 06/12] oh precommits --- .../xpack/core/security/authz/support/DLSRoleQueryValidator.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java index a89e2d46c5182..71a89f7c9a9b2 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java @@ -6,7 +6,6 @@ package org.elasticsearch.xpack.core.security.authz.support; import org.elasticsearch.ElasticsearchParseException; -import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.ParsingException; import org.elasticsearch.common.bytes.BytesReference; From a1112a5d855809589db2fd4f0ff7be34c3f373c6 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Thu, 12 Sep 2019 13:15:22 +1000 Subject: [PATCH 07/12] Address review comments - 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 --- .../authz/support/DLSRoleQueryValidator.java | 85 ++++++++++--------- .../role/TransportPutRoleActionTests.java | 2 +- 2 files changed, 45 insertions(+), 42 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java index 71a89f7c9a9b2..e8aafb9a76b86 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java @@ -50,42 +50,45 @@ private DLSRoleQueryValidator() { public static void validateQueryField(RoleDescriptor.IndicesPrivileges[] indicesPrivileges, NamedXContentRegistry xContentRegistry) { if (indicesPrivileges != null) { - for (RoleDescriptor.IndicesPrivileges idp : indicesPrivileges) { - BytesReference query = idp.getQuery(); - if (query != null) { - if (isTemplateQuery(query, xContentRegistry)) { - // skip template query, this requires runtime information like 'User' information. - continue; - } + 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; + } - validateAndVerifyRoleQuery(query.utf8ToString(), xContentRegistry); + validateAndVerifyRoleQuery(query.utf8ToString(), xContentRegistry); + } + } catch (ParsingException | IllegalArgumentException | IOException e) { + throw new ElasticsearchParseException("failed to parse field 'query' for [" + i + "]th index privilege " + + "from role descriptor", e); } } } } - private static boolean isTemplateQuery(BytesReference query, NamedXContentRegistry xContentRegistry) { - try { - try (XContentParser parser = XContentType.JSON.xContent().createParser(xContentRegistry, - LoggingDeprecationHandler.INSTANCE, query.utf8ToString())) { - expectedToken(parser.nextToken(), parser, XContentParser.Token.START_OBJECT); - expectedToken(parser.nextToken(), parser, XContentParser.Token.FIELD_NAME); - String fieldName = parser.currentName(); - if ("template".equals(fieldName)) { - return true; - } + 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; } - } catch (XContentParseException | IOException e) { - throw new ElasticsearchParseException("failed to determine if the query is a template query", e); } - return false; - } - private static void expectedToken(XContentParser.Token read, XContentParser parser, XContentParser.Token expected) { - if (read != expected) { - throw new XContentParseException(parser.getTokenLocation(), - "expected [" + expected + "] but found [" + read + "] instead"); - } + return false; } /** @@ -107,22 +110,22 @@ public static QueryBuilder validateAndVerifyRoleQuery(BytesReference query, Scri if (query != null) { String templateResult = SecurityQueryTemplateEvaluator.evaluateTemplate(query.utf8ToString(), scriptService, user); - queryBuilder = validateAndVerifyRoleQuery(templateResult, xContentRegistry); + try { + queryBuilder = validateAndVerifyRoleQuery(templateResult, xContentRegistry); + } catch (ElasticsearchParseException | ParsingException | XContentParseException | IOException e) { + throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e); + } } return queryBuilder; } - private static QueryBuilder validateAndVerifyRoleQuery(String query, NamedXContentRegistry xContentRegistry) { + private static QueryBuilder validateAndVerifyRoleQuery(String query, NamedXContentRegistry xContentRegistry) throws IOException { QueryBuilder queryBuilder = null; if (query != null) { - try { - try (XContentParser parser = XContentFactory.xContent(query).createParser(xContentRegistry, - LoggingDeprecationHandler.INSTANCE, query)) { - queryBuilder = AbstractQueryBuilder.parseInnerQueryBuilder(parser); - verifyRoleQuery(queryBuilder); - } - } catch (ElasticsearchParseException | ParsingException | XContentParseException | IOException e) { - throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e); + try (XContentParser parser = XContentFactory.xContent(query).createParser(xContentRegistry, + LoggingDeprecationHandler.INSTANCE, query)) { + queryBuilder = AbstractQueryBuilder.parseInnerQueryBuilder(parser); + verifyRoleQuery(queryBuilder); } } return queryBuilder; @@ -142,18 +145,18 @@ public static void verifyRoleQuery(QueryBuilder queryBuilder) { } else if (queryBuilder instanceof GeoShapeQueryBuilder) { GeoShapeQueryBuilder geoShapeQueryBuilder = (GeoShapeQueryBuilder) queryBuilder; if (geoShapeQueryBuilder.shape() == null) { - throw new IllegalArgumentException("geoshape query referring to indexed shapes isn't support as part of a role query"); + 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 support as part of a role query"); + 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 support as part of a role query"); + 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 support as part of a role query"); + 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 clauses = new ArrayList<>(); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java index 9126e4feecff6..c3438d5260757 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java @@ -247,6 +247,6 @@ public void onFailure(Exception e) { assertThat(responseRef.get(), is(nullValue())); assertThat(throwableRef.get(), is(notNullValue())); Throwable t = throwableRef.get(); - assertThat(t, instanceOf(IllegalArgumentException.class)); + assertThat(t, instanceOf(ElasticsearchParseException.class)); } } From 145338e05e9fd7728b502f635338da832dbcc450 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Fri, 13 Sep 2019 00:10:21 +1000 Subject: [PATCH 08/12] Address review comments - public to package protected method - return immediately for simpler methods - correct the logic in file roles and add tests --- .../authz/permission/DocumentPermissions.java | 2 +- .../authz/support/DLSRoleQueryValidator.java | 21 ++++--- .../SecurityQueryTemplateEvaluator.java | 2 +- .../permission/DocumentPermissionsTests.java | 48 -------------- .../support/DLSRoleQueryValidatorTests.java | 63 +++++++++++++++++++ .../security/authz/store/FileRolesStore.java | 1 + .../authz/store/FileRolesStoreTests.java | 8 ++- .../xpack/security/authz/store/roles.yml | 10 ++- 8 files changed, 93 insertions(+), 62 deletions(-) create mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidatorTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java index 6aed0d1213816..712e45dae1658 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissions.java @@ -116,7 +116,7 @@ private static void buildRoleQuery(User user, ScriptService scriptService, Shard BooleanQuery.Builder filter) throws IOException { for (BytesReference bytesReference : queries) { QueryShardContext queryShardContext = queryShardContextProvider.apply(shardId); - QueryBuilder queryBuilder = DLSRoleQueryValidator.validateAndVerifyRoleQuery(bytesReference, scriptService, + QueryBuilder queryBuilder = DLSRoleQueryValidator.evaluateAndVerifyRoleQuery(bytesReference, scriptService, queryShardContext.getXContentRegistry(), user); if (queryBuilder != null) { failIfQueryUsesClient(queryBuilder, queryShardContext); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java index e8aafb9a76b86..52fd8a353c07b 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java @@ -59,7 +59,7 @@ public static void validateQueryField(RoleDescriptor.IndicesPrivileges[] indices continue; } - validateAndVerifyRoleQuery(query.utf8ToString(), xContentRegistry); + evaluateAndVerifyRoleQuery(query.utf8ToString(), xContentRegistry); } } catch (ParsingException | IllegalArgumentException | IOException e) { throw new ElasticsearchParseException("failed to parse field 'query' for [" + i + "]th index privilege " + @@ -104,31 +104,31 @@ private static boolean isTemplateQuery(BytesReference query, NamedXContentRegist * * does not have a query field then it returns {@code null}. */ @Nullable - public static QueryBuilder validateAndVerifyRoleQuery(BytesReference query, ScriptService scriptService, + public static QueryBuilder evaluateAndVerifyRoleQuery(BytesReference query, ScriptService scriptService, NamedXContentRegistry xContentRegistry, User user) { - QueryBuilder queryBuilder = null; if (query != null) { String templateResult = SecurityQueryTemplateEvaluator.evaluateTemplate(query.utf8ToString(), scriptService, user); try { - queryBuilder = validateAndVerifyRoleQuery(templateResult, xContentRegistry); + return evaluateAndVerifyRoleQuery(templateResult, xContentRegistry); } catch (ElasticsearchParseException | ParsingException | XContentParseException | IOException e) { throw new ElasticsearchParseException("failed to parse field 'query' from the role descriptor", e); } } - return queryBuilder; + return null; } - private static QueryBuilder validateAndVerifyRoleQuery(String query, NamedXContentRegistry xContentRegistry) throws IOException { - QueryBuilder queryBuilder = 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 = AbstractQueryBuilder.parseInnerQueryBuilder(parser); + QueryBuilder queryBuilder = AbstractQueryBuilder.parseInnerQueryBuilder(parser); verifyRoleQuery(queryBuilder); + return queryBuilder; } } - return queryBuilder; + return null; } /** @@ -136,7 +136,8 @@ private static QueryBuilder validateAndVerifyRoleQuery(String query, NamedXConte * * @param queryBuilder {@link QueryBuilder} for given query */ - public static void verifyRoleQuery(QueryBuilder queryBuilder) { + // pkg protected for testing + static void verifyRoleQuery(QueryBuilder queryBuilder) { if (queryBuilder instanceof TermsQueryBuilder) { TermsQueryBuilder termsQueryBuilder = (TermsQueryBuilder) queryBuilder; if (termsQueryBuilder.termsLookup() != null) { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java index d08d73457c4ce..0fac101634234 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/SecurityQueryTemplateEvaluator.java @@ -75,7 +75,7 @@ public static String evaluateTemplate(final String querySource, final ScriptServ return querySource; } } catch (IOException ioe) { - throw new ElasticsearchParseException("failed to parse query"); + throw new ElasticsearchParseException("failed to parse query", ioe); } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissionsTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissionsTests.java index 799b0cec0d38d..37b6dd70e5dae 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissionsTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/permission/DocumentPermissionsTests.java @@ -6,25 +6,15 @@ package org.elasticsearch.xpack.core.security.authz.permission; -import org.apache.lucene.search.join.ScoreMode; import org.elasticsearch.client.Client; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.settings.Settings; -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.MatchAllQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.QueryRewriteContext; import org.elasticsearch.index.query.TermsQueryBuilder; -import org.elasticsearch.index.query.functionscore.FunctionScoreQueryBuilder; import org.elasticsearch.indices.TermsLookup; -import org.elasticsearch.join.query.HasChildQueryBuilder; -import org.elasticsearch.join.query.HasParentQueryBuilder; import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator; import java.io.IOException; import java.util.Collections; @@ -69,44 +59,6 @@ public void testHasDocumentPermissions() throws IOException { assertThat(ae.getMessage(), containsString("nested scoping for document permissions is not permitted")); } - public void testVerifyRoleQuery() throws Exception { - QueryBuilder queryBuilder1 = new TermsQueryBuilder("field", "val1", "val2"); - DLSRoleQueryValidator.verifyRoleQuery(queryBuilder1); - - QueryBuilder queryBuilder2 = new TermsQueryBuilder("field", new TermsLookup("_index", "_type", "_id", "_path")); - Exception e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder2)); - assertThat(e.getMessage(), equalTo("terms query with terms lookup isn't supported as part of a role query")); - - QueryBuilder queryBuilder3 = new GeoShapeQueryBuilder("field", "_id", "_type"); - e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder3)); - assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't support as part of a role query")); - - QueryBuilder queryBuilder4 = new HasChildQueryBuilder("_type", new MatchAllQueryBuilder(), ScoreMode.None); - e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder4)); - assertThat(e.getMessage(), equalTo("has_child query isn't support as part of a role query")); - - QueryBuilder queryBuilder5 = new HasParentQueryBuilder("_type", new MatchAllQueryBuilder(), false); - e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder5)); - assertThat(e.getMessage(), equalTo("has_parent query isn't support as part of a role query")); - - QueryBuilder queryBuilder6 = new BoolQueryBuilder().must(new GeoShapeQueryBuilder("field", "_id", "_type")); - e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder6)); - assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't support as part of a role query")); - - QueryBuilder queryBuilder7 = new ConstantScoreQueryBuilder(new GeoShapeQueryBuilder("field", "_id", "_type")); - e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder7)); - assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't support as part of a role query")); - - QueryBuilder queryBuilder8 = new FunctionScoreQueryBuilder(new GeoShapeQueryBuilder("field", "_id", "_type")); - e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder8)); - assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't support as part of a role query")); - - QueryBuilder queryBuilder9 = new BoostingQueryBuilder(new GeoShapeQueryBuilder("field", "_id", "_type"), - new MatchAllQueryBuilder()); - e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder9)); - assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't support as part of a role query")); - } - public void testFailIfQueryUsesClient() throws Exception { Client client = mock(Client.class); when(client.settings()).thenReturn(Settings.EMPTY); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidatorTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidatorTests.java new file mode 100644 index 0000000000000..eaa1074102cad --- /dev/null +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidatorTests.java @@ -0,0 +1,63 @@ +/* + * 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.apache.lucene.search.join.ScoreMode; +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.MatchAllQueryBuilder; +import org.elasticsearch.index.query.QueryBuilder; +import org.elasticsearch.index.query.TermsQueryBuilder; +import org.elasticsearch.index.query.functionscore.FunctionScoreQueryBuilder; +import org.elasticsearch.indices.TermsLookup; +import org.elasticsearch.join.query.HasChildQueryBuilder; +import org.elasticsearch.join.query.HasParentQueryBuilder; +import org.elasticsearch.test.ESTestCase; + +import static org.hamcrest.Matchers.equalTo; + +public class DLSRoleQueryValidatorTests extends ESTestCase { + + public void testVerifyRoleQuery() throws Exception { + QueryBuilder queryBuilder1 = new TermsQueryBuilder("field", "val1", "val2"); + DLSRoleQueryValidator.verifyRoleQuery(queryBuilder1); + + QueryBuilder queryBuilder2 = new TermsQueryBuilder("field", new TermsLookup("_index", "_type", "_id", "_path")); + Exception e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder2)); + assertThat(e.getMessage(), equalTo("terms query with terms lookup isn't supported as part of a role query")); + + QueryBuilder queryBuilder3 = new GeoShapeQueryBuilder("field", "_id", "_type"); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder3)); + assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't supported as part of a role query")); + + QueryBuilder queryBuilder4 = new HasChildQueryBuilder("_type", new MatchAllQueryBuilder(), ScoreMode.None); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder4)); + assertThat(e.getMessage(), equalTo("has_child query isn't supported as part of a role query")); + + QueryBuilder queryBuilder5 = new HasParentQueryBuilder("_type", new MatchAllQueryBuilder(), false); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder5)); + assertThat(e.getMessage(), equalTo("has_parent query isn't supported as part of a role query")); + + QueryBuilder queryBuilder6 = new BoolQueryBuilder().must(new GeoShapeQueryBuilder("field", "_id", "_type")); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder6)); + assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't supported as part of a role query")); + + QueryBuilder queryBuilder7 = new ConstantScoreQueryBuilder(new GeoShapeQueryBuilder("field", "_id", "_type")); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder7)); + assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't supported as part of a role query")); + + QueryBuilder queryBuilder8 = new FunctionScoreQueryBuilder(new GeoShapeQueryBuilder("field", "_id", "_type")); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder8)); + assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't supported as part of a role query")); + + QueryBuilder queryBuilder9 = new BoostingQueryBuilder(new GeoShapeQueryBuilder("field", "_id", "_type"), + new MatchAllQueryBuilder()); + e = expectThrows(IllegalArgumentException.class, () -> DLSRoleQueryValidator.verifyRoleQuery(queryBuilder9)); + assertThat(e.getMessage(), equalTo("geoshape query referring to indexed shapes isn't supported as part of a role query")); + } +} diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java index 6a95e299e56ab..b1059c46cc668 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java @@ -320,6 +320,7 @@ private static RoleDescriptor checkDescriptor(RoleDescriptor descriptor, Path pa logger.error((Supplier) () -> new ParameterizedMessage( "invalid role definition [{}] in roles file [{}]. failed to validate query field. skipping role...", roleName, path.toAbsolutePath()), e); + return null; } } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java index 9265c1fe5e454..99ae113e15fe9 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/FileRolesStoreTests.java @@ -246,6 +246,8 @@ public void testParseFile() throws Exception { assertTrue(group.getFieldPermissions().grantsAccessTo("boo")); assertTrue(group.getFieldPermissions().hasFieldLevelSecurity()); assertThat(group.getQuery(), notNullValue()); + + assertThat(roles.get("role_query_invalid"), nullValue()); } public void testParseFileWithFLSAndDLSDisabled() throws Exception { @@ -261,8 +263,9 @@ public void testParseFileWithFLSAndDLSDisabled() throws Exception { assertThat(roles.get("role_fields"), nullValue()); assertThat(roles.get("role_query"), nullValue()); assertThat(roles.get("role_query_fields"), nullValue()); + assertThat(roles.get("role_query_invalid"), nullValue()); - assertThat(events, hasSize(3)); + assertThat(events, hasSize(4)); assertThat( events.get(0), startsWith("invalid role definition [role_fields] in roles file [" + path.toAbsolutePath() + @@ -273,6 +276,9 @@ public void testParseFileWithFLSAndDLSDisabled() throws Exception { assertThat(events.get(2), startsWith("invalid role definition [role_query_fields] in roles file [" + path.toAbsolutePath() + "]. document and field level security is not enabled.")); + assertThat(events.get(3), + startsWith("invalid role definition [role_query_invalid] in roles file [" + path.toAbsolutePath() + + "]. document and field level security is not enabled.")); } public void testParseFileWithFLSAndDLSUnlicensed() throws Exception { diff --git a/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles.yml b/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles.yml index 0d262addafb07..c1c8bc4b1d7f8 100644 --- a/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles.yml +++ b/x-pack/plugin/security/src/test/resources/org/elasticsearch/xpack/security/authz/store/roles.yml @@ -69,4 +69,12 @@ role_query_fields: field_security: grant: - foo - - boo \ No newline at end of file + - boo + +role_query_invalid: + indices: + - names: + - 'query_idx' + privileges: + - READ + query: '{ "unknown": {} }' \ No newline at end of file From 1bcdf18f4eb40dd8bc79dcc32168745b855d4371 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Mon, 23 Sep 2019 16:36:20 +1000 Subject: [PATCH 09/12] throw error when has privileges request contains index privilege with DLS query --- .../user/TransportHasPrivilegesAction.java | 16 ++-- .../TransportHasPrivilegesActionTests.java | 73 +++++++++++++++++++ 2 files changed, 83 insertions(+), 6 deletions(-) create mode 100644 x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesActionTests.java diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java index e8d8b6c4db03a..aaa5e7c8e8426 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java @@ -9,6 +9,7 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.HandledTransportAction; +import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.tasks.Task; @@ -67,12 +68,15 @@ protected void doExecute(Task task, HasPrivilegesRequest request, ActionListener return; } - if (request.indexPrivileges() != null) { - try { - DLSRoleQueryValidator.validateQueryField(request.indexPrivileges(), xContentRegistry); - } catch (ElasticsearchException | IllegalArgumentException e) { - listener.onFailure(e); - return; + final RoleDescriptor.IndicesPrivileges[] indicesPrivileges = request.indexPrivileges(); + if (indicesPrivileges != null) { + for (int i = 0; i < indicesPrivileges.length; i++) { + BytesReference query = indicesPrivileges[i].getQuery(); + if (query != null) { + listener.onFailure( + new IllegalArgumentException("users may only check the index privileges without any DLS role query")); + return; + } } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesActionTests.java new file mode 100644 index 0000000000000..144f637ab4579 --- /dev/null +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesActionTests.java @@ -0,0 +1,73 @@ +package org.elasticsearch.xpack.security.action.user; + +import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.action.support.PlainActionFuture; +import org.elasticsearch.common.bytes.BytesArray; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.tasks.Task; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.threadpool.TestThreadPool; +import org.elasticsearch.threadpool.ThreadPool; +import org.elasticsearch.transport.TransportService; +import org.elasticsearch.xpack.core.security.SecurityContext; +import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesRequest; +import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesResponse; +import org.elasticsearch.xpack.core.security.authc.Authentication; +import org.elasticsearch.xpack.core.security.authc.AuthenticationField; +import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; +import org.elasticsearch.xpack.core.security.user.User; +import org.elasticsearch.xpack.security.authz.AuthorizationService; +import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore; +import org.junit.After; +import org.junit.Before; + +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.notNullValue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class TransportHasPrivilegesActionTests extends ESTestCase { + private ThreadPool threadPool; + + @Before + public void createThreadPool() { + threadPool = new TestThreadPool("has privileges action tests"); + } + + @After + public void stopThreadPool() { + terminate(threadPool); + } + + public void testHasPrivilegesRequestDoesNotAllowDLSRoleQueryBasedIndicesPrivileges() { + final ThreadContext threadContext = threadPool.getThreadContext(); + final SecurityContext context = mock(SecurityContext.class); + final User user = new User("user-1", "superuser"); + final Authentication authentication = new Authentication(user, + new Authentication.RealmRef("native", "default_native", "node1"), null); + when(context.getAuthentication()).thenReturn(authentication); + threadContext.putTransient(AuthenticationField.AUTHENTICATION_KEY, authentication); + final TransportHasPrivilegesAction transportHasPrivilegesAction = new TransportHasPrivilegesAction(threadPool, + mock(TransportService.class), mock(ActionFilters.class), mock(AuthorizationService.class), mock(NativePrivilegeStore.class), + context, xContentRegistry()); + + final HasPrivilegesRequest request = new HasPrivilegesRequest(); + final RoleDescriptor.IndicesPrivileges[] indicesPrivileges = new RoleDescriptor.IndicesPrivileges[randomIntBetween(1, 5)]; + for (int i = 0; i < indicesPrivileges.length; i++) { + indicesPrivileges[i] = RoleDescriptor.IndicesPrivileges.builder() + .privileges(randomFrom("read", "write")) + .indices(randomAlphaOfLengthBetween(2, 8)) + .query(new BytesArray(randomAlphaOfLength(5))) + .build(); + } + request.indexPrivileges(indicesPrivileges); + request.username("user-1"); + + final PlainActionFuture listener = new PlainActionFuture<>(); + transportHasPrivilegesAction.doExecute(mock(Task.class), request, listener); + + final IllegalArgumentException ile = expectThrows(IllegalArgumentException.class, () -> listener.actionGet()); + assertThat(ile, notNullValue()); + assertThat(ile.getMessage(), containsString("users may only check the index privileges without any DLS role query")); + } +} From dc5afd4bb79acd91a44c9c3bbea0713b18e86b24 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Mon, 23 Sep 2019 17:30:56 +1000 Subject: [PATCH 10/12] add missing license header --- .../security/action/user/TransportHasPrivilegesAction.java | 2 -- .../action/user/TransportHasPrivilegesActionTests.java | 5 +++++ 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java index aaa5e7c8e8426..7401463bf5741 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java @@ -5,7 +5,6 @@ */ package org.elasticsearch.xpack.security.action.user; -import org.elasticsearch.ElasticsearchException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.HandledTransportAction; @@ -22,7 +21,6 @@ import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor; -import org.elasticsearch.xpack.core.security.authz.support.DLSRoleQueryValidator; import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.security.authz.AuthorizationService; import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore; diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesActionTests.java index 144f637ab4579..7cd2ed7a2f730 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesActionTests.java @@ -1,3 +1,8 @@ +/* + * 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.security.action.user; import org.elasticsearch.action.support.ActionFilters; From bc901cee85f0027617ff58c0bd4e22f68315a836 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Mon, 23 Sep 2019 20:01:06 +1000 Subject: [PATCH 11/12] add better message to help users and is machine readable --- .../core/security/authz/support/DLSRoleQueryValidator.java | 6 ++++-- .../security/action/role/TransportPutRoleActionTests.java | 7 +++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java index 52fd8a353c07b..6e35172fcee65 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java @@ -8,6 +8,7 @@ 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; @@ -62,8 +63,9 @@ public static void validateQueryField(RoleDescriptor.IndicesPrivileges[] indices evaluateAndVerifyRoleQuery(query.utf8ToString(), xContentRegistry); } } catch (ParsingException | IllegalArgumentException | IOException e) { - throw new ElasticsearchParseException("failed to parse field 'query' for [" + i + "]th index privilege " + - "from role descriptor", e); + throw new ElasticsearchParseException("failed to parse field 'query' for indices [" + + Strings.arrayToCommaDelimitedString(indicesPrivileges[i].getIndices()) + + "] at [" + i + "]th index privilege from role descriptor", e); } } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java index c3438d5260757..04e68f6eb4737 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java @@ -10,6 +10,7 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.settings.Settings; @@ -215,6 +216,9 @@ public void onFailure(Exception e) { assertThat(throwableRef.get(), is(notNullValue())); Throwable t = throwableRef.get(); assertThat(t, instanceOf(ElasticsearchParseException.class)); + assertThat(t.getMessage(), containsString("failed to parse field 'query' for indices [" + + Strings.arrayToCommaDelimitedString(new String[]{"idx1"}) + + "] at [0]th index privilege from role descriptor")); } public void testCreationOfRoleWithUnsupportedQueryFails() throws Exception { @@ -248,5 +252,8 @@ public void onFailure(Exception e) { assertThat(throwableRef.get(), is(notNullValue())); Throwable t = throwableRef.get(); assertThat(t, instanceOf(ElasticsearchParseException.class)); + assertThat(t.getMessage(), containsString("failed to parse field 'query' for indices [" + + Strings.arrayToCommaDelimitedString(new String[]{"idx1"}) + + "] at [0]th index privilege from role descriptor")); } } From 0708e44e3177cdd23ca612c0c3a00f54506ff044 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Wed, 25 Sep 2019 15:16:01 +1000 Subject: [PATCH 12/12] Address review comments --- .../core/security/authz/support/DLSRoleQueryValidator.java | 2 +- .../security/action/role/TransportPutRoleActionTests.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java index 6e35172fcee65..1e4df7e8a4a94 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/support/DLSRoleQueryValidator.java @@ -65,7 +65,7 @@ public static void validateQueryField(RoleDescriptor.IndicesPrivileges[] indices } catch (ParsingException | IllegalArgumentException | IOException e) { throw new ElasticsearchParseException("failed to parse field 'query' for indices [" + Strings.arrayToCommaDelimitedString(indicesPrivileges[i].getIndices()) + - "] at [" + i + "]th index privilege from role descriptor", e); + "] at index privilege [" + i + "] of role descriptor", e); } } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java index 04e68f6eb4737..036ac75459b61 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/role/TransportPutRoleActionTests.java @@ -218,7 +218,7 @@ public void onFailure(Exception e) { assertThat(t, instanceOf(ElasticsearchParseException.class)); assertThat(t.getMessage(), containsString("failed to parse field 'query' for indices [" + Strings.arrayToCommaDelimitedString(new String[]{"idx1"}) + - "] at [0]th index privilege from role descriptor")); + "] at index privilege [0] of role descriptor")); } public void testCreationOfRoleWithUnsupportedQueryFails() throws Exception { @@ -254,6 +254,6 @@ public void onFailure(Exception e) { assertThat(t, instanceOf(ElasticsearchParseException.class)); assertThat(t.getMessage(), containsString("failed to parse field 'query' for indices [" + Strings.arrayToCommaDelimitedString(new String[]{"idx1"}) + - "] at [0]th index privilege from role descriptor")); + "] at index privilege [0] of role descriptor")); } }