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

[Feature]Introduces Resource Sharing and Access Control #16030

Conversation

DarshitChanpura
Copy link
Member

@DarshitChanpura DarshitChanpura commented Sep 22, 2024

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:

  1. Interfaces to be extended by security plugin for concrete implementation, and to be used by plugins when authorizing the requested resources.
  2. Adds a Default implementation when security plugin is disabled.

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

  • 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: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: Darshit Chanpura <[email protected]>
* @param scope the scope being requested
* @return true if current user has access, false otherwise
*/
default boolean hasPermission(String resourceId, String resourceIndex, String scope) {
Copy link
Member

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?

Copy link
Member

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) {
Copy link
Member

@cwperks cwperks Dec 31, 2024

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;
}

/**
Copy link
Member

@cwperks cwperks Dec 31, 2024

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?

Copy link
Contributor

✅ Gradle check result for 532d13a: SUCCESS

*
* @opensearch.experimental
*/
public interface ResourceAccessControlPlugin {
Copy link
Member

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

@cwperks
Copy link
Member

cwperks commented Jan 2, 2025

@reta @jainankitk @nibix Would you mind giving this a review? I'm targeting 2.19 to get this change shipped.

@DarshitChanpura I am off for the next few days, had time to glance through the change very quickly. The one thing that stood to me right away is disconnect from the identity management (IdentityPlugin & Subject), solely focusing on presence of the user, it does not seem right.

@DarshitChanpura I agree with @reta here. The notion of user, roles or backend roles should not be introduced into the core. I know we had some back-and-forth on the RFC about the extensibility model and I really do think the SPI model is appropriate here, with the caveat that there would either need to be the introduction in the core of optionalExtendedPlugins or make extendedPlugins themselves optional.

I created a spike on the idea here: cwperks/security#26

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.

  1. ResourceAccessControlPlugin
public interface ResourceAccessControlPlugin {
    void assignResourceSharingService(SharableResourceType resourceType);
}
  1. ResourcePlugin
public interface ResourcePlugin {
    List<SharableResourceType> getResourceTypes();
}

and the associated interfaces SharableResourceType, ResourceSharingService and SharableResource

a. SharableResourceType

public interface SharableResourceType {
    /**
     * Type of the resource. This must be unique across all registered resource types.
     * @return a string containing the type of the resource
     */
    String getResourceType();

    /**
     * The index where resource metadata is stored
     * @return the name of the index where resource metadata is stored
     */
    String getResourceIndex();

    /**
     * This method is called when initializing ResourcePlugins to assign an instance
     * of {@link ResourceSharingService} to the resource type.
     */
    void assignResourceSharingService(ResourceSharingService resourceSharingService);

    /**
     * @return returns a parser for this resource
     */
    default ResourceParser<? extends SharableResource> getResourceParser() {
        return null;
    };
}

b. ResourceSharingService

public interface ResourceSharingService {

    /**
     * Returns the resource type this service is responsible for.
     *
     * @return The resource type. Will match {@link SharableResourceType#getResourceType()} for the respective
     * sharable resource type
     */
    String getResourceType();

    /**
     * Checks if the resource is shared with the current requester.
     *
     * @param resourceId The resource id
     * @param shareListener The listener to be called when the check is complete
     */
    void isSharedWithCurrentRequester(String resourceId, ActionListener<Boolean> shareListener);
}

c. SharableResource

/**
 * Interface for a generic Sharable Resource. Resources are entities created by plugins that are typically
 * stored in system indices. Access Control is provided by the ResourceAccessControlPlugin.
 */
public interface SharableResource extends NamedWriteable, ToXContentObject {

    /**
     * @return resource name.
     */
    String getName();

    /**
     * @return resource last update time.
     */
    Instant getLastUpdateTime();
}

@reta
Copy link
Collaborator

reta commented Jan 2, 2025

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

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:

  • the core has no understanding what the "resource" is
  • as such, the core has no any centralized / coordinated logic to manage access to "resources"
  • it merely serves as a proxy / mediator, without having any control over the "resources"

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.

@cwperks
Copy link
Member

cwperks commented Jan 2, 2025

@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 extendedPlugins are mandatory. Is there any concern in making extendedPlugins optional or introducing a notion of optionalExtendedPlugins? f.e. #16909

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)

@reta
Copy link
Collaborator

reta commented Jan 2, 2025

@reta Any comment on the SPI approach where the security plugin provides a SPI that other plugins can extend?

@cwperks it is still not clear to me what the role of the security plugin is going to be, as far as "resources" are accessed in a freestyle fashion, why do we need these SPIs?

@cwperks
Copy link
Member

cwperks commented Jan 2, 2025

@reta Any comment on the SPI approach where the security plugin provides a SPI that other plugins can extend?

@cwperks it is still not clear to me what the role of the security plugin is going to be, as far as "resources" are accessed in a freestyle fashion, why do we need these SPIs?

Take a look at the description on cwperks/security#26

Basically, there is the widespread use amongst plugins for a setting called filter_by_backend_role. What this value does, is a crude implementation of sharing where the creator of a resource can share it with other users in the cluster. i.e. The creator of an anomaly detector can specify backend roles with whom to share the detector with. What the end user can do with that detector is based off the roles they are mapped to. i.e. If the end-user is mapped to anomaly_detection_read_only then they cannot modify the detector, but if they are mapped to anomaly_detection_full_access then they have full access on the detector. The user who creates the detector has no mechanism to specify the level of access when sharing.

This PR is working towards fine-grained access control for plugin resource:

  1. Centralize the logic for filter_by_backend_role so the the security plugin is the central place where access checks are performed
  2. Ultimately, provide fine-grained access for plugin resources where the sharer can specify the level of access when sharing

Imagine an example of a Searchable Photo Album Plugin (relevant to me as I got married recently and we had to allow guests access to upload photos, but want to prohibit removing)

Let's say this plugin defines 3 default roles: photo_album_full_access, photo_album_comment, photo_album_read_only

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:

  1. Centralize access checks to the security plugin
  2. Provide fine-grained access to plugin resources that allow for a sharing model where the resource owner can specify level of access to the target user

This is an area I would like to improve across the plugins. To implement filter_by_backend_role, plugin developers will use common-utils to parse the user from the ThreadContext and persist the user outside the security index. The user they parse from the ThreadContext is a snapshot of the user when the resource is created and any changes to roles mappings would not be propagated because security is unaware of these copies stored around the cluster.

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.

  1. Add dependency to common-utils
  2. Parse the user and persist w/ resource
  3. Allow the resource owner to add backend roles (This requires tight coupling w/ the security plugin where other plugins are aware of what backend roles are)

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.

@reta
Copy link
Collaborator

reta commented Jan 2, 2025

Basically, there is the widespread use amongst plugins for a setting called filter_by_backend_role. What this value does, is a crude implementation of sharing where the creator of a resource can share it with other users in the cluster. i.e. The creator of an anomaly detector can specify backend roles with whom to share the detector with. What the end user can do with that detector is based off the roles they are mapped to. i.e. If the end-user is mapped to anomaly_detection_read_only then they cannot modify the detector, but if they are mapped to anomaly_detection_full_access then they have full access on the detector. The user who creates the detector has no mechanism to specify the level of access when sharing.

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/

@cwperks
Copy link
Member

cwperks commented Jan 2, 2025

Basically, there is the widespread use amongst plugins for a setting called filter_by_backend_role. What this value does, is a crude implementation of sharing where the creator of a resource can share it with other users in the cluster. i.e. The creator of an anomaly detector can specify backend roles with whom to share the detector with. What the end user can do with that detector is based off the roles they are mapped to. i.e. If the end-user is mapped to anomaly_detection_read_only then they cannot modify the detector, but if they are mapped to anomaly_detection_full_access then they have full access on the detector. The user who creates the detector has no mechanism to specify the level of access when sharing.

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.

@reta
Copy link
Collaborator

reta commented Jan 2, 2025

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.

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?

@cwperks
Copy link
Member

cwperks commented Jan 2, 2025

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.

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.

@DarshitChanpura
Copy link
Member Author

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

  1. We keep existing changes as is, meaning all APIs are java.

    1. Pros:
      1. Plugins are agnostic to the underlying resource-access-control plugin. They can have their own REST APIs exposed to collect sharing information which would then call underlying java APIs.
      2. Since plugins have complete control over exposing the sharing APIs, it will be easy to integrated with their dashboard components
    2. Cons:
      1. This design is tightly coupled with security plugin. If any other plugin, decides to be a ResourceAccessControl plugin, they will have to conform to the ShareWith data-structure designed here. Note: There will only be one RAC plugin at a time, meaning the custom RAC plugin can only come into effect if security plugin is disabled and vice-versa. (See here)
      2. Each plugin must expose their own REST API to allow sharing and revoking access
  2. Mixed model: Where hasPermission() is exposed as JAVA API, while rest of the methods will be converted to REST APIs. (i.e. Move all shareWith and related class to security plugin). This approach requires exposing custom attributes to individual ResourcePlugin (via /dashboardsinfo ??) who can then update their front-end component for resource sharing.

    1. Pros:
      1. This gives custom RAC plugins flexibility to design their own data-structures, while maintaining the central goal, which is to allow plugin developer to call hasPermission to perform access checks.
      2. Exposing a central REST APIs would allow users to have a uniform experience sharing and revoking access to their own resources.
    2. Cons:
      1. Each ResourcePlugin must register their resource-types. Each resource-type must be linked to a unique index where that resource type is stored. (e.g. Pair<DETECTOR, ".opendistro_anomaly_detectors">)
      2. Each ResourcePlugin must ensure that their dashboard components are calling the correct resource-access-control plugin APIs when a user clicks on "share" button.
      3. If a ResourcePlugin would like their current behavior to share a resource with all resource-owner's backend_roles, they must explicitly call share_with REST API.

I would agree with Path 2, mixed model approach, however this would require detailed plugin-developer/user-friendly documentation.

NOTE:

  • Scopes are nothing but action-groups. They will play a major role in the next iteration of this project where we move resource-level authorization as a standalone item by introducing a ResourceRequest as a subclass of ActionRequest.

I would like to finalize an approach for this as I'm targeting 2.19 release with @opensearch.experimental flag.
Thoughts?

@reta
Copy link
Collaborator

reta commented Jan 2, 2025

Why should the plugins store a copy of the user/roles/backend_roles outside the security index?

It shouldn't, I believe the same filters should be working for plugin specific indices, this is essentially just term predicate on "user.backend_roles.keyword, right?

DLS solves a different problem and is applicable to regular users searching in a cluster, not for this resource use case.

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)

@reta
Copy link
Collaborator

reta commented Jan 2, 2025

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 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.

[1] https://opensearch.org/docs/latest/security/access-control/document-level-security/#parameter-substitution

@cwperks
Copy link
Member

cwperks commented Jan 2, 2025

Why should the plugins store a copy of the user/roles/backend_roles outside the security index?

It shouldn't, I believe the same filters should be working for plugin specific indices, this is essentially just term predicate on "user.backend_roles.keyword, right?

DLS solves a different problem and is applicable to regular users searching in a cluster, not for this resource use case.

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)

Different indices.

In the current impl for filter_by_backend_role plugins will store the user info along with resource metadata.

i.e. Here's an example anomaly detector entry (.opendistro-anomaly-detectors index): https://opensearch.org/docs/latest/observing-your-data/ad/api/#example-response-4

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.

Index Name: .resource_sharing

Example Document
{
    "resource_index": ".opendistro-anomaly-detectors",
    "resource_id": "<detector_id>",
    "resource_user": {
        "name": "admin",
        "backend_roles": [
          "admin"
        ],
        "roles": [
          "own_index",
          "all_access"
        ]
    },
    "share_with": {
        "unlimited": {
            "users": ["craig"],
            "roles": ["hr"]
        },
        "anomaly_detection_read_only": {
            "users": ["bob"],
            "roles": ["it"]
        }
    }
}

The .resource_sharing index would need to be consulted to see if a resource has been shared with the current requester.

@DarshitChanpura
Copy link
Member Author

+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 filter_by_backend_role for a lack of centralized one. The biggest drawback of this approach is that these plugins store a snapshot of the user object's state at the time of creation of the resource, into that resource document. This is a bad practice as: 1. Security plugin is not aware of these copies and can't update them. 2. These copies maybe stale once any information changes for that user in security plugin.

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.

@reta
Copy link
Collaborator

reta commented Jan 2, 2025

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:

  • why we need to introduce "resources" if those are just documents?
  • why we need to introduce yet another sharing mechanism that we cannot reliably enforce?
  • why DLS could not be enhanced to support the use case(s) in question?

The .resource_sharing index would need to be consulted to see if a resource has been shared with the current requester.

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 .resource_sharing anyway?

In any case, I don't see how it belongs to core, it relies on security plugin entirely and pursuing the SPI route suggested by @cwperks does make more sense.

@DarshitChanpura
Copy link
Member Author

@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.

@reta
Copy link
Collaborator

reta commented Jan 2, 2025

@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.

@DarshitChanpura the feature will not work without security plugin in any case, right? I think we should not be using core as means to just simplify code sharing.

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.

I don't know what is ResourceRequest what are future plans for Resource Authorization, the design document [1] does not mention it. In any case, when the role of the core becomes clear, we could always reconsider the approach.

[1] opensearch-project/security#4500

@DarshitChanpura
Copy link
Member Author

DarshitChanpura commented Jan 2, 2025

why we need to introduce "resources" if those are just documents?

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.

why we need to introduce yet another sharing mechanism that we cannot reliably enforce?

Can you help me understand why you feel this mechanism might be unreliable?

why DLS could not be enhanced to support the use case(s) in question?

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

@DarshitChanpura the feature will not work without security plugin in any case, right? I think we should not be using core as means to just simplify code sharing.

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.

I don't know what is ResourceRequest what are future plans for Resource Authorization, the design document [1] does not mention it. In any case, when the role of the core becomes clear, we could always reconsider the approach.

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.

@reta
Copy link
Collaborator

reta commented Jan 3, 2025

Can you help me understand why you feel this mechanism might be unreliable?

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 security plugin), there is nothing required from users and/or developers to make sure it happens, as well as nothing could be done for that not to happen.

DLS doesn't grant a user ability to share individual resources.

The limitations are understood, the question is - why DLS could not be enhanced to support the use case(s) in question?

@DarshitChanpura
Copy link
Member Author

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 security plugin), there is nothing required from users and/or developers to make sure it happens, as well as nothing could be done for that not to happen.

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.

The limitations are understood, the question is - why DLS could not be enhanced to support the use case(s) in question?

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

In any case, when the role of the core becomes clear, we could always reconsider the approach.

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.

@reta
Copy link
Collaborator

reta commented Jan 3, 2025

How do we avoid human factor completely?

Do not require any manual API calls, if possible.

I'm still unclear and would like to understand when to use SPI vs when to make changes in core.

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 ResourceService? If you have move it off to a standalone library (or SPI fe), it could be used the same way, it does not need to be in core.

@DarshitChanpura
Copy link
Member Author

Closing as the approach has been moved to a SPI model. opensearch-project/security#5016

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

Successfully merging this pull request may close these issues.

6 participants