[Forwardport 1.x] For read-only tenants filter with allow list (opens…

…earch-project#3529)

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]>
(cherry picked from commit 4e962f2)

Co-authored-by: Peter Nied <[email protected]>

src/main/java/org/opensearch/security/configuration/PrivilegesInterceptorImpl.java

@@ -55,6 +55,7 @@
 import org.opensearch.security.user.User;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 
 public class PrivilegesInterceptorImpl extends PrivilegesInterceptor {
 
@@ -65,6 +66,15 @@ public class PrivilegesInterceptorImpl extends PrivilegesInterceptor {
             IndexMetadata.SETTING_AUTO_EXPAND_REPLICAS, "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(IndexNameExpressionResolver resolver, ClusterService clusterService, Client client, ThreadPool threadPool) {
@@ -83,7 +93,7 @@ private boolean isTenantAllowed(final ActionRequest request, final String action
                 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;
             }

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.opensearch.action.admin.indices.alias.Alias;
@@ -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
@@ -391,4 +395,94 @@ public void testDashboardsAlias65() throws Exception {
         Assert.assertTrue(res.getBody().contains(".kibana_-900636979_kibanaro"));
     }
 
+    @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"));
+    }
+
+    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;
+    }
 }

src/test/resources/multitenancy/roles_mapping.yml

@@ -160,3 +160,31 @@ opendistro_security_kibana4_testindex:
   - "test"
   and_backend_roles: []
   description: "Migrated from v6"
+opendistro_security_anonymous_multitenancy:
+  reserved: false
+  hidden: false
+  backend_roles:
+  - opendistro_security_anonymous_backendrole
+  hosts: []
+  users:
+  - "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"

src/test/resources/multitenancy/roles_tenants.yml

@@ -54,3 +54,7 @@ human_resources:
   reserved: false
   hidden: false
   description: "Migrated from v6"
+opendistro_security_anonymous:
+  reserved: false
+  hidden: false
+  description: ""