From b5b63057bb64b9d9d0bcdd160cf5a57263db1ab5 Mon Sep 17 00:00:00 2001 From: Rutuja Surve Date: Tue, 26 Jul 2022 12:55:37 +0530 Subject: [PATCH] backport cluster evaluation permissions #1885 to opendistro-1.2 Signed-off-by: Rutuja Surve (cherry picked from commit 702c81aaec710a881e8f480502c60d3caf78fa0a) --- .../privileges/PrivilegesEvaluator.java | 1 + .../resolver/IndexResolverReplacer.java | 2 +- ...exTemplateClusterPermissionsCheckTest.java | 64 +++++++++++++++ .../security/test/helper/rest/RestHelper.java | 79 ++++++++++++++++++- src/test/resources/internal_users.yml | 7 ++ src/test/resources/roles.yml | 78 ++++++++++++++++++ src/test/resources/roles_mapping.yml | 36 +++++++++ 7 files changed, 264 insertions(+), 3 deletions(-) create mode 100644 src/test/java/com/amazon/opendistroforelasticsearch/security/IndexTemplateClusterPermissionsCheckTest.java diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/com/amazon/opendistroforelasticsearch/security/privileges/PrivilegesEvaluator.java index 1f23251ad1..2543af8721 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/security/privileges/PrivilegesEvaluator.java @@ -585,6 +585,7 @@ private Set evaluateAdditionalIndexPermissions(final ActionRequest reque private static boolean isClusterPerm(String action0) { return ( action0.startsWith("cluster:") || action0.startsWith("indices:admin/template/") + || action0.startsWith("indices:admin/index_template/") || action0.startsWith(SearchScrollAction.NAME) || (action0.equals(BulkAction.NAME)) diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/security/resolver/IndexResolverReplacer.java b/src/main/java/com/amazon/opendistroforelasticsearch/security/resolver/IndexResolverReplacer.java index 471e8db77b..48781b00db 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/security/resolver/IndexResolverReplacer.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/security/resolver/IndexResolverReplacer.java @@ -698,4 +698,4 @@ else if (RestoreSnapshotRequest.class.isInstance(localRequest)) { public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { respectRequestIndicesOptions = dcm.isRespectRequestIndicesEnabled(); } -} \ No newline at end of file +} diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/security/IndexTemplateClusterPermissionsCheckTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/IndexTemplateClusterPermissionsCheckTest.java new file mode 100644 index 0000000000..26aec2481f --- /dev/null +++ b/src/test/java/com/amazon/opendistroforelasticsearch/security/IndexTemplateClusterPermissionsCheckTest.java @@ -0,0 +1,64 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security; + +import org.apache.http.HttpStatus; +import org.junit.Assert; +import org.junit.Before; +import org.junit.Test; + +import org.opensearch.security.test.SingleClusterTest; +import org.opensearch.security.test.helper.rest.RestHelper; +import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; + +public class IndexTemplateClusterPermissionsCheckTest extends SingleClusterTest{ + private RestHelper rh; + + final static String indexTemplateBody = "{ \"index_patterns\": [\"sem1234*\"], \"template\": { \"settings\": { \"number_of_shards\": 2, \"number_of_replicas\": 1 }, \"mappings\": { \"properties\": { \"timestamp\": { \"type\": \"date\", \"format\": \"yyyy-MM-dd HH:mm:ss||yyyy-MM-dd||epoch_millis\" }, \"value\": { \"type\": \"double\" } } } } }"; + + private String getFailureResponseReason(String user) { + return "no permissions for [indices:admin/index_template/put] and User [name=" + user + ", backend_roles=[], requestedTenant=null]"; + } + + @Before + public void setupRestHelper() throws Exception{ + setup(); + rh = nonSslRestHelper(); + } + @Test + public void testPutIndexTemplateByNonPrivilegedUser() throws Exception { + String expectedFailureResponse = getFailureResponseReason("ds3"); + + // should fail, as user `ds3` doesn't have correct permissions + HttpResponse response = rh.executePutRequest("/_index_template/sem1234", indexTemplateBody, encodeBasicHeader("ds3", "nagilum")); + Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + Assert.assertEquals(expectedFailureResponse, response.findValueInJson("error.root_cause[0].reason")); + } + + @Test + public void testPutIndexTemplateByPrivilegedUser() throws Exception { + // should pass, as user `sem-user` has correct permissions + HttpResponse response = rh.executePutRequest("/_index_template/sem1234", indexTemplateBody, encodeBasicHeader("sem-user", "nagilum")); + Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + } + + @Test + public void testPutIndexTemplateAsIndexLevelPermission() throws Exception { + String expectedFailureResponse = getFailureResponseReason("sem-user2"); + + // should fail, as user `sem-user2` is assigned `put-template` permission as index-level, not cluster-level + HttpResponse response = rh.executePutRequest("/_index_template/sem1234", indexTemplateBody, encodeBasicHeader("sem-user2", "nagilum")); + Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + Assert.assertEquals(expectedFailureResponse, response.findValueInJson("error.root_cause[0].reason")); + } + + } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/security/test/helper/rest/RestHelper.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/test/helper/rest/RestHelper.java index 287e591d09..a837f332b4 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/security/test/helper/rest/RestHelper.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/security/test/helper/rest/RestHelper.java @@ -37,12 +37,16 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Scanner; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.Future; +import java.util.regex.Matcher; +import java.util.regex.Pattern; import javax.net.ssl.SSLContext; +import com.fasterxml.jackson.databind.JsonNode; import org.apache.commons.io.IOUtils; import org.apache.http.Header; import org.apache.http.HttpEntity; @@ -71,8 +75,11 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import com.amazon.opendistroforelasticsearch.security.test.helper.cluster.ClusterInfo; -import com.amazon.opendistroforelasticsearch.security.test.helper.file.FileHelper; +import org.opensearch.security.DefaultObjectMapper; +import org.opensearch.security.test.helper.cluster.ClusterInfo; +import org.opensearch.security.test.helper.file.FileHelper; + +import static org.junit.jupiter.api.Assertions.fail; public class RestHelper { @@ -342,6 +349,74 @@ public String toString() { + ", statusReason=" + statusReason + "]"; } + private static void findArrayAccessor(String input) { + final Pattern r = Pattern.compile("(.+?)\\[(\\d+)\\]"); + final Matcher m = r.matcher(input); + if(m.find()) { + System.out.println("'" + input + "'\t Name was: " + m.group(1) + ",\t index position: " + m.group(2)); + } else { + System.out.println("'" + input + "'\t No Match"); + } + } + + /** + * Given a json path with dots delimiated returns the object at the leaf + */ + public String findValueInJson(final String jsonDotPath) { + // Make sure its json / then parse it + if (!isJsonContentType()) { + fail("Response was expected to be JSON, body was: \n" + body); + } + JsonNode currentNode = null; + try { + currentNode = DefaultObjectMapper.readTree(body); + } catch (final Exception e) { + throw new RuntimeException(e); + } + + // Break the path into parts, and scan into the json object + try (final Scanner jsonPathScanner = new Scanner(jsonDotPath).useDelimiter("\\.")) { + if (!jsonPathScanner.hasNext()) { + fail("Invalid json dot path '" + jsonDotPath + "', rewrite with '.' characters between path elements."); + } + do { + String pathEntry = jsonPathScanner.next(); + // if pathEntry is an array lookup + int arrayEntryIdx = -1; + + // Looks for an array-lookup pattern in the path + // e.g. root_cause[1] -> will match + // e.g. root_cause[2aasd] -> won't match + final Pattern r = Pattern.compile("(.+?)\\[(\\d+)\\]"); + final Matcher m = r.matcher(pathEntry); + if(m.find()) { + pathEntry = m.group(1); + arrayEntryIdx = Integer.parseInt(m.group(2)); + } + + if (!currentNode.has(pathEntry)) { + fail("Unable to resolve '" + jsonDotPath + "', on path entry '" + pathEntry + "' from available fields " + currentNode.toPrettyString()); + } + currentNode = currentNode.get(pathEntry); + + // if it's an Array lookup we get the requested index item + if (arrayEntryIdx > -1) { + if(!currentNode.isArray()) { + fail("Unable to resolve '" + jsonDotPath + "', the '" + pathEntry + "' field is not an array " + currentNode.toPrettyString()); + } else if (!currentNode.has(arrayEntryIdx)) { + fail("Unable to resolve '" + jsonDotPath + "', index '" + arrayEntryIdx + "' is out of bounds for array '" + pathEntry + "' \n" + currentNode.toPrettyString()); + } + currentNode = currentNode.get(arrayEntryIdx); + } + } while (jsonPathScanner.hasNext()); + + if (!currentNode.isValueNode()) { + fail("Unexpected value note, index directly to the object to reference, object\n" + currentNode.toPrettyString()); + } + return currentNode.asText(); + } + } + private static HttpResponse from(Future future) { try { return future.get(); diff --git a/src/test/resources/internal_users.yml b/src/test/resources/internal_users.yml index 31401a5370..7ab1306899 100644 --- a/src/test/resources/internal_users.yml +++ b/src/test/resources/internal_users.yml @@ -338,3 +338,10 @@ hidden_test: hash: $2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m opendistro_security_roles: - hidden_test +sem-user: + hash: $2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m + #password is: nagilum +sem-user2: + hash: $2a$12$n5nubfWATfQjSYHiWtUyeOxMIxFInUHOAx8VMmGmxFNPGpaBmeB.m + #password is: nagilum + diff --git a/src/test/resources/roles.yml b/src/test/resources/roles.yml index d61d12775f..36d848d5ab 100644 --- a/src/test/resources/roles.yml +++ b/src/test/resources/roles.yml @@ -1069,11 +1069,89 @@ xyz_sr_reserved: - "*" tenant_permissions: [] +<<<<<<< HEAD hidden_test: cluster_permissions: - SGS_CLUSTER_COMPOSITE_OPS +======= +index_template_perm: + reserved: true + hidden: false + description: "Migrated from v6 (all types mapped)" + cluster_permissions: [] + index_permissions: + - index_patterns: + - "*" + allowed_actions: + - "indices:admin/index_template/*" + +data_stream_0: + reserved: true + hidden: false + description: "Migrated from v6 (all types mapped)" + cluster_permissions: [] + index_permissions: + - index_patterns: + - "*my-data-stream2*" + allowed_actions: + - "indices:admin/get" + +data_stream_1: + reserved: true + hidden: false + description: "Migrated from v6 (all types mapped)" + cluster_permissions: [ "indices:admin/index_template/put" ] + index_permissions: + - index_patterns: + - "my-data-stream*" + allowed_actions: + - "indices:admin/data_stream/get" + - "indices:admin/data_stream/create" + +data_stream_2: + reserved: true + hidden: false + description: "Migrated from v6 (all types mapped)" + cluster_permissions: [] +>>>>>>> 702c81aa (backport cluster evaluation permissions #1885 to 2.0) index_permissions: - index_patterns: - hidden_test_not_hidden allowed_actions: - "*" +<<<<<<< HEAD +======= + allowed_actions: + - "DATASTREAM_ALL" + +hidden_test: + cluster_permissions: + - SGS_CLUSTER_COMPOSITE_OPS + index_permissions: + - index_patterns: + - hidden_test_not_hidden + allowed_actions: + - "*" + +sem-role: + reserved: true + hidden: false + description: "Migrated from v6 (all types mapped)" + cluster_permissions: [ "cluster_monitor", "indices:admin/index_template/put" ] + index_permissions: + - index_patterns: + - "sem*" + allowed_actions: + - "*" + +sem-role2: + reserved: true + hidden: false + description: "Migrated from v6 (all types mapped)" + cluster_permissions: [ "cluster_monitor" ] + index_permissions: + - index_patterns: + - "sem*" + allowed_actions: + - "indices:admin/index_template/put" +>>>>>>> 702c81aa (backport cluster evaluation permissions #1885 to 2.0) diff --git a/src/test/resources/roles_mapping.yml b/src/test/resources/roles_mapping.yml index d62758a26c..435cc8c71b 100644 --- a/src/test/resources/roles_mapping.yml +++ b/src/test/resources/roles_mapping.yml @@ -388,3 +388,39 @@ xyz_sr_hidden: bulk_test_user_role: users: - "bulk_test_user" +index_template_perm: + reserved: false + hidden: false + users: + - "ds1" +data_stream_0: + reserved: false + hidden: false + users: + - "ds0" +data_stream_1: + reserved: false + hidden: false + users: + - "ds1" +data_stream_2: + reserved: false + hidden: false + users: + - "ds2" +data_stream_3: + reserved: false + hidden: false + users: + - "ds3" +sem-role: + reserved: false + hidden: false + users: + - "sem-user" +sem-role2: + reserved: false + hidden: false + users: + - "sem-user2" +