Skip to content

Commit

Permalink
For read-only tenants filter with allow list
Browse files Browse the repository at this point in the history
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 0914f8c)
  • Loading branch information
peternied authored and DarshitChanpura committed Oct 11, 2023
1 parent 2763812 commit 8e40807
Show file tree
Hide file tree
Showing 4 changed files with 158 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
18 changes: 18 additions & 0 deletions src/test/resources/multitenancy/roles_mapping.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
4 changes: 4 additions & 0 deletions src/test/resources/multitenancy/roles_tenants.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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"

0 comments on commit 8e40807

Please sign in to comment.