Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4665

cwperks · 2024-08-19T21:31:53Z

Description

Companion PR in core: opensearch-project/OpenSearch#14630

This PR by itself does not add additional functionality, it simply implements the new extension points in core and introduces a new class called ContextProvidingPluginSubject which populates a header in the ThreadContext with the canonical class name of the plugin that is executing code using pluginSystemSubject.runAs(() -> { ... }). See ./gradlew integrationTest --tests SystemIndexTests for an example that verifies that a block of code run with pluginSystemSubject.runAs(() -> { ... }) has contextual info in the thread context.

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Enhancement

Issues Resolved

Related to #4439

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.

…dingPluginSubject Signed-off-by: Craig Perkins <[email protected]>

Signed-off-by: Craig Perkins <[email protected]>

… plugin user Signed-off-by: Craig Perkins <[email protected]>

Signed-off-by: Craig Perkins <[email protected]>

src/main/java/org/opensearch/security/support/SafeSerializationUtils.java

Signed-off-by: Craig Perkins <[email protected]>

…ackwards compatibility Signed-off-by: Craig Perkins <[email protected]>

Signed-off-by: Craig Perkins <[email protected]>

src/main/java/org/opensearch/security/securityconf/SecurityRoles.java

Signed-off-by: Craig Perkins <[email protected]>

peternied

@cwperks Thanks for working on this - great forward movement to granularize the permissions available to plugins.

I'm having a little trouble grokking the scope of these changes - it seems like there should be a new component that serves as a source of truth for plugin permissions that is missing - I've called this out.

Could you create a diagram showing the new parts of the system and how they interplay with with the existing security architecture?

Finally, can you speak to where this design change is in the security review process?

peternied · 2024-09-20T16:28:20Z

src/main/java/org/opensearch/security/securityconf/SecurityRoles.java

