From eead2912c0983460d40d6544d9cce880c636f9d6 Mon Sep 17 00:00:00 2001 From: cliu123 Date: Wed, 1 Jun 2022 15:34:55 -0700 Subject: [PATCH] Prevent recursive action groups Signed-off-by: cliu123 --- .../dlic/rest/api/ActionGroupsApiAction.java | 37 +++++++++++++++++++ .../rest/api/PatchableResourceApiAction.java | 28 ++++++++++++++ .../dlic/rest/api/ActionGroupsApiTest.java | 27 ++++++++++++++ 3 files changed, 92 insertions(+) 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..e27905f66e 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,11 @@ package org.opensearch.security.dlic.rest.api; +import java.io.IOException; import java.nio.file.Path; import java.util.List; +import com.fasterxml.jackson.databind.JsonNode; import com.google.common.collect.ImmutableList; import org.opensearch.client.Client; @@ -25,9 +27,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 +39,8 @@ 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.securityconf.impl.v7.ActionGroupsV7; import org.opensearch.security.ssl.transport.PrincipalExtractor; import org.opensearch.threadpool.ThreadPool; @@ -97,4 +103,35 @@ 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 existingRoles = load(CType.ROLES, false); + for (String role : existingRoles.getCEntries().keySet()) { + if (role.equals(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 existingActionGroups = load(getConfigName(), false); + existingActionGroups.putCObject(name, DefaultObjectMapper.readTree(content, existingActionGroups.getImplementingClass())); + + for (String allowed_action : ((ActionGroupsV7) existingActionGroups.getCEntry(name)).getAllowed_actions()) { + if (allowed_action.equals(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..1d4abd4f0f 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 @@ -47,7 +47,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 +158,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(selfReferencingActionGroup(mdc, name)) { + badRequestResponse(channel, name + " cannot be an allowed_action of itself"); + return; + } + } + saveAnUpdateConfigs(client, request, getConfigName(), mdc, new OnSucessActionListener(channel){ @Override @@ -224,6 +233,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(selfReferencingActionGroup(mdc, actiongroup)) { + badRequestResponse(channel, actiongroup + " cannot be an allowed_action of itself"); + return; + } + } + } + saveAnUpdateConfigs(client, request, getConfigName(), mdc, new OnSucessActionListener(channel) { @Override @@ -260,4 +278,14 @@ 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. + private Boolean selfReferencingActionGroup(SecurityDynamicConfiguration mdc, String name) { + for (String allowed_action : ((ActionGroupsV7) mdc.getCEntry(name)).getAllowed_actions()) { + if (allowed_action.equals(name)) { + return true; + } + } + return false; + } } 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..5e54d11402 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,21 @@ 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")); + + response = rh.executePatchRequest(ENDPOINT, "[{ \"op\": \"add\", \"path\": \"/BULKNEW1\", \"value\": {\"allowed_actions\": [\"BULKNEW2\"] } }," + "{ \"op\": \"add\", \"path\": \"/BULKNEW2\", \"value\": {\"allowed_actions\": [\"BULKNEW2\"] } }]", new Header[0]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertTrue(response.getBody().contains("BULKNEW2 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]);