From 974e2ea27b41636d8fd05cf66619568cf7357dde Mon Sep 17 00:00:00 2001 From: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> Date: Wed, 11 Oct 2023 20:03:52 -0400 Subject: [PATCH] [Backport 2.x] For read-only tenants filter with allow list (#3528) When tenants were considered readonly an deny list was used, this could be prone to error, switch to an allow list instead. Added tests to confirm prevent and new state. (cherry picked from commit 0914f8ce51d25203a2a5dd9ba133d82e10cfed45) Co-authored-by: Peter Nied --- .../PrivilegesInterceptorImpl.java | 12 +- .../multitenancy/test/MultitenancyTests.java | 125 ++++++++++++++++++ .../resources/multitenancy/roles_mapping.yml | 18 +++ .../resources/multitenancy/roles_tenants.yml | 4 + 4 files changed, 158 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/configuration/PrivilegesInterceptorImpl.java b/src/main/java/org/opensearch/security/configuration/PrivilegesInterceptorImpl.java index 3f7b4737bf..e4d75c5611 100644 --- a/src/main/java/org/opensearch/security/configuration/PrivilegesInterceptorImpl.java +++ b/src/main/java/org/opensearch/security/configuration/PrivilegesInterceptorImpl.java @@ -15,6 +15,7 @@ import java.util.Set; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -62,6 +63,15 @@ public class PrivilegesInterceptorImpl extends PrivilegesInterceptor { "0-1" ); + private static final ImmutableSet READ_ONLY_ALLOWED_ACTIONS = ImmutableSet.of( + "indices:admin/get", + "indices:data/read/get", + "indices:data/read/search", + "indices:data/read/msearch", + "indices:data/read/mget", + "indices:data/read/mget[shard]" + ); + protected final Logger log = LogManager.getLogger(this.getClass()); public PrivilegesInterceptorImpl( @@ -90,7 +100,7 @@ private boolean isTenantAllowed( log.debug("request " + request.getClass()); } - if (tenants.get(requestedTenant) == Boolean.FALSE && action.startsWith("indices:data/write")) { + if (tenants.get(requestedTenant) == Boolean.FALSE && !READ_ONLY_ALLOWED_ACTIONS.contains(action)) { log.warn("Tenant {} is not allowed to write (user: {})", requestedTenant, user.getName()); return false; } 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 598d1662d7..e31e4de425 100644 --- a/src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java +++ b/src/test/java/org/opensearch/security/multitenancy/test/MultitenancyTests.java @@ -623,4 +623,129 @@ public void testMultitenancyAnonymousUser() throws Exception { res = rh.executeGetRequest(url, new BasicHeader("securitytenant", "human_resources")); Assert.assertEquals(HttpStatus.SC_FORBIDDEN, res.getStatusCode()); } + + @Test + public void testMultitenancyUserReadOnlyActions() throws Exception { + setup(Settings.EMPTY); + + /* Create the tenant for the anonymous user to run the tests */ + final String tenant = "test_tenant_ro"; + + final TenantExpectation tenantExpectation = new TenantExpectation(); + tenantExpectation.isTenantWritable = "false"; + tenantExpectation.createDocStatusCode = HttpStatus.SC_FORBIDDEN; + tenantExpectation.updateDocStatusCode = HttpStatus.SC_FORBIDDEN; + tenantExpectation.updateIndexStatusCode = HttpStatus.SC_FORBIDDEN; + tenantExpectation.deleteIndexStatuCode = HttpStatus.SC_FORBIDDEN; + + verifyTenantActions(nonSslRestHelper(), tenant, tenantExpectation, encodeBasicHeader("user_a", "user_a")); + } + + @Test + public void testMultitenancyUserReadWriteActions() throws Exception { + setup(Settings.EMPTY); + + /* Create the tenant for the anonymous user to run the tests */ + final String tenant = "opendistro_security_anonymous"; + + final TenantExpectation tenantExpectation = new TenantExpectation(); + tenantExpectation.isTenantWritable = "true"; + tenantExpectation.createDocStatusCode = HttpStatus.SC_CREATED; + tenantExpectation.updateDocStatusCode = HttpStatus.SC_OK; + tenantExpectation.updateIndexStatusCode = HttpStatus.SC_OK; + tenantExpectation.deleteIndexStatuCode = HttpStatus.SC_BAD_REQUEST; // tenant index cannot be deleted because its an alias + + verifyTenantActions(nonSslRestHelper(), tenant, tenantExpectation, encodeBasicHeader("admin", "admin")); + } + + @Test + public void testMultitenancyAnonymousUserReadOnlyActions() throws Exception { + setup(Settings.EMPTY, new DynamicSecurityConfig().setConfig("config_anonymous.yml"), Settings.EMPTY); + + /* Create the tenant for the anonymous user to run the tests */ + final String tenant = "anonymous_tenant"; + + final TenantExpectation tenantExpectation = new TenantExpectation(); + tenantExpectation.isTenantWritable = "false"; + tenantExpectation.createDocStatusCode = HttpStatus.SC_FORBIDDEN; + tenantExpectation.updateDocStatusCode = HttpStatus.SC_FORBIDDEN; + tenantExpectation.updateIndexStatusCode = HttpStatus.SC_FORBIDDEN; + tenantExpectation.deleteIndexStatuCode = HttpStatus.SC_FORBIDDEN; + + verifyTenantActions(nonSslRestHelper(), tenant, tenantExpectation, /* Anonymous user*/ null); + } + + @Test + public void testMultitenancyAnonymousUserWriteActionAllowed() throws Exception { + setup(Settings.EMPTY, new DynamicSecurityConfig().setConfig("config_anonymous.yml"), Settings.EMPTY); + + /* Create the tenant for the anonymous user to run the tests */ + final String tenant = "opendistro_security_anonymous"; + + final TenantExpectation tenantExpectation = new TenantExpectation(); + tenantExpectation.isTenantWritable = "true"; + tenantExpectation.createDocStatusCode = HttpStatus.SC_FORBIDDEN; // tenant write permissions != index write permissions + tenantExpectation.updateDocStatusCode = HttpStatus.SC_FORBIDDEN; // tenant write permissions != index write permissions + tenantExpectation.updateIndexStatusCode = HttpStatus.SC_OK; + tenantExpectation.deleteIndexStatuCode = HttpStatus.SC_BAD_REQUEST; // tenant index cannot be deleted because its an alias + + verifyTenantActions(nonSslRestHelper(), tenant, tenantExpectation, /* Anonymous user*/ null); + } + + private static void verifyTenantActions( + final RestHelper rh, + final String tenant, + final TenantExpectation tenantExpectation, + final Header asUser + ) { + final BasicHeader inTenant = new BasicHeader("securitytenant", tenant); + final HttpResponse adminIndexDocToCreateTenant = rh.executePutRequest( + ".kibana/_doc/5.6.0", + "{\"buildNum\": 15460, \"defaultIndex\": \"anon\", \"tenant\": \"" + tenant + "\"}", + encodeBasicHeader("admin", "admin"), + inTenant + ); + assertThat(adminIndexDocToCreateTenant.getBody(), adminIndexDocToCreateTenant.getStatusCode(), equalTo(HttpStatus.SC_CREATED)); + + final HttpResponse authInfo = rh.executeGetRequest("/_opendistro/_security/authinfo?pretty", inTenant, asUser); + assertThat(authInfo.getBody(), authInfo.findValueInJson("tenants." + tenant), equalTo(tenantExpectation.isTenantWritable)); + + final HttpResponse search = rh.executeGetRequest(".kibana/_search", inTenant, asUser); + assertThat(search.getBody(), search.getStatusCode(), equalTo(HttpStatus.SC_OK)); + + final HttpResponse msearch = rh.executePostRequest(".kibana/_msearch", "{}\n{\"query\":{\"match_all\":{}}}\n", inTenant, asUser); + assertThat(msearch.getBody(), msearch.getStatusCode(), equalTo(HttpStatus.SC_OK)); + + final HttpResponse mget = rh.executePostRequest(".kibana/_mget", "{\"docs\":[{\"_id\":\"5.6.0\"}]}", inTenant, asUser); + assertThat(mget.getBody(), mget.getStatusCode(), equalTo(HttpStatus.SC_OK)); + + final HttpResponse getDoc = rh.executeGetRequest(".kibana/_doc/5.6.0", inTenant, asUser); + assertThat(getDoc.getBody(), getDoc.getStatusCode(), equalTo(HttpStatus.SC_OK)); + + final HttpResponse createDoc = rh.executePostRequest(".kibana/_doc", "{}", inTenant, asUser); + assertThat(createDoc.getBody(), createDoc.getStatusCode(), equalTo(tenantExpectation.createDocStatusCode)); + + final HttpResponse updateDoc = rh.executePutRequest(".kibana/_doc/5.6.0", "{}", inTenant, asUser); + assertThat(updateDoc.getBody(), updateDoc.getStatusCode(), equalTo(tenantExpectation.updateDocStatusCode)); + + final HttpResponse deleteDoc = rh.executeDeleteRequest(".kibana/_doc/5.6.0", inTenant, asUser); + assertThat(deleteDoc.getBody(), deleteDoc.getStatusCode(), equalTo(tenantExpectation.updateDocStatusCode)); + + final HttpResponse getKibana = rh.executeGetRequest(".kibana", inTenant, asUser); + assertThat(getKibana.getBody(), getKibana.getStatusCode(), equalTo(HttpStatus.SC_OK)); + + final HttpResponse closeKibana = rh.executePostRequest(".kibana/_close", "{}", inTenant, asUser); + assertThat(closeKibana.getBody(), closeKibana.getStatusCode(), equalTo(tenantExpectation.updateIndexStatusCode)); + + final HttpResponse deleteKibana = rh.executeDeleteRequest(".kibana", inTenant, asUser); + assertThat(deleteKibana.getBody(), deleteKibana.getStatusCode(), equalTo(tenantExpectation.deleteIndexStatuCode)); + } + + private static class TenantExpectation { + private String isTenantWritable; + private int createDocStatusCode; + private int updateDocStatusCode; + private int updateIndexStatusCode; + private int deleteIndexStatuCode; + } } diff --git a/src/test/resources/multitenancy/roles_mapping.yml b/src/test/resources/multitenancy/roles_mapping.yml index 397171a360..d1fb92b471 100644 --- a/src/test/resources/multitenancy/roles_mapping.yml +++ b/src/test/resources/multitenancy/roles_mapping.yml @@ -184,3 +184,21 @@ opendistro_security_anonymous_multitenancy: - "opendistro_security_anonymous" and_backend_roles: [] description: "PR#2459" +opendistro_security_kibana_testindex: + reserved: false + hidden: false + backend_roles: + hosts: [] + users: + - "user_a" + and_backend_roles: [] + description: "" +all_access: + reserved: false + hidden: false + backend_roles: + hosts: [] + users: + - "admin" + and_backend_roles: [] + description: "admin user can do everything" diff --git a/src/test/resources/multitenancy/roles_tenants.yml b/src/test/resources/multitenancy/roles_tenants.yml index a4025e94da..8402a6f301 100644 --- a/src/test/resources/multitenancy/roles_tenants.yml +++ b/src/test/resources/multitenancy/roles_tenants.yml @@ -66,3 +66,7 @@ anonymous_tenant: reserved: false hidden: false description: "PR#2459" +opendistro_security_anonymous: + reserved: false + hidden: false + description: "tenant that is writable for anonymous users"