Skip to content

Commit

Permalink
Prevent recursive action groups (opensearch-project#1868)
Browse files Browse the repository at this point in the history 
Signed-off-by: cliu123 <[email protected]>
  • Loading branch information
@cliu123
cliu123 authored Jun 6, 2022
1 parent 1db7703 commit 9789f02
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 0 deletions.
33 changes: 33 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 java.util.Set;

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.ssl.transport.PrincipalExtractor;
import org.opensearch.threadpool.ThreadPool;

Expand Down Expand Up @@ -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<String> 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);
}
}
25 changes: 25 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 @@ -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;
Expand Down Expand Up @@ -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;

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

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

@Override
Expand Down Expand Up @@ -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<String> allowedActions = ((ActionGroupsV7) mdc.getCEntry(name)).getAllowed_actions();
return allowedActions.contains(name);
}
}
23 changes: 23 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,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]);
Expand Down

0 comments on commit 9789f02

Please sign in to comment.