[Resource Access Control] [Part2] Introduces a client for Resource Access Control and adds concrete implementation for resource access control

[DarshitChanpura]

#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 adds concrete 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.

Refer to the RESOURCE_ACCESS_CONTROL_FOR_PLUGINS.md file to understand in-depth implementation of this feature.

Issues Resolved

• Resolves [Proposal] Resource Permissions and Sharing #4500

Testing

• automated tests + manual 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.

[nibix]

Thanks for this interesting PR @DarshitChanpura !

I have added a couple of comments and questions. As I went through the code linearly, I also commented both on conceptual things and code level things (which I mostly marked with Nit:). I would recommend that you first have a look on conceptual things ... we can talk about the Nit stuff later.

If you want, we can also schedule a conversation at some point in time.

[codecov]

Codecov Report

Attention: Patch coverage is 11.24314% with 971 lines in your changes missing coverage. Please review.

Project coverage is 68.83%. Comparing base (4e30396) to head (877eacb).
Report is 101 commits behind head on feature/resource-permissions.

Files with missing lines	Patch %	Lines
...ecurity/resources/ResourceSharingIndexHandler.java	5.68%	330 Missing and 2 partials ⚠️
...arch/security/resources/ResourceAccessHandler.java	3.44%	196 Missing ⚠️
...urces/rest/revoke/RevokeResourceAccessRequest.java	0.00%	60 Missing ⚠️
...ity/resources/rest/share/ShareResourceRequest.java	0.00%	55 Missing ⚠️
...rces/rest/list/ListAccessibleResourcesRequest.java	0.00%	36 Missing ⚠️
...urces/rest/verify/VerifyResourceAccessRequest.java	0.00%	36 Missing ⚠️
...s/rest/list/ListAccessibleResourcesRestAction.java	12.90%	27 Missing ⚠️
...arch/security/resources/ResourceIndexListener.java	16.12%	26 Missing ⚠️
...es/rest/revoke/RevokeResourceAccessRestAction.java	14.28%	24 Missing ⚠️
.../resources/rest/share/ShareResourceRestAction.java	14.28%	24 Missing ⚠️
... and 15 more

Additional details and impacted files

@@                       Coverage Diff                        @@
##           feature/resource-permissions    #5194      +/-   ##
================================================================
- Coverage                         71.43%   68.83%   -2.60%     
================================================================
  Files                               334      362      +28     
  Lines                             22556    23832    +1276     
  Branches                           3589     3660      +71     
================================================================
+ Hits                              16113    16405     +292     
- Misses                             4649     5630     +981     
- Partials                           1794     1797       +3     

Files with missing lines	Coverage Δ
.../org/opensearch/security/auth/BackendRegistry.java	77.77% <ø> (-0.28%)	⬇️
...g/opensearch/security/dlic/rest/support/Utils.java	58.66% <ø> (-0.80%)	⬇️
...urces/rest/list/ListAccessibleResourcesAction.java	100.00% <100.00%> (ø)
...ources/rest/revoke/RevokeResourceAccessAction.java	100.00% <100.00%> (ø)
...rity/resources/rest/share/ShareResourceAction.java	100.00% <100.00%> (ø)
...ources/rest/verify/VerifyResourceAccessAction.java	100.00% <100.00%> (ø)
...g/opensearch/security/support/ConfigConstants.java	95.23% <ø> (ø)
...g/opensearch/security/support/WildcardMatcher.java	70.55% <100.00%> (ø)
.../org/opensearch/security/auth/UserSubjectImpl.java	81.81% <50.00%> (ø)
...pensearch/security/resources/ResourceProvider.java	0.00% <0.00%> (ø)
... and 23 more

... and 20 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nibix]

I am wondering if we really need this version check here, for the following reason:

• Plugins always have a dependency on OpenSearch with a specific version
• Plugins must be built newly for each new OpenSearch version. It is not possible to use a plugin which was built for OpenSearch 2.18.0 with OpenSearch 2.19.0.
• Thus, if the Plugin depends on this client of version X and also on other OpenSearch jars of the same version, we already have the information whether the feature is available: If there's a client jar available with this particular version, then the feature will be there.

[DarshitChanpura]

I agree. But this is for safety in case of mixed cluster (i.e. 3.0 handling 2.19 requests). Let me know if you think that this will not be necessary.

[nibix]

I am still not 100% happy with accessing security plugin settings outside of the security plugin.

Maybe it's just my OCD, but it looks like internals where exposure should be usually avoided.

Let me spin one idea, also relating to this comment: opensearch-project/anomaly-detection#1400 (comment) ... disclaimer: I am not very familiar with the use of extensions in OpenSearch, so maybe it is infeasible:

Update: The following idea is not possible. Just ignore it. I am leaving it for documentary purposes.

Would it be possible to add to the ResourceSharingExtension interface a method like this:

void onResourceSharingSecurityInitialized(ResourceSharingClient resourceSharingClient);

A plugin would implement it like this:

class MyPlugin implements ResourceSharingExtension
   ResourceSharingClient resourceSharingClient;
 
   @Override
   public void onResourceSharingSecurityInitialized(ResourceSharingClient resourceSharingClient) {
      this.resourceSharingClient = resourceSharingClient;
   }

   // Pass on resourceSharingClient to other methods. These perform access control if it is != null. If it is null, resourceSharing is disabled
}

~~Likely, one also would need to make the ResourceSharingClient resourceSharingClient via injection, though ... ~~

I am also not so super happy about this idea, but I just wanted to share it.

[cwperks]

I left a similar comment here: #5185 (comment)

I would prefer to feedback any Service/Client class through the interface. If the extensions are loaded then this service/client class would be supplied otherwise it would not be provided and plugins can handle accordingly.

I had a spike branch that's quite old at this point, but had a method called assignResourceSharingService that could be initialized by security and fed back to a plugin after security loads the extensions. ref: https://github.com/cwperks/security/pull/26/files#diff-977b27ef7df234c1583ce06a1eac45757bbf8d4ad3ba97c6515ee7450d8e8811

[DarshitChanpura]

@nibix The idea is to check whether security plugin is installed from the client of security plugin. Wouldn't any plugin be able to read any other plugin's settings with the reference object? The idea was to determine whether security plugin is installed and feature is enabled to allow throwing 501s in-case it is not.

Even in case where security plugin is not installed, security client will always be available. When we pass in the onResourceSharingSecurityInitialized we still have to determine should we choose Noop or actual implementation based on whether security plugin is installed. The client maintains a clear separation of functionalities where SPI provides interfaces to allows plugins to declare themselves as resource plugins, while client allows interaction with the security plugin when enabled, else go through the Noop route.

Maybe I'm not seeing this correctly. Would you mind elaborating why combining the functionality with SPI makes more sense?

Also, adding the actual handling inside SPI creates a cyclic dependency between SPI and sec plugin, since SPI defines interfaces that interact with the transport actions for resource access-control defined in sec plugin, and sec plugin needs the objects (Recipients.java, ShareableResource.java) defined by SPI. The option to resolve this is by moving transport actions to SPI which changes the purpose of SPI.

Let me know if its still not feasible and needs merging in with SPI.

[DarshitChanpura]

Discussed with @cwperks . The approaches are similar, just the instantiation piece was different. I will be raising a new PR with no-client approach soon. Meanwhile, @nibix if you want to elaborate on the non-feasible approach on your thread, it would help us understand the reason.

[nibix]

I see the addition of the method void assignResourceSharingClient(ResourceSharingClient client) to the SPI, but in order to judge this, I would need to see the implementation in action in the sample plugin.

The main challenge I am seeing here:

Plugins usually need to use the client in the transport actions. The official way to provide dependencies to transport actions is dependency injection.

However, the class extending ResourceSharingExtension likely won't be able to make ResourceSharingClient available via dependency injection; I'll try to break it down:

• The security plugin is responsible to call the assignResourceSharingClient() method of the other plugins.
• This is only possible after the security plugin knows these plugins. This information is provided by the loadExtensions() method. Only then it can call assignResourceSharingClient(). In the present implementation, this is done in the createComponents() method of the security plugin. If other plugins want to use the ResourceSharingClient to make it available via dependency injection, it must be ensured that their assignResourceSharingClient() implementation is called before their createComponents() method. That requires a very precisely orchestrated initialization order. I do not believe (but would be glad to be proven wrong) that the core can make guarantees about this initialization order.

Additionally: I do not believe that dependency injection can be used in a straight forward manner to provide the ResourceSharingClient to transport actions. Dependency injection works (AFAIK) globally, but here, you'd have several plugins trying to inject the object which would lead to conflicts (but again, i did not check this, please feel welcome to verify that this is not the case).

One could possibly create a plugin specific wrapper class which is then injected. That might solve it, but it also has a strong "hack taste" (but maybe that would be the way to go ... I still have to digest that). One could also resort again to singleton approaches (using static attributes), but I'd believe that this would be actually worse than a wrapper class.

Do we know other cases where plugins implementing ExtensiblePlugin use the SPI to provide dependencies to the extensions? It would be interesting to study how it is done there. I tried to do a brief research, but I could not find other cases.

[nibix]

I'll try to think about the problem a bit more in depth. If you want, we can also have another call.

However, I'll be OOO for most of the day, but would be available again in the CET evening.

[cwperks]

One could possibly create a plugin specific wrapper class which is then injected. That might solve it, but it also has a strong "hack taste" (but maybe that would be the way to go ... I still have to digest that).

This was the solution that I found. The holder could be instantiated in the resource plugin's createComponents and initialized during the call to assignResourceSharingClient.

[nibix]

After a while of back and forth thinking, I must say that - even though this solution feels a bit fragile - this is probably the best solution we can get.

So, in order to get this thing moving, we can go with this solution for now.

Still, I would recommend two things:

• File an RFC in core discussing this dependency situation and asking for opinions how this is achieved best and what could be done in core to improve the situation.
• I think we still need to work a bit on the "disabled security" or "diabled resource sharing access control" case; in these cases, the ResourceSharingClient reference would be just null, right? So, a null ResourceSharingClient would mean: skip access controls. This is a bit fragile, as a ResourceSharingClient could be also null for other reasons, for example due to exceptions in the code path leading to the initialization. I would recommend to check whether we can find solutions/patterns which can increase robustness in this regard a bit.

[DarshitChanpura]

Closing this PR to leverage the new non-client approach in #5240. Moving all discussions there.