From 5e0d10e6d736fe5a0c8e6d51764976d10b26141b Mon Sep 17 00:00:00 2001 From: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com> Date: Fri, 7 Jul 2023 17:14:13 -0400 Subject: [PATCH 1/4] Authorize rest requests (#2753) * WIP on rest layer authz Signed-off-by: Craig Perkins * WIP on rest-layer authz Signed-off-by: Craig Perkins * Extension handshake Signed-off-by: Craig Perkins * Extension TLS Signed-off-by: Craig Perkins * Remove SecurityRestFilterChanges to isolate extension TLS change Signed-off-by: Craig Perkins * Remove SecurityRestFilterChanges to isolate extension TLS change Signed-off-by: Craig Perkins * Remove SecurityRestFilterChanges to isolate extension TLS change Signed-off-by: Craig Perkins * Remove SecurityRestFilterChanges to isolate extension TLS change Signed-off-by: Craig Perkins * Remove SecurityRestFilterChanges to isolate extension TLS change Signed-off-by: Craig Perkins * WIP for HelloWorld sample extension role Signed-off-by: Craig Perkins * Initial implementation of authz check in REST layer Signed-off-by: Craig Perkins * Remove header Signed-off-by: Craig Perkins * Create authorizeRequest method Signed-off-by: Craig Perkins * small fix Signed-off-by: Craig Perkins * Change to ProtectedRoute Signed-off-by: Craig Perkins * Remove extension permissions Signed-off-by: Craig Perkins * Initial implementation of authz check in REST layer Signed-off-by: Craig Perkins * Extension TLS Signed-off-by: Craig Perkins * Adds dummy roles for testing rest authorization against legacy permissions Signed-off-by: Darshit Chanpura * Adds support for legacy permissions to perform rest authorization Signed-off-by: Darshit Chanpura * Fixes white-space changes Signed-off-by: Darshit Chanpura * Rebases ConfigConstants with main Signed-off-by: Darshit Chanpura * Implements a new logic for rest permissions check to be more flexible Signed-off-by: Darshit Chanpura * Fixes spotless errors Signed-off-by: Darshit Chanpura * Adds regex to match against current role permissions when comparing new permission with legacy ones Signed-off-by: Darshit Chanpura * Moves legacy permission check logic to ConfigModelV7 Signed-off-by: Darshit Chanpura * Fixes extra new-lines Signed-off-by: Darshit Chanpura * Fixes unused imports Signed-off-by: Darshit Chanpura * Fixes out-of-scope white space changes Signed-off-by: Darshit Chanpura * Fixes code-ql errors Signed-off-by: Darshit Chanpura * Fixes spotless and code-ql errors Signed-off-by: Darshit Chanpura * Fixes variable name and remove references to whitelist in javadoc Signed-off-by: Darshit Chanpura * Adds tests for rest layer privilege evaluator Signed-off-by: Darshit Chanpura * Adds license header to the test file Signed-off-by: Darshit Chanpura * Updates zstd dependency to fetch from core version.properties Signed-off-by: Darshit Chanpura * Updates action name in the regex to be dynamic Signed-off-by: Darshit Chanpura * Adds support for allowing evaluation against multiple actions names for a registered named route Signed-off-by: Darshit Chanpura * Updates tests Signed-off-by: Darshit Chanpura * Adds null check Signed-off-by: Darshit Chanpura * Makes authorize logic clearer to follow Signed-off-by: Darshit Chanpura * Adds extra check to ensure new actions are also evaluated against transport actions Signed-off-by: Darshit Chanpura * Fixes spotless errors Signed-off-by: Darshit Chanpura * Fixes security rest filter setup Signed-off-by: Darshit Chanpura * Removes extension reference Signed-off-by: Darshit Chanpura * turn on audit logging Signed-off-by: Maciej Mierzwa * Adds unit tests for restPathMatches method Signed-off-by: Darshit Chanpura * Cleans up TODOs Signed-off-by: Darshit Chanpura * Organizes demo users and roles for extension Signed-off-by: Darshit Chanpura * Address PR feedback Signed-off-by: Darshit Chanpura * Adds more comments Signed-off-by: Darshit Chanpura * add privileges info Signed-off-by: Maciej Mierzwa * Makes whoami action a named route and fixes license header check Signed-off-by: Darshit Chanpura * Adds integ tests for whoami route Signed-off-by: Darshit Chanpura * Change permissions order in roles.yml Signed-off-by: Darshit Chanpura * Adds developer documentation for authorization in REST layer Signed-off-by: Darshit Chanpura * Fixes broken tests Signed-off-by: Darshit Chanpura * Fixes checkstyle errors Signed-off-by: Darshit Chanpura * Addresses feedback and cleans up logic for super admin check Signed-off-by: Darshit Chanpura * Addresses Plugin Install CI failure Signed-off-by: Darshit Chanpura * Fixes failing citest task Signed-off-by: Darshit Chanpura * Modifies WhoAmI integ tests Signed-off-by: Darshit Chanpura * Adds a new endpoint called whoamiprotected and removes changes made to whoami route Signed-off-by: Darshit Chanpura * Updates documentation to reflect the new API Signed-off-by: Darshit Chanpura * Addresses PR feedback Signed-off-by: Darshit Chanpura * Renames action0 to actions Signed-off-by: Darshit Chanpura --------- Signed-off-by: Craig Perkins Signed-off-by: Darshit Chanpura Signed-off-by: Maciej Mierzwa Co-authored-by: Craig Perkins Co-authored-by: MaciejMierzwa (cherry picked from commit a53a8a6c9d990911cb5f6a42e937b395c598b0d5) --- DEVELOPER_GUIDE.md | 95 ++++++++-- REST_AUTHZ_FOR_PLUGINS.md | 136 +++++++++++++++ .../opensearch/security/rest/WhoAmITests.java | 107 ++++++++++++ .../security/OpenSearchSecurityPlugin.java | 7 + .../auditlog/impl/AbstractAuditLog.java | 1 + .../security/dlic/rest/support/Utils.java | 15 +- .../security/filter/SecurityRestFilter.java | 118 +++++++++++-- .../RestLayerPrivilegesEvaluator.java | 130 ++++++++++++++ .../security/rest/SecurityWhoAmIAction.java | 12 +- .../security/SystemIntegratorsTests.java | 2 +- .../security/filter/RestPathMatchesTest.java | 66 +++++++ .../RestLayerPrivilegesEvaluatorTest.java | 163 ++++++++++++++++++ src/test/resources/roles.yml | 6 + src/test/resources/roles_mapping.yml | 5 + 14 files changed, 823 insertions(+), 40 deletions(-) create mode 100644 REST_AUTHZ_FOR_PLUGINS.md create mode 100644 src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java create mode 100644 src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java create mode 100644 src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java create mode 100644 src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java diff --git a/DEVELOPER_GUIDE.md b/DEVELOPER_GUIDE.md index 01f672ec95..9b0e9d62c4 100644 --- a/DEVELOPER_GUIDE.md +++ b/DEVELOPER_GUIDE.md @@ -1,5 +1,6 @@ # Developer Guide -So you want to contribute code to this project? Excellent! We're glad you're here. Here's what you need to do. + +So you want to contribute code to OpenSearch Security? Excellent! We're glad you're here. Here's what you need to do. - [Developer Guide](#developer-guide) - [Prerequisites](#prerequisites) @@ -9,6 +10,7 @@ So you want to contribute code to this project? Excellent! We're glad you're her - [Running integration tests](#running-integration-tests) - [Bulk test runs](#bulk-test-runs) - [Checkstyle Violations](#checkstyle-violations) + - [Authorization in REST Layer](#authorization-in-rest-layer) - [Submitting Changes](#submitting-changes) - [Backports](#backports) @@ -16,16 +18,17 @@ So you want to contribute code to this project? Excellent! We're glad you're her > Please make sure to follow the OpenSearch [Install Prerequisites](https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md#install-prerequisites) before starting for the first time. -This project runs as a plugin of OpenSearch. You can [download a minimal release of OpenSearch](https://opensearch.org/downloads.html#minimal) and then install this plugin there. However, we will compile it using source code so that we are pulling in changes from the latest commit. +OpenSearch Security runs as a plugin of OpenSearch. You can [download a minimal release of OpenSearch](https://opensearch.org/downloads.html#minimal) and then install the Security plugin there. However, we will compile OpenSearch Security using source code so that we are pulling in changes from the latest commit. ### Native platforms -Not all platforms natively support OpenSearch, to check distribution avaliability please check these [issues](https://github.com/opensearch-project/opensearch-build/labels/distributions). -On MacOS / PC the OpenSearch distribution can be run with docker. This distribution contains the released version of OpenSearch including the security plugin. For development we do not recommend using this docker image. +Not all platforms natively support OpenSearch, to view distribution availability please check these [issues](https://github.com/opensearch-project/opensearch-build/issues?q=label%3Adistributions). + +On MacOS / PC the OpenSearch distribution can be run with Docker. This distribution contains the released version of OpenSearch including the security plugin. If you wish to use the Docker image for development, you will need to follow the steps found on the [Developing with Docker](DEVELOPING_WITH_DOCKER.md) guide. -To get started, follow the [getting started section](https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md#getting-started) of OpenSearch's developer guide. This will get OpenSearch up and running built from source code. You can skip the `./gradlew check` step to save some time. Reach to the point where you can run a successful `curl localhost:9200` call. Great! now kill the server with `Ctrl+C`. +To get started, follow the [getting started section](https://github.com/opensearch-project/OpenSearch/blob/main/DEVELOPER_GUIDE.md#getting-started) of OpenSearch's developer guide. This will get OpenSearch up and running built from source code. You can skip the `./gradlew check` step to save some time. You should follow the steps until you reach the point where you can run a successful `curl localhost:9200` call. Great! now kill the server with `Ctrl+C`. -Next, run the following commands to copy the built code (snapshot) to a new folder in a different location. (This where you'll be running OpenSearch service). Run this from the base directory of the OpenSearch fork you cloned above: +Next, run the following commands to copy the built code (snapshot) to a new folder in a different location. (This where you'll be running the OpenSearch service). Run this from the base directory of the OpenSearch fork you cloned above: ```bash export OPENSEARCH_HOME=~//opensearch-* export OPENSEARCH_BUILD=distribution/archives/darwin-tar/build/install/opensearch-* @@ -41,20 +44,28 @@ cd $OPENSEARCH_HOME ./bin/opensearch ``` -The `curl localhost:9200` call should succeed again. Kill the server with `Ctrl+c`. We are ready to install the security plugin. +The `curl localhost:9200` call should succeed again. Kill the server with `Ctrl+c`. We are now ready to install the security plugin. + >Worth noting:\ -> The version of OpenSearch and the security plugin must match as there is an explicit version check at startup. This can be a bit confusing as, for example, at the time of writing this guide, the `main` branch of this security plugin builds version `1.3.0.0-SNAPSHOT` compatible with OpenSearch `1.3.0-SNAPSHOT` that gets built from branch `1.x`. Check the expected compatible version [here](https://github.com/opensearch-project/security/blob/main/plugin-descriptor.properties#L27) and make sure you get the correct branch from OpenSearch when building that project. +> The version of OpenSearch and the security plugin must match as there is an explicit version check at startup. This can be a bit confusing as, for example, at the time of writing this guide, the `main` branch of this security plugin builds version `3.0.0.0-SNAPSHOT` compatible with OpenSearch `3.0.0`. Check the expected compatible version in `build.gradle` file [here](https://github.com/opensearch-project/security/blob/main/build.gradle) and make sure you get the correct branch from OpenSearch when building that project. +> +> The line to look for: `opensearch_version = System.getProperty("opensearch.version", "x")` +> +> Alternatively, you can find the compatible version of OpenSearch by running in project root folder +> ``` +> ./gradlew properties -q | grep -E '^version:' | awk '{print $2}' +> ``` ## Building -First create a fork of this repo and clone it locally. Changing to directory containing this clone and run this to build the project: +First create a fork of this repo and clone it locally. You should then change to the directory containing the clone and run this to build the project: ```bash ./gradlew clean assemble ``` -Install the built plugin into the OpenSearch server: +To install the built plugin into the OpenSearch server run: ```bash export OPENSEARCH_SECURITY_HOME=$OPENSEARCH_HOME/plugins/opensearch-security @@ -68,7 +79,52 @@ mv config/* $OPENSEARCH_HOME/config/opensearch-security/ rm -rf config/ ``` -Install the demo certificates and default configuration, answer `y` to the first two questions and `n` to the last one. The log should look like below: +### Installing demo extension users and roles + +If you are working with an extension and want to set up demo users for the Hello-World extension, append following items to files inside `$OPENSEARCH_HOME/config/opensearch-security/`: +1. In **internal_users.yml** +```yaml +hw-user: + hash: "$2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG" + reserved: true + description: "Demo user for ext-test" +``` + +2. In **roles.yml** +```yaml +extension_hw_greet: + reserved: true + cluster_permissions: + - 'hw:greet' + +extension_hw_full: + reserved: true + cluster_permissions: + - 'hw:goodbye' + - 'hw:greet' + - 'hw:greet_with_adjective' + - 'hw:greet_with_name' + +legacy_hw_greet_with_name: + reserved: true + cluster_permissions: + - 'cluster:admin/opensearch/hw/greet_with_name' +``` + +3. In **roles_mapping.yml** +```yaml +legacy_hw_greet_with_name: + reserved: true + users: + - "hw-user" + +extension_hw_greet: + reserved: true + users: + - "hw-user" +``` + +To install the demo certificates and default configuration, answer `y` to the first two questions and `n` to the last one. The log should look like below: ```bash ./tools/install_demo_configuration.sh @@ -103,7 +159,7 @@ Detected OpenSearch Security Version: * ``` Now if we start our server again and try the original `curl localhost:9200`, it will fail. -Try this one instead: `curl -XGET https://localhost:9200 -u 'admin:admin' --insecure`. It should succeed. +Try this command instead: `curl -XGET https://localhost:9200 -u 'admin:admin' --insecure`. It should succeed. You can also make this call to return the authenticated user details: @@ -140,15 +196,17 @@ Launch IntelliJ IDEA, choose **Project from Existing Sources**, and select direc ## Running integration tests -Locally these can be run with `./gradlew test` with detailed results being avaliable at `${project-root}/build/reports/tests/test/index.html`, or run through an IDEs JUnit test runner. +Locally these can be run with `./gradlew test` with detailed results being available at `${project-root}/build/reports/tests/test/index.html`. You can also run tests through an IDEs JUnit test runner. -Integration tests are automatically run on all pull requests for all supported versions of the JDK. These must pass for change(s) to be merged. Detailed logs of these test results are avaliable by going to the GitHub action workflow's summary view and downloading the associated jdk version run of the tests, after extracting this file onto your local machine integration tests results are at `./tests/tests/index.html`. +Integration tests are automatically run on all pull requests for all supported versions of the JDK. These must pass for change(s) to be merged. Detailed logs of these test results are available by going to the GitHub Actions workflow summary view and downloading the workflow run of the tests. If you see multiple tests listed with different JDK versions, you can download the version with whichever JDK you are interested in. After extracting the test file on your local machine, integration tests results can be found at `./tests/tests/index.html`. ### Bulk test runs -To collect reliability data on test runs there is a manual GitHub action workflow called `Bulk Integration Test`. The workflow is started for a branch on this project or in a fork by going to [GitHub action workflows](https://github.com/opensearch-project/security/actions/workflows/integration-tests.yml) and selecting `Run Workflow`. + +To collect reliability data on test runs, there is a manual GitHub action workflow called `Bulk Integration Test`. The workflow is started for a branch on this project or in a fork by going to [GitHub action workflows](https://github.com/opensearch-project/security/actions/workflows/integration-tests.yml) and selecting `Run Workflow`. ### Checkstyle Violations -Checkstyle enforced several rules within this codebase. Sometimes exceptions will be necessary for components that are set for deprecation but the new version is unavailable. There are two formats of suppression that can be used when dealing with violations of this nature, one for disabling a single rule, or another for disabling all rules - its best to be as specific as possible. + +Checkstyle enforces several rules within this codebase. Sometimes it will be necessary for exceptions to be made when dealing with components that are set for deprecation. This can happen when the new version of a deprecation-path component is unavailable. There are two formats of suppression that can be used when dealing with violations of this nature, one for disabling a single rule, or another for disabling all rules. It is best to only disable specific rules when possible. *Execute Checkstyle* ``` @@ -176,6 +234,11 @@ Checkstyle enforced several rules within this codebase. Sometimes exceptions wi // CS-ENFORCE-ALL ``` +## Authorization in REST Layer + +See [REST_AUTHZ_FOR_PLUGINS](REST_AUTHZ_FOR_PLUGINS.md). + + ## Submitting Changes See [CONTRIBUTING](CONTRIBUTING.md). diff --git a/REST_AUTHZ_FOR_PLUGINS.md b/REST_AUTHZ_FOR_PLUGINS.md new file mode 100644 index 0000000000..b0f30ed04b --- /dev/null +++ b/REST_AUTHZ_FOR_PLUGINS.md @@ -0,0 +1,136 @@ +# Authorization at REST Layer for plugins + +This feature is introduced as an added layer of security on top of existing TransportLayer authorization framework. In order to leverage these feature some core changes need to be made at Route registration level. This document talks about how you can achieve this. + +**NOTE:** This doesn't replace Transport Layer Authorization. Plugin developers may choose to skip creating transport actions for APIs that do not need interaction with the Transport Layer. + +## Pre-requisites + +The security plugin must be installed and operational in your OpenSearch cluster for this feature to work. + +### How does NamedRoute authorization work? + +Once the routes are defined as NamedRoute, they, along-with their handlers, will be registered the same way as Route objects. When a request comes in, `SecurityRestFilter.java` applies an authorization check which extracts information about the NamedRoute. +Next we get the unique name and actionNames associated with that route and evaluate these against existing `cluster_permissions` across all roles of the requesting user. If the authorization check succeeds, the request chain proceeds as normal. If it fails, a 401 response is returned to the user. + +NOTE: +1. The action names defined in roles must exactly match the names of registered routes, or else, the request would be deemed unauthorized. +2. This check will not be implemented for plugins who do not use NamedRoutes. + + + +### How to translate an existing Route to be a NamedRoute? + +Here is a sample of an existing route converted to a named route: +Before: +``` +public List routes() { + return ImmutableList.of( + new Route(GET, "/uri") + ); +} +``` +With new scheme: +``` +public List routes() { + return ImmutableList.of( + new NamedRoute.Builder().method(GET).path("/uri").uniqueName("plugin:uri").actionNames(Set.of("cluster:admin/opensearch/plugin/uri")).build() + ); +} +``` + +`actionNames()` are optional. They correspond to any current actions defined as permissions in roles. +Ensure that these name-to-route mappings are easily accessible to the cluster admins to allow granting access to these APIs. + +### How does authorization in the REST Layer work? + +We will continue on the above example of translating `/uri` from Route to NamedRoute. + +Consider these roles are defined in the cluster: +```yaml +plugin_role: + reserved: true + cluster_permissions: + - 'plugin:uri' + +plugin_role_legacy: + reserved: true + cluster_permissions: + - 'cluster:admin/opensearch/plugin/uri' +``` + +Successful authz scenarios for a user: +1. The user is mapped either to `plugin_role` OR `plugin_role_legacy`. +2. The user is mapped to both of these roles. +3. The user is mapped to `plugin_role` even if no `actionNames()` were registered for this route. + +Unsuccessful authz scenarios for a user: +1. The user is not mapped any roles. +2. The user is mapped to a different role which doesn't grant the cluster permissions: `plugin:uri` OR `cluster:admin/opensearch/plugin/uri`/ +3. The user is mapped to a role `plugin_role_other` which has a typo in action name, i.e.`plugin:uuri`. + + +### Sample API in Security Plugin + +As part of this effort a new uri `GET /whoamiprotected` was introduced as a NamedRoute version of `GET /whoami`. Here is how you can test it: + +#### roles.yml +```yaml +who_am_i_role: + reserved: true + cluster_permissions: + - 'security:whoamiprotected' + +who_am_i_role_legacy: + reserved: true + cluster_permissions: + - 'cluster:admin/opendistro_security/whoamiprotected' + +who_am_i_role_no_perm: + reserved: true + cluster_permissions: + - 'some_invalid_perm' + +``` + +#### internal_users.yml +```yaml +who_am_i-user: + hash: "$2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG" #admin + reserved: true + description: "Demo user for ext-test" + +who_am_i_legacy-user: + hash: "$2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG" + reserved: true + description: "Demo user for ext-test" + +who_am_i_no_perm-user: + hash: "$2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG" + reserved: true + description: "Demo user for ext-test" +``` + +#### roles_mapping.yml +```yaml +who_am_i_role: + reserved: true + users: + - "who_am_i-user" + +who_am_i_role_legacy: + reserved: true + users: + - "who_am_i_legacy-user" + +who_am_i_role_no_perm: + reserved: true + users: + - "who_am_i_no_perm-user" +``` + +Follow [DEVELOPER_GUIDE](DEVELOPER_GUIDE.md) to setup OpenSearch cluster and initialize security plugin. Once you have verified that security plugin is installed correctly and OpenSearch is running, execute following curl requests: +1. `curl -XGET https://who_am_i-user:admin@localhost:9200/_plugins/_security/whoami --insecure` should succeed. +2. `curl -XGET https://who_am_i_legacy-user:admin@localhost:9200/_plugins/_security/whoami --insecure` should succeed. +3. `curl -XGET https://who_am_i_no-perm-user:admin@localhost:9200/_plugins/_security/whoami --insecure` should fail. +4. `curl -XPOST ` to `/whoami` with all 3 users should succeed. This is because POST route is not a NamedRoute and hence no authorization check was made. diff --git a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java new file mode 100644 index 0000000000..5e9992c8ad --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java @@ -0,0 +1,107 @@ +/* +* SPDX-License-Identifier: Apache-2.0 +* +* The OpenSearch Contributors require contributions made to +* this file be licensed under the Apache-2.0 license or a +* compatible open source license. +* +* Modifications Copyright OpenSearch Contributors. See +* GitHub history for details. +*/ + +package org.opensearch.security.rest; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.apache.hc.core5.http.HttpStatus; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.TestSecurityConfig.Role; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class WhoAmITests { + protected final static TestSecurityConfig.User WHO_AM_I = new TestSecurityConfig.User("who_am_i_user").roles( + new Role("who_am_i_role").clusterPermissions("security:whoamiprotected") + ); + + protected final static TestSecurityConfig.User WHO_AM_I_LEGACY = new TestSecurityConfig.User("who_am_i_user_legacy").roles( + new Role("who_am_i_role_legacy").clusterPermissions("cluster:admin/opendistro_security/whoamiprotected") + ); + + protected final static TestSecurityConfig.User WHO_AM_I_NO_PERM = new TestSecurityConfig.User("who_am_i_user_no_perm").roles( + new Role("who_am_i_role_no_perm") + ); + + protected final static TestSecurityConfig.User WHO_AM_I_UNREGISTERED = new TestSecurityConfig.User("who_am_i_user_no_perm"); + + public static final String WHOAMI_ENDPOINT = "_plugins/_security/whoami"; + public static final String WHOAMI_PROTECTED_ENDPOINT = "_plugins/_security/whoamiprotected"; + + @ClassRule + public static LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(WHO_AM_I, WHO_AM_I_LEGACY, WHO_AM_I_NO_PERM) + .build(); + + @Test + public void testWhoAmIWithGetPermissions() throws Exception { + try (TestRestClient client = cluster.getRestClient(WHO_AM_I)) { + assertThat(client.get(WHOAMI_PROTECTED_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + + try (TestRestClient client = cluster.getRestClient(WHO_AM_I)) { + assertThat(client.get(WHOAMI_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + } + + @Test + public void testWhoAmIWithGetPermissionsLegacy() throws Exception { + try (TestRestClient client = cluster.getRestClient(WHO_AM_I_LEGACY)) { + assertThat(client.get(WHOAMI_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + + try (TestRestClient client = cluster.getRestClient(WHO_AM_I_LEGACY)) { + assertThat(client.get(WHOAMI_PROTECTED_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + } + + @Test + public void testWhoAmIWithoutGetPermissions() throws Exception { + try (TestRestClient client = cluster.getRestClient(WHO_AM_I_NO_PERM)) { + assertThat(client.get(WHOAMI_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + + try (TestRestClient client = cluster.getRestClient(WHO_AM_I_NO_PERM)) { + assertThat(client.get(WHOAMI_PROTECTED_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_UNAUTHORIZED)); + } + } + + @Test + public void testWhoAmIPost() throws Exception { + try (TestRestClient client = cluster.getRestClient(WHO_AM_I)) { + assertThat(client.post(WHOAMI_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + + try (TestRestClient client = cluster.getRestClient(WHO_AM_I_LEGACY)) { + assertThat(client.post(WHOAMI_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + + try (TestRestClient client = cluster.getRestClient(WHO_AM_I_NO_PERM)) { + assertThat(client.post(WHOAMI_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + + try (TestRestClient client = cluster.getRestClient(WHO_AM_I_UNREGISTERED)) { + assertThat(client.post(WHOAMI_ENDPOINT).getStatusCode(), equalTo(HttpStatus.SC_OK)); + } + + } +} diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index d3aa900556..95ef409b05 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -145,6 +145,7 @@ import org.opensearch.security.http.XFFResolver; import org.opensearch.security.privileges.PrivilegesEvaluator; import org.opensearch.security.privileges.PrivilegesInterceptor; +import org.opensearch.security.privileges.RestLayerPrivilegesEvaluator; import org.opensearch.security.resolver.IndexResolverReplacer; import org.opensearch.security.rest.DashboardsInfoAction; import org.opensearch.security.rest.SecurityConfigUpdateAction; @@ -203,6 +204,7 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin private volatile SecurityInterceptor si; private volatile PrivilegesEvaluator evaluator; private volatile UserService userService; + private volatile RestLayerPrivilegesEvaluator restLayerEvaluator; private volatile ThreadPool threadPool; private volatile ConfigurationRepository cr; private volatile AdminDNs adminDns; @@ -1017,8 +1019,11 @@ public Collection createComponents( principalExtractor = ReflectionHelper.instantiatePrincipalExtractor(principalExtractorClass); } + restLayerEvaluator = new RestLayerPrivilegesEvaluator(clusterService, threadPool, auditLog, cih, namedXContentRegistry); + securityRestHandler = new SecurityRestFilter( backendRegistry, + restLayerEvaluator, auditLog, threadPool, principalExtractor, @@ -1033,6 +1038,7 @@ public Collection createComponents( dcf.registerDCFListener(irr); dcf.registerDCFListener(xffResolver); dcf.registerDCFListener(evaluator); + dcf.registerDCFListener(restLayerEvaluator); dcf.registerDCFListener(securityRestHandler); if (!(auditLog instanceof NullAuditLog)) { // Don't register if advanced modules is disabled in which case auditlog is instance of NullAuditLog @@ -1070,6 +1076,7 @@ public Collection createComponents( components.add(xffResolver); components.add(backendRegistry); components.add(evaluator); + components.add(restLayerEvaluator); components.add(si); components.add(dcf); components.add(userService); diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java index 37b98ed67e..cf6db75d84 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java @@ -184,6 +184,7 @@ public void logMissingPrivileges(String privilege, String effectiveUser, RestReq msg.addRemoteAddress(remoteAddress); msg.addRestRequestInfo(request, auditConfigFilter); msg.addEffectiveUser(effectiveUser); + msg.addPrivilege(privilege); save(msg); } diff --git a/src/main/java/org/opensearch/security/dlic/rest/support/Utils.java b/src/main/java/org/opensearch/security/dlic/rest/support/Utils.java index 74908dbf60..65524a8bf7 100644 --- a/src/main/java/org/opensearch/security/dlic/rest/support/Utils.java +++ b/src/main/java/org/opensearch/security/dlic/rest/support/Utils.java @@ -44,6 +44,7 @@ import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.core.xcontent.ToXContent; import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.rest.NamedRoute; import org.opensearch.rest.RestHandler.DeprecatedRoute; import org.opensearch.rest.RestHandler.Route; import org.opensearch.security.DefaultObjectMapper; @@ -240,9 +241,17 @@ public static List addRoutesPrefix(List routes) { * Total number of routes will be expanded len(prefixes) as much comparing to the list passed in */ public static List addRoutesPrefix(List routes, final String... prefixes) { - return routes.stream() - .flatMap(r -> Arrays.stream(prefixes).map(p -> new Route(r.getMethod(), p + r.getPath()))) - .collect(ImmutableList.toImmutableList()); + return routes.stream().flatMap(r -> Arrays.stream(prefixes).map(p -> { + if (r instanceof NamedRoute) { + NamedRoute nr = (NamedRoute) r; + return new NamedRoute.Builder().method(nr.getMethod()) + .path(p + nr.getPath()) + .uniqueName(nr.name()) + .legacyActionNames(nr.actionNames()) + .build(); + } + return new Route(r.getMethod(), p + r.getPath()); + })).collect(ImmutableList.toImmutableList()); } /** diff --git a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java index 80bba54ea2..cfc157d334 100644 --- a/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java +++ b/src/main/java/org/opensearch/security/filter/SecurityRestFilter.java @@ -27,6 +27,9 @@ package org.opensearch.security.filter; import java.nio.file.Path; +import java.util.List; +import java.util.Optional; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -37,10 +40,10 @@ import org.greenrobot.eventbus.Subscribe; import org.opensearch.OpenSearchException; -import org.opensearch.client.node.NodeClient; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.rest.BytesRestResponse; +import org.opensearch.rest.NamedRoute; import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestHandler; import org.opensearch.rest.RestRequest; @@ -52,6 +55,8 @@ import org.opensearch.security.configuration.AdminDNs; import org.opensearch.security.configuration.CompatConfig; import org.opensearch.security.dlic.rest.api.AllowlistApiAction; +import org.opensearch.security.privileges.PrivilegesEvaluatorResponse; +import org.opensearch.security.privileges.RestLayerPrivilegesEvaluator; import org.opensearch.security.securityconf.impl.AllowlistingSettings; import org.opensearch.security.securityconf.impl.WhitelistingSettings; import org.opensearch.security.ssl.transport.PrincipalExtractor; @@ -70,6 +75,7 @@ public class SecurityRestFilter { protected final Logger log = LogManager.getLogger(this.getClass()); private final BackendRegistry registry; + private final RestLayerPrivilegesEvaluator evaluator; private final AuditLog auditLog; private final ThreadContext threadContext; private final PrincipalExtractor principalExtractor; @@ -88,6 +94,7 @@ public class SecurityRestFilter { public SecurityRestFilter( final BackendRegistry registry, + final RestLayerPrivilegesEvaluator evaluator, final AuditLog auditLog, final ThreadPool threadPool, final PrincipalExtractor principalExtractor, @@ -97,6 +104,7 @@ public SecurityRestFilter( ) { super(); this.registry = registry; + this.evaluator = evaluator; this.auditLog = auditLog; this.threadContext = threadPool.getThreadContext(); this.principalExtractor = principalExtractor; @@ -109,28 +117,27 @@ public SecurityRestFilter( /** * This function wraps around all rest requests - * If the request is authenticated, then it goes through a whitelisting check. - * The whitelisting check works as follows: - * If whitelisting is not enabled, then requests are handled normally. - * If whitelisting is enabled, then SuperAdmin is allowed access to all APIs, regardless of what is currently whitelisted. - * If whitelisting is enabled, then Non-SuperAdmin is allowed to access only those APIs that are whitelisted in {@link #requests} - * For example: if whitelisting is enabled and requests = ["/_cat/nodes"], then SuperAdmin can access all APIs, but non SuperAdmin + * If the request is authenticated, then it goes through a allowlisting check. + * The allowlisting check works as follows: + * If allowlisting is not enabled, then requests are handled normally. + * If allowlisting is enabled, then SuperAdmin is allowed access to all APIs, regardless of what is currently allowlisted. + * If allowlisting is enabled, then Non-SuperAdmin is allowed to access only those APIs that are allowlisted in {@link #requests} + * For example: if allowlisting is enabled and requests = ["/_cat/nodes"], then SuperAdmin can access all APIs, but non SuperAdmin * can only access "/_cat/nodes" - * Further note: Some APIs are only accessible by SuperAdmin, regardless of whitelisting. For example: /_opendistro/_security/api/whitelist is only accessible by SuperAdmin. + * Further note: Some APIs are only accessible by SuperAdmin, regardless of allowlisting. For example: /_opendistro/_security/api/whitelist is only accessible by SuperAdmin. * See {@link AllowlistApiAction} for the implementation of this API. * SuperAdmin is identified by credentials, which can be passed in the curl request. */ public RestHandler wrap(RestHandler original, AdminDNs adminDNs) { - return new RestHandler() { - - @Override - public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { - org.apache.logging.log4j.ThreadContext.clearAll(); - if (!checkAndAuthenticateRequest(request, channel, client)) { - User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); - if (userIsSuperAdmin(user, adminDNs) - || (whitelistingSettings.checkRequestIsAllowed(request, channel, client) - && allowlistingSettings.checkRequestIsAllowed(request, channel, client))) { + return (request, channel, client) -> { + org.apache.logging.log4j.ThreadContext.clearAll(); + if (!checkAndAuthenticateRequest(request, channel)) { + User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); + boolean isSuperAdminUser = userIsSuperAdmin(user, adminDNs); + if (isSuperAdminUser + || (whitelistingSettings.checkRequestIsAllowed(request, channel, client) + && allowlistingSettings.checkRequestIsAllowed(request, channel, client))) { + if (isSuperAdminUser || authorizeRequest(original, request, channel, user)) { original.handleRequest(request, channel, client); } } @@ -145,7 +152,54 @@ private boolean userIsSuperAdmin(User user, AdminDNs adminDNs) { return user != null && adminDNs.isAdmin(user); } - private boolean checkAndAuthenticateRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + private boolean authorizeRequest(RestHandler original, RestRequest request, RestChannel channel, User user) { + + List restRoutes = original.routes(); + Optional handler = restRoutes.stream() + .filter(rh -> rh.getMethod().equals(request.method())) + .filter(rh -> restPathMatches(request.path(), rh.getPath())) + .findFirst(); + final boolean routeSupportsRestAuthorization = handler.isPresent() && handler.get() instanceof NamedRoute; + if (routeSupportsRestAuthorization) { + PrivilegesEvaluatorResponse pres = new PrivilegesEvaluatorResponse(); + NamedRoute route = ((NamedRoute) handler.get()); + // if actionNames are present evaluate those first + Set actionNames = route.actionNames(); + if (actionNames != null && !actionNames.isEmpty()) { + pres = evaluator.evaluate(user, actionNames); + } + + // now if pres.allowed is still false check for the NamedRoute name as a permission + if (!pres.isAllowed()) { + String action = route.name(); + pres = evaluator.evaluate(user, Set.of(action)); + } + + if (log.isDebugEnabled()) { + log.debug(pres.toString()); + } + if (pres.isAllowed()) { + log.debug("Request has been granted"); + auditLog.logGrantedPrivileges(user.getName(), request); + } else { + auditLog.logMissingPrivileges(route.name(), user.getName(), request); + String err; + if (!pres.getMissingSecurityRoles().isEmpty()) { + err = String.format("No mapping for %s on roles %s", user, pres.getMissingSecurityRoles()); + } else { + err = String.format("no permissions for %s and %s", pres.getMissingPrivileges(), user); + } + log.debug(err); + channel.sendResponse(new BytesRestResponse(RestStatus.UNAUTHORIZED, err)); + return false; + } + } + + // if handler is not an instance of NamedRoute then we pass through to eval at Transport Layer. + return true; + } + + private boolean checkAndAuthenticateRequest(RestRequest request, RestChannel channel) throws Exception { threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_ORIGIN, Origin.REST.toString()); @@ -217,4 +271,30 @@ public void onWhitelistingSettingChanged(WhitelistingSettings whitelistingSettin public void onAllowlistingSettingChanged(AllowlistingSettings allowlistingSettings) { this.allowlistingSettings = allowlistingSettings; } + + /** + * Determines if the request's path is a match for the configured handler path. + * + * @param requestPath The path from the {@link NamedRoute} + * @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) { + // Check exact match + if (handlerPath.equals(requestPath)) { + return true; + } + // Split path to evaluate named params + String[] handlerSplit = handlerPath.split("/"); + String[] requestSplit = requestPath.split("/"); + if (handlerSplit.length != requestSplit.length) { + return false; + } + for (int i = 0; i < handlerSplit.length; i++) { + if (!(handlerSplit[i].equals(requestSplit[i]) || (handlerSplit[i].startsWith("{") && handlerSplit[i].endsWith("}")))) { + return false; + } + } + return true; + } } diff --git a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java new file mode 100644 index 0000000000..301207022b --- /dev/null +++ b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java @@ -0,0 +1,130 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.privileges; + +import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.greenrobot.eventbus.Subscribe; + +import org.opensearch.OpenSearchSecurityException; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.transport.TransportAddress; +import org.opensearch.common.util.concurrent.ThreadContext; +import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.security.auditlog.AuditLog; +import org.opensearch.security.configuration.ClusterInfoHolder; +import org.opensearch.security.securityconf.ConfigModel; +import org.opensearch.security.securityconf.DynamicConfigModel; +import org.opensearch.security.securityconf.SecurityRoles; +import org.opensearch.security.support.ConfigConstants; +import org.opensearch.security.user.User; +import org.opensearch.threadpool.ThreadPool; + +public class RestLayerPrivilegesEvaluator { + protected final Logger log = LogManager.getLogger(this.getClass()); + private final ClusterService clusterService; + private final AuditLog auditLog; + private ThreadContext threadContext; + private final ClusterInfoHolder clusterInfoHolder; + private ConfigModel configModel; + private DynamicConfigModel dcm; + private final AtomicReference namedXContentRegistry; + + public RestLayerPrivilegesEvaluator( + final ClusterService clusterService, + final ThreadPool threadPool, + AuditLog auditLog, + final ClusterInfoHolder clusterInfoHolder, + AtomicReference namedXContentRegistry + ) { + this.clusterService = clusterService; + this.auditLog = auditLog; + + this.threadContext = threadPool.getThreadContext(); + + this.clusterInfoHolder = clusterInfoHolder; + this.namedXContentRegistry = namedXContentRegistry; + } + + @Subscribe + public void onConfigModelChanged(ConfigModel configModel) { + this.configModel = configModel; + } + + @Subscribe + public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { + this.dcm = dcm; + } + + private SecurityRoles getSecurityRoles(Set roles) { + return configModel.getSecurityRoles().filter(roles); + } + + public boolean isInitialized() { + return configModel != null && configModel.getSecurityRoles() != null && dcm != null; + } + + public PrivilegesEvaluatorResponse evaluate(final User user, Set actions) { + + if (!isInitialized()) { + throw new OpenSearchSecurityException("OpenSearch Security is not initialized."); + } + + final PrivilegesEvaluatorResponse presponse = new PrivilegesEvaluatorResponse(); + + final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS); + + Set mappedRoles = mapRoles(user, caller); + + presponse.resolvedSecurityRoles.addAll(mappedRoles); + final SecurityRoles securityRoles = getSecurityRoles(mappedRoles); + + final boolean isDebugEnabled = log.isDebugEnabled(); + if (isDebugEnabled) { + log.debug("Evaluate permissions for {} on {}", user, clusterService.localNode().getName()); + log.debug("Action: {}", actions); + log.debug("Mapped roles: {}", mappedRoles.toString()); + } + + for (String action : actions) { + if (!securityRoles.impliesClusterPermissionPermission(action)) { + presponse.missingPrivileges.add(action); + presponse.allowed = false; + log.info( + "No permission match for {} [Action [{}]] [RolesChecked {}]. No permissions for {}", + user, + action, + securityRoles.getRoleNames(), + presponse.missingPrivileges + ); + } else { + if (isDebugEnabled) { + log.debug("Allowed because we have permissions for {}", actions); + } + presponse.allowed = true; + + // break the loop as we found the matching permission + break; + } + } + + return presponse; + } + + public Set mapRoles(final User user, final TransportAddress caller) { + return this.configModel.mapSecurityRoles(user, caller); + } + +} diff --git a/src/main/java/org/opensearch/security/rest/SecurityWhoAmIAction.java b/src/main/java/org/opensearch/security/rest/SecurityWhoAmIAction.java index 8f20a0b9a2..36af8b4ffa 100644 --- a/src/main/java/org/opensearch/security/rest/SecurityWhoAmIAction.java +++ b/src/main/java/org/opensearch/security/rest/SecurityWhoAmIAction.java @@ -15,6 +15,7 @@ import java.nio.file.Path; import java.util.Collections; import java.util.List; +import java.util.Set; import com.google.common.collect.ImmutableList; import org.apache.logging.log4j.LogManager; @@ -25,6 +26,7 @@ import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.rest.BaseRestHandler; import org.opensearch.rest.BytesRestResponse; +import org.opensearch.rest.NamedRoute; import org.opensearch.rest.RestChannel; import org.opensearch.rest.RestController; import org.opensearch.rest.RestRequest; @@ -44,7 +46,15 @@ public class SecurityWhoAmIAction extends BaseRestHandler { private static final List routes = addRoutesPrefix( - ImmutableList.of(new Route(GET, "/whoami"), new Route(POST, "/whoami")), + ImmutableList.of( + new Route(GET, "/whoami"), + new Route(POST, "/whoami"), + new NamedRoute.Builder().method(GET) + .path("/whoamiprotected") + .uniqueName("security:whoamiprotected") + .legacyActionNames(Set.of("cluster:admin/opendistro_security/whoamiprotected")) + .build() + ), "/_plugins/_security" ); diff --git a/src/test/java/org/opensearch/security/SystemIntegratorsTests.java b/src/test/java/org/opensearch/security/SystemIntegratorsTests.java index 83359c0a15..18670e6f4d 100644 --- a/src/test/java/org/opensearch/security/SystemIntegratorsTests.java +++ b/src/test/java/org/opensearch/security/SystemIntegratorsTests.java @@ -179,7 +179,7 @@ public void testInjectedUser() throws Exception { Assert.assertTrue(resc.getBody().contains("\"remote_address\":\"8.8.8.8:8\"")); Assert.assertTrue(resc.getBody().contains("\"backend_roles\":[\"role1\",\"role2\"]")); // mapped by username - Assert.assertTrue(resc.getBody().contains("\"roles\":[\"opendistro_security_all_access\"")); + Assert.assertTrue(resc.getBody().contains("\"opendistro_security_all_access\"")); Assert.assertTrue(resc.getBody().contains("\"custom_attribute_names\":[\"key1\",\"key2\"]")); resc = rh.executeGetRequest( diff --git a/src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java b/src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java new file mode 100644 index 0000000000..9a5335bdb7 --- /dev/null +++ b/src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java @@ -0,0 +1,66 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.security.filter; + +import org.junit.Before; +import org.junit.Test; + +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; + +public class RestPathMatchesTest { + Method restPathMatches; + SecurityRestFilter securityRestFilter; + + @Before + public void setUp() throws NoSuchMethodException { + securityRestFilter = mock(SecurityRestFilter.class); + restPathMatches = SecurityRestFilter.class.getDeclaredMethod("restPathMatches", String.class, String.class); + restPathMatches.setAccessible(true); + } + + @Test + public void testExactMatch() throws InvocationTargetException, IllegalAccessException { + String requestPath = "_plugins/security/api/x/y"; + String handlerPath = "_plugins/security/api/x/y"; + assertTrue((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); + } + + @Test + public void testPartialMatch() throws InvocationTargetException, IllegalAccessException { + String requestPath = "_plugins/security/api/x/y"; + String handlerPath = "_plugins/security/api/x/z"; + assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); + } + + @Test + public void testNamedParamsMatch() throws InvocationTargetException, IllegalAccessException { + String requestPath = "_plugins/security/api/123/y"; + String handlerPath = "_plugins/security/api/{id}/y"; + assertTrue((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); + } + + @Test + public void testDifferentPathLength() throws InvocationTargetException, IllegalAccessException { + String requestPath = "_plugins/security/api/x/y/z"; + String handlerPath = "_plugins/security/api/x/y"; + assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); + } + + @Test + public void testDifferentPathSegments() throws InvocationTargetException, IllegalAccessException { + String requestPath = "_plugins/security/api/a/b"; + String handlerPath = "_plugins/security/api/x/y"; + assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); + } +} diff --git a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java new file mode 100644 index 0000000000..3c8145c393 --- /dev/null +++ b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java @@ -0,0 +1,163 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.security.privileges; + +import java.util.Collections; +import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; + +import org.apache.logging.log4j.Logger; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mock; +import org.mockito.MockitoAnnotations; + +import org.opensearch.OpenSearchSecurityException; +import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.concurrent.ThreadContext; +import org.opensearch.core.xcontent.NamedXContentRegistry; +import org.opensearch.security.auditlog.AuditLog; +import org.opensearch.security.configuration.ClusterInfoHolder; +import org.opensearch.security.securityconf.ConfigModel; +import org.opensearch.security.securityconf.DynamicConfigModel; +import org.opensearch.security.securityconf.SecurityRoles; +import org.opensearch.security.user.User; +import org.opensearch.threadpool.ThreadPool; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class RestLayerPrivilegesEvaluatorTest { + + @Mock + private ClusterService clusterService; + @Mock + private ThreadPool threadPool; + @Mock + private AtomicReference namedXContentRegistry; + @Mock + private ConfigModel configModel; + @Mock + private DynamicConfigModel dcm; + @Mock + private PrivilegesEvaluatorResponse presponse; + @Mock + private Logger log; + + private RestLayerPrivilegesEvaluator privilegesEvaluator; + + private static final User TEST_USER = new User("test_user"); + + @Before + public void setUp() throws InstantiationException, IllegalAccessException { + MockitoAnnotations.openMocks(this); + + ThreadContext context = new ThreadContext(Settings.EMPTY); + when(threadPool.getThreadContext()).thenReturn(context); + + privilegesEvaluator = new RestLayerPrivilegesEvaluator( + clusterService, + threadPool, + mock(AuditLog.class), + mock(ClusterInfoHolder.class), + namedXContentRegistry + ); + privilegesEvaluator.onConfigModelChanged(configModel); + privilegesEvaluator.onDynamicConfigModelChanged(dcm); + + when(log.isDebugEnabled()).thenReturn(false); + } + + @Test + public void testEvaluate_Initialized_Success() { + String action = "action"; + SecurityRoles securityRoles = mock(SecurityRoles.class); + when(configModel.getSecurityRoles()).thenReturn(securityRoles); + when(configModel.getSecurityRoles().filter(Collections.emptySet())).thenReturn(securityRoles); + when(securityRoles.impliesClusterPermissionPermission(action)).thenReturn(false); + + PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action)); + + assertNotNull(response); + assertFalse(response.isAllowed()); + assertFalse(response.getMissingPrivileges().isEmpty()); + assertTrue(response.getResolvedSecurityRoles().isEmpty()); + verify(configModel, times(3)).getSecurityRoles(); + } + + @Test(expected = OpenSearchSecurityException.class) + public void testEvaluate_NotInitialized_ExceptionThrown() throws Exception { + String action = "action"; + privilegesEvaluator.evaluate(TEST_USER, Set.of(action)); + } + + @Test + public void testMapRoles_ReturnsMappedRoles() { + Set mappedRoles = Collections.singleton("role1"); + when(configModel.mapSecurityRoles(any(), any())).thenReturn(mappedRoles); + + Set result = privilegesEvaluator.mapRoles(any(), any()); + + assertEquals(mappedRoles, result); + verify(configModel).mapSecurityRoles(any(), any()); + } + + @Test + public void testEvaluate_Successful_NewPermission() { + String action = "hw:greet"; + SecurityRoles securityRoles = mock(SecurityRoles.class); + when(configModel.getSecurityRoles()).thenReturn(securityRoles); + when(configModel.getSecurityRoles().filter(Collections.emptySet())).thenReturn(securityRoles); + when(securityRoles.impliesClusterPermissionPermission(action)).thenReturn(true); + + PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action)); + + assertTrue(response.allowed); + verify(securityRoles).impliesClusterPermissionPermission(any()); + } + + @Test + public void testEvaluate_Successful_LegacyPermission() { + String action = "cluster:admin/opensearch/hw/greet"; + SecurityRoles securityRoles = mock(SecurityRoles.class); + when(configModel.getSecurityRoles()).thenReturn(securityRoles); + when(configModel.getSecurityRoles().filter(Collections.emptySet())).thenReturn(securityRoles); + when(securityRoles.impliesClusterPermissionPermission(action)).thenReturn(true); + + PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action)); + + assertTrue(response.allowed); + verify(securityRoles).impliesClusterPermissionPermission(any()); + } + + @Test + public void testEvaluate_Unsuccessful() { + String action = "action"; + SecurityRoles securityRoles = mock(SecurityRoles.class); + when(configModel.getSecurityRoles()).thenReturn(securityRoles); + when(configModel.getSecurityRoles().filter(Collections.emptySet())).thenReturn(securityRoles); + when(securityRoles.impliesClusterPermissionPermission(action)).thenReturn(false); + + PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action)); + + assertFalse(response.allowed); + verify(securityRoles).impliesClusterPermissionPermission(any()); + } +} diff --git a/src/test/resources/roles.yml b/src/test/resources/roles.yml index 898ea9215f..89c9ba04c0 100644 --- a/src/test/resources/roles.yml +++ b/src/test/resources/roles.yml @@ -1357,3 +1357,9 @@ sem-role2: - "sem*" allowed_actions: - "indices:admin/index_template/put" + +who_am_i: + reserved: true + hidden: false + description: "Test role to grant access to /whoami endpoint" + cluster_permissions: [ "security:whoamiprotected" ] diff --git a/src/test/resources/roles_mapping.yml b/src/test/resources/roles_mapping.yml index 2233827e51..68ec0f5e48 100644 --- a/src/test/resources/roles_mapping.yml +++ b/src/test/resources/roles_mapping.yml @@ -498,3 +498,8 @@ sem-role2: hidden: false users: - "sem-user2" +who_am_i: + reserved: false + hidden: false + users: + - "nagilum" From 18046d5b7677b774ae13ddcfafc0d4e38d2f8706 Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Thu, 24 Aug 2023 11:54:46 -0400 Subject: [PATCH 2/4] Fixes Transport Address import in Rest evaluator Signed-off-by: Darshit Chanpura --- .../security/privileges/RestLayerPrivilegesEvaluator.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java index 301207022b..8602fb91e6 100644 --- a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java @@ -20,7 +20,7 @@ import org.opensearch.OpenSearchSecurityException; import org.opensearch.cluster.service.ClusterService; -import org.opensearch.common.transport.TransportAddress; +import org.opensearch.core.common.transport.TransportAddress; import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.security.auditlog.AuditLog; From 24ace5c5762d78a47a39a5185dff1431d1d5cbdf Mon Sep 17 00:00:00 2001 From: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Date: Wed, 23 Aug 2023 17:26:13 -0400 Subject: [PATCH 3/4] Enhance RestLayerPrivilegesEvaluator Code Coverage (#3218) Enhance RestLayerPrivilegesEvaluator Code Coverage Signed-off-by: Stephen Crawford Signed-off-by: Peter Nied Signed-off-by: Darshit Chanpura Co-authored-by: Peter Nied Co-authored-by: Darshit Chanpura (cherry picked from commit 46dfd841abeae193075d8cf95dd4023e9ea71ef9) --- .../security/OpenSearchSecurityPlugin.java | 2 +- .../RestLayerPrivilegesEvaluator.java | 47 ++----- ...hesTest.java => RestPathMatchesTests.java} | 23 +++- ...lterTest.java => SecurityFilterTests.java} | 4 +- ...Test.java => SecurityRestFilterTests.java} | 2 +- .../RestLayerPrivilegesEvaluatorTest.java | 121 ++++++++++-------- 6 files changed, 106 insertions(+), 93 deletions(-) rename src/test/java/org/opensearch/security/filter/{RestPathMatchesTest.java => RestPathMatchesTests.java} (70%) rename src/test/java/org/opensearch/security/filter/{SecurityFilterTest.java => SecurityFilterTests.java} (97%) rename src/test/java/org/opensearch/security/filter/{SecurityRestFilterTest.java => SecurityRestFilterTests.java} (99%) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 789531bb2b..45000f311e 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -1021,7 +1021,7 @@ public Collection createComponents( principalExtractor = ReflectionHelper.instantiatePrincipalExtractor(principalExtractorClass); } - restLayerEvaluator = new RestLayerPrivilegesEvaluator(clusterService, threadPool, auditLog, cih, namedXContentRegistry); + restLayerEvaluator = new RestLayerPrivilegesEvaluator(clusterService, threadPool); securityRestHandler = new SecurityRestFilter( backendRegistry, diff --git a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java index 8602fb91e6..dc95e98d11 100644 --- a/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java @@ -12,7 +12,6 @@ package org.opensearch.security.privileges; import java.util.Set; -import java.util.concurrent.atomic.AtomicReference; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -22,11 +21,7 @@ import org.opensearch.cluster.service.ClusterService; import org.opensearch.core.common.transport.TransportAddress; import org.opensearch.common.util.concurrent.ThreadContext; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.security.auditlog.AuditLog; -import org.opensearch.security.configuration.ClusterInfoHolder; import org.opensearch.security.securityconf.ConfigModel; -import org.opensearch.security.securityconf.DynamicConfigModel; import org.opensearch.security.securityconf.SecurityRoles; import org.opensearch.security.support.ConfigConstants; import org.opensearch.security.user.User; @@ -35,49 +30,28 @@ public class RestLayerPrivilegesEvaluator { protected final Logger log = LogManager.getLogger(this.getClass()); private final ClusterService clusterService; - private final AuditLog auditLog; private ThreadContext threadContext; - private final ClusterInfoHolder clusterInfoHolder; private ConfigModel configModel; - private DynamicConfigModel dcm; - private final AtomicReference namedXContentRegistry; - - public RestLayerPrivilegesEvaluator( - final ClusterService clusterService, - final ThreadPool threadPool, - AuditLog auditLog, - final ClusterInfoHolder clusterInfoHolder, - AtomicReference namedXContentRegistry - ) { - this.clusterService = clusterService; - this.auditLog = auditLog; + public RestLayerPrivilegesEvaluator(final ClusterService clusterService, final ThreadPool threadPool) { + this.clusterService = clusterService; this.threadContext = threadPool.getThreadContext(); - - this.clusterInfoHolder = clusterInfoHolder; - this.namedXContentRegistry = namedXContentRegistry; } @Subscribe - public void onConfigModelChanged(ConfigModel configModel) { + public void onConfigModelChanged(final ConfigModel configModel) { this.configModel = configModel; } - @Subscribe - public void onDynamicConfigModelChanged(DynamicConfigModel dcm) { - this.dcm = dcm; - } - - private SecurityRoles getSecurityRoles(Set roles) { + SecurityRoles getSecurityRoles(final Set roles) { return configModel.getSecurityRoles().filter(roles); } - public boolean isInitialized() { - return configModel != null && configModel.getSecurityRoles() != null && dcm != null; + boolean isInitialized() { + return configModel != null && configModel.getSecurityRoles() != null; } - public PrivilegesEvaluatorResponse evaluate(final User user, Set actions) { - + public PrivilegesEvaluatorResponse evaluate(final User user, final Set actions) { if (!isInitialized()) { throw new OpenSearchSecurityException("OpenSearch Security is not initialized."); } @@ -86,7 +60,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, Set actions final TransportAddress caller = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS); - Set mappedRoles = mapRoles(user, caller); + final Set mappedRoles = mapRoles(user, caller); presponse.resolvedSecurityRoles.addAll(mappedRoles); final SecurityRoles securityRoles = getSecurityRoles(mappedRoles); @@ -98,7 +72,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, Set actions log.debug("Mapped roles: {}", mappedRoles.toString()); } - for (String action : actions) { + for (final String action : actions) { if (!securityRoles.impliesClusterPermissionPermission(action)) { presponse.missingPrivileges.add(action); presponse.allowed = false; @@ -123,8 +97,7 @@ public PrivilegesEvaluatorResponse evaluate(final User user, Set actions return presponse; } - public Set mapRoles(final User user, final TransportAddress caller) { + Set mapRoles(final User user, final TransportAddress caller) { return this.configModel.mapSecurityRoles(user, caller); } - } diff --git a/src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java b/src/test/java/org/opensearch/security/filter/RestPathMatchesTests.java similarity index 70% rename from src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java rename to src/test/java/org/opensearch/security/filter/RestPathMatchesTests.java index 9a5335bdb7..eed095c9b6 100644 --- a/src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java +++ b/src/test/java/org/opensearch/security/filter/RestPathMatchesTests.java @@ -18,7 +18,7 @@ import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; -public class RestPathMatchesTest { +public class RestPathMatchesTests { Method restPathMatches; SecurityRestFilter securityRestFilter; @@ -63,4 +63,25 @@ public void testDifferentPathSegments() throws InvocationTargetException, Illega String handlerPath = "_plugins/security/api/x/y"; assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); } + + @Test + public void testRequestPathWithNamedParam() throws InvocationTargetException, IllegalAccessException { + String requestPath = "_plugins/security/api/123/y"; + String handlerPath = "_plugins/security/api/{id}/z"; + assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); + } + + @Test + public void testRequestPathMismatch() throws InvocationTargetException, IllegalAccessException { + String requestPath = "_plugins/security/api/x/y"; + String handlerPath = "_plugins/security/api/z/y"; + 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"; + assertFalse((Boolean) restPathMatches.invoke(securityRestFilter, requestPath, handlerPath)); + } } diff --git a/src/test/java/org/opensearch/security/filter/SecurityFilterTest.java b/src/test/java/org/opensearch/security/filter/SecurityFilterTests.java similarity index 97% rename from src/test/java/org/opensearch/security/filter/SecurityFilterTest.java rename to src/test/java/org/opensearch/security/filter/SecurityFilterTests.java index ea2978302e..58a12a84a8 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityFilterTest.java +++ b/src/test/java/org/opensearch/security/filter/SecurityFilterTests.java @@ -47,12 +47,12 @@ import static org.mockito.Mockito.when; @RunWith(Parameterized.class) -public class SecurityFilterTest { +public class SecurityFilterTests { private final Settings settings; private final WildcardMatcher expected; - public SecurityFilterTest(Settings settings, WildcardMatcher expected) { + public SecurityFilterTests(Settings settings, WildcardMatcher expected) { this.settings = settings; this.expected = expected; } diff --git a/src/test/java/org/opensearch/security/filter/SecurityRestFilterTest.java b/src/test/java/org/opensearch/security/filter/SecurityRestFilterTests.java similarity index 99% rename from src/test/java/org/opensearch/security/filter/SecurityRestFilterTest.java rename to src/test/java/org/opensearch/security/filter/SecurityRestFilterTests.java index f58ba25f15..1c037505da 100644 --- a/src/test/java/org/opensearch/security/filter/SecurityRestFilterTest.java +++ b/src/test/java/org/opensearch/security/filter/SecurityRestFilterTests.java @@ -27,7 +27,7 @@ * Currently tests that the whitelisting functionality works correctly. * Uses the test/resources/restapi folder for setup. */ -public class SecurityRestFilterTest extends AbstractRestApiUnitTest { +public class SecurityRestFilterTests extends AbstractRestApiUnitTest { private RestHelper.HttpResponse response; diff --git a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java index 3c8145c393..2f6189bab2 100644 --- a/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java +++ b/src/test/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluatorTest.java @@ -13,76 +13,77 @@ import java.util.Collections; import java.util.Set; -import java.util.concurrent.atomic.AtomicReference; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.core.config.Configurator; +import org.hamcrest.Matchers; +import org.junit.After; import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; - +import org.mockito.junit.MockitoJUnitRunner; +import org.mockito.quality.Strictness; import org.opensearch.OpenSearchSecurityException; +import org.opensearch.cluster.node.DiscoveryNode; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.ThreadContext; -import org.opensearch.core.xcontent.NamedXContentRegistry; -import org.opensearch.security.auditlog.AuditLog; -import org.opensearch.security.configuration.ClusterInfoHolder; import org.opensearch.security.securityconf.ConfigModel; -import org.opensearch.security.securityconf.DynamicConfigModel; import org.opensearch.security.securityconf.SecurityRoles; import org.opensearch.security.user.User; import org.opensearch.threadpool.ThreadPool; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.any; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.Assert.assertThrows; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.withSettings; +@RunWith(MockitoJUnitRunner.class) public class RestLayerPrivilegesEvaluatorTest { - @Mock + @Mock(strictness = Mock.Strictness.LENIENT) private ClusterService clusterService; @Mock private ThreadPool threadPool; @Mock - private AtomicReference namedXContentRegistry; - @Mock private ConfigModel configModel; - @Mock - private DynamicConfigModel dcm; - @Mock - private PrivilegesEvaluatorResponse presponse; - @Mock - private Logger log; private RestLayerPrivilegesEvaluator privilegesEvaluator; private static final User TEST_USER = new User("test_user"); - @Before - public void setUp() throws InstantiationException, IllegalAccessException { - MockitoAnnotations.openMocks(this); + private void setLoggingLevel(final Level level) { + final Logger restLayerPrivilegesEvaluatorLogger = LogManager.getLogger(RestLayerPrivilegesEvaluator.class); + Configurator.setLevel(restLayerPrivilegesEvaluatorLogger, level); + } - ThreadContext context = new ThreadContext(Settings.EMPTY); - when(threadPool.getThreadContext()).thenReturn(context); + @Before + public void setUp() { + when(threadPool.getThreadContext()).thenReturn(new ThreadContext(Settings.EMPTY)); + when(clusterService.localNode()).thenReturn(mock(DiscoveryNode.class, withSettings().strictness(Strictness.LENIENT))); privilegesEvaluator = new RestLayerPrivilegesEvaluator( clusterService, - threadPool, - mock(AuditLog.class), - mock(ClusterInfoHolder.class), - namedXContentRegistry + threadPool ); - privilegesEvaluator.onConfigModelChanged(configModel); - privilegesEvaluator.onDynamicConfigModelChanged(dcm); + privilegesEvaluator.onConfigModelChanged(configModel); // Defaults to the mocked config model + verify(threadPool).getThreadContext(); // Called during construction of RestLayerPrivilegesEvaluator + setLoggingLevel(Level.DEBUG); // Enable debug logging scenarios for verification + } - when(log.isDebugEnabled()).thenReturn(false); + @After + public void after() { + setLoggingLevel(Level.INFO); } @Test @@ -95,28 +96,45 @@ public void testEvaluate_Initialized_Success() { PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action)); - assertNotNull(response); - assertFalse(response.isAllowed()); - assertFalse(response.getMissingPrivileges().isEmpty()); - assertTrue(response.getResolvedSecurityRoles().isEmpty()); + assertThat(response.isAllowed(), equalTo(false)); + assertThat(response.getMissingPrivileges(), equalTo(Set.of(action))); + assertThat(response.getResolvedSecurityRoles(), Matchers.empty()); verify(configModel, times(3)).getSecurityRoles(); } - @Test(expected = OpenSearchSecurityException.class) - public void testEvaluate_NotInitialized_ExceptionThrown() throws Exception { - String action = "action"; - privilegesEvaluator.evaluate(TEST_USER, Set.of(action)); + @Test + public void testEvaluate_NotInitialized_NullModel_ExceptionThrown() { + // Null out the config model + privilegesEvaluator.onConfigModelChanged(null); + final OpenSearchSecurityException exception = assertThrows( + OpenSearchSecurityException.class, + () -> privilegesEvaluator.evaluate(TEST_USER, null) + ); + assertThat(exception.getMessage(), equalTo("OpenSearch Security is not initialized.")); + verify(configModel, never()).getSecurityRoles(); + } + + @Test + public void testEvaluate_NotInitialized_NoSecurityRoles_ExceptionThrown() { + final OpenSearchSecurityException exception = assertThrows( + OpenSearchSecurityException.class, + () -> privilegesEvaluator.evaluate(TEST_USER, null) + ); + assertThat(exception.getMessage(), equalTo("OpenSearch Security is not initialized.")); + verify(configModel).getSecurityRoles(); } @Test public void testMapRoles_ReturnsMappedRoles() { - Set mappedRoles = Collections.singleton("role1"); + final User user = mock(User.class); + final Set mappedRoles = Collections.singleton("role1"); when(configModel.mapSecurityRoles(any(), any())).thenReturn(mappedRoles); - Set result = privilegesEvaluator.mapRoles(any(), any()); + final Set result = privilegesEvaluator.mapRoles(user, null); - assertEquals(mappedRoles, result); - verify(configModel).mapSecurityRoles(any(), any()); + assertThat(result, equalTo(mappedRoles)); + verifyNoInteractions(user); + verify(configModel).mapSecurityRoles(user, null); } @Test @@ -129,8 +147,8 @@ public void testEvaluate_Successful_NewPermission() { PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action)); - assertTrue(response.allowed); - verify(securityRoles).impliesClusterPermissionPermission(any()); + assertThat(response.allowed, equalTo(true)); + verify(securityRoles).impliesClusterPermissionPermission(action); } @Test @@ -143,8 +161,9 @@ public void testEvaluate_Successful_LegacyPermission() { PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action)); - assertTrue(response.allowed); - verify(securityRoles).impliesClusterPermissionPermission(any()); + assertThat(response.allowed, equalTo(true)); + verify(securityRoles).impliesClusterPermissionPermission(action); + verify(configModel, times(3)).getSecurityRoles(); } @Test @@ -157,7 +176,7 @@ public void testEvaluate_Unsuccessful() { PrivilegesEvaluatorResponse response = privilegesEvaluator.evaluate(TEST_USER, Set.of(action)); - assertFalse(response.allowed); - verify(securityRoles).impliesClusterPermissionPermission(any()); + assertThat(response.allowed, equalTo(false)); + verify(securityRoles).impliesClusterPermissionPermission(action); } } From c459df5e02ce5324ac2373193c2a0e05fbfbe0fe Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Fri, 29 Sep 2023 18:42:56 -0400 Subject: [PATCH 4/4] Fixes http5 import Signed-off-by: Darshit Chanpura --- .../java/org/opensearch/security/rest/WhoAmITests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java index 5e9992c8ad..59e9e192f9 100644 --- a/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java +++ b/src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java @@ -12,7 +12,7 @@ package org.opensearch.security.rest; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; -import org.apache.hc.core5.http.HttpStatus; +import org.apache.http.HttpStatus; import org.junit.ClassRule; import org.junit.Test; import org.junit.runner.RunWith;