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

Jump to bottom

Improves Rest Layer Authz robustness #4879

Closed
wants to merge 3 commits into from
Closed

Improves Rest Layer Authz robustness #4879

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6108c30
Improves Rest Layer Authz robustness
DarshitChanpura Nov 5, 2024
e10c2fe
Adds and updates tests
DarshitChanpura Nov 5, 2024
8983158
Replaces * import with single line
DarshitChanpura Nov 6, 2024
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
14 changes: 14 additions & 0 deletions src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,13 @@
import org.opensearch.test.framework.audit.AuditLogsRule;
import org.opensearch.test.framework.cluster.ClusterManager;
import org.opensearch.test.framework.cluster.LocalCluster;
import org.opensearch.test.framework.cluster.RestClientException;
import org.opensearch.test.framework.cluster.TestRestClient;
import org.opensearch.test.framework.testplugins.dummy.CustomLegacyTestPlugin;
import org.opensearch.test.framework.testplugins.dummyprotected.CustomRestProtectedTestPlugin;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.opensearch.rest.RestRequest.Method.GET;
import static org.opensearch.rest.RestRequest.Method.POST;
Expand All @@ -40,6 +42,7 @@
import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL;
import static org.opensearch.test.framework.audit.AuditMessagePredicate.privilegePredicateRESTLayer;
import static org.opensearch.test.framework.audit.AuditMessagePredicate.privilegePredicateTransportLayer;
import static org.junit.Assert.assertThrows;

@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
@ThreadLeakScope(ThreadLeakScope.Scope.NONE)
Expand Down Expand Up @@ -111,6 +114,17 @@ public void testAccessDeniedForUserWithNoPermissions() {
}
}

@Test
public void testShouldFailWithoutPermForPathWithoutLeadingSlashes() {
try (TestRestClient client = cluster.getRestClient(NO_PERM)) {

// Protected Routes plugin
Exception exception = assertThrows(RestClientException.class, () -> { client.getWithoutLeadingSlash(PROTECTED_API); });

assertThat(exception.getMessage(), containsString("Error occured during HTTP request execution"));
}
}

/** AuthZ in REST Layer check */

@Test
Expand Down
15 changes: 15 additions & 0 deletions src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,13 @@
import org.opensearch.test.framework.audit.AuditLogsRule;
import org.opensearch.test.framework.cluster.ClusterManager;
import org.opensearch.test.framework.cluster.LocalCluster;
import org.opensearch.test.framework.cluster.RestClientException;
import org.opensearch.test.framework.cluster.TestRestClient;

import joptsimple.internal.Strings;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThan;
import static org.hamcrest.Matchers.lessThan;
Expand All @@ -52,6 +54,7 @@
import static org.opensearch.test.framework.audit.AuditMessagePredicate.grantedPrivilege;
import static org.opensearch.test.framework.audit.AuditMessagePredicate.privilegePredicateRESTLayer;
import static org.opensearch.test.framework.audit.AuditMessagePredicate.userAuthenticatedPredicate;
import static org.junit.Assert.assertThrows;
import static org.junit.Assert.assertTrue;

@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class)
Expand Down Expand Up @@ -138,6 +141,18 @@ public void testWhoAmIWithoutGetPermissions() {
}
}

@Test
public void testWhoAmIWithoutGetPermissionsWithoutLeadingSlashInPath() {
try (TestRestClient client = cluster.getRestClient(WHO_AM_I_NO_PERM)) {
Exception exception = assertThrows(
RestClientException.class,
() -> { client.getWithoutLeadingSlash(WHOAMI_PROTECTED_ENDPOINT); }
);

assertThat(exception.getMessage(), containsString("Error occured during HTTP request execution"));
}
}

