diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java index aa64b421b2..5c3201aaeb 100644 --- a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java +++ b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java @@ -33,6 +33,7 @@ import java.util.concurrent.Executors; import java.util.concurrent.Future; import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; import java.util.stream.Collectors; import com.google.common.base.Joiner; @@ -97,13 +98,13 @@ public ConfigModelV7( log.error("Cannot apply roles mapping resolution", e); rolesMappingResolution = ConfigConstants.RolesMappingResolution.MAPPING_ONLY; } - + agr = reloadActionGroups(actiongroups); securityRoles = reload(roles); tenantHolder = new TenantHolder(roles, tenants); roleMappingHolder = new RoleMappingHolder(rolemappings, dcm.getHostsResolverMode()); } - + public Set getAllConfiguredTenantNames() { return Collections.unmodifiableSet(tenants.getCEntries().keySet()); } @@ -115,7 +116,7 @@ public SecurityRoles getSecurityRoles() { private static interface ActionGroupResolver { Set resolvedActions(final List actions); } - + private ActionGroupResolver reloadActionGroups(SecurityDynamicConfiguration actionGroups) { return new ActionGroupResolver() { @@ -1014,10 +1015,6 @@ private static boolean impliesTypePerm(Set ipatterns, Resolved res ); } - - - //####### - private class TenantHolder { private SetMultimap> tenantsMM = null; @@ -1028,7 +1025,7 @@ public TenantHolder(SecurityDynamicConfiguration roles, SecurityDynamicC final ExecutorService execs = Executors.newFixedThreadPool(10); for(Entry securityRole: roles.getCEntries().entrySet()) { - + if(securityRole.getValue() == null) { continue; } @@ -1038,14 +1035,21 @@ public TenantHolder(SecurityDynamicConfiguration roles, SecurityDynamicC public Tuple>> call() throws Exception { final Set> tuples = new HashSet<>(); final List tenants = securityRole.getValue().getTenant_permissions(); - if (tenants != null) { for (RoleV7.Tenant tenant : tenants) { - - for(String matchingTenant: WildcardMatcher.from(tenant.getTenant_patterns()).getMatchAny(definedTenants.getCEntries().keySet(), Collectors.toList())) { + + // find Wildcarded tenant patterns + List matchingTenants = WildcardMatcher.from(tenant.getTenant_patterns()).getMatchAny(definedTenants.getCEntries().keySet(), Collectors.toList()) ; + for(String matchingTenant: matchingTenants ) { tuples.add(new Tuple(matchingTenant, agr.resolvedActions(tenant.getAllowed_actions()).contains("kibana:saved_objects/*/write"))); } + // find parameter substitution specified tenant + Pattern parameterPattern = Pattern.compile("^\\$\\{attr"); + List matchingParameterTenantList = tenant.getTenant_patterns().stream().filter(parameterPattern.asPredicate()).collect(Collectors.toList()); + for(String matchingParameterTenant : matchingParameterTenantList ) { + tuples.add(new Tuple(matchingParameterTenant,agr.resolvedActions(tenant.getAllowed_actions()).contains("kibana:saved_objects/*/write"))) ; + } } } @@ -1096,11 +1100,22 @@ public Map mapTenants(final User user, Set roles) { result.put(user.getName(), true); tenantsMM.entries().stream().filter(e -> roles.contains(e.getKey())).filter(e -> !user.getName().equals(e.getValue().v1())).forEach(e -> { - final String tenant = e.getValue().v1(); + + // replaceProperties for tenant name because + // at this point e.getValue().v1() can be in this form : "${attr.[internal|jwt|proxy|ldap].*}" + // let's substitute it with the eventual value of the user's attribute + final String tenant = replaceProperties(e.getValue().v1(),user); final boolean rw = e.getValue().v2(); if (rw || !result.containsKey(tenant)) { //RW outperforms RO - result.put(tenant, rw); + + // We want to make sure that we add a tenant that exissts + // Indeed, because we don't have control over what will be + // passed on as values of users' attributes, we have to make + // sure that we don't allow them to select tenants that do not exist. + if(ConfigModelV7.this.tenants.getCEntries().keySet().contains(tenant)) { + result.put(tenant, rw); + } } }); diff --git a/src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java b/src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java index f78d86490c..3824a21eab 100644 --- a/src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java +++ b/src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java @@ -18,6 +18,7 @@ import java.util.HashMap; import java.util.Map; +import org.apache.http.Header; import org.apache.http.HttpStatus; import org.apache.http.message.BasicHeader; import org.junit.Assert; @@ -40,6 +41,9 @@ import org.opensearch.security.test.helper.rest.RestHelper; import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; + public class MultitenancyTests extends SingleClusterTest { @Override @@ -387,4 +391,54 @@ public void testDashboardsAlias65() throws Exception { Assert.assertTrue(res.getBody().contains(".kibana_-900636979_kibanaro")); } + + @Test + public void testTenantParametersSubstitution() throws Exception { + final Settings settings = Settings.builder() + .build(); + setup(settings); + final RestHelper rh = nonSslRestHelper(); + + HttpResponse res; + final String url = ".kibana/_doc/5.6.0?pretty"; + + final String tenantName = "tenant_parameters_substitution"; + final String createTenantBody = "{\"buildNum\": 15460, \"defaultIndex\": \"plop\", \"tenant\": \"" + tenantName + "\"}"; + final Header asNoAccessUser = encodeBasicHeader("hr_employee", "hr_employee"); + final Header asUser = encodeBasicHeader("user_tenant_parameters_substitution", "user_tenant_parameters_substitution"); + + final Header actOnNoAccessTenant = new BasicHeader("securitytenant", "blafasel"); + final Header actOnUserTenant = new BasicHeader("securitytenant", tenantName); + + res = rh.executePutRequest(url, createTenantBody, asUser, actOnNoAccessTenant); + assertThat(res.getStatusCode(), equalTo(HttpStatus.SC_FORBIDDEN)); + + res = rh.executePutRequest(url, createTenantBody, asNoAccessUser, actOnUserTenant); + assertThat(res.getStatusCode(), equalTo(HttpStatus.SC_FORBIDDEN)); + + res = rh.executePutRequest(url, createTenantBody, asUser, actOnUserTenant); + assertThat(res.getStatusCode(), equalTo(HttpStatus.SC_CREATED)); + + res = rh.executeGetRequest(url, asUser, actOnUserTenant); + assertThat(res.getStatusCode(), equalTo(HttpStatus.SC_OK)); + assertThat(res.findValueInJson("_source.tenant"), equalTo(tenantName)); + + + final String tenantNameAppended = "tenant_parameters_substitution_1"; + final String createTenantAppendedBody = "{\"buildNum\": 15460, \"defaultIndex\": \"plop\", \"tenant\": \"" + tenantNameAppended + "\"}"; + final Header userTenantAppended = new BasicHeader("securitytenant", tenantNameAppended); + + res = rh.executeGetRequest(url, asNoAccessUser, userTenantAppended); + assertThat(res.getStatusCode(), equalTo(HttpStatus.SC_FORBIDDEN)); + + res = rh.executeGetRequest(url, asUser, userTenantAppended); + assertThat(res.getStatusCode(), equalTo(HttpStatus.SC_NOT_FOUND)); + + res = rh.executePutRequest(url, createTenantAppendedBody, asUser, userTenantAppended); + assertThat(res.getStatusCode(), equalTo(HttpStatus.SC_CREATED)); + + res = rh.executeGetRequest(url, asUser, userTenantAppended); + assertThat(res.getStatusCode(), equalTo(HttpStatus.SC_OK)); + assertThat(res.findValueInJson("_source.tenant"), equalTo(tenantNameAppended)); + } } diff --git a/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java b/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java index 0ee0c90198..d26e04ddc7 100644 --- a/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java +++ b/src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java @@ -38,12 +38,14 @@ 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 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; @@ -72,9 +74,12 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +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 { protected final Logger log = LogManager.getLogger(RestHelper.class); @@ -357,6 +362,41 @@ public String toString() { + ", statusReason=" + statusReason + "]"; } + /** + * 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 { + final String pathEntry = jsonPathScanner.next(); + if (!currentNode.has(pathEntry)) { + fail("Unable to resolve '" + jsonDotPath + "', on path entry '" + pathEntry + "' from available fields " + currentNode.toPrettyString()); + } + currentNode = currentNode.get(pathEntry); + } 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/multitenancy/internal_users.yml b/src/test/resources/multitenancy/internal_users.yml index ecb24a31b0..809d268710 100644 --- a/src/test/resources/multitenancy/internal_users.yml +++ b/src/test/resources/multitenancy/internal_users.yml @@ -148,3 +148,11 @@ kibanaro: backend_roles: [] attributes: {} description: "Migrated from v6" +user_tenant_parameters_substitution: + hash: "$2y$12$xgJfGiHpYOkRpF9W9dXYZOpJJ4bHz3VTwdv7ZZYTwlvx7NbH62qUi" + reserved: false + hidden: false + backend_roles: [] + attributes: + attribute1: "tenant_parameters_substitution" + description: "PR# 819 / Issue#817" diff --git a/src/test/resources/multitenancy/roles.yml b/src/test/resources/multitenancy/roles.yml index 45c5e23af0..e930156b70 100644 --- a/src/test/resources/multitenancy/roles.yml +++ b/src/test/resources/multitenancy/roles.yml @@ -463,3 +463,20 @@ opendistro_security_role_starfleet_captains: - "command_tenant" allowed_actions: - "kibana_all_write" +opendistro_security_role_tenant_parameters_substitution: + reserved: false + hidden: false + description: "PR#819 / Issue#817" + cluster_permissions: + - "OPENDISTRO_SECURITY_CLUSTER_COMPOSITE_OPS" + index_permissions: + - index_patterns: + - "?kibana" + allowed_actions: + - "ALL" + tenant_permissions: + - tenant_patterns: + - "${attr.internal.attribute1}" + - "${attr.internal.attribute1}_1" + allowed_actions: + - "kibana_all_write" diff --git a/src/test/resources/multitenancy/roles_mapping.yml b/src/test/resources/multitenancy/roles_mapping.yml index 466e738a4d..5d6ea857d2 100644 --- a/src/test/resources/multitenancy/roles_mapping.yml +++ b/src/test/resources/multitenancy/roles_mapping.yml @@ -160,3 +160,12 @@ opendistro_security_kibana4_testindex: - "test" and_backend_roles: [] description: "Migrated from v6" +opendistro_security_role_tenant_parameters_substitution: + reserved: false + hidden: false + backend_roles: [] + hosts: [] + users: + - "user_tenant_parameters_substitution" + and_backend_roles: [] + description: "PR#819 / Issue#817" diff --git a/src/test/resources/multitenancy/roles_tenants.yml b/src/test/resources/multitenancy/roles_tenants.yml index bd58b5595b..26e082a42a 100644 --- a/src/test/resources/multitenancy/roles_tenants.yml +++ b/src/test/resources/multitenancy/roles_tenants.yml @@ -54,3 +54,11 @@ human_resources: reserved: false hidden: false description: "Migrated from v6" +tenant_parameters_substitution: + reserved: false + hidden: false + description: "PR#819 / Issue#817" +tenant_parameters_substitution_1: + reserved: false + hidden: false + description: "PR#819 / Issue#817"