-
Couldn't load subscription status.
- Fork 337
[Resource Access Control] [Part2] Introduces a client for Resource Access Control and adds concrete implementation via common package #5186
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
Conversation
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
…ntrol implementation in common package Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
b8c4c74 to
6b83ebf
Compare
|
CI should pass once #5185 is merged. |
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
|
@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 { |
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.
Following up on the comment from the previous PR:
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
Userclass 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: ... My gut would say that this will throw a
security/common/src/main/java/org/opensearch/security/common/resources/ResourceAccessHandler.java
Line 84 in cdd7252
final UserSubjectImpl userSubject = (UserSubjectImpl) threadContext.getPersistent( ClassCastException, as these will be actually two differentUserclasses.
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.
|
@DarshitChanpura what is the purpose of the |
|
|
||
| --- | ||
|
|
||
| ### **4. `listAllAccessibleResources`** |
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.
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.
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
50c4940 to
f18a530
Compare
Signed-off-by: Darshit Chanpura <dchanp@amazon.com>
#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
Check List
- [ ] API changes companion pull request createdBy 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.