diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java index 38501dd655..7489812f02 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java @@ -15,9 +15,12 @@ package org.opensearch.security.dlic.rest.api; +import java.io.IOException; import java.nio.file.Path; import java.util.List; +import java.util.Set; +import com.fasterxml.jackson.databind.JsonNode; import com.google.common.collect.ImmutableList; import org.opensearch.client.Client; @@ -25,9 +28,11 @@ import org.opensearch.common.bytes.BytesReference; import org.opensearch.common.inject.Inject; import org.opensearch.common.settings.Settings; +import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestController; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; +import org.opensearch.security.DefaultObjectMapper; import org.opensearch.security.auditlog.AuditLog; import org.opensearch.security.configuration.AdminDNs; import org.opensearch.security.configuration.ConfigurationRepository; @@ -35,6 +40,7 @@ import org.opensearch.security.dlic.rest.validation.ActionGroupValidator; import org.opensearch.security.privileges.PrivilegesEvaluator; import org.opensearch.security.securityconf.impl.CType; +import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; import org.opensearch.security.ssl.transport.PrincipalExtractor; import org.opensearch.threadpool.ThreadPool; @@ -97,4 +103,31 @@ protected void consumeParameters(final RestRequest request) { request.param("name"); } + @Override + protected void handlePut(RestChannel channel, RestRequest request, Client client, JsonNode content) throws IOException { + final String name = request.param("name"); + + if (name == null || name.length() == 0) { + badRequestResponse(channel, "No " + getResourceName() + " specified."); + return; + } + + // Prevent the case where action group and role share a same name. + SecurityDynamicConfiguration existingRolesConfig = load(CType.ROLES, false); + Set existingRoles = existingRolesConfig.getCEntries().keySet(); + if (existingRoles.contains(name)) { + badRequestResponse(channel, name + " is an existing role. A action group cannot be named with an existing role name."); + return; + } + + // Prevent the case where action group references to itself in the allowed_actions. + final SecurityDynamicConfiguration existingActionGroupsConfig = load(getConfigName(), false); + existingActionGroupsConfig.putCObject(name, DefaultObjectMapper.readTree(content, existingActionGroupsConfig.getImplementingClass())); + if (hasActionGroupSelfReference(existingActionGroupsConfig, name)) { + badRequestResponse(channel, name + " cannot be an allowed_action of itself"); + return; + } + + super.handlePut(channel, request, client, content); + } } diff --git a/src/main/java/org/opensearch/security/dlic/rest/api/PatchableResourceApiAction.java b/src/main/java/org/opensearch/security/dlic/rest/api/PatchableResourceApiAction.java index 2dac1e2dd2..126dfb9e16 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/api/PatchableResourceApiAction.java +++ b/src/main/java/org/opensearch/security/dlic/rest/api/PatchableResourceApiAction.java @@ -19,6 +19,7 @@ import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.util.Iterator; +import java.util.List; import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.JsonNode; @@ -47,7 +48,9 @@ import org.opensearch.security.dlic.rest.support.Utils; import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator; import org.opensearch.security.privileges.PrivilegesEvaluator; +import org.opensearch.security.securityconf.impl.CType; import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration; +import org.opensearch.security.securityconf.impl.v7.ActionGroupsV7; import org.opensearch.security.ssl.transport.PrincipalExtractor; import org.opensearch.threadpool.ThreadPool; @@ -156,6 +159,13 @@ private void handleSinglePatch(RestChannel channel, RestRequest request, Client SecurityDynamicConfiguration mdc = SecurityDynamicConfiguration.fromNode(updatedAsJsonNode, existingConfiguration.getCType() , existingConfiguration.getVersion(), existingConfiguration.getSeqNo(), existingConfiguration.getPrimaryTerm()); + if (existingConfiguration.getCType().equals(CType.ACTIONGROUPS)) { + if(hasActionGroupSelfReference(mdc, name)) { + badRequestResponse(channel, name + " cannot be an allowed_action of itself"); + return; + } + } + saveAnUpdateConfigs(client, request, getConfigName(), mdc, new OnSucessActionListener(channel){ @Override @@ -224,6 +234,15 @@ private void handleBulkPatch(RestChannel channel, RestRequest request, Client cl SecurityDynamicConfiguration mdc = SecurityDynamicConfiguration.fromNode(patchedAsJsonNode, existingConfiguration.getCType() , existingConfiguration.getVersion(), existingConfiguration.getSeqNo(), existingConfiguration.getPrimaryTerm()); + if (existingConfiguration.getCType().equals(CType.ACTIONGROUPS)) { + for (String actiongroup : mdc.getCEntries().keySet()) { + if(hasActionGroupSelfReference(mdc, actiongroup)) { + badRequestResponse(channel, actiongroup + " cannot be an allowed_action of itself"); + return; + } + } + } + saveAnUpdateConfigs(client, request, getConfigName(), mdc, new OnSucessActionListener(channel) { @Override @@ -260,4 +279,10 @@ private AbstractConfigurationValidator getValidator(RestRequest request, JsonNod DefaultObjectMapper.objectMapper.writeValueAsString(patchedResource).getBytes(StandardCharsets.UTF_8)); return getValidator(request, patchedResourceAsByteReference); } + + // Prevent the case where action group references to itself in the allowed_actions. + protected Boolean hasActionGroupSelfReference(SecurityDynamicConfiguration mdc, String name) { + List allowedActions = ((ActionGroupsV7) mdc.getCEntry(name)).getAllowed_actions(); + return allowedActions.contains(name); + } } diff --git a/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiTest.java b/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiTest.java index effae8f237..29edff8e60 100644 --- a/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiTest.java +++ b/src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiTest.java @@ -194,6 +194,18 @@ public void testActionGroupsApi() throws Exception { Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode()); Assert.assertFalse(response.getBody().contains("Resource 'GET_UT' is read-only.")); + // PUT with role name + rh.sendAdminCertificate = true; + response = rh.executePutRequest(ENDPOINT+"/kibana_user", FileHelper.loadFile("restapi/actiongroup_read.json"), new Header[0]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertTrue(response.getBody().contains("kibana_user is an existing role. A action group cannot be named with an existing role name.")); + + // PUT with self-referencing action groups + rh.sendAdminCertificate = true; + response = rh.executePutRequest(ENDPOINT+"/reference_itself", "{\"allowed_actions\": [\"reference_itself\"]}", new Header[0]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertTrue(response.getBody().contains("reference_itself cannot be an allowed_action of itself")); + // -- GET_UT hidden resource, must be 404 but super admin can find it rh.sendAdminCertificate = true; response = rh.executeGetRequest(ENDPOINT+"/INTERNAL", new Header[0]); @@ -223,6 +235,17 @@ public void testActionGroupsApi() throws Exception { response = rh.executePatchRequest(ENDPOINT+"/GET_UT", "[{ \"op\": \"add\", \"path\": \"/description\", \"value\": \"foo\" }]", new Header[0]); Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode()); + // PATCH with self-referencing action groups + rh.sendAdminCertificate = true; + response = rh.executePatchRequest(ENDPOINT+"/GET_UT", "[{ \"op\": \"add\", \"path\": \"/allowed_actions/-\", \"value\": \"GET_UT\" }]", new Header[0]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertTrue(response.getBody().contains("GET_UT cannot be an allowed_action of itself")); + + // bulk PATCH with self-referencing action groups + response = rh.executePatchRequest(ENDPOINT, "[{ \"op\": \"add\", \"path\": \"/BULKNEW1\", \"value\": {\"allowed_actions\": [\"BULKNEW1\"] } }," + "{ \"op\": \"add\", \"path\": \"/BULKNEW2\", \"value\": {\"allowed_actions\": [\"READ_UT\"] } }]", new Header[0]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertTrue(response.getBody().contains("BULKNEW1 cannot be an allowed_action of itself")); + // PATCH hidden resource, must be not found, can be found by superadmin, but fails with no path exist error rh.sendAdminCertificate = true; response = rh.executePatchRequest(ENDPOINT+"/INTERNAL", "[{ \"op\": \"add\", \"path\": \"/a/b/c\", \"value\": [ \"foo\", \"bar\" ] }]", new Header[0]);