@@ -97,5 +98,7 @@ Set<String> getAllPermittedIndicesForDashboards(

    SecurityRoles filter(Set<String> roles);

+    SecurityRoles createSecurityRole(String roleName, Set<String> clusterPerms, Map<String, Set<String>> indexPatternToAllowedActions);


createSecurityRole doesn't align with how these configuration has been used before, its meant to be a representation of the security configuration from the security index, not a way to update/generate/create security objects.

Rather than overload SecurityRole with this responsibility, lets make a new component that is initialized in its own way and separately from our existing configuration. Its strange to have the logic to create these roles inside of configuration model specific code and for its state to be held inside of the privilege evaluator.

peternied · 2024-09-20T16:30:31Z

src/main/java/org/opensearch/security/user/User.java

+    /**
+     * @return true if this instance is of the type PluginUser
+     */
+    public boolean isPluginUser() {


This goes against the pattern of polymorphism, the base class shouldn't be updated to know if it is a subclass. This forces callers of User to determine what subclasses are supported.

Changed to using instanceof. I'll look into other ways to do this as well, but I'm reticent to user the same user attributes approach that we used with service accounts.

Can you help me understand why not? To take this to an extreme, if a user object representing plugin could have bad interactions with existing scenarios, why re-use the User object at all?

cwperks · 2024-09-20T16:54:52Z

@peternied Thanks for the review! I'll get back to the comments ASAP.

I think you may be interested in this PR I made in core that allows a plugin dev to specify what transport actions a plugin would need to perform outside the authenticated user context. The security plugin is a good example because there's a use-case where security needs to be able to write to the audit log index (if using an opensearch index) regardless of the authenticated user's permissions.

Does that help answer your first question?

I'm having a little trouble grokking the scope of these changes - it seems like there should be a new component that serves as a source of truth for plugin permissions that is missing - I've called this out.

In the PR in core, the cluster admin will be prompted at installation time what a plugin is requesting to be able to perform.

That PR was made in response to what you called out earlier and hearing back from plugin devs on opensearch-project/opensearch-plugins#238

Edit: Plugins will also inherently have system index access to the system indices that they have registered with SystemIndexPlugin.getSystemIndexDescriptors

src/main/java/org/opensearch/security/user/PluginUser.java

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2024-09-20T19:18:29Z

createSecurityRole doesn't align with how these configuration has been used before, its meant to be a representation of the security configuration from the security index, not a way to update/generate/create security objects.

Rather than overload SecurityRole with this responsibility, lets make a new component that is initialized in its own way and separately from our existing configuration. Its strange to have the logic to create these roles inside of configuration model specific code and for its state to be held inside of the privilege evaluator.

That's a good point. I pushed a commit to abstract this out into a separate class called InMemorySecurityRolesV7 and made it distinct from any of the ConfigV7/ConfigV6 classes which are backed by the security index.

cwperks · 2024-09-20T19:33:37Z

@peternied Here's some mermaid diagrams that I made with the help of ChatGPT-4o

Here's the prompt:

In the plugin installation process:

OpenSearch will read both plugin-security.policy (JSM Permissions) + plugin-permissions.yml (requested transport actions) and prompt the administrator about the requested permissions. Administrator can choose to accept the risks or abort the plugin installation process.

In the OpenSearch Bootstrap process:

The IdentityService is initialized using a singular IdentityPlugin
For each IdentityAwarePlugin, the IdentityPlugin will assign a PluginSubject
The plugin uses the pluginSubject to execute transport actions using the identity associated with the plugin

Low-Level Detail

The permissions defined in plugin-permissions.yml are used to create an In Memory role that allows the security plugin to re-use all existing authz logic

pluginSubject.runAs(() -> { ... }) is a direct replacement for try (ThreadContext.StoredContext ctx = threadContext.stashContext()) { ... }

Plugin Installation Sequence

sequenceDiagram
    participant Admin as Administrator
    participant OpenSearch as OpenSearch
    participant Plugin as Plugin
    
    OpenSearch ->> Plugin: Reads `plugin-security.policy` (JSM Permissions)
    OpenSearch ->> Plugin: Reads `plugin-permissions.yml` (Transport Actions)
    OpenSearch ->> Admin: Prompt for requested permissions
    
    alt Admin accepts risks
        Admin ->> OpenSearch: Accept permissions
        OpenSearch ->> Plugin: Continue with installation
    else Admin aborts
        Admin ->> OpenSearch: Abort installation
    end

OpenSearch Bootstrap Sequence:

sequenceDiagram
    participant Bootstrap as OpenSearch Bootstrap
    participant IdentityService as IdentityService
    participant IdentityPlugin as IdentityPlugin
    participant Plugin as IdentityAwarePlugin
    
    Bootstrap ->> IdentityService: Initialize IdentityService
    IdentityService ->> IdentityPlugin: Use singular IdentityPlugin
    
    loop For each IdentityAwarePlugin
        IdentityPlugin ->> Plugin: Assign PluginSubject
        Plugin ->> IdentityPlugin: PluginSubject executes transport actions
    end
    
    Plugin ->> Plugin: Executes actions with associated identity

nibix · 2024-09-20T20:06:00Z

Just one heads up that one goal of #4380 is to remove SecurityRoles alltogether.

Signed-off-by: Craig Perkins <[email protected]>

peternied

Thanks for iterating on this @cwperks we are getting closer.

@nibix if you wouldn't mind could you make sure the outstanding comment threads I've created are resolved as appropriate - when this is done please dismiss my review if you think the sprint of the feedback has been addressed. Thanks!

peternied · 2024-09-23T13:43:09Z

src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java

                    presponse.markComplete();
                    return;
                }
        }

+        if (user instanceof PluginUser) {


This doesn't seem necessary, if the user principle that represents a plugin is trustworthy the lookup for allowed system indices should return an empty set for a normal user. Lets treat all users the same in code, but allow there configuration data to be populated in a way that distinguishes them well.

By special casing for a plugin user vs normal user create a huge potential for bugs down the line where these permissions are not accounted correctly that would result in a CVE. Note; If there are performance concerns then we need to revisit how these data structures are build to be O(1) lookup.

That makes sense, I just pushed a change gets rid of the need for the subclass.

I plan to use a username convention for these special "plugin users". The name will be plugin:<pluginCanonicalClassName>. I'm choosing this because : is restricted from usernames, so this will provide assurance that this is a special kind of system username and not a user that could have been created through any other means.

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2024-10-02T21:11:16Z

Sync'ed with the latest from main

cwperks · 2024-10-04T17:35:20Z

Resolved conflicts after merge of latest PRs

cwperks · 2024-10-04T17:41:25Z

Just one heads up that one goal of #4380 is to remove SecurityRoles alltogether.

Thank you @nibix! I will check to see what changes are necessary to incorporate this change with Optimized Privileges Evaluation. I'm introducing the "in-memory" role for a plugin in this PR with the goal of eventually incorporating the changes from this PR in core so that we can re-use the authz checks when plugins want to operate outside the authenticated user context.

peternied

Thanks for making progress here, I've created some new comment threads and followed up on existing ones.

peternied · 2024-10-14T18:47:01Z

src/main/java/org/opensearch/security/identity/PluginContextSwitcher.java

+
+import org.opensearch.identity.PluginSubject;
+
+public class PluginContextSwitcher {


All User's support runAs(...), not sure what the case for this class is

Its a vehicle for carrying the subject that's been assigned to a plugin to the transport layer. Take a look at SystemIndexPlugin1 where it instantiates class and returns it from createComponents so that its injectable to transport actions.

src/main/java/org/opensearch/security/identity/SecurityUserSubject.java

peternied · 2024-10-14T18:52:33Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

+        if (user.isPluginUser()) {
+            securityRoles = getSecurityRoleForPlugin(user.getName());
+        } else {
+            securityRoles = getSecurityRoles(mappedRoles);
+        }


Shouldn't special case user vs non-user, seems like they could both be checked against either registry.

Abstracted this logic to a separate method. The problem with the latter in this block is that its actually a filtering operation and it filters based on the roles in the security index.

For these plugin users, the permissions are not tied to the security index. If opensearch-project/OpenSearch#15778 gets merged, then the permissions given to a plugin are defined in a yaml file that is read on bootstrap and kept in the memory of the process. There is no plan to support dynamic updates to these permissions as they are static and prompted to a cluster administrator on plugin install to accept.

peternied · 2024-10-14T18:53:00Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluatorResponse.java

@@ -47,6 +47,10 @@ public Set<String> getMissingPrivileges() {
        return new HashSet<String>(missingPrivileges);
    }

+    public boolean addMissingPrivileges(String action) {


IMO This should always have been exposed through methods instead of directly accessing fields of this class.

peternied · 2024-10-14T18:53:53Z

src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java

                    presponse.markComplete();
                    return;
                }
        }

+        if (user.isPluginUser()) {


Can we remove the distinction between different kinds of users, it adds complexity in these downstream systems that will make the code harder to maintain over time.

Note; It seems reasonable that no matter the source so long as the expected permissions grants are on the user (if it represents a person or a plugin) they can access system indices even if practically they aren't available at this point in time. What do you think?

I think this distinction should be kept here since this is an "ownership" model. Authorization is done based on identity here and not a role. i.e. PluginA registers SystemIndexA and gets full access to it by default.

We have a separate mechanism for provisioning system index access to regular user's but it relies on a feature being enabled. The feature flag is plugins.security.system_indices.permission.enabled and its off by default.

Authorization is done based on identity here and not a role. i.e. PluginA registers SystemIndexA and gets full access to it by default.

Why add an alternative authorization model for this case? Seems like this makes the system more complex to understand and verify vs role checks.

@nibix @DarshitChanpura I've got a preference for consistent treatment Users objects no matter if they represent a person or a plugin, I'd be curious for your opinions one way or the other.

@peternied For what its worth, there is service account specific logic in the same file. Service account is analogous to plugin user, but the difference is that its meant for extensions. i.e. If there are extensions in a cluster that register system indices, then on bootstrap the security plugin issues a token to the extension that it can use to access its registered system indices if it wants to use OpenSearch as a datastore.

Edit: IMO I'd be in favor of unifying the service account logic and plugin user logic longer-term. I think there would need to be some additional changes in the extension registration process to allow configuration of security settings like reserving (system) indices.

From what I understand, we are trying to introduce a concept of Plugin Awareness. This requires Identification of the plugin. To enforce this identity we are leveraging already existing User authz flow. Each plugin should have access to its own resource by default, but no other indices. We already have "permission-types" in place, so IMO introducing a plugin user is manageable. We can mark plugin specific users and roles as reserved and static (probly hidden?) to prevent them from being unintentionally modified.

We can mark plugin specific users and roles as reserved and static (probly hidden?) to prevent them from being unintentionally modified.

These aren't tied to an index. They are purely in-memory and cannot be altered.

peternied · 2024-10-14T19:03:21Z

src/main/java/org/opensearch/security/user/User.java

+    /**
+     * @return true if this instance is of the type PluginUser
+     */
+    public boolean isPluginUser() {


Can you help me understand why not? To take this to an extreme, if a user object representing plugin could have bad interactions with existing scenarios, why re-use the User object at all?

cwperks · 2024-10-14T19:30:13Z

Can you help me understand why not? To take this to an extreme, if a user object representing plugin could have bad interactions with existing scenarios, why re-use the User object at all?

The only real distinction is in that logic to get the "roles". For a regular user, it would filter from the roles that are in the cache, but for a plugin user it would get or create an instance of a role that's only kept in the memory of the process and not backed by the security index. With this PR, pluginSubject's are limited to interactions only with their own system indices. I plan to do a follow-up to this PR once another corresponding PR in core is merged that let's plugins request additional permissions at installation time.

Again, the distinction is in how to compute the role or roles that are used to evaluate permissions. I will abstract the logic into a single method.

Signed-off-by: Craig Perkins <[email protected]>

…ic logic into method Signed-off-by: Craig Perkins <[email protected]>

Implement new extension points in IdentityPlugin and add ContextProvi…

e65b248

…dingPluginSubject Signed-off-by: Craig Perkins <[email protected]>

cwperks requested review from cliu123, DarshitChanpura, peternied, RyanL1997, reta and willyborankin as code owners August 19, 2024 21:31

cwperks added 7 commits August 20, 2024 09:01

Remove code from UserService

55a7e45

Signed-off-by: Craig Perkins <[email protected]>

Remove commented out code

44c0d76

Signed-off-by: Craig Perkins <[email protected]>

Use guice dependencies

3e17b57

Signed-off-by: Craig Perkins <[email protected]>

Move TransportActionDependencies

0add3d8

Signed-off-by: Craig Perkins <[email protected]>

Use NamedPrincipal

b76a5b8

Signed-off-by: Craig Perkins <[email protected]>

Add end to end test to show stronger index protection for plugins

2da6f2a

Signed-off-by: Craig Perkins <[email protected]>

Add end to end test to show stronger index protection for plugins

9cf80ae

Signed-off-by: Craig Perkins <[email protected]>

cwperks mentioned this pull request Aug 20, 2024

Add runAs to Subject interface and introduce IdentityAwarePlugin extension point opensearch-project/OpenSearch#14630

Merged

3 tasks

cwperks added 6 commits August 20, 2024 15:02

Move to constructor

21fc225

Signed-off-by: Craig Perkins <[email protected]>

Remove unused constant

f08f311

Signed-off-by: Craig Perkins <[email protected]>

Add test that demonstrates cluster action forbidden when running with…

2de9a5f

… plugin user Signed-off-by: Craig Perkins <[email protected]>

Add bulk index test

22795df

Signed-off-by: Craig Perkins <[email protected]>

Ensure bulk request with mix of actions blocks as expected

e075ae8

Signed-off-by: Craig Perkins <[email protected]>

Use subclass instead of attribute

e12f673

Signed-off-by: Craig Perkins <[email protected]>

cwperks commented Aug 21, 2024

View reviewed changes

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

cwperks added 2 commits August 21, 2024 14:22

Rename TransportActionDependencies to PluginContextSwitcher

f9adcc4

Signed-off-by: Craig Perkins <[email protected]>

Prevent pluginUser from being serialized to nodes before 2.17.0 for b…

2074dab

…ackwards compatibility Signed-off-by: Craig Perkins <[email protected]>

cwperks requested a review from stephen-crawford as a code owner August 22, 2024 13:31

Demonstrate how a role can be created in-memory to re-use authz

0960ce0

Signed-off-by: Craig Perkins <[email protected]>

cwperks commented Aug 22, 2024

View reviewed changes

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

cwperks added 3 commits August 22, 2024 15:59

Add missing license headers

8c2b7ba

Signed-off-by: Craig Perkins <[email protected]>

Remove unused code

675f20a

Signed-off-by: Craig Perkins <[email protected]>

Remove unused field

8d3eb1c

Signed-off-by: Craig Perkins <[email protected]>

cwperks mentioned this pull request Sep 20, 2024

Remove Identity FeatureFlag opensearch-project/OpenSearch#16024

Merged

3 tasks

peternied requested changes Sep 20, 2024

View reviewed changes

Merge branch 'main' into security-subject

23e62bf

nibix reviewed Sep 20, 2024

View reviewed changes

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

Change calls to isPluginUser and create InMemorySecurityRoles

e5855ab

Signed-off-by: Craig Perkins <[email protected]>

cwperks dismissed DarshitChanpura’s stale review via e5855ab September 20, 2024 19:16

cwperks mentioned this pull request Sep 20, 2024

[RFC] Strengthen System Index Protection in the Plugin Ecosystem #4439

Open

Add missing license header

b8c333c

Signed-off-by: Craig Perkins <[email protected]>

peternied reviewed Sep 23, 2024

View reviewed changes

cwperks added 6 commits September 23, 2024 14:12

Use plugin: prefix

9b0c3df

Signed-off-by: Craig Perkins <[email protected]>

Remove PluginUser

abea484

Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'main' into security-subject

86c3336

Remove Identity FeatureFlag

ed13ecb

Signed-off-by: Craig Perkins <[email protected]>

Fix tests

68ff270

Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'main' into security-subject

70bb6ef

Merge branch 'main' into security-subject

dbb723a

Merge branch 'main' into security-subject

275a346

cwperks requested a review from derek-ho as a code owner October 14, 2024 17:27

peternied reviewed Oct 14, 2024

View reviewed changes

cwperks added 3 commits October 14, 2024 15:34

Address code review feedback

0216cc0

Signed-off-by: Craig Perkins <[email protected]>

Create common method for getting SecurityRoles and move plugin specif…

05890d5

…ic logic into method Signed-off-by: Craig Perkins <[email protected]>

Merge branch 'main' into security-subject

877fd42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4665

Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4665

cwperks commented Aug 19, 2024 •

edited

Loading

peternied left a comment

peternied Sep 20, 2024

peternied Sep 20, 2024

cwperks Sep 20, 2024

peternied Oct 14, 2024

cwperks commented Sep 20, 2024 •

edited

Loading

cwperks commented Sep 20, 2024

cwperks commented Sep 20, 2024

nibix commented Sep 20, 2024

peternied left a comment

peternied Sep 23, 2024

cwperks Sep 23, 2024

cwperks commented Oct 2, 2024

cwperks commented Oct 4, 2024

cwperks commented Oct 4, 2024

peternied left a comment

peternied Oct 14, 2024

cwperks Oct 14, 2024 •

edited

Loading

peternied Oct 14, 2024

cwperks Oct 15, 2024

peternied Oct 14, 2024

cwperks Oct 14, 2024 •

edited

Loading

peternied Oct 14, 2024

peternied Oct 14, 2024

cwperks Oct 14, 2024 •

edited

Loading

peternied Oct 15, 2024

peternied Oct 15, 2024

cwperks Oct 15, 2024 •

edited

Loading

DarshitChanpura Oct 15, 2024

cwperks Oct 15, 2024

peternied Oct 14, 2024

cwperks commented Oct 14, 2024

		@@ -97,5 +98,7 @@ Set<String> getAllPermittedIndicesForDashboards(

		SecurityRoles filter(Set<String> roles);

		SecurityRoles createSecurityRole(String roleName, Set<String> clusterPerms, Map<String, Set<String>> indexPatternToAllowedActions);


		import org.opensearch.identity.PluginSubject;

		public class PluginContextSwitcher {

Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4665

Are you sure you want to change the base?

Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4665

Conversation

cwperks commented Aug 19, 2024 • edited Loading

Description

Issues Resolved

Check List

peternied left a 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

cwperks commented Sep 20, 2024 • edited Loading

cwperks commented Sep 20, 2024

cwperks commented Sep 20, 2024

nibix commented Sep 20, 2024

peternied left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks commented Oct 2, 2024

cwperks commented Oct 4, 2024

cwperks commented Oct 4, 2024

peternied left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks Oct 14, 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

cwperks Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks Oct 15, 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

cwperks commented Oct 14, 2024

cwperks commented Aug 19, 2024 •

edited

Loading

cwperks commented Sep 20, 2024 •

edited

Loading

cwperks Oct 14, 2024 •

edited

Loading

cwperks Oct 14, 2024 •

edited

Loading

cwperks Oct 14, 2024 •

edited

Loading

cwperks Oct 15, 2024 •

edited

Loading