From 4fba30e4738daa2090d69f34998f56ad5ca5beed Mon Sep 17 00:00:00 2001 From: cliu123 Date: Wed, 1 Jun 2022 15:34:55 -0700 Subject: [PATCH 1/5] Prevent recursive action groups Signed-off-by: cliu123 --- .../dlic/rest/api/ActionGroupsApiAction.java | 37 +++++++++++++++++++ .../rest/api/PatchableResourceApiAction.java | 28 ++++++++++++++ .../dlic/rest/api/ActionGroupsApiTest.java | 23 ++++++++++++ 3 files changed, 88 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..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]); From 5e1ff855d26d1ef3ec323b7a1174007bf7379669 Mon Sep 17 00:00:00 2001 From: cliu123 Date: Thu, 2 Jun 2022 11:21:50 -0700 Subject: [PATCH 2/5] Resovle review comments Signed-off-by: cliu123 --- .../dlic/rest/api/ActionGroupsApiAction.java | 26 +++++++++---------- .../rest/api/PatchableResourceApiAction.java | 15 +++++------ 2 files changed, 18 insertions(+), 23 deletions(-) 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 e27905f66e..f8531bb72f 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 @@ -18,6 +18,7 @@ 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; @@ -113,23 +114,20 @@ protected void handlePut(RestChannel channel, RestRequest request, Client client } // 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; - } + 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 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; - } + final SecurityDynamicConfiguration existingActionGroupsConfig = load(getConfigName(), false); + existingActionGroupsConfig.putCObject(name, DefaultObjectMapper.readTree(content, existingActionGroupsConfig.getImplementingClass())); + List allowedActions = ((ActionGroupsV7) existingActionGroupsConfig.getCEntry(name)).getAllowed_actions(); + if (allowedActions.contains(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 1d4abd4f0f..3d5529ee8e 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; @@ -159,7 +160,7 @@ private void handleSinglePatch(RestChannel channel, RestRequest request, Client , existingConfiguration.getVersion(), existingConfiguration.getSeqNo(), existingConfiguration.getPrimaryTerm()); if (existingConfiguration.getCType().equals(CType.ACTIONGROUPS)) { - if(selfReferencingActionGroup(mdc, name)) { + if(hasActionGroupSelfReference(mdc, name)) { badRequestResponse(channel, name + " cannot be an allowed_action of itself"); return; } @@ -235,7 +236,7 @@ private void handleBulkPatch(RestChannel channel, RestRequest request, Client cl if (existingConfiguration.getCType().equals(CType.ACTIONGROUPS)) { for (String actiongroup : mdc.getCEntries().keySet()) { - if(selfReferencingActionGroup(mdc, actiongroup)) { + if(hasActionGroupSelfReference(mdc, actiongroup)) { badRequestResponse(channel, actiongroup + " cannot be an allowed_action of itself"); return; } @@ -280,12 +281,8 @@ private AbstractConfigurationValidator getValidator(RestRequest request, JsonNod } // 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; + private Boolean hasActionGroupSelfReference(SecurityDynamicConfiguration mdc, String name) { + List allowedActions = ((ActionGroupsV7) mdc.getCEntry(name)).getAllowed_actions(); + return allowedActions.contains(name); } } From a126c15a200fb6289dab7bb10635344ef3d2a0c7 Mon Sep 17 00:00:00 2001 From: cliu123 Date: Thu, 2 Jun 2022 14:33:09 -0700 Subject: [PATCH 3/5] Resolve review comments Signed-off-by: cliu123 --- .../security/dlic/rest/api/ActionGroupsApiAction.java | 2 +- .../security/dlic/rest/api/PatchableResourceApiAction.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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 f8531bb72f..98ae804899 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 @@ -125,7 +125,7 @@ protected void handlePut(RestChannel channel, RestRequest request, Client client final SecurityDynamicConfiguration existingActionGroupsConfig = load(getConfigName(), false); existingActionGroupsConfig.putCObject(name, DefaultObjectMapper.readTree(content, existingActionGroupsConfig.getImplementingClass())); List allowedActions = ((ActionGroupsV7) existingActionGroupsConfig.getCEntry(name)).getAllowed_actions(); - if (allowedActions.contains(name)) { + if (hasActionGroupSelfReference(existingActionGroupsConfig, name)) { badRequestResponse(channel, name + " cannot be an allowed_action of itself"); return; } 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 3d5529ee8e..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 @@ -281,7 +281,7 @@ private AbstractConfigurationValidator getValidator(RestRequest request, JsonNod } // Prevent the case where action group references to itself in the allowed_actions. - private Boolean hasActionGroupSelfReference(SecurityDynamicConfiguration mdc, String name) { + protected Boolean hasActionGroupSelfReference(SecurityDynamicConfiguration mdc, String name) { List allowedActions = ((ActionGroupsV7) mdc.getCEntry(name)).getAllowed_actions(); return allowedActions.contains(name); } From 1cf532e2d7c427de301585f9318b1ebe4823644d Mon Sep 17 00:00:00 2001 From: cliu123 Date: Thu, 2 Jun 2022 15:18:02 -0700 Subject: [PATCH 4/5] Resolve review comment Signed-off-by: cliu123 --- .../opensearch/security/dlic/rest/api/ActionGroupsApiAction.java | 1 - 1 file changed, 1 deletion(-) 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 98ae804899..865064d00d 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 @@ -124,7 +124,6 @@ protected void handlePut(RestChannel channel, RestRequest request, Client client // 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())); - List allowedActions = ((ActionGroupsV7) existingActionGroupsConfig.getCEntry(name)).getAllowed_actions(); if (hasActionGroupSelfReference(existingActionGroupsConfig, name)) { badRequestResponse(channel, name + " cannot be an allowed_action of itself"); return; From f6a9dcb9d9b11bbc013713d91baecd0f822a3b84 Mon Sep 17 00:00:00 2001 From: cliu123 Date: Thu, 2 Jun 2022 15:22:41 -0700 Subject: [PATCH 5/5] Remove unused import Signed-off-by: cliu123 --- .../opensearch/security/dlic/rest/api/ActionGroupsApiAction.java | 1 - 1 file changed, 1 deletion(-) 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 865064d00d..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 @@ -41,7 +41,6 @@ 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;