From 493c5e1dbfe4f56553e85e0806c8cac92bced84a Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 6 Nov 2024 14:17:31 -0500 Subject: [PATCH 1/3] Improves Rest Layer Authz robustness Signed-off-by: Darshit Chanpura --- .../security/rest/AuthZinRestLayerTests.java | 14 +++ .../opensearch/security/rest/WhoAmITests.java | 15 ++++ .../framework/cluster/TestRestClient.java | 4 + .../security/filter/SecurityRestFilter.java | 5 +- .../filter/SecurityRestFilterUnitTests.java | 86 +++++++++++++++++++ 5 files changed, 123 insertions(+), 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java b/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java index c38ecbb611..72f60166e4 100644 --- a/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java +++ b/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java @@ -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; @@ -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) @@ -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 diff --git a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java index c41b5f4cda..a614eb4096 100644 --- a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java +++ b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java @@ -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; @@ -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) @@ -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)) { diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java index e7355711fc..c751aae083 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java @@ -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); } diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index b56f3e951d..0fc18588bb 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -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; diff --git a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java index 1727fddcc3..c387dd68ac 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java +++ b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java @@ -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; @@ -41,6 +43,7 @@ import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; +@RunWith(Enclosed.class) public class SecurityRestFilterUnitTests { SecurityRestFilter sf; @@ -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)); + } + } } From 6bcf705461e77d0d7c6941469c9d4de1e267c27e Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Wed, 6 Nov 2024 14:44:45 -0500 Subject: [PATCH 2/3] Updates restPathMatches unit tests Signed-off-by: Darshit Chanpura --- .../security/filter/SecurityRestFilter.java | 2 +- .../security/filter/RestPathMatchesTests.java | 33 +++++-- .../filter/SecurityRestFilterUnitTests.java | 85 +------------------ 3 files changed, 29 insertions(+), 91 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 0fc18588bb..a4c12bc28d 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -335,7 +335,7 @@ public void onAllowlistingSettingChanged(AllowlistingSettings allowlistingSettin * @param handlerPath The path from the {@link RestHandler.Route} * @return true if the request path matches the route */ - boolean restPathMatches(String requestPath, String handlerPath) { + private boolean restPathMatches(String requestPath, String handlerPath) { // Trim leading and trailing slashes requestPath = requestPath.replaceAll("^/+", "").replaceAll("/+$", ""); handlerPath = handlerPath.replaceAll("^/+", "").replaceAll("/+$", ""); diff --git a/src/test/java/org/opensearch/security/filter/RestPathMatchesTests.java b/src/test/java/org/opensearch/security/filter/RestPathMatchesTests.java index fd686bf857..0c094033f3 100644 --- a/src/test/java/org/opensearch/security/filter/RestPathMatchesTests.java +++ b/src/test/java/org/opensearch/security/filter/RestPathMatchesTests.java @@ -72,16 +72,37 @@ public void testRequestPathWithNamedParam() throws InvocationTargetException, Il } @Test - public void testRequestPathMismatch() throws InvocationTargetException, IllegalAccessException { - String requestPath = "_plugins/security/api/x/y"; - String handlerPath = "_plugins/security/api/z/y"; + public void testMatchWithLeadingSlashDifference() throws InvocationTargetException, IllegalAccessException { + String requestPath = "api/v1/resource"; + String handlerPath = "/api/v1/resource"; + assertTrue((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); + } + + @Test + public void testMatchWithTrailingSlashDifference() throws InvocationTargetException, IllegalAccessException { + String requestPath = "/api/v1/resource/"; + String handlerPath = "/api/v1/resource"; + assertTrue((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); + } + + @Test + public void testPathsMatchWithMultipleNamedParameters() throws InvocationTargetException, IllegalAccessException { + String requestPath = "/api/v1/resource/123/details"; + String handlerPath = "/api/v1/resource/{id}/details"; + assertTrue((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); + } + + @Test + public void testPathsDoNotMatchWithNonMatchingNamedParameterSegment() throws InvocationTargetException, IllegalAccessException { + String requestPath = "/api/v1/resource/123/details"; + String handlerPath = "/api/v1/resource/{id}/summary"; assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); } @Test - public void testRequestPathWithExtraSegments() throws InvocationTargetException, IllegalAccessException { - String requestPath = "_plugins/security/api/x/y/z"; - String handlerPath = "_plugins/security/api/x/y"; + public void testDifferentSegmentCount() throws InvocationTargetException, IllegalAccessException { + String requestPath = "/api/v1/resource/123/extra"; + String handlerPath = "/api/v1/resource/{id}"; assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); } } diff --git a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java index c387dd68ac..862aec17e4 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java +++ b/src/test/java/org/opensearch/security/filter/SecurityRestFilterUnitTests.java @@ -15,8 +15,6 @@ 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; @@ -43,7 +41,6 @@ import static org.mockito.Mockito.spy; import static org.mockito.Mockito.verify; -@RunWith(Enclosed.class) public class SecurityRestFilterUnitTests { SecurityRestFilter sf; @@ -104,86 +101,6 @@ 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)); - } + // unit tests for restPathMatches are in RestPathMatchesTests.java - @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)); - } - } } From 739c47f90181333980bdc9525b85acb8840bf9bc Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Thu, 7 Nov 2024 13:26:05 -0500 Subject: [PATCH 3/3] Updates integTests Signed-off-by: Darshit Chanpura --- .../security/rest/AuthZinRestLayerTests.java | 7 +------ .../java/org/opensearch/security/rest/WhoAmITests.java | 10 +--------- .../test/framework/cluster/TestRestClient.java | 6 ++++-- 3 files changed, 6 insertions(+), 17 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java b/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java index 72f60166e4..959ceb944b 100644 --- a/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java +++ b/src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java @@ -26,13 +26,11 @@ 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; @@ -42,7 +40,6 @@ 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) @@ -119,9 +116,7 @@ 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")); + assertThat(client.getWithoutLeadingSlash(PROTECTED_API).getStatusCode(), equalTo(HttpStatus.SC_UNAUTHORIZED)); } } diff --git a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java index a614eb4096..2cbdcd5d44 100644 --- a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java +++ b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java @@ -37,13 +37,11 @@ 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; @@ -54,7 +52,6 @@ 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) @@ -144,12 +141,7 @@ 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")); + assertThat(client.getWithoutLeadingSlash(WHOAMI_PROTECTED_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_UNAUTHORIZED)); } } diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java index c751aae083..f560ef713f 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/TestRestClient.java @@ -110,8 +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 getWithoutLeadingSlash(String path, Header... headers) { + HttpUriRequest req = new HttpGet(getHttpServerUri()); + req.setPath(path); + return executeRequest(req, headers); } public HttpResponse getAuthInfo(Header... headers) {