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

cwperks · 2023-03-29T18:30:25Z

Description

This PR relies on an earlier PR to setup extension TLS and handshake with security plugin: #2599

This PR has companion PRs in Core and SDK repo:

Core - opensearch-project/OpenSearch#6870
SDK - opensearch-project/opensearch-sdk-java#622

Isolated changes for this PR can be viewed with the compare tool: cwperks/security@extension-tls...extension-role

This PR creates a RestLayerPrivilegesEvaluator and adds a new area of the role definition called extension_permissions that act the same as cluster permissions, but are evaluated on the REST-layer for requests bound for extensions.

See an example role with extension permissions:

# Roles for Hello World extension, roles that extensions would like to add should be ultimately be defined elsewhere
extension_hw_greet:
  reserved: true
  extension_permissions:
    - 'extension:hw/greet'

extension_hw_full:
  reserved: true
  extension_permissions:
    - 'extension:hw/greet'
    - 'extension:hw/greet_with_adjective'
    - 'extension:hw/great_with_name'
    - 'extension:hw/goodbye'

I added these roles here, but roles like this should be requests by the extension to add to a cluster and if granted by the cluster admin, added to the full roles list.

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

New Feature

Issues Resolved

Map extension routes to permission-able action names #2589

Testing

Tested by assigning users the roles created in this PR and verified that they are permitted or blocked based on what has been granted. Automated tests to follow, I wanted to get a draft out to determine if this is the right direction to head in.

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.

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

stephen-crawford

Hi Craig, great work with this. I know you are still in a draft but I wanted to give you feedback early to help. Overall I think this looks good, I just had a few comments but nothing major outside of eventually hoping for tests once you get the PR further along. Looks good!

stephen-crawford · 2023-04-17T14:17:51Z

config/roles.yml

@@ -326,3 +326,17 @@ security_analytics_ack_alerts:
  reserved: true
  cluster_permissions:
    - 'cluster:admin/opensearch/securityanalytics/alerts/*'
+
+# Roles for Hello World extension, roles that extensions would like to add should be ultimately be defined elsewhere
+extension_hw_greet:


Is it possible to parse this from the Hello World config? I know you have made changes in the SDK, I am not sure what would be required to read in these roles as part of the registration/installation process.

Yes, that is ultimately where these should go! I wanted to open up a PR to show REST-layer authz in action with an example role. This issue is about letting an extension pre-define roles and sourcing them into the security configuration.

stephen-crawford · 2023-04-17T14:31:57Z

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

+        return configModel !=null && configModel.getSecurityRoles() != null && dcm != null;
+    }
+
+    public PrivilegesEvaluatorResponse evaluate(final User user, String action0) {


This may be hit further up in the call chain. For instance the plugin itself probably cannot get the request to this point if security is not initialized.

That's exactly how this would work. This evaluator is used in the REST handler wrapper which means this is executed before the original handler is called. If the request is not authorized, the original handler will never be called.

stephen-crawford · 2023-04-17T14:34:13Z

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

Do you think it makes more sense to have this as a separate file or as part of the existing privileges evaluator logic? It may be possible to add this as a subroutine of the existing code if you were so inclined.

I think it makes sense as a separate file since it takes different parameters to instantiate this evaluator. This is a fresh slate to work with and much smaller in size that the transport layer PrivilegesEvaluator.

This PR doesn't cover how index permissions from plugins will be converted to extensions. i.e.

cross_cluster_replication_leader_full_access: reserved: true index_permissions: - index_patterns: - '*' allowed_actions: - "indices:admin/plugins/replication/index/setup/validate" - "indices:data/read/plugins/replication/changes" - "indices:data/read/plugins/replication/file_chunk"

will not be able to be ported with this PR alone. There will need to be work done to authorize requests that take in resource names like index patterns.

This PR will work for all plugin cluster_permissions.

Sounds good to me!

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

stephen-crawford · 2023-05-05T20:54:24Z

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

+                    .findFirst();
+            if (handler.isPresent() && handler.get() instanceof ProtectedRoute) {
+                String action = ((ProtectedRoute)handler.get()).name();
+                PrivilegesEvaluatorResponse pres = evaluator.evaluate(user, action);


I know this is a draft so maybe you have not gotten there yet, but I thought that the evaluator only operates based on role resolution. If this is the case, what is your plan for hosting the REST routes in this mechanism? Are you making each extension add a role with the route as a permission string or are we going to add a new role used to hold these routes or something else?

stephen-crawford · 2023-05-05T20:56:49Z

src/main/resources/static_config/static_roles.yml

@@ -20,6 +20,8 @@ all_access:
        - "*"
      allowed_actions:
        - "kibana_all_write"
+  extension_permissions:


Are we going to be adding a new field to all roles designated as extension permissions?

peternied

This design tightly couples extensions to the security plugin's roles and permissions. As extensions are new, we should use the simplest concepts to provide permissions and access.

The features to govern extension capabilities are being developed within OpenSearch core. Lets wait on that work to come together before spending more cycles here.

cwperks · 2023-05-08T14:05:15Z

@peternied We need guidance on how to proceed then. This introduces an entirely new section in a roles definition to make it clear that these are permissions associated with an extension action, provide flexibility for the future if extension permissions can include resource names like index patterns and will allow for backward compatibility between the plugin version of an extension and an extension.

cwperks · 2023-05-08T14:39:35Z

@peternied This PR creates an analog to roles that plugins add to the security plugin's roles.yml. These roles give access to users to use different REST actions registered by an extension the same way that an admin would for plugins. Can you please list out the design concerns you have specifically so they can be addressed? This PR has been open for over a month for review. Please be specific.

cwperks · 2023-05-08T14:57:10Z

As extensions are new, we should use the simplest concepts to provide permissions and access.

@peternied This doesn't meet the objective of being a replacement for an existing plugin like AD. Without any guidance more specific I am planning to continue working on the design as-is. Please comment on the design with any specific concerns so they can be addressed. This PR has been open for more than a month and there is ample comments on multiple issues about the design.

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

cwperks · 2023-05-08T19:40:58Z

@peternied This now gets rid of the extensions_permission section and instead utilizes existing cluster_permissions. The roles for the HelloWorld Extension are now defined as:

extension_hw_greet:
  reserved: true
  cluster_permissions:
    - 'hw:greet'

extension_hw_full:
  reserved: true
  cluster_permissions:
    - 'hw:greet'
    - 'hw:greet_with_adjective'
    - 'hw:great_with_name'
    - 'hw:goodbye'

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

DarshitChanpura · 2023-05-16T13:32:00Z

@cwperks Would you want to close this in lieu of #2753. I'm fine either way, but wanted to know your thoughts

cwperks · 2023-05-16T13:36:28Z

@DarshitChanpura I will close this one, it looks like your branch is a bit further along. I will also synchronize the PRs in core and the SDK today and try to chase the maintainers for a review of those PRs as they have been open for a while.

cwperks · 2023-05-22T13:38:59Z

Closing in favor of: #2753

cwperks added 12 commits March 27, 2023 18:47

WIP on rest layer authz

e2b750d

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

WIP on rest-layer authz

478523a

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

Extension handshake

3444295

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

Extension TLS

eb03f38

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

Remove SecurityRestFilterChanges to isolate extension TLS change

85dba00

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

Remove SecurityRestFilterChanges to isolate extension TLS change

3419692

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

Remove SecurityRestFilterChanges to isolate extension TLS change

9ef4e0f

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

Remove SecurityRestFilterChanges to isolate extension TLS change

b7686dd

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

Remove SecurityRestFilterChanges to isolate extension TLS change

298aa27

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

Merge branch 'extension-tls' into extension-role

5b6ca7b

WIP for HelloWorld sample extension role

e3c1b47

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

Initial implementation of authz check in REST layer

f3dadf8

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

cwperks mentioned this pull request Mar 29, 2023

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

Closed

cwperks changed the title ~~Extension role~~ Authorize Requests bound for extensions in the REST-Layer, add extension_permissions section to roles Mar 29, 2023

Remove header

a1826d5

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

This was referenced Mar 31, 2023

[META] Authorization in the REST layer #2590

Closed

Authorizing User against requested extension endpoint #2622

Closed

cwperks mentioned this pull request Mar 31, 2023

Map extension routes to permission-able action names #2589

Closed

cwperks added 6 commits April 1, 2023 21:25

Create authorizeRequest method

49fff14

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

Remove local check

5a64ef7

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

Use extensions feature flag

62a076c

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

Merge branch 'extension-tls' into extension-role

66f4d5c

Merge branch 'main' into extension-tls

87c421b

Merge branch 'extension-tls' into extension-role

e83aace

stephen-crawford reviewed Apr 17, 2023

View reviewed changes

cwperks added 5 commits April 21, 2023 21:24

Merge branch 'main' into extension-tls

40b9c98

Switch to values.iterator

b1f7369

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

Merge branch 'extension-tls' into extension-role

e30e8be

small fix

1ef9083

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

Merge branch 'main' into extension-tls

a740ea4

cwperks added 4 commits April 27, 2023 13:50

Remove ImmutableOpenMap

6cdda66

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

Merge branch 'extension-tls' into extension-role

f2430ff

Change to ProtectedRoute

364793c

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

Merge branch 'main' into extension-role

0dae911

stephen-crawford reviewed May 5, 2023

View reviewed changes

peternied requested changes May 5, 2023

View reviewed changes

Remove extension permissions

0e9dc24

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

Use lookupExtensionSettingsById

30d7899

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

DarshitChanpura mentioned this pull request May 9, 2023

Authorize rest requests #2753

Merged

3 tasks

cwperks changed the title ~~Authorize Requests bound for extensions in the REST-Layer, add extension_permissions section to roles~~ Authorize Requests bound for extensions in the REST-Layer May 15, 2023

cwperks closed this May 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cwperks commented Mar 29, 2023

stephen-crawford left a comment

stephen-crawford Apr 17, 2023

cwperks Apr 17, 2023

stephen-crawford Apr 17, 2023

cwperks Apr 17, 2023

stephen-crawford Apr 17, 2023

cwperks Apr 17, 2023

stephen-crawford Apr 19, 2023

stephen-crawford May 5, 2023

stephen-crawford May 5, 2023

peternied left a comment

cwperks commented May 8, 2023

cwperks commented May 8, 2023

cwperks commented May 8, 2023

cwperks commented May 8, 2023

DarshitChanpura commented May 16, 2023

cwperks commented May 16, 2023

cwperks commented May 22, 2023

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

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

Conversation

cwperks commented Mar 29, 2023

Description

Issues Resolved

Testing

Check List

stephen-crawford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

cwperks commented May 8, 2023

cwperks commented May 8, 2023

cwperks commented May 8, 2023

cwperks commented May 8, 2023

DarshitChanpura commented May 16, 2023

cwperks commented May 16, 2023

cwperks commented May 22, 2023