Skip to content

Commit

Permalink
Prevent recursive action groups
Browse files Browse the repository at this point in the history 
Signed-off-by: cliu123 <[email protected]>
  • Loading branch information
@cliu123
cliu123 committed Jun 1, 2022
1 parent 94cc878 commit eead291
Show file tree
Hide file tree
Showing 3 changed files with 92 additions and 0 deletions.
37 changes: 37 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,32 @@

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;
import org.opensearch.cluster.service.ClusterService;
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;
import org.opensearch.security.dlic.rest.validation.AbstractConfigurationValidator;
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;

Expand Down Expand Up @@ -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);
}
}
28 changes: 28 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/PatchableResourceApiAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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<IndexResponse>(channel){

@Override
Expand Down Expand Up @@ -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<IndexResponse>(channel) {

@Override
Expand Down Expand Up @@ -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;
}
}
27 changes: 27 additions & 0 deletions src/test/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Expand Down Expand Up @@ -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]);
Expand Down

0 comments on commit eead291

Please sign in to comment.