[Feature] Introduces Centralized Resource Access Control and Sharing #5016

DarshitChanpura · 2025-01-10T18:27:13Z

Description

Introduces a new authorization mechanism to control access to resources defined by plugins.
There are 4 major components to this PR:

Introduces an SPI that defines the contracts for a resource sharing plugin as well as Resource Access Control plugin.
Introduces 4 new APIs that expose resource-sharing and access verification capabilities.
Modifies DLS to implicitly check for resource-access for requesting user if incoming request is for a resource
Adds a sample resource plugin with tests to demonstrate the usage of this new feature.
Adds a feature flag to toggle the feature. Feature is enabled by default.

Category : New feature
Why these changes are required?
- At present, plugins have implemented in-house authorization mechanisms for a lack of centralized framework. This PR introduces a centralized way to offload resource permissions check to security plugin. Plugins will leverage the java APIs introduced in core.

Issues Resolved

Related to [Proposal] Resource Permissions and Sharing #4500

Testing

automated and manual testing

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.

Signed-off-by: Darshit Chanpura <[email protected]>

…in action Signed-off-by: Darshit Chanpura <[email protected]>

Signed-off-by: Darshit Chanpura <[email protected]>

… term Signed-off-by: Darshit Chanpura <[email protected]>

Signed-off-by: Darshit Chanpura <[email protected]>

… resource-sharing-spi

Signed-off-by: Darshit Chanpura <[email protected]>

codecov · 2025-01-13T17:05:17Z

Codecov Report

Attention: Patch coverage is 9.57661% with 897 lines in your changes missing coverage. Please review.

