Skip to content

Commit

Permalink
Adds more tests for user with no system index access and renames the …
Browse files Browse the repository at this point in the history 
…feature flag to be more intuitive

Signed-off-by: Darshit Chanpura <[email protected]>
  • Loading branch information
@DarshitChanpura
DarshitChanpura committed Sep 1, 2023
1 parent 3a222d7 commit 58666fe
Show file tree
Hide file tree
Showing 11 changed files with 331 additions and 152 deletions.
4 changes: 2 additions & 2 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1805,8 +1805,8 @@ public List<Setting<?>> getSettings() {
);
settings.add(
Setting.boolSetting(
ConfigConstants.SECURITY_SYSTEM_INDICES_ADDITIONAL_CONTROL_ENABLED_KEY,
ConfigConstants.SECURITY_SYSTEM_INDICES_ADDITIONAL_CONTROL_ENABLED_DEFAULT,
ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY,
ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_DEFAULT,
Property.NodeScope,
Property.Filtered
)
Expand Down
10 changes: 5 additions & 5 deletions src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ public class SecurityIndexAccessEvaluator {
private final WildcardMatcher denylistIndexMatcher;

private final boolean systemIndexEnabled;
private boolean systemIndicesAdditionalControlFlag;
private final boolean systemIndicesPermissionsEnabled;

public SecurityIndexAccessEvaluator(final Settings settings, AuditLog auditLog, IndexResolverReplacer irr) {
this.securityIndex = settings.get(
Expand Down Expand Up @@ -102,9 +102,9 @@ public SecurityIndexAccessEvaluator(final Settings settings, AuditLog auditLog,
securityDeniedActionMatcher = WildcardMatcher.from(
restoreSecurityIndexEnabled ? securityIndexDeniedActionPatternsList : securityIndexDeniedActionPatternsListNoSnapshot
);
systemIndicesAdditionalControlFlag = settings.getAsBoolean(
ConfigConstants.SECURITY_SYSTEM_INDICES_ADDITIONAL_CONTROL_ENABLED_KEY,
ConfigConstants.SECURITY_SYSTEM_INDICES_ADDITIONAL_CONTROL_ENABLED_DEFAULT
systemIndicesPermissionsEnabled = settings.getAsBoolean(
ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY,
ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_DEFAULT
);
}

Expand All @@ -120,7 +120,7 @@ public PrivilegesEvaluatorResponse evaluate(

// As per issue #2845, the legacy access control to indices with additional protection should be kept in place in the meantime and
// be the default.
if (systemIndicesAdditionalControlFlag) {
if (systemIndicesPermissionsEnabled) {
evaluateNewSecuredIndicesAccess(action, requestedResolved, request, task, presponse, securityRoles, isDebugEnabled);
} else {
evaluateLegacySecuredIndicesAccess(action, requestedResolved, request, task, presponse, isDebugEnabled);
Expand Down
5 changes: 2 additions & 3 deletions src/main/java/org/opensearch/security/support/ConfigConstants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -314,9 +314,8 @@ public enum RolesMappingResolution {
public static final String SYSTEM_INDEX_PERMISSION = "system:admin/system_index";
public static final String SECURITY_SYSTEM_INDICES_ENABLED_KEY = "plugins.security.system_indices.enabled";
public static final Boolean SECURITY_SYSTEM_INDICES_ENABLED_DEFAULT = false;
public static final String SECURITY_SYSTEM_INDICES_ADDITIONAL_CONTROL_ENABLED_KEY =
"plugins.security.system_indices.additional_control.enabled";
public static final Boolean SECURITY_SYSTEM_INDICES_ADDITIONAL_CONTROL_ENABLED_DEFAULT = false;
public static final String SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY = "plugins.security.system_indices.permission.enabled";
public static final Boolean SECURITY_SYSTEM_INDICES_PERMISSIONS_DEFAULT = false;
public static final String SECURITY_SYSTEM_INDICES_KEY = "plugins.security.system_indices.indices";
public static final List<String> SECURITY_SYSTEM_INDICES_DEFAULT = Collections.emptyList();

Expand Down
4 changes: 1 addition & 3 deletions src/test/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoInteractions;
import static org.mockito.Mockito.verifyNoMoreInteractions;
import static org.mockito.Mockito.when;

Expand Down Expand Up @@ -67,7 +66,7 @@ public void setupEvaluatorWithSystemIndicesControl(boolean systemIndexPermission
evaluator = new SecurityIndexAccessEvaluator(
Settings.EMPTY.builder()
.put("plugins.security.system_indices.indices", ".testSystemIndex")
.put(ConfigConstants.SECURITY_SYSTEM_INDICES_ADDITIONAL_CONTROL_ENABLED_KEY, true)
.put(ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY, true)
.put("plugins.security.system_indices.enabled", true)
.build(),
auditLog,
Expand Down Expand Up @@ -119,7 +118,6 @@ public void disableCacheOrRealtimeOnSystemIndex() {
evaluator.evaluate(searchRequest, null, UNPROTECTED_ACTION, resolved, presponse, securityRoles);
evaluator.evaluate(realtimeRequest, null, UNPROTECTED_ACTION, resolved, presponse, securityRoles);

verifyNoInteractions(presponse);
verify(searchRequest).requestCache(Boolean.FALSE);
verify(realtimeRequest).realtime(Boolean.FALSE);

Expand Down
5 changes: 4 additions & 1 deletion src/test/java/org/opensearch/security/system_indices/AbstractSystemIndicesTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ public abstract class AbstractSystemIndicesTests extends SingleClusterTest {
static final String normalUser = "normal_user";
static final Header normalUserHeader = encodeBasicHeader(normalUser, normalUser);

static final String normalUserWithoutSystemIndex = "normal_user_without_system_index";
static final Header normalUserWithoutSystemIndexHeader = encodeBasicHeader(normalUserWithoutSystemIndex, normalUserWithoutSystemIndex);

static final String createIndexSettings = "{\n"
+ " \"settings\" : {\n"
+ " \"index\" : {\n"
Expand All @@ -73,7 +76,7 @@ void setupWithSsl(boolean isSystemIndexEnabled, boolean isSystemIndexPermissionE

Settings systemIndexSettings = Settings.builder()
.put(ConfigConstants.SECURITY_SYSTEM_INDICES_ENABLED_KEY, isSystemIndexEnabled)
.put(ConfigConstants.SECURITY_SYSTEM_INDICES_ADDITIONAL_CONTROL_ENABLED_KEY, isSystemIndexPermissionEnabled)
.put(ConfigConstants.SECURITY_SYSTEM_INDICES_PERMISSIONS_ENABLED_KEY, isSystemIndexPermissionEnabled)
.putList(ConfigConstants.SECURITY_SYSTEM_INDICES_KEY, SYSTEM_INDICES)
.put("plugins.security.ssl.http.enabled", true)
.put("plugins.security.ssl.http.keystore_filepath", FileHelper.getAbsoluteFilePathFromClassPath("node-0-keystore.jks"))
Expand Down
137 changes: 85 additions & 52 deletions src/test/java/org/opensearch/security/system_indices/SystemIndexDisabledTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

package org.opensearch.security.system_indices;

import org.apache.hc.core5.http.Header;
import org.apache.hc.core5.http.HttpStatus;
import org.junit.Before;
import org.junit.Test;
Expand All @@ -19,6 +20,8 @@
import org.opensearch.core.rest.RestStatus;
import org.opensearch.security.test.helper.rest.RestHelper;

import java.io.IOException;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
Expand Down Expand Up @@ -72,23 +75,32 @@ public void testSearchAsAdmin() throws Exception {

@Test
public void testSearchAsNormalUser() throws Exception {
testSearchWithUser(normalUser, normalUserHeader);
}

@Test
public void testSearchAsNormalUserWithoutSystemIndexAccess() throws Exception {
testSearchWithUser(normalUserWithoutSystemIndex, normalUserWithoutSystemIndexHeader);
}

private void testSearchWithUser(String user, Header header) throws IOException {
RestHelper restHelper = sslRestHelper();

// search system indices
for (String index : SYSTEM_INDICES) {
// security index is only accessible by super-admin
RestHelper.HttpResponse response = restHelper.executePostRequest(index + "/_search", "", normalUserHeader);
RestHelper.HttpResponse response = restHelper.executePostRequest(index + "/_search", "", header);
if (index.equals(ACCESSIBLE_ONLY_BY_SUPER_ADMIN)) {
validateForbiddenResponse(response, "indices:data/read/search", normalUser);
validateForbiddenResponse(response, "indices:data/read/search", user);
} else {
validateSearchResponse(response, 1);
}
}

// search all indices
RestHelper.HttpResponse response = restHelper.executePostRequest("/_search", "", normalUserHeader);
RestHelper.HttpResponse response = restHelper.executePostRequest("/_search", "", header);
assertEquals(RestStatus.FORBIDDEN.getStatus(), response.getStatusCode());
validateForbiddenResponse(response, "indices:data/read/search", normalUser);
validateForbiddenResponse(response, "indices:data/read/search", user);
}

/**
Expand All @@ -109,27 +121,28 @@ public void testDeleteAsSuperAdmin() {

@Test
public void testDeleteAsAdmin() {
RestHelper restHelper = sslRestHelper();

for (String index : SYSTEM_INDICES) {
RestHelper.HttpResponse response = restHelper.executeDeleteRequest(index + "/_doc/document1", allAccessUserHeader);
allowedExceptSecurityIndex(index, response, "", allAccessUser);

response = restHelper.executeDeleteRequest(index, allAccessUserHeader);
allowedExceptSecurityIndex(index, response, "", allAccessUser);
}
testDeleteWithUser(allAccessUser, allAccessUserHeader);
}

@Test
public void testDeleteAsNormalUser() {
testDeleteWithUser(normalUser, normalUserHeader);
}

@Test
public void testDeleteAsNormalUserWithoutSystemIndexAccess() {
testDeleteWithUser(normalUserWithoutSystemIndex, normalUserWithoutSystemIndexHeader);
}

private void testDeleteWithUser(String user, Header header) {
RestHelper restHelper = sslRestHelper();

for (String index : SYSTEM_INDICES) {
RestHelper.HttpResponse response = restHelper.executeDeleteRequest(index + "/_doc/document1", normalUserHeader);
allowedExceptSecurityIndex(index, response, "", normalUser);
RestHelper.HttpResponse response = restHelper.executeDeleteRequest(index + "/_doc/document1", header);
allowedExceptSecurityIndex(index, response, "", user);

response = restHelper.executeDeleteRequest(index, normalUserHeader);
allowedExceptSecurityIndex(index, response, "", normalUser);
response = restHelper.executeDeleteRequest(index, header);
allowedExceptSecurityIndex(index, response, "", user);
}
}

Expand Down Expand Up @@ -157,23 +170,32 @@ public void testCloseOpenAsAdmin() {
RestHelper.HttpResponse response = restHelper.executePostRequest(index + "/_close", "", allAccessUserHeader);
allowedExceptSecurityIndex(index, response, "", allAccessUser);

// admin can open security index but cannot close it
// User can open the index but cannot close it
response = restHelper.executePostRequest(index + "/_open", "", allAccessUserHeader);
assertEquals(RestStatus.OK.getStatus(), response.getStatusCode());
}
}

@Test
public void testCloseOpenAsNormalUser() {
testCloseOpenWithUser(normalUser, normalUserHeader);
}

@Test
public void testCloseOpenAsNormalUserWithoutSystemIndexAccess() {
testCloseOpenWithUser(normalUserWithoutSystemIndex, normalUserWithoutSystemIndexHeader);
}

private void testCloseOpenWithUser(String user, Header header) {
RestHelper restHelper = sslRestHelper();

for (String index : SYSTEM_INDICES) {
RestHelper.HttpResponse response = restHelper.executePostRequest(index + "/_close", "", normalUserHeader);
allowedExceptSecurityIndex(index, response, "", normalUser);
RestHelper.HttpResponse response = restHelper.executePostRequest(index + "/_close", "", header);
allowedExceptSecurityIndex(index, response, "", user);

// normal user cannot open or close security index
response = restHelper.executePostRequest(index + "/_open", "", normalUserHeader);
allowedExceptSecurityIndex(index, response, "indices:admin/open", normalUser);
// User can open the index but cannot close it
response = restHelper.executePostRequest(index + "/_open", "", header);
allowedExceptSecurityIndex(index, response, "indices:admin/open", user);
}
}

Expand All @@ -195,26 +217,27 @@ public void testCreateIndexAsSuperAdmin() {

@Test
public void testCreateIndexAsAdmin() {
RestHelper restHelper = sslRestHelper();

for (String index : INDICES_FOR_CREATE_REQUEST) {
RestHelper.HttpResponse responseIndex = restHelper.executePutRequest(index, createIndexSettings, allAccessUserHeader);
assertEquals(RestStatus.OK.getStatus(), responseIndex.getStatusCode());

RestHelper.HttpResponse response = restHelper.executePostRequest(index + "/_doc", "{\"foo\": \"bar\"}", allAccessUserHeader);
assertEquals(RestStatus.CREATED.getStatus(), response.getStatusCode());
}
testCreateIndexWithUser(allAccessUserHeader);
}

@Test
public void testCreateIndexAsNormalUser() {
testCreateIndexWithUser(normalUserHeader);
}

@Test
public void testCreateIndexAsNormalUserWithoutSystemIndexAccess() {
testCreateIndexWithUser(normalUserWithoutSystemIndexHeader);
}

private void testCreateIndexWithUser(Header header) {
RestHelper restHelper = sslRestHelper();

for (String index : INDICES_FOR_CREATE_REQUEST) {
RestHelper.HttpResponse response = restHelper.executePutRequest(index, createIndexSettings, normalUserHeader);
RestHelper.HttpResponse response = restHelper.executePutRequest(index, createIndexSettings, header);
assertEquals(RestStatus.OK.getStatus(), response.getStatusCode());

response = restHelper.executePostRequest(index + "/_doc", "{\"foo\": \"bar\"}", normalUserHeader);
response = restHelper.executePostRequest(index + "/_doc", "{\"foo\": \"bar\"}", header);
assertEquals(RestStatus.CREATED.getStatus(), response.getStatusCode());
}
}
Expand All @@ -237,27 +260,28 @@ public void testUpdateAsSuperAdmin() {

@Test
public void testUpdateMappingsAsAdmin() {
RestHelper restHelper = sslRestHelper();

for (String index : SYSTEM_INDICES) {
RestHelper.HttpResponse response = restHelper.executePutRequest(index + "/_mapping", newMappings, allAccessUserHeader);
allowedExceptSecurityIndex(index, response, "", allAccessUser);

response = restHelper.executePutRequest(index + "/_settings", updateIndexSettings, allAccessUserHeader);
allowedExceptSecurityIndex(index, response, "", allAccessUser);
}
testUpdateWithUser(allAccessUser, allAccessUserHeader);
}

@Test
public void testUpdateAsNormalUser() {
testUpdateWithUser(normalUser, normalUserHeader);
}

@Test
public void testUpdateAsNormalUserWithoutSystemIndexAccess() {
testUpdateWithUser(normalUserWithoutSystemIndex, normalUserWithoutSystemIndexHeader);
}

private void testUpdateWithUser(String user, Header header) {
RestHelper restHelper = sslRestHelper();

for (String index : SYSTEM_INDICES) {
RestHelper.HttpResponse response = restHelper.executePutRequest(index + "/_mapping", newMappings, normalUserHeader);
allowedExceptSecurityIndex(index, response, "", normalUser);
RestHelper.HttpResponse response = restHelper.executePutRequest(index + "/_mapping", newMappings, header);
allowedExceptSecurityIndex(index, response, "", user);

response = restHelper.executePutRequest(index + "/_settings", updateIndexSettings, normalUserHeader);
allowedExceptSecurityIndex(index, response, "", normalUser);
response = restHelper.executePutRequest(index + "/_settings", updateIndexSettings, header);
allowedExceptSecurityIndex(index, response, "", user);
}
}

Expand Down Expand Up @@ -326,6 +350,15 @@ public void testSnapshotSystemIndicesAsAdmin() {

@Test
public void testSnapshotSystemIndicesAsNormalUser() {
testSnapshotWithUser(normalUser, normalUserHeader);
}

@Test
public void testSnapshotSystemIndicesAsNormalUserWithoutSystemIndexAccess() {
testSnapshotWithUser(normalUserWithoutSystemIndex, normalUserWithoutSystemIndexHeader);
}

private void testSnapshotWithUser(String user, Header header) {
createSnapshots();

try (Client tc = getClient()) {
Expand All @@ -340,18 +373,18 @@ public void testSnapshotSystemIndicesAsNormalUser() {
RestHelper.HttpResponse res = restHelper.executeGetRequest(snapshotRequest);
assertEquals(HttpStatus.SC_UNAUTHORIZED, res.getStatusCode());

res = restHelper.executePostRequest(snapshotRequest + "/_restore?wait_for_completion=true", "", normalUserHeader);
allowedExceptSecurityIndex(index, res, "", normalUser);
res = restHelper.executePostRequest(snapshotRequest + "/_restore?wait_for_completion=true", "", header);
allowedExceptSecurityIndex(index, res, "", user);

res = restHelper.executePostRequest(
snapshotRequest + "/_restore?wait_for_completion=true",
"{ \"rename_pattern\": \"(.+)\", \"rename_replacement\": \"restored_index_with_global_state_$1\" }",
normalUserHeader
header
);
if (index.equals(ACCESSIBLE_ONLY_BY_SUPER_ADMIN)) {
validateForbiddenResponse(res, "", normalUser);
validateForbiddenResponse(res, "", user);
} else {
validateForbiddenResponse(res, "indices:data/write/index, indices:admin/create", normalUser);
validateForbiddenResponse(res, "indices:data/write/index, indices:admin/create", user);
}
}
}
Expand Down
Loading

0 comments on commit 58666fe

Please sign in to comment.