-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[Feature]Introduces Resource Sharing and Access Control #16030
[Feature]Introduces Resource Sharing and Access Control #16030
Conversation
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
…urceService Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
60c9d1a
to
532d13a
Compare
* @param scope the scope being requested | ||
* @return true if current user has access, false otherwise | ||
*/ | ||
default boolean hasPermission(String resourceId, String resourceIndex, String scope) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is a scope exactly? Is it the same as security's notion of an action group or something different? How is it defined and why is it a String?
If resourceIndex
is passed in here then is it assumed that resources are stored in an index? Should this be abstracted to not necessarily assume that resources are stored in an index and leave the implementation up to the plugin that provides access control for resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this always be able to computed synchronously? If it can't be synchronous then does this need to accept an ActionListener?
* @param resourceIndex index where this resource is stored | ||
* @return true if resource record was deleted, false otherwise | ||
*/ | ||
default boolean deleteResourceSharingRecord(String resourceId, String resourceIndex) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of this method here? If a user wants to make a resource private then the resource sharing record would still exist, but any shared with entry would be emptied and it entry would keep track of the resource user (owner of the resource)
IMO this should be left up to the IndexOperationListener on postDelete. If an entry in the resourceIndex is deleted then we can also delete its corresponding entry in the .resource-sharing
index.
return true; | ||
} | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wdyt about implementing this and the method below as a REST API in the plugin that implements resource access control?
i.e. Security plugin could have an API
PUT /resource/{resource_type}/{id}/share_with
{
"resource_id": "...",
"resource_type": "...", // resource_type is 1 <-> 1 with resource index, but is a human-readable type
"share_with": {
"users": ["userB"]
}
}
Does this need to be part of the interface?
* | ||
* @opensearch.experimental | ||
*/ | ||
public interface ResourceAccessControlPlugin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When thinking about the role of this plugin, I was thinking that its job is to initialize separate Resource(Sharing)Service per resource type and the role of the service would be the method below this one. i.e. resourceService.hasBeenSharedWith(String resourceId)
- the resourceService is specific to the type of resource and would already have info necessary required to do the lookup.
These resourceServices would be assigned back to the ResourcePlugins which can be used to determine if a resource has been shared with the currently authenticated user.
When it comes to re-usable generic actions to get a resource or search for resources, we could consider adding a library that plugins can add a dependency on that has the re-usable generic actions. The ResourcePlugins could need to supply a ResourceParser so that the generic actions would know how to parse entries from the resource index. The jobParser from the job-scheduler has a model for generic parsing: https://github.com/opensearch-project/job-scheduler/blob/main/spi/src/main/java/org/opensearch/jobscheduler/spi/JobSchedulerExtension.java#L30-L33
I have been thinking about the different approaches (SPI vs new extension points) over the last couple of days and I think this approach makes sense, but I would suggest some changes to the interfaces. I think the extension point approach is sensible because it allows other plugins (not only the default security plugin) to define the ResourceAccessControl interface whereas a SPI would make it so that only the security plugin could provide an implementation. These are some ideas I had about the interface and wanted to get your thoughts. See example here.
and the associated interfaces a.
b.
c.
|
Thanks @cwperks , the APIs refinement is on the right track I believe, but I have larger concerns, @DarshitChanpura this is not the first attempt to bring the notion of "resources" to core. There is probably a need to introduce some abstractions around that so the plugin authors should not reinvent the wheel, I got that, but, at least per this change:
In my opinion, as it stands now, I don't see why it has to be part of the core. It seems to me it could be a part of standalone library instead (so plugin authors could integrated it and reuse the building blocks but fully manage the access aspects, the core could provide the identity (requestor) piece. If we want to make it a part of the core, we need to design the unified "resources" access semantics, not only some abstract notions and largely disconnected API. |
@reta Any comment on the SPI approach where the security plugin provides a SPI that other plugins can extend? The main problem with a SPI approach is that security is an optional plugin and currently i.e. in that case other plugins can extend the SPI provided by security plugin, but additionally handle the case where the security plugin is not installed (i.e. no fine-grained access control for plugin resources) |
Take a look at the description on cwperks/security#26 Basically, there is the widespread use amongst plugins for a setting called This PR is working towards fine-grained access control for plugin resource:
Imagine an example of a Let's say this plugin defines 3 default roles: In the current security model, when the creator of an album shares it with other users they cannot specify that the target user only has read access or comment access. They can share it with a target user, but the target user's level of access is determined by their roles mapping. So there's a couple of things we are trying to solve:
This is an area I would like to improve across the plugins. To implement I think we have an opportunity to make the resource security model more sensible and improve the plugin developer experience by introducing formal APIs for them to use. I would like to publish developer documentation for plugin developers and the current security model for resources makes my head hurt. i.e.
Also have to elaborate on how the resource owner has no authority over the level of access when sharing...there ought to be a way the resource owner has control over this. |
Got it, does the "resource" in this context is equivalent to document with restricted access [1]? In any case, I believe building up on security plugin foundation is the right direction, the core is not (yet) aware of permissions, roles, etc .... [1] https://opensearch.org/docs/latest/security/access-control/document-level-security/ |
Kind of, but in practice plugin developers don't use DLS to get the resources that have been shared with the authenticated user. f.e. AnomalyDetection will create a clause in a search query with the backend roles that they have persisted. Called from here. I think the interfaces outlined in #16030 (comment) will provide a generic interface for a SharableResource and what's required for both a plugin that creates Sharable Resource (ResourcePlugin) and the plugin that provides authz for these resources. |
Thanks, why DLS could not be used? There are documents, user and roles, the filtering should happen the same way the user / plugin is querying the data. Isn't this a problem that we have to invent another mechanism to achieve what supposed to be working out of the box? |
Why should the plugins store a copy of the user/roles/backend_roles outside the security index? When a plugin is writing a search query on their system index where resources are stored, how will they know what clause to use? DLS solves a different problem and is applicable to regular users searching in a cluster, not for this resource use case. |
@reta @cwperks thank you for your detailed reviews. Concept of resource is tied to a document in an index for this project. The use-case that we are trying to solve is to allow individual users complete control over their own resources. With this iteration they must still have access to the index where they sit, but in next iteration that will be control by scopes. I've introduced a centralized java APIs for plugins to leverage the sharing information for an individual resource. But after talking with @cwperks, we think that these APIs might need to be exposed as a mix of JAVA and REST APIs. So here are the possible paths forward:
I would agree with Path 2, mixed model approach, however this would require detailed plugin-developer/user-friendly documentation. NOTE:
I would like to finalize an approach for this as I'm targeting 2.19 release with |
It shouldn't, I believe the same filters should be working for plugin specific indices, this is essentially just term predicate on
I didn't get this part, the Anomaly Detection Plugin link that you've shared (https://github.com/opensearch-project/anomaly-detection/blob/main/src/main/java/org/opensearch/timeseries/util/ParseUtils.java#L446-L468) uses user to search the index, how is that different from users searching in a cluster? (sorry if I am missing something) |
I am very confused now, if resource == document in a index, and user is, well, just a user, why DSL does not work? We even do have this scenario documented [1], granting user access its documents. |
Different indices. In the current impl for i.e. Here's an example anomaly detector entry ( The filter in the link above works because the copy of the user is stored with the detector's metadata. What @DarshitChanpura is proposing here is to yank the user info out of the detectors index and instead to centrally store this data in an index managed by the security plugin. f.e.
The |
+1 to @cwperks's comment. At the very core, the problem we are trying to solve is to have a centralized way of authorizing access to resources (i.e. detectors, monitors, model-groups, etc). These are stored as documents in an index which is what I referred to earlier in my comment above. The plugins that define these resources have already defined their own authorization schemes, which is DLS allows you to filter out resources that you don't want a particular user or role to see. However, DLS doesn't grant control over individual resource and its sharing. This approach is not meant to replace DLS in any form. Rather it works as a separate entity which will control access to each individual resource and grant the resource-owners control over that document. |
I think this is a message I am trying to convey here: agree that duplication and reinventing the wheel is not sustainable way forward. However, we do have pretty powerful mechanisms to control access to the individual documents (sharing is, by and large, granting the access). However, I do not understand:
Who will consult this index and when exactly? If we need the APIs to be called by plugin developers, it inherently prone to errors and would not be reliable :( And won't DLS conflict with In any case, I don't see how it belongs to core, it relies on |
@reta With SPI approach it would be necessary to install the SPI library even if security is disabled (in order to compile). I'm not against the idea, but as Craig mentioned this will require some changes in core. Wouldn't we need to add a setup for handling Resource Authorization once we plan to add a new class of request called ResourceRequest? Or will this also be in SPI? I'm not sure. is SPI model purely to avoid getting any resource related code in core? I'm just trying to understand when to use SPI vs when to not use. |
@DarshitChanpura the feature will not work without
I don't know what is |
Resources are a subset of documents in an index. Not all documents are owned by a user, however some documents are still share-able with users/roles, etc. These subsets are what we refer to as resources here. Although, resources could be anything and will be expanded to be anything in future iterations.
Can you help me understand why you feel this mechanism might be unreliable?
DLS doesn't grant a user ability to share individual resources. This is also another major use-case we are trying to solve, by allowing
Agreed. Right now the default behavior is a pass-through implementation. I introduced the idea in core as a means to list out API contracts which other plugins can leverage without the awareness of security plugin or any other resource access control plugin. This will also allow custom plugins their own way of storing and managing resource access. However, I'm still unclear of when to use SPI model vs when to not use one.
ResourceRequest will be a subclass of ActionRequest which will then be used PrivilegesEvaluator to perform resource access evaluation. This is not present on the original proposal as this has not be ideated completely. The initial version of this approach is to provide a centralized way for plugins to authorize resource access. |
If some specific APIs have to be called manually (by plugin developers fe), it is human factor. Let me give you an example, the way we check REST actions now: every single call goes over the request wrapper (and ends in
The limitations are understood, the question is - why DLS could not be enhanced to support the use case(s) in question? |
How do we avoid human factor completely? From what I understand that wrapper has been implemented specifically for security plugin from what i understand (or a plugin that offers security). Are you saying that we should not add any more of these wrappers? Or that this form of authorization might be unreliable.
Introducing new abstractions to manage resource ownership, permissions, and sharing metadata is beyond DLS’s original purpose. DLS filters data by applying static rules at query time. Adding resource sharing capability could potentially increase query complexity and response times. Adding resource sharing capability also makes DLS's original meaning nebulous. Whereas a new and clean resource sharing framework maintains separation of concerns. DLS is also limited to a document in an index, but resource sharing can be expanded to records from other database systems (fe, an entry in dynamodb).
This was designed with two things in mind. Allowing abstraction while following concepts similar to already established ones around security (fe, wrapper) and another keeping it open for future enhancements including the capability to allow admins to choose a resource management plugin if there is a custom plugin out there that implements its own resource authz. This will also contribute towards the larger goal of moving security in core. Since the primary purpose of code in this PR is to introduce an API to check permissions, I can bring rest of the changes, like revokeAccess(), shareWith(), etc and related code over to security plugin and expose these methods as centralized APIs. I'm still unclear and would like to understand when to use SPI vs when to make changes in core. |
Do not require any manual API calls, if possible.
I think this is somewhat clear in respect to this change: the components in this pull request are disconnected from anything that OpenSearch core does (the core does not know anything about "resources" and doesn't use them anywhere). For example, where core make use of |
Closing as the approach has been moved to a SPI model. opensearch-project/security#5016 |
companion PR: opensearch-project/security#4746
Description
This PR introduces a new capability (with
@opensearch-experimental
flag) to enable access-control and sharing of resources. This PR introduces:At present, plugins have implemented in-house authorization mechanisms to control access to their resources. This framework enables capability to have a centralized resource-authorization framework.
This feature adds authorization flow on-top-of existing authorization scheme. Meaning, a user X declaring a resource RX shares with user Y. user Y must have access to the index defining the resource.
Please review feature proposal here that discusses the problem-statement and design approach. opensearch-project/security#4500
Plugins will leverage the APIs introduced in this to check user access to resources.
Documentation update on the project website will be added once the feature is in.
A future update to this design will introduce a new request type called ResourceRequest which will then be utilized in security plugin to invoke a new authorization flow that will run in-parallel with Transport authorization.
Related Issues
Check List
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.