Project coverage is 68.85%. Comparing base (16c9332) to head (e1e876f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ecurity/resources/ResourceSharingIndexHandler.java	3.00%	258 Missing ⚠️
...arch/security/resources/ResourceAccessHandler.java	9.60%	113 Missing ⚠️
...opensearch/security/resources/ResourceSharing.java	0.00%	68 Missing ⚠️
...opensearch/security/resources/SharedWithScope.java	0.00%	57 Missing ⚠️
...a/org/opensearch/security/resources/CreatedBy.java	0.00%	26 Missing ⚠️
...a/org/opensearch/security/resources/ShareWith.java	0.00%	25 Missing ⚠️
...s/access/list/ListAccessibleResourcesResponse.java	0.00%	23 Missing ⚠️
...ces/access/revoke/RevokeResourceAccessRequest.java	0.00%	23 Missing ⚠️
...es/access/TransportVerifyResourceAccessAction.java	0.00%	22 Missing ⚠️
.../opensearch/security/OpenSearchSecurityPlugin.java	52.27%	18 Missing and 3 partials ⚠️
... and 26 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5016      +/-   ##
==========================================
- Coverage   71.45%   68.85%   -2.61%     
==========================================
  Files         334      367      +33     
  Lines       22556    23536     +980     
  Branches     3589     3680      +91     
==========================================
+ Hits        16118    16205      +87     
- Misses       4644     5525     +881     
- Partials     1794     1806      +12

Files with missing lines	Coverage Δ
.../org/opensearch/security/auth/BackendRegistry.java	`78.32% <100.00%> (+0.27%)`	⬆️
...org/opensearch/security/filter/SecurityFilter.java	`66.03% <100.00%> (-0.32%)`	⬇️
...rch/security/privileges/dlsfls/DlsRestriction.java	`94.59% <100.00%> (ø)`
...security/privileges/dlsfls/DocumentPrivileges.java	`100.00% <100.00%> (ø)`
...a/org/opensearch/security/resources/Recipient.java	`100.00% <100.00%> (ø)`
...g/opensearch/security/support/ConfigConstants.java	`95.23% <ø> (ø)`
...g/opensearch/security/resources/RecipientType.java	`60.00% <60.00%> (ø)`
...rces/ResourceSharingIndexManagementRepository.java	`70.00% <70.00%> (ø)`
...ces/access/list/ListAccessibleResourcesAction.java	`0.00% <0.00%> (ø)`
...rces/access/revoke/RevokeResourceAccessAction.java	`0.00% <0.00%> (ø)`
... and 32 more

... and 6 files with indirect coverage changes

Signed-off-by: Darshit Chanpura <[email protected]>

…I to maven publish task and updates SPI readme Signed-off-by: Darshit Chanpura <[email protected]>

sample-resource-plugin/src/main/java/org/opensearch/sample/SampleResourcePlugin.java

nibix · 2025-01-14T10:53:13Z

spi/src/main/java/org/opensearch/security/spi/resources/Resource.java

+/**
+ * Marker interface for all resources
+ */
+public abstract class Resource implements NamedWriteable, ToXContentFragment {


Why is this an abstract class instead of an interface? With an abstract class, the implementer cannot extend other classes for their resources.

The idea is that a Resource must only extend this class. The reason why it is abstract is that I want to enforce a constructor of type Resource(StreamInput in) be created.

nibix · 2025-01-14T11:33:47Z

src/integrationTest/java/org/opensearch/security/SearchOperationTest.java

@@ -383,6 +384,7 @@ public class SearchOperationTest {
            new AuditConfiguration(true).compliance(new AuditCompliance().enabled(true))
                .filters(new AuditFilters().enabledRest(true).enabledTransport(true))
        )
+        .nodeSettings(Map.of(OPENSEARCH_RESOURCE_SHARING_ENABLED, false))


Why is this necessary here?

The feature is enabled by default. Which means that the resource sharing index will be created which caused shouldGetFieldCapabilitiesForAllIndexes_positive to fail on containsExactlyIndices assertion. The other option to not adding this flag is to add that assertion. However, for the sake of this test, since it is not resource-sharing related, I added a flag to disable it.

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

nibix · 2025-01-14T11:37:27Z

src/main/java/org/opensearch/security/auth/BackendRegistry.java

@@ -224,6 +224,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
        if (adminDns.isAdminDN(sslPrincipal)) {
            // PKI authenticated REST call
            threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, new User(sslPrincipal));
+            threadContext.putPersistent(ConfigConstants.OPENDISTRO_SECURITY_USER, new User(sslPrincipal));


How is this change related to resource sharing?

Since writing to a resource index maybe a stashed operation, we might lose the context of the requested user. To prevent that we store the user as a persistent header which will then be retrieved while fetching current user's resources, verifying access or creating a resource.

In long term we do intend to change this to a persistent header and remove transient completely.

nibix · 2025-01-14T11:44:08Z

src/main/java/org/opensearch/security/resources/ResourceSharingIndexHandler.java

+
+        searchRequest.source(searchSourceBuilder);
+
+        SearchResponse searchResponse = client.search(searchRequest).actionGet();


Sync operations in low level OpenSearch code are bound to make issues. As soon this is executed on a transport thread, this will fail.

Could you explain a bit more in detail?

nibix · 2025-01-14T11:46:10Z

src/main/java/org/opensearch/security/configuration/SecurityFlsDlsIndexSearcherWrapper.java

+        if (this.isResourceSharingEnabled
+            && !Strings.isNullOrEmpty(indexName)
+            && OpenSearchSecurityPlugin.getResourceIndices().contains(indexName)) {
+            resourceIds = this.resourceAccessHandler.getAccessibleResourceIdsForCurrentUser(indexName);


Under the hood, this makes a sync index operation.

We are here in an extremely low level operation. I am pretty sure that this will not work, even if it would be an async operation (which it cannot be due to the API constraints).

Could you explain a bit more in detail?

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura added 30 commits November 20, 2024 17:34

Add a base setup for resource access evaluation

831dca3

Signed-off-by: Darshit Chanpura <[email protected]>

Adds handler and other access management components for resource sharing

1c65eff

Signed-off-by: Darshit Chanpura <[email protected]>

Adds sample resource plugin

118cb07

Signed-off-by: Darshit Chanpura <[email protected]>

Removes node_modules entry from gitingore

c41b67d

Signed-off-by: Darshit Chanpura <[email protected]>

Handles changes related to scope

45d4fa5

Signed-off-by: Darshit Chanpura <[email protected]>

Updates sample plugin to implement a custom scope

ae2464d

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes Checkstyle and spotless issues

aea2253

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes initialization error

1e17dc0

Signed-off-by: Darshit Chanpura <[email protected]>

Renames sample resource plugin and adds a logger statement

84746e8

Signed-off-by: Darshit Chanpura <[email protected]>

Changes package name for sample plugin

83e4da0

Signed-off-by: Darshit Chanpura <[email protected]>

Re-organizes and renames sample plugin files

4b9b9b1

Signed-off-by: Darshit Chanpura <[email protected]>

Updates method references to conform to core

81216f1

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes compile errors

1e33dad

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes some names and method implementations

a671cc1

Signed-off-by: Darshit Chanpura <[email protected]>

Adds few concrete method implementations in security plugin

47b73da

Signed-off-by: Darshit Chanpura <[email protected]>

Adds capability to introduce index listeners for all resource plugins

8942a80

Signed-off-by: Darshit Chanpura <[email protected]>

Removes sampleplugin to be added in a separate PR

2b06603

Signed-off-by: Darshit Chanpura <[email protected]>

Updates settings.gradle

6f42bf1

Signed-off-by: Darshit Chanpura <[email protected]>

Adds a sample resource plugin to demonstrate resource access control …

561e294

…in action Signed-off-by: Darshit Chanpura <[email protected]>

Fixes imports

45b002e

Signed-off-by: Darshit Chanpura <[email protected]>

Cleans up create action

57661e7

Signed-off-by: Darshit Chanpura <[email protected]>

Adds concrete implementations of remainder methods

a30be57

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes create API

d68f349

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes spotless errors

58003f6

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes log statement

078a976

Signed-off-by: Darshit Chanpura <[email protected]>

Adds Revoke API and cleans up existing APIs

0460549

Signed-off-by: Darshit Chanpura <[email protected]>

Renames ResourceManagement repository and add keyword to search query…

8e44cf3

… term Signed-off-by: Darshit Chanpura <[email protected]>

Fixes delete method

16a0ba6

Signed-off-by: Darshit Chanpura <[email protected]>

Adds delete API and refactors package structure

46960ea

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes updateByQuery painless script

ac53c8f

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura added 5 commits January 9, 2025 12:58

Updates settings gradle to add spi

fce2b90

Signed-off-by: Darshit Chanpura <[email protected]>

Updates DLS Search handler to filter out resources

1aec47a

Signed-off-by: Darshit Chanpura <[email protected]>

Updates if clause to match correctly

11d8d6d

Signed-off-by: Darshit Chanpura <[email protected]>

Updates resource tests

07b26b0

Signed-off-by: Darshit Chanpura <[email protected]>

Merge remote-tracking branch 'origin/sample-resource-plugin-spi' into…

c3ad714

… resource-sharing-spi

DarshitChanpura added 3 commits January 10, 2025 13:34

Changes SPI dependency visibility

5e5c243

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes CI errors

289659f

Signed-off-by: Darshit Chanpura <[email protected]>

Fixes tests running in SSL only mode

512c1a8

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the resource-sharing-spi branch from 6498c44 to decf32d Compare January 10, 2025 20:10

Fixes checkstyle errors

85c4556

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the resource-sharing-spi branch from decf32d to 85c4556 Compare January 10, 2025 20:34

DarshitChanpura added 2 commits January 10, 2025 16:45

Merge remote-tracking branch 'upstream/main' into resource-sharing-spi

6849242

Adds featureFlag for resource sharing

3e6531d

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura added 4 commits January 13, 2025 15:04

Fixes spotbugsintegtest error

5cab8af

Signed-off-by: Darshit Chanpura <[email protected]>

Merge remote-tracking branch 'upstream/main' into resource-sharing-spi

1e658e2

Fixes SafeSerializationUtils test

8366a05

Signed-off-by: Darshit Chanpura <[email protected]>

Fix CI workflow that called assemble instead of :assemble and adds SP…

534838f

…I to maven publish task and updates SPI readme Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the resource-sharing-spi branch from f6472d8 to 534838f Compare January 13, 2025 21:36

nibix reviewed Jan 14, 2025

View reviewed changes

sample-resource-plugin/src/main/java/org/opensearch/sample/SampleResourcePlugin.java Outdated Show resolved Hide resolved

nibix reviewed Jan 14, 2025

View reviewed changes

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java Outdated Show resolved Hide resolved

nibix reviewed Jan 14, 2025

View reviewed changes

Removes Guice reference from Sample plugin and changes warn log to info

e1e876f

Signed-off-by: Darshit Chanpura <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Introduces Centralized Resource Access Control and Sharing #5016

[Feature] Introduces Centralized Resource Access Control and Sharing #5016

DarshitChanpura commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

nibix Jan 14, 2025

DarshitChanpura Jan 14, 2025

nibix Jan 14, 2025

DarshitChanpura Jan 14, 2025

nibix Jan 14, 2025

DarshitChanpura Jan 14, 2025

DarshitChanpura Jan 14, 2025

nibix Jan 14, 2025

DarshitChanpura Jan 14, 2025

nibix Jan 14, 2025

DarshitChanpura Jan 14, 2025


		searchRequest.source(searchSourceBuilder);

		SearchResponse searchResponse = client.search(searchRequest).actionGet();

[Feature] Introduces Centralized Resource Access Control and Sharing #5016

Are you sure you want to change the base?

[Feature] Introduces Centralized Resource Access Control and Sharing #5016

Conversation

DarshitChanpura commented Jan 10, 2025 • edited Loading

Description

Issues Resolved

Testing

Check List

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading