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

[Resource Access Control] [Part2] Introduces a client for Resource Access Control and adds concrete implementation via common package #5186

Conversation

DarshitChanpura
Copy link
Member

@DarshitChanpura DarshitChanpura commented Mar 17, 2025

#5185 must be merged before merging this.

#5016 is being broken down into smaller pieces. This is part 2.

Description

Introduces a client to be consumed by plugins, and a common package that contains changes relevant to the implementation of ResourceAccessControl.

There are 4 java APIs as well as 4 REST APIs introduced as part of this PR. Plugins will leverage the client to call the java APIs to implement resource access control.

Issues Resolved

Related to #4500

Testing

  • automated tests

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
    - [ ] API changes companion pull request created
  • 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.

@DarshitChanpura DarshitChanpura changed the title [Resource Access Control] [Part2] Introduces SPI for resource access control [Resource Access Control] [Part2] Introduces a client for Resource Access Control and adds concrete implementation via common package Mar 17, 2025
…ntrol implementation in common package

Signed-off-by: Darshit Chanpura <[email protected]>
@DarshitChanpura
Copy link
Member Author

CI should pass once #5185 is merged.

@DarshitChanpura DarshitChanpura marked this pull request as ready for review March 17, 2025 22:47
DarshitChanpura added a commit to DarshitChanpura/security that referenced this pull request Mar 17, 2025
@DarshitChanpura DarshitChanpura added the resource-permissions Label to track all items related to resource permissions label Mar 18, 2025
@cwperks
Copy link
Member

cwperks commented Mar 18, 2025

@DarshitChanpura can you elaborate on the java + REST APIs this PR introduces in the PR description?

* <p/>
* <b>Do not subclass from this class!</b>
*/
public class User implements Serializable, Writeable, CustomAttributesAware {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Following up on the comment from the previous PR:

#5016 (comment)

To be honest, I am a bit skeptical about moving core security classes like User and AdminDNs to an artifact that makes these classes available for other plugins.

This is for several reasons:

  • As long as the User class is not immutable, this class exposes setters which allows then other plugins to modify the user object.
  • Generally, it reduces barriers between the security plugin and other plugins. If the goal is to make the plugin ecosystem more secure or even zero-trust, this is counter productive.
  • It creates class loader oddities. In OpenSearch, each plugin lives in its own isolated class loader (AFAIK, but this is certainly not the area i am most proficient in). Having a common module now used by several plugins should create several isolated class instances. I am actually wondering why this functionality works in the context of another plugin:
    final UserSubjectImpl userSubject = (UserSubjectImpl) threadContext.getPersistent(
    ... My gut would say that this will throw a ClassCastException, as these will be actually two different User classes.

The ml-commons project uses a similar pattern: #5016 (comment)

I do not know the ml-commons well, but I tried to skim through it to understand the idea and the architecture a bit better.

As far I can see, most classes in the common sub module of ml-commons exist to support plugins to send transport actions to the ml-commons plugin. So, there is only little business logic inside. Additionally, no actual Java objects are exchanged between the plugins. As everything is routed via transport messages, each object gets serialized using the OpenSearch internal serialization protocol on one side and de-serialized again on the other side. Thus, only the actual data is shared, not the objects themselves.

@cwperks
Copy link
Member

cwperks commented Mar 19, 2025

@DarshitChanpura what is the purpose of the common module. The purpose of a SPI is to expose a library to plugins.


---

### **4. `listAllAccessibleResources`**
Copy link
Member

Choose a reason for hiding this comment

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

I know there's been a bit of back-and-forth on design, but I think we should avoid methods like listAll and use standard search APIs. Previously, I know we discussed a DLS-Style "clause injection" on top of any search query that a plugin performs on a resource index and I think something like that would cover any type of search request that a plugin would perform.

DarshitChanpura added a commit to DarshitChanpura/security that referenced this pull request Mar 19, 2025
@DarshitChanpura DarshitChanpura force-pushed the resource-sharing-common-client branch from 50c4940 to f18a530 Compare March 19, 2025 22:53
@DarshitChanpura DarshitChanpura deleted the resource-sharing-common-client branch March 20, 2025 17:59
@DarshitChanpura DarshitChanpura restored the resource-sharing-common-client branch March 20, 2025 18:00
@DarshitChanpura DarshitChanpura deleted the resource-sharing-common-client branch March 20, 2025 18:01
@DarshitChanpura
Copy link
Member Author

DarshitChanpura commented Mar 20, 2025

@cwperks @nibix I have removed the common package.

Apologies, but I had some local github issues leading to me having to delete the branch. I have opened a new PR: #5194

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resource-permissions Label to track all items related to resource permissions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants