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.

Sign up for GitHub

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

Jump to bottom

Extend ActionRequest and TransportService for Resource permissions #15230

Closed
wants to merge 2 commits into from
Closed

Extend ActionRequest and TransportService for Resource permissions #15230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27b2f15
Extend ActionRequest and TransportService for Resource permissions
stephen-crawford Aug 13, 2024
7b1a6ae
Extend ActionRequest and TransportService for Resource permissions
stephen-crawford Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- Add took time to request nodes stats ([#15054](https://github.com/opensearch-project/OpenSearch/pull/15054))
- [Workload Management] QueryGroup resource tracking framework changes ([#13897](https://github.com/opensearch-project/OpenSearch/pull/13897))
- Add slice execution listeners to SearchOperationListener interface ([#15153](https://github.com/opensearch-project/OpenSearch/pull/15153))
- Modify ActionRequest and TransportService to allow for defining resources while making a request ([#15230]Extend ActionRequest and TransportService for Resource permissions)

### Dependencies
- Bump `netty` from 4.1.111.Final to 4.1.112.Final ([#15081](https://github.com/opensearch-project/OpenSearch/pull/15081))
Expand Down
10 changes: 10 additions & 0 deletions server/src/main/java/org/opensearch/action/ActionRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@
import org.opensearch.transport.TransportRequest;

import java.io.IOException;
import java.util.Collections;
import java.util.List;

/**
* Base action request implemented by plugins.
Expand All @@ -58,6 +60,14 @@ public ActionRequest(StreamInput in) throws IOException {
super(in);
}

/**
* 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 •
edited
Loading

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 :)

return Collections.emptyList();
}

public abstract ActionRequestValidationException validate();

/**
Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/opensearch/transport/TransportService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1106,7 +1106,8 @@ public TransportAddress[] addressesFromString(String address) throws UnknownHost
"cluster:monitor",
"cluster:internal",
"internal:",
"views:"
"views:",
"resource:"
)
)
);
Expand Down
Loading