[DRAFT] [Feature] Introduces resource sharing and access-control #4746

DarshitChanpura · 2024-09-22T14:41:18Z

companion PR: opensearch-project/OpenSearch#16030

Description

Introduces a new authorization mechanism to control access to resources defined by plugins.

This PR also introduces a sample resource plugin to show the APIs in action.

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.

nibix · 2024-09-25T06:08:04Z

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

+        return services;
+    }
+
+    public static class GuiceHolder implements LifecycleComponent {


This GuiceHolder pattern from the security plugin is IMHO only a hack to get access to dependencies which are available via dependency injection, but not via the createComponents() API in the Plugin interface. The un-hackish way to give plugins access to ResourceService would be to add it to the param list of createComponents().

DarshitChanpura · 2024-10-04T18:11:47Z

Resource-sharing index gets created successfully upon first cluster start:

[2024-10-04T14:09:04,570][INFO ][o.o.p.PluginsService     ] [80a9970ace0f] PluginService:onIndexModule index:[.opensearch_resource_sharing/G4yJ1aVmRRaQStn45aWdDg]
[2024-10-04T14:09:04,604][INFO ][o.o.c.m.MetadataCreateIndexService] [80a9970ace0f] [.opensearch_resource_sharing] creating index, cause [api], templates [], shards [1]/[1]
[2024-10-04T14:09:04,607][INFO ][o.o.c.r.a.AllocationService] [80a9970ace0f] updating number_of_replicas to [0] for indices [.opensearch_resource_sharing]
[2024-10-04T14:09:04,608][WARN ][o.o.c.r.a.AllocationService] [80a9970ace0f] Falling back to single shard assignment since batch mode disable or multiple custom allocators set
[2024-10-04T14:09:04,638][INFO ][o.o.p.PluginsService     ] [80a9970ace0f] PluginService:onIndexModule index:[.opensearch_resource_sharing/G4yJ1aVmRRaQStn45aWdDg]
[2024-10-04T14:09:04,748][INFO ][o.o.c.r.a.AllocationService] [80a9970ace0f] Cluster health status changed from [YELLOW] to [GREEN] (reason: [shards started [[.opensearch_resource_sharing][0]]]).
[2024-10-04T14:09:04,768][INFO ][o.o.s.r.ResourceSharingIndexHandler] [80a9970ace0f] Resource sharing index .opensearch_resource_sharing created.

and subsequent starts show that the index is present:

[2024-10-04T14:11:08,755][INFO ][o.o.s.r.ResourceSharingIndexHandler] [80a9970ace0f] Index .opensearch_resource_sharing already exists.

Sample Resource Plugin loaded successfully:

[2024-10-04T14:49:06,630][INFO ][o.o.s.s.SampleResourcePlugin] [80a9970ace0f] Loaded SampleResourcePlugin components.

DarshitChanpura · 2024-11-11T18:26:24Z

Moved sample resource plugin code to a separate PR: #4893

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

derek-ho · 2024-12-20T16:38:51Z

.gitignore

-node_modules/
-package-lock.json


Why is this necessary?

it was automatically added. But we don't add node_modules and package-lock since we don't have any front-end components in this project.

derek-ho · 2024-12-20T16:41:13Z

src/main/java/org/opensearch/security/filter/SecurityFilter.java

@@ -345,6 +345,7 @@ private <Request extends ActionRequest, Response extends ActionResponse> void ap
                        log.info("Transport auth in passive mode and no user found. Injecting default user");
                        user = User.DEFAULT_TRANSPORT_USER;
                        threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, user);
+                        threadContext.putPersistent(ConfigConstants.OPENDISTRO_SECURITY_USER, user);


I noticed a few of these putPersistent being added. I do not know the full context, but is there any security implications with putting it in persistently? For example maybe stashing the context wouldn't work if the user is persisted into the threadcontext. Any ideas on this usage @cwperks ?

I want to add the same code in #4896. I added the notion of persistent TC headers in the core a while ago with the intent that these headers are not stashable and I'd like to move to the authenticated user as a non-stashable header.

Its added here because Security is adding an IndexOperationListener on the resourceIndex and the ThreadContext has already been stashed by the sample plugin so that it can write to the resourceIndex. By adding this call here, @DarshitChanpura is guaranteed that he can read the userinfo from the TC inside the IndexOperationListener.

I believe the postIndex hook is running within the same block as the IndexRequest to the resourceIndex.

+1 to @cwperks answer. User names are particularly required when creating or listing resources. This would ensure that a user who is current context-user is always present and thus not result in null value. At some point in future, the putTransient calls will be removed completely.

derek-ho · 2024-12-20T16:44:15Z

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