@Test
public void testWhoAmIPost() {
try (TestRestClient client = cluster.getRestClient(WHO_AM_I)) {
Expand Down
4 changes: 4 additions & 0 deletions src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,10 @@ public HttpResponse get(String path, Header... headers) {
return executeRequest(new HttpGet(getHttpServerUri() + "/" + path), headers);
}

public void getWithoutLeadingSlash(String path, Header... headers) {
executeRequest(new HttpGet(getHttpServerUri() + path), headers);
}

public HttpResponse getAuthInfo(Header... headers) {
return executeRequest(new HttpGet(getHttpServerUri() + "/_opendistro/_security/authinfo?pretty"), headers);
}
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/org/opensearch/security/filter/SecurityRestFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,10 @@ public void onAllowlistingSettingChanged(AllowlistingSettings allowlistingSettin
* @param handlerPath The path from the {@link RestHandler.Route}
* @return true if the request path matches the route
*/
private boolean restPathMatches(String requestPath, String handlerPath) {
boolean restPathMatches(String requestPath, String handlerPath) {
// Trim leading and trailing slashes
requestPath = requestPath.replaceAll("^/+", "").replaceAll("/+$", "");
handlerPath = handlerPath.replaceAll("^/+", "").replaceAll("/+$", "");
// Check exact match
if (handlerPath.equals(requestPath)) {
return true;
Expand Down
86 changes: 86 additions & 0 deletions src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@

import org.junit.Before;
import org.junit.Test;
import org.junit.experimental.runners.Enclosed;
import org.junit.runner.RunWith;

import org.opensearch.client.node.NodeClient;
import org.opensearch.common.settings.Settings;
Expand All @@ -41,6 +43,7 @@
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;

@RunWith(Enclosed.class)
public class SecurityRestFilterUnitTests {

SecurityRestFilter sf;
Expand Down Expand Up @@ -100,4 +103,87 @@ public void testDoesCallDelegateOnSuccessfulAuthorization() throws Exception {

verify(testRestHandlerSpy).handleRequest(any(), any(), any());
}

// Mini Test Suite for restPathMatches
public static class RestPathMatchesTests {

private SecurityRestFilter sf;

@Before
public void setUp() {
sf = new SecurityRestFilter(
mock(BackendRegistry.class),
mock(RestLayerPrivilegesEvaluator.class),
mock(AuditLog.class),
mock(ThreadPool.class),
mock(PrincipalExtractor.class),
Settings.EMPTY,
mock(Path.class),
mock(CompatConfig.class)
);
}

@Test
public void testExactPathMatch() {
String requestPath = "/api/v1/resource";
String handlerPath = "/api/v1/resource";
assertTrue(sf.restPathMatches(requestPath, handlerPath));
}

@Test
public void testPathsDoNotMatch() {
String requestPath = "/api/v1/resource";
String handlerPath = "/api/v2/resource";
assertFalse(sf.restPathMatches(requestPath, handlerPath));
}

@Test
public void testMatchWithLeadingSlashDifference() {
String requestPath = "api/v1/resource";
String handlerPath = "/api/v1/resource";
assertTrue(sf.restPathMatches(requestPath, handlerPath));
}

@Test
public void testMatchWithTrailingSlashDifference() {
String requestPath = "/api/v1/resource/";
String handlerPath = "/api/v1/resource";
assertTrue(sf.restPathMatches(requestPath, handlerPath));
}

@Test
public void testPathsMatchWithNamedParameter() {
String requestPath = "/api/v1/resource/123";
String handlerPath = "/api/v1/resource/{id}";
assertTrue(sf.restPathMatches(requestPath, handlerPath));
}

@Test
public void testPathsDoNotMatchWithDifferentNamedParameter() {
String requestPath = "/api/v1/resource/123";
String handlerPath = "/api/v1/resource/{name}";
assertTrue(sf.restPathMatches(requestPath, handlerPath));
}

@Test
public void testDifferentSegmentCount() {
String requestPath = "/api/v1/resource/123/extra";
String handlerPath = "/api/v1/resource/{id}";
assertFalse(sf.restPathMatches(requestPath, handlerPath));
}

@Test
public void testPathsMatchWithMultipleNamedParameters() {
String requestPath = "/api/v1/resource/123/details";
String handlerPath = "/api/v1/resource/{id}/details";
assertTrue(sf.restPathMatches(requestPath, handlerPath));
}

@Test
public void testPathsDoNotMatchWithNonMatchingNamedParameterSegment() {
String requestPath = "/api/v1/resource/123/details";
String handlerPath = "/api/v1/resource/{id}/summary";
assertFalse(sf.restPathMatches(requestPath, handlerPath));
}
}
}
Loading