Authorize rest requests #2753

DarshitChanpura · 2023-05-09T21:42:59Z

Description

Continuation of #2601. Adds support for legacy naming conventions

Category : Enhancement
Why these changes are required? To support legacy action names to authorize REST requests

Issues Resolved

#2752

Testing

Manual testing.

Setup opensearch-cluster and helloWorld extension following this developer guide.
Check-out this branch.
Install security in opensearch.
Ensure certificates are setup correctly in config folder and in opensearch.yml
Following should be the output of each curl command:

➜  ~ curl -XGET https://new-user:admin@localhost:9200/_extensions/_hw/hello --insecure
Hello, World!%
➜  ~ curl -XPUT https://new-user:admin@localhost:9200/_extensions/_hw/hello/HW --insecure -H 'Content-Type: application/json' -d '{}'
Updated the world's name to HW%
➜  ~ curl -XGET https://new-user:admin@localhost:9200/_extensions/_hw/hello --insecure
Hello, HW!%
➜  ~ curl -XPOST https://new-user:admin@localhost:9200/_extensions/_hw/hello --insecure -H 'Content-Type: application/json' -d '{ }'
no permissions for [hw:greet_with_adjective] and User [name=new-user, backend_roles=[], requestedTenant=null]%
➜  ~ curl -XDELETE https://new-user:admin@localhost:9200/_extensions/_hw/goodbye --insecure
no permissions for [hw:goodbye] and User [name=new-user, backend_roles=[], requestedTenant=null]%

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java

stephen-crawford

Hi @DarshitChanpura, looks good outside two blank lines and Craig;s comment.

config/roles.yml

src/main/java/org/opensearch/security/filter/SecurityRestFilter.java

stephen-crawford

What are your plans for testing these changes? Are you just trying to let the existing tests cover the changes? Is there a way to add explicit checks for the changes you have introduced?

clicked wrong button

DarshitChanpura · 2023-05-23T16:25:33Z

What are your plans for testing these changes? Are you just trying to let the existing tests cover the changes? Is there a way to add explicit checks for the changes you have introduced?

This will be covered once end to end testing is implemented.

Signed-off-by: Craig Perkins <[email protected]>

…o whoami route Signed-off-by: Darshit Chanpura <[email protected]>

Signed-off-by: Darshit Chanpura <[email protected]>

cwperks

Took another pass. @DarshitChanpura Is it possible to remove the impliesLegacyPermission? This PR looks close, but I'd like to get resolution on this first.

DEVELOPER_GUIDE.md

REST_AUTHZ_FOR_PLUGINS.md

src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java

src/main/java/org/opensearch/security/action/whoami/WhoAmIAction.java

src/main/java/org/opensearch/security/filter/SecurityRestFilter.java

cwperks · 2023-07-07T19:27:27Z

src/main/java/org/opensearch/security/filter/SecurityRestFilter.java

+     * @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) {


Units like this should have tests with them. I've seen code similar to this in core, can this be extracted to a library?

Test file already exists. Check it out https://github.com/opensearch-project/security/blob/9b05d44a6ba15bca521e1ccbd6e33124facddcc0/src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java

can this be extracted to a library?

It's only used at 2 places and I don't think it will be used at other places in future. I think keeping it here is fine.

src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java

RyanL1997

Hi @DarshitChanpura, in generally LGTM beyond some of the suggestions that Craig and Stephen left and thanks for all the work on this one!

src/main/java/org/opensearch/security/action/whoami/WhoAmIAction.java

src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java

Signed-off-by: Darshit Chanpura <[email protected]>

All requested changes have been addressed.

DarshitChanpura · 2023-07-07T20:38:46Z

Can we change an existing action in the security plugin to use NamedRoute so we can write an integration test that confirms AuthZ for rest works?

@peternied Added a new endpoint GET /whoamiprotected and integrations tests for it WhoAmITests.java.

cwperks · 2023-07-07T20:39:27Z

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\""));


Was this change needed?

Yes. because nagilum is now associated with another role called who_am_i, the request body won't always start with \"roles\":[

Similar to the assertion above with backend roles, I think this should assert all expected roles

stephen-crawford

Great job DC!

cwperks · 2023-07-07T20:42:19Z

src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java

+            log.debug("Mapped roles: {}", mappedRoles.toString());
+        }
+
+        for (String action : action0) {


nit: I had to reference up above that action0 is a set. This looks like its looping through a singular action.

It will be singular for name, but it will be in loop for actionNames

Signed-off-by: Darshit Chanpura <[email protected]>

opensearch-trigger-bot · 2023-07-07T21:14:33Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2753-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a53a8a6c9d990911cb5f6a42e937b395c598b0d5
# Push it to GitHub
git push --set-upstream origin backport/backport-2753-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-2753-to-2.x.

* WIP on rest layer authz Signed-off-by: Craig Perkins <[email protected]> * WIP on rest-layer authz Signed-off-by: Craig Perkins <[email protected]> * Extension handshake Signed-off-by: Craig Perkins <[email protected]> * Extension TLS Signed-off-by: Craig Perkins <[email protected]> * Remove SecurityRestFilterChanges to isolate extension TLS change Signed-off-by: Craig Perkins <[email protected]> * Remove SecurityRestFilterChanges to isolate extension TLS change Signed-off-by: Craig Perkins <[email protected]> * Remove SecurityRestFilterChanges to isolate extension TLS change Signed-off-by: Craig Perkins <[email protected]> * Remove SecurityRestFilterChanges to isolate extension TLS change Signed-off-by: Craig Perkins <[email protected]> * Remove SecurityRestFilterChanges to isolate extension TLS change Signed-off-by: Craig Perkins <[email protected]> * WIP for HelloWorld sample extension role Signed-off-by: Craig Perkins <[email protected]> * Initial implementation of authz check in REST layer Signed-off-by: Craig Perkins <[email protected]> * Remove header Signed-off-by: Craig Perkins <[email protected]> * Create authorizeRequest method Signed-off-by: Craig Perkins <[email protected]> * small fix Signed-off-by: Craig Perkins <[email protected]> * Change to ProtectedRoute Signed-off-by: Craig Perkins <[email protected]> * Remove extension permissions Signed-off-by: Craig Perkins <[email protected]> * Initial implementation of authz check in REST layer Signed-off-by: Craig Perkins <[email protected]> * Extension TLS Signed-off-by: Craig Perkins <[email protected]> * Adds dummy roles for testing rest authorization against legacy permissions Signed-off-by: Darshit Chanpura <[email protected]> * Adds support for legacy permissions to perform rest authorization Signed-off-by: Darshit Chanpura <[email protected]> * Fixes white-space changes Signed-off-by: Darshit Chanpura <[email protected]> * Rebases ConfigConstants with main Signed-off-by: Darshit Chanpura <[email protected]> * Implements a new logic for rest permissions check to be more flexible Signed-off-by: Darshit Chanpura <[email protected]> * Fixes spotless errors Signed-off-by: Darshit Chanpura <[email protected]> * Adds regex to match against current role permissions when comparing new permission with legacy ones Signed-off-by: Darshit Chanpura <[email protected]> * Moves legacy permission check logic to ConfigModelV7 Signed-off-by: Darshit Chanpura <[email protected]> * Fixes extra new-lines Signed-off-by: Darshit Chanpura <[email protected]> * Fixes unused imports Signed-off-by: Darshit Chanpura <[email protected]> * Fixes out-of-scope white space changes Signed-off-by: Darshit Chanpura <[email protected]> * Fixes code-ql errors Signed-off-by: Darshit Chanpura <[email protected]> * Fixes spotless and code-ql errors Signed-off-by: Darshit Chanpura <[email protected]> * Fixes variable name and remove references to whitelist in javadoc Signed-off-by: Darshit Chanpura <[email protected]> * Adds tests for rest layer privilege evaluator Signed-off-by: Darshit Chanpura <[email protected]> * Adds license header to the test file Signed-off-by: Darshit Chanpura <[email protected]> * Updates zstd dependency to fetch from core version.properties Signed-off-by: Darshit Chanpura <[email protected]> * Updates action name in the regex to be dynamic Signed-off-by: Darshit Chanpura <[email protected]> * Adds support for allowing evaluation against multiple actions names for a registered named route Signed-off-by: Darshit Chanpura <[email protected]> * Updates tests Signed-off-by: Darshit Chanpura <[email protected]> * Adds null check Signed-off-by: Darshit Chanpura <[email protected]> * Makes authorize logic clearer to follow Signed-off-by: Darshit Chanpura <[email protected]> * Adds extra check to ensure new actions are also evaluated against transport actions Signed-off-by: Darshit Chanpura <[email protected]> * Fixes spotless errors Signed-off-by: Darshit Chanpura <[email protected]> * Fixes security rest filter setup Signed-off-by: Darshit Chanpura <[email protected]> * Removes extension reference Signed-off-by: Darshit Chanpura <[email protected]> * turn on audit logging Signed-off-by: Maciej Mierzwa <[email protected]> * Adds unit tests for restPathMatches method Signed-off-by: Darshit Chanpura <[email protected]> * Cleans up TODOs Signed-off-by: Darshit Chanpura <[email protected]> * Organizes demo users and roles for extension Signed-off-by: Darshit Chanpura <[email protected]> * Address PR feedback Signed-off-by: Darshit Chanpura <[email protected]> * Adds more comments Signed-off-by: Darshit Chanpura <[email protected]> * add privileges info Signed-off-by: Maciej Mierzwa <[email protected]> * Makes whoami action a named route and fixes license header check Signed-off-by: Darshit Chanpura <[email protected]> * Adds integ tests for whoami route Signed-off-by: Darshit Chanpura <[email protected]> * Change permissions order in roles.yml Signed-off-by: Darshit Chanpura <[email protected]> * Adds developer documentation for authorization in REST layer Signed-off-by: Darshit Chanpura <[email protected]> * Fixes broken tests Signed-off-by: Darshit Chanpura <[email protected]> * Fixes checkstyle errors Signed-off-by: Darshit Chanpura <[email protected]> * Addresses feedback and cleans up logic for super admin check Signed-off-by: Darshit Chanpura <[email protected]> * Addresses Plugin Install CI failure Signed-off-by: Darshit Chanpura <[email protected]> * Fixes failing citest task Signed-off-by: Darshit Chanpura <[email protected]> * Modifies WhoAmI integ tests Signed-off-by: Darshit Chanpura <[email protected]> * Adds a new endpoint called whoamiprotected and removes changes made to whoami route Signed-off-by: Darshit Chanpura <[email protected]> * Updates documentation to reflect the new API Signed-off-by: Darshit Chanpura <[email protected]> * Addresses PR feedback Signed-off-by: Darshit Chanpura <[email protected]> * Renames action0 to actions Signed-off-by: Darshit Chanpura <[email protected]> --------- Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Darshit Chanpura <[email protected]> Signed-off-by: Maciej Mierzwa <[email protected]> Co-authored-by: Craig Perkins <[email protected]> Co-authored-by: MaciejMierzwa <[email protected]> (cherry picked from commit a53a8a6)

DarshitChanpura requested review from cliu123, cwperks, davidlago, peternied, RyanL1997, stephen-crawford and reta as code owners May 9, 2023 21:42

DarshitChanpura changed the title ~~Authorize rest requests~~ Authorize rest requests bound to Extensions May 9, 2023

DarshitChanpura changed the title ~~Authorize rest requests bound to Extensions~~ [Feature/Extensions] Authorize rest requests bound to Extensions May 9, 2023

DarshitChanpura marked this pull request as draft May 9, 2023 21:51

DarshitChanpura mentioned this pull request May 16, 2023

Authorize Requests bound for extensions in the REST-Layer #2601

Closed

3 tasks

cwperks mentioned this pull request May 16, 2023

[Feature/Extensions] Implement PrivilegeEvaluator on REST Layer. #2751

Closed

1 task

DarshitChanpura marked this pull request as ready for review May 19, 2023 17:06

cwperks mentioned this pull request May 22, 2023

Map extension routes to permission-able action names opensearch-project/opensearch-sdk-java#622

Closed

cwperks reviewed May 23, 2023

View reviewed changes

src/main/java/org/opensearch/security/privileges/RestLayerPrivilegesEvaluator.java Outdated Show resolved Hide resolved

stephen-crawford previously approved these changes May 23, 2023

View reviewed changes

config/roles.yml Outdated Show resolved Hide resolved

src/main/java/org/opensearch/security/filter/SecurityRestFilter.java Outdated Show resolved Hide resolved

stephen-crawford reviewed May 23, 2023

View reviewed changes

stephen-crawford self-requested a review May 23, 2023 16:23

cwperks added 10 commits May 24, 2023 09:39

WIP on rest layer authz

a8a1660

Signed-off-by: Craig Perkins <[email protected]>

WIP on rest-layer authz

0fedc03

Signed-off-by: Craig Perkins <[email protected]>

Extension handshake

1793a5b

Signed-off-by: Craig Perkins <[email protected]>

Extension TLS

32d8763

Signed-off-by: Craig Perkins <[email protected]>

Remove SecurityRestFilterChanges to isolate extension TLS change

a5a44b8

Signed-off-by: Craig Perkins <[email protected]>

Remove SecurityRestFilterChanges to isolate extension TLS change

99bfd94

Signed-off-by: Craig Perkins <[email protected]>

Remove SecurityRestFilterChanges to isolate extension TLS change

f62f590

Signed-off-by: Craig Perkins <[email protected]>

Remove SecurityRestFilterChanges to isolate extension TLS change

7596b7e

Signed-off-by: Craig Perkins <[email protected]>

Remove SecurityRestFilterChanges to isolate extension TLS change

1fb5316

Signed-off-by: Craig Perkins <[email protected]>

WIP for HelloWorld sample extension role

25fda77

Signed-off-by: Craig Perkins <[email protected]>

DarshitChanpura added 2 commits July 7, 2023 14:45

Adds a new endpoint called whoamiprotected and removes changes made t…

ffdd927

…o whoami route Signed-off-by: Darshit Chanpura <[email protected]>

Updates documentation to reflect the new API

9b05d44

Signed-off-by: Darshit Chanpura <[email protected]>

stephen-crawford previously approved these changes Jul 7, 2023

View reviewed changes

cwperks reviewed Jul 7, 2023

View reviewed changes

DarshitChanpura mentioned this pull request Jul 7, 2023

RestLayerPrivilegesEvaluator must have 100% branch code coverage #2929

Closed

RyanL1997 reviewed Jul 7, 2023

View reviewed changes

src/main/java/org/opensearch/security/action/whoami/WhoAmIAction.java Outdated Show resolved Hide resolved

src/test/java/org/opensearch/security/filter/RestPathMatchesTest.java Show resolved Hide resolved

Addresses PR feedback

a21c4c0

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura dismissed stephen-crawford’s stale review via a21c4c0 July 7, 2023 20:29

RyanL1997 previously approved these changes Jul 7, 2023

View reviewed changes

cwperks reviewed Jul 7, 2023

View reviewed changes

stephen-crawford previously approved these changes Jul 7, 2023

View reviewed changes

cwperks reviewed Jul 7, 2023

View reviewed changes

cwperks previously approved these changes Jul 7, 2023

View reviewed changes

Renames action0 to actions

6c5ee8e

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura dismissed stale reviews from cwperks, stephen-crawford, and RyanL1997 via 6c5ee8e July 7, 2023 20:45

RyanL1997 approved these changes Jul 7, 2023

View reviewed changes

stephen-crawford approved these changes Jul 7, 2023

View reviewed changes

cwperks approved these changes Jul 7, 2023

View reviewed changes

DarshitChanpura merged commit a53a8a6 into opensearch-project:main Jul 7, 2023

pjfitzgibbons mentioned this pull request Dec 1, 2023

[BUG][2.11] Home fails to load if OS Security plugin installed and OSD Security plugin _not_ installed opensearch-project/OpenSearch-Dashboards#5562

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authorize rest requests #2753

Authorize rest requests #2753

DarshitChanpura commented May 9, 2023 •

edited

Loading

stephen-crawford left a comment

stephen-crawford left a comment

DarshitChanpura commented May 23, 2023 •

edited

Loading

cwperks left a comment

cwperks Jul 7, 2023

DarshitChanpura Jul 7, 2023

RyanL1997 left a comment

DarshitChanpura commented Jul 7, 2023

cwperks Jul 7, 2023

DarshitChanpura Jul 7, 2023

cwperks Jul 7, 2023

stephen-crawford left a comment

cwperks Jul 7, 2023

DarshitChanpura Jul 7, 2023

opensearch-trigger-bot bot commented Jul 7, 2023

Authorize rest requests #2753

Authorize rest requests #2753

Conversation

DarshitChanpura commented May 9, 2023 • edited Loading

Description

Issues Resolved

Testing

Check List

stephen-crawford left a comment

Choose a reason for hiding this comment

stephen-crawford left a comment

Choose a reason for hiding this comment

DarshitChanpura commented May 23, 2023 • edited Loading

cwperks left a comment

Choose a reason for hiding this comment

cwperks Jul 7, 2023

Choose a reason for hiding this comment

DarshitChanpura Jul 7, 2023

Choose a reason for hiding this comment

RyanL1997 left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Jul 7, 2023

cwperks Jul 7, 2023

Choose a reason for hiding this comment

DarshitChanpura Jul 7, 2023

Choose a reason for hiding this comment

cwperks Jul 7, 2023

Choose a reason for hiding this comment

stephen-crawford left a comment

Choose a reason for hiding this comment

cwperks Jul 7, 2023

Choose a reason for hiding this comment

DarshitChanpura Jul 7, 2023

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jul 7, 2023

DarshitChanpura commented May 9, 2023 •

edited

Loading

DarshitChanpura commented May 23, 2023 •

edited

Loading