+    public void createResourceSharingIndexIfAbsent() {
+        // TODO check if this should be wrapped in an atomic completable future
+
+        this.resourceSharingIndexHandler.createResourceSharingIndexIfAbsent(() -> null);


Do we need this () -> null

it's callback functionality. Its not used currently but was added in case there were any listeners or related items are to be added.

derek-ho · 2024-12-20T16:46:42Z

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

+    private static final ResourceSharingIndexListener INSTANCE = new ResourceSharingIndexListener();
+    private ResourceSharingIndexHandler resourceSharingIndexHandler;
+
+    private boolean initialized;


Can we not use this extra boolean and just base it on the indexlistener itself?

Its to ensure that ReosurceSharingIndexListener is only initialized once.

derek-ho · 2024-12-20T16:48:03Z

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

+
+        boolean success = this.resourceSharingIndexHandler.deleteResourceSharingRecord(resourceId, resourceIndex);
+        if (success) {
+            log.info("Successfully deleted resource sharing entries for resource {}", resourceId);


Nit. Should this be singular since there would only be one resource sharing entry for each resource?

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

derek-ho · 2024-12-20T16:54:32Z

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

@@ -2082,6 +2127,7 @@ public Collection<Class<? extends LifecycleComponent>> getGuiceServiceClasses()

        final List<Class<? extends LifecycleComponent>> services = new ArrayList<>(1);
        services.add(GuiceHolder.class);
+        log.info("Guice service classes loaded");


Do we want to change this to something lower like debug?

Good catch. I will remove it.

derek-ho · 2024-12-20T16:54:58Z

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

-        return Collections.singletonList(systemIndexDescriptor);
+        final SystemIndexDescriptor securityIndexDescriptor = new SystemIndexDescriptor(indexPattern, "Security index");
+        final SystemIndexDescriptor resourceSharingIndexDescriptor = new SystemIndexDescriptor(
+            ConfigConstants.OPENSEARCH_RESOURCE_SHARING_INDEX,


Nit: Do we want to add a TODO to support custom resource sharing names?

Not right now no. Maybe in the next iteration if there is a request for it.

derek-ho · 2024-12-20T16:55:28Z

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

+            String resourceIndex = resourcePlugin.getResourceIndex();
+
+            this.indicesToListen.add(resourceIndex);
+            log.info("Preparing to listen to index: {} of plugin: {}", resourceIndex, resourcePlugin);


Same, debug?

Changed it to warn. Good catch.

derek-ho · 2024-12-20T16:56:36Z

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

+        if (areArgumentsInvalid(resourceIndex, clazz)) {
+            return Collections.emptySet();
+        }
+        final User user = threadContext.getPersistent(ConfigConstants.OPENDISTRO_SECURITY_USER);


Why are we putting this persistently here?

Is it to access the system index within the handler?

This is to get user which is then used when creating a resource or listing resources. Without this the values may be null.

derek-ho · 2024-12-20T16:58:33Z

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

+        if (adminDNs.isAdmin(user)) {
+            return true;
+        }


I don't know if this design decision has already been made, but at least with tenancy there is a concept of a private tenant, which can contain private work/shouldn't be accessible to other people (except for I guess super admin which can search the index directly), do we have any plans to implement a similar feature here?

adminDNs is super-admin, AFAIK

non-super-admin will not be able to access these resources.

derek-ho · 2024-12-20T16:59:50Z

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

+        LOGGER.info("User {} revoking access to resource {} for {} for scopes {} ", user.getName(), resourceId, revokeAccess, scopes);
+
+        // check if user is admin, if yes the user has permission
+        boolean isAdmin = adminDNs.isAdmin(user);


do we want to split this out into a utility function such as hasAdminLevelAcess? Maybe there are additional use cases that we want to support here, such as a "super" role, and not just based on the DN... wdyt?

This here is specific to super-admin. All "super" roles will require user to have the resource shared with them with that scope.

derek-ho

Can we also add some test cases for the various handlers?

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

…trol

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura · 2024-12-20T22:39:04Z

Can we also add some test cases for the various handlers?

This is a WIP. Will flip to ready for review once test cases are added.

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura mentioned this pull request Sep 22, 2024

[Feature]Introduces Resource Sharing and Access Control opensearch-project/OpenSearch#16030

Open

3 tasks

nibix reviewed Sep 25, 2024

View reviewed changes

DarshitChanpura changed the base branch from main to feature/resource-permissions November 11, 2024 18:06

DarshitChanpura mentioned this pull request Nov 11, 2024

Adds a sample resource plugin to demonstrate resource access control in action #4893

Draft

2 tasks

DarshitChanpura added 18 commits November 20, 2024 17:34

Add a base setup for resource access evaluation

831dca3

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds handler and other access management components for resource sharing

1c65eff

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds sample resource plugin

118cb07

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Removes node_modules entry from gitingore

c41b67d

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Handles changes related to scope

45d4fa5

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Updates sample plugin to implement a custom scope

ae2464d

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes Checkstyle and spotless issues

aea2253

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes initialization error

1e17dc0

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Renames sample resource plugin and adds a logger statement

84746e8

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Changes package name for sample plugin

83e4da0

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Re-organizes and renames sample plugin files

4b9b9b1

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Updates method references to conform to core

81216f1

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes compile errors

1e33dad

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes some names and method implementations

a671cc1

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds few concrete method implementations in security plugin

47b73da

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds capability to introduce index listeners for all resource plugins

8942a80

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Removes sampleplugin to be added in a separate PR

2b06603

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Updates settings.gradle

6f42bf1

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura force-pushed the resource-access-control branch from bdb5d4e to b019547 Compare November 20, 2024 22:39

Fixes imports

45b002e

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura force-pushed the resource-access-control branch from b019547 to 45b002e Compare November 20, 2024 22:41

DarshitChanpura added 3 commits November 27, 2024 14:18

Adds concrete implementations of remainder methods

a30be57

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes spotless errors

58003f6

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes log statement

078a976

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura added 8 commits December 7, 2024 18:27

Adds validation for resource ownership when granting and revoking access

5ad813b

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds super-admin bypass

c08a992

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Updates method names and adds missing java doc

8e3d41c

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Updates methods to return actual resources instead of resource ids

cabbcd6

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Stash context to fetch resources from a system index

dc964ac

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Optimize call to fetch resources

cc973c6

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Updates javadoc

3ce3d92

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds input validation

428e11e

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

derek-ho reviewed Dec 20, 2024

View reviewed changes

Adds auditlog capability and conform to changes in core

a55ac22

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

derek-ho reviewed Dec 20, 2024

View reviewed changes

DarshitChanpura added 4 commits December 20, 2024 12:27

Conforms to type bounding change introduced in core

1c62d36

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Merge remote-tracking branch 'upstream/main' into resource-access-con…

6a23f46

…trol

Stashes context while fetching resource sharing record

c24323c

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Fixes accessDeclaredMembers error caused in AuditConfig class

f514859

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Changes log levels and improves log statements

193e846

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura changed the base branch from feature/resource-permissions to main December 27, 2024 17:07

DarshitChanpura changed the base branch from main to feature/resource-permissions December 27, 2024 17:07

Bring user notion to security plugin

13fdb81

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DRAFT] [Feature] Introduces resource sharing and access-control #4746

[DRAFT] [Feature] Introduces resource sharing and access-control #4746

DarshitChanpura commented Sep 22, 2024 •

edited

Loading

nibix Sep 25, 2024

DarshitChanpura commented Oct 4, 2024 •

edited

Loading

DarshitChanpura commented Nov 11, 2024

derek-ho Dec 20, 2024

DarshitChanpura Dec 20, 2024

derek-ho Dec 20, 2024

cwperks Dec 20, 2024

DarshitChanpura Dec 20, 2024

derek-ho Dec 20, 2024

DarshitChanpura Dec 20, 2024

derek-ho Dec 20, 2024

DarshitChanpura Dec 20, 2024

derek-ho Dec 20, 2024

DarshitChanpura Dec 20, 2024

derek-ho Dec 20, 2024

DarshitChanpura Dec 20, 2024 •

edited

Loading

derek-ho Dec 20, 2024

DarshitChanpura Dec 20, 2024

derek-ho Dec 20, 2024

DarshitChanpura Dec 20, 2024 •

edited

Loading

derek-ho Dec 20, 2024

derek-ho Dec 20, 2024

DarshitChanpura Dec 20, 2024

derek-ho Dec 20, 2024

DarshitChanpura Dec 20, 2024 •

edited

Loading

DarshitChanpura Dec 20, 2024

derek-ho Dec 20, 2024

DarshitChanpura Dec 20, 2024

derek-ho left a comment

DarshitChanpura commented Dec 20, 2024

		node_modules/
		package-lock.json

[DRAFT] [Feature] Introduces resource sharing and access-control #4746

Are you sure you want to change the base?

[DRAFT] [Feature] Introduces resource sharing and access-control #4746

Conversation

DarshitChanpura commented Sep 22, 2024 • edited Loading

Description

Issues Resolved

Testing

Check List

Choose a reason for hiding this comment

DarshitChanpura commented Oct 4, 2024 • edited Loading

DarshitChanpura commented Nov 11, 2024

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

Choose a reason for hiding this comment

DarshitChanpura Dec 20, 2024 • edited Loading

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 Dec 20, 2024 • edited Loading

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 Dec 20, 2024 • edited Loading

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

derek-ho left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Dec 20, 2024

DarshitChanpura commented Sep 22, 2024 •

edited

Loading

DarshitChanpura commented Oct 4, 2024 •

edited

Loading

DarshitChanpura Dec 20, 2024 •

edited

Loading

DarshitChanpura Dec 20, 2024 •

edited

Loading

DarshitChanpura Dec 20, 2024 •

edited

Loading