Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent recursive action groups #1868

Merged
merged 5 commits into from
Jun 6, 2022
Merged

Prevent recursive action groups #1868

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4fba30e
Prevent recursive action groups
cliu123 Jun 1, 2022
5e1ff85
Resovle review comments
cliu123 Jun 2, 2022
a126c15
Resolve review comments
cliu123 Jun 2, 2022
1cf532e
Resolve review comment
cliu123 Jun 2, 2022
f6a9dcb
Remove unused import
cliu123 Jun 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,33 @@

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.securityconf.impl.v7.ActionGroupsV7;
import org.opensearch.security.ssl.transport.PrincipalExtractor;
import org.opensearch.threadpool.ThreadPool;

Expand Down Expand Up @@ -97,4 +104,32 @@ 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()));
List<String> allowedActions = ((ActionGroupsV7) existingActionGroupsConfig.getCEntry(name)).getAllowed_actions();
peternied marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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