[Backport 2.11] For read-only tenants filter with allow list

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. Signed-off-by: Peter Nied <[email protected]>
opensearch-project · Oct 11, 2023 · c3e53e2 · c3e53e2
1 parent b3f1a29
commit c3e53e2
Show file tree

Hide file tree

Showing 4 changed files with 158 additions and 1 deletion.
diff --git 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<String> 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
@@ -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
@@ -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
@@ -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"