Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend ActionRequest and TransportService for Resource permissions #15230

Conversation

stephen-crawford
Copy link
Contributor

Description

[Describe what this change achieves]
This PR makes a minor change to the ActionRequest and TransportService files. In ActionRequest, it adds a new method to the interface

     /**
     * Should be overwritten by implementing plugins with the resources each actionRequest requires.
     * Bypasses resource evaluation by default.
     */
    public List<String> getResources() {
        return Collections.emptyList();
    }

As described, this method is intended to be overwritten by plugins so that their ActionRequest implementations can return a list of the resources they act upon.

The TransportService change simply adds a new accepted prefix for actions of resource:

Related Issues

Resolves #[Issue number to be closed when this PR is merged]
This change is part of:
opensearch-project/security#4500

It is required for further work on:
opensearch-project/security#4562

An example of the work can be found here: opensearch-project/security#4638

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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: Stephen Crawford <steecraw@amazon.com>
Copy link
Contributor

❕ Gradle check result for 27b2f15: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.cluster.MinimumClusterManagerNodesIT.testThreeNodesNoClusterManagerBlock

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link

codecov bot commented Aug 13, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.91%. Comparing base (b6c80b1) to head (27b2f15).
Report is 3 commits behind head on main.

Files Patch % Lines
...main/java/org/opensearch/action/ActionRequest.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #15230      +/-   ##
============================================
+ Coverage     71.90%   71.91%   +0.01%     
- Complexity    63033    63078      +45     
============================================
  Files          5197     5197              
  Lines        295313   295314       +1     
  Branches      42677    42677              
============================================
+ Hits         212354   212386      +32     
+ Misses        65552    65517      -35     
- Partials      17407    17411       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Stephen Crawford <steecraw@amazon.com>
* Should be overwritten by implementing plugins with the resources each actionRequest requires.
* Bypasses resource evaluation by default.
*/
public List<String> getResources() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have already touched upon resources: what are resources? how they are related to action? (and action request)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HI @reta, thanks for following up.

I am not sure what you mean by touched upon resources, but for context this is coming from: opensearch-project/security#4500.

This framework modifies the ActionRequest with the override-able method so that a plugin can define a set of resources its actions interact with. The expectation is that plugins such as the Ml-Commons which uses model-groups would override this method so that the request can pass along that their action interacts on "model-group-1" for example.

From this information, you can then perform roles based access control or an an alternative form of authc/z on the request.

The change to the transportService is required so that we can define TransportRequests following the pattern /plugin/resource/add/ etc.

Copy link
Collaborator

@reta reta Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean by touched upon resources, but for context this is coming from: opensearch-project/security#4500.

I still don't understand this concept:

  • why resources are just arbitrary strings?
  • how the resources are being used?
  • what is the difference between resource and view?

I will try to find out, meanwhile leaving it for review to someone who has the context.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ActionRequest class should not be modified. With ActionRequests that pertain to actions that involve indices, there is another interface called IndicesRequest which they will implement to indicate that the request uses indices. Items within BulkRequests or MultiGet requests can be IndicesRequests.

If Resource Permissions are implemented as an abstraction of Index Permissions, then I think following a similar pattern with a new interface would make sense, but there is some discussion on the proposal about different approaches to providing a mechanism from the security plugin that plugins can utilize to protect "resources" created by the plugin: opensearch-project/security#4500

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cwperks please look at the discussion on the linked RFC @DarshitChanpura created. I understand you and he had some differences of opinion but the finalized design explicitly details modification of the ActionRequest as discussed by him and Nils.

You have been advocating a re-design of the resource permissions to make them a form of index permission however this was never agreed upon on the RFC. To say that this class "should not be modified" based on your preferred alternative is not a fair representation in my opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have been advocating a re-design of the resource permissions to make them a form of index permission however this was never agreed upon on the RFC. To say that this class "should not be modified" based on your preferred alternative is not a fair representation in my opinion.

The proposed design in the RFC is about making a resource_permissions section in a role definition where resource_permissions are an abstraction of index_permissions and the same glob matching done with index names would be applied to actions that apply to resources.

I wanted to leave a comment about how IndicesRequests worked because not all ActionRequests are IndicesRequests. The Security plugin has some pretty convoluted logic to resolve concrete indices from a raw action request. It would be nice if there were a better way to resolve indices from a raw action request.

I left a comment about an idea of "resource sharing" which could provide a more backward compatible approach for ml commons. In Model Access Control for ML Commons, there is a notion of public, private and restricted that specify how a model group can be shared. How would these translate to the new model for resource security?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up @cwperks.

I understand your concerns and am not saying that your preferred alternative may not be an advisable approach.

My point is simply that the existing design makes it explicit how these changes should occur--and they would require modifying the ActionRequest specifically.

I will caution against the creation of any concrete Resource object inside OpenSearch. As you know, resolving indices is challenging to begin with and I don't know how easy it would be to resolve Resources as a concrete object across the cluster.

For your concern on ML commons I would expect Roles based access control to be followed: opensearch-project/security#4638 (comment)

Perhaps you think this would not work though? I cede to your judgement there, but that is my interpretation of the existing design intent.

Just let me know what you think :)

Copy link
Contributor

❕ Gradle check result for 7b1a6ae: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants