[Feature/Identity] Basic permissions check #6029

cwperks · 2023-01-26T18:43:02Z

Description

This PR shows a rudimentary implementation for permissions checking that works in common OpenSearch scenarios including any cluster related actions and indices actions like creating an index and search. This provides a base to build upon as more actions are tested with this permissions model.

In this PR the permissions are directly saved onto the user, though permissions resolution is still an open issue.

The InternalRealm has been switched to extend shiro's Authorizing realm which means that a function called doGetAuthorizationInfo is implemented to resolve a user to a list of permissions.

Actions are compared using shiro's WildcardPermission. For requests requiring indices, all indices in the request are compared against index patterns for the action that the user has been granted to and this implementation uses the internal Glob.globMatch to determine if the index pattern requested matches a pattern that's been granted access.

Issues Resolved

First step of: #5859

Related PRs:

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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: Craig Perkins <[email protected]>

cwperks · 2023-01-26T18:44:08Z

sandbox/modules/identity/src/main/java/org/opensearch/identity/authz/OpenSearchPermission.java

+            return false;
+        }
+
+        // Uncomment the following lines if index name -> concrete index resolution is required


To fully uncomment this block and use the IndexNameExpressionResolver then the following needs to be added to IdentityPlugin:

@Override public Collection<Class<? extends LifecycleComponent>> getGuiceServiceClasses() { if (!enabled) { return Collections.emptyList(); } final List<Class<? extends LifecycleComponent>> services = new ArrayList<>(1); services.add(GuiceHolder.class); return services; } public static class GuiceHolder implements LifecycleComponent { private static ClusterService clusterService; private static RepositoriesService repositoriesService; @Inject public GuiceHolder(final RepositoriesService repositoriesService, final ClusterService clusterService) { GuiceHolder.repositoriesService = repositoriesService; GuiceHolder.clusterService = clusterService; } public static RepositoriesService getRepositoriesService() { return repositoriesService; } public static ClusterService getClusterService() { return clusterService; } @Override public void close() {} @Override public Lifecycle.State lifecycleState() { return null; } @Override public void addLifecycleListener(LifecycleListener listener) {} @Override public void removeLifecycleListener(LifecycleListener listener) {} @Override public void start() {} @Override public void stop() {} }

I don't think we should be attempting to resolve the name ever, if you have permissions to the URL you get access to target, so if there is an alias, so long as you have permission, its allowed. This prevents 'transitive' permissions due to the creation of aliases or other systems... views?, food food thought.

.../identity/src/main/java/org/opensearch/identity/authz/IndexNameExpressionResolverHolder.java

cwperks · 2023-01-26T18:46:48Z

sandbox/modules/identity/src/main/java/org/opensearch/identity/SecurityFilter.java

+            permission += "|" + Strings.join(resources.get(), ",");
+        }
+
+        if (log.isDebugEnabled()) {


This block logs out the result of privilege evaluation.

Add logger.org.opensearch.identity: debug to opensearch.yml to enable debug logging for this module.

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

github-actions · 2023-01-26T19:04:01Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9977/
CommitID: a6eddc0
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-01-26T19:20:25Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9978/
CommitID: 7f9d16e
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

sandbox/libs/authn/src/main/java/org/opensearch/authn/noop/NoopSubject.java

stephen-crawford · 2023-01-26T19:08:48Z

sandbox/modules/identity/src/main/java/org/opensearch/identity/SecurityFilter.java

@@ -67,6 +72,26 @@ private <Request extends ActionRequest, Response extends ActionResponse> void ap
        ActionListener<Response> listener,
        ActionFilterChain<Request, Response> chain
    ) {
+        // TODO The section below shows permission check working for index create and index search, but needs


Do you know what changes will be required here for the plugins and extension functionality? We have the dual meaning of the permissions and extensions since they can both be the subject and have permissions regarding them. Do we need to define any requests for either to move things forward?

Extensions can register actions and those actions need to be permissible for cluster admins to be able to permit users to use features of an extension.

Plugins run in the same JVM as the OpenSearch node, but extensions run outside. Extensions interact with the OpenSearch cluster using a rest client to make requests to the cluster. Users have sets of permissions associated with them and extensions may as well, to give cluster admins the ability to further restrict how requests originating from an extension can interact with a cluster. At the very least, an extension making a request to a cluster on behalf of a user should perform the request with the same user permissions + restrictions, restrictions like preventing the ability to query system indices even if a user has access.

I was trying to make an analogy with microsoft word and saving files to the filesystem, but I believe that word is able to save to the same folders as the current logged in user.

Oh I see. Thank you for the explanation of what we will need to do moving forward. I have mostly been designing from the standpoint of generic permissions so wanted to see how this would fit with a more user-centric version like you have here. That makes sense though.

sandbox/modules/identity/src/main/java/org/opensearch/identity/User.java

.../identity/src/main/java/org/opensearch/identity/authz/IndexNameExpressionResolverHolder.java

stephen-crawford · 2023-01-26T19:21:16Z

sandbox/modules/identity/src/main/java/org/opensearch/identity/authz/OpenSearchPermission.java

+        // Set<String> concretePermissionIndexNames = iner.resolveExpressions(cs, this.indexPatterns.toArray(new String[0]));
+        // Set<String> concreteRequestedIndexNames = iner.resolveExpressions(cs, requestedPermission.indexPatterns.toArray(new String[0]));
+
+        boolean allRequestedPatternsMatchGranted = requestedPermission.indexPatterns.stream()


This may need an inline comment just cause it is somewhat complicated. It first checks for any match then tries to all match after so it is just a little complex.

Sure, I can add one in. Capturing it here now:

User granted index patterns: my-index*,other-index* Request permission on: my-index1 For all requested index patterns: requested index pattern must match at least one pattern that the user has been granted access to

Great thank you!

sandbox/modules/identity/src/main/java/org/opensearch/identity/realm/InternalRealm.java

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

github-actions · 2023-01-26T20:59:53Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9986/
CommitID: 91ed20b
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

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

github-actions · 2023-01-27T16:16:55Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/10048/
CommitID: 310661d
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

github-actions · 2023-01-27T20:44:07Z

Gradle Check (Jenkins) Run Completed with:

RESULT: UNSTABLE ❕
TEST FAILURES:

      1 org.opensearch.identity.UserPersistenceIT.testUserPersistenceInvalidYml
      1 org.opensearch.identity.UserPersistenceIT.testUserPersistence
      1 org.opensearch.identity.BasicAuthIT.testBasicAuthUnauthorized
      1 org.opensearch.identity.BasicAuthIT.testBasicAuthSuccess
      1 org.opensearch.identity.BasicAuthIT.testBasicAuthSuccess
      1 org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testIndexCreateBlockWithAReadOnlyBlock
      1 org.opensearch.cluster.routing.allocation.decider.DiskThresholdDeciderIT.testIndexCreateBlockIsRemovedWhenAnyNodesNotExceedHighWatermark

URL: https://build.ci.opensearch.org/job/gradle-check/10073/
CommitID: 8b16b01
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

codecov-commenter · 2023-01-27T20:45:15Z

Codecov Report

Merging #6029 (8b16b01) into feature/identity (6abe64a) will decrease coverage by 0.48%.
The diff coverage is 30.76%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@                  Coverage Diff                   @@
##             feature/identity    #6029      +/-   ##
======================================================
- Coverage               71.08%   70.61%   -0.48%     
+ Complexity              59272    58899     -373     
======================================================
  Files                    4829     4831       +2     
  Lines                  282163   282225      +62     
  Branches                40686    40695       +9     
======================================================
- Hits                   200578   199294    -1284     
- Misses                  65477    66580    +1103     
- Partials                16108    16351     +243

Impacted Files	Coverage Δ
...in/java/org/opensearch/authn/noop/NoopSubject.java	`60.00% <0.00%> (-6.67%)`	⬇️
...identity/authmanager/internal/InternalSubject.java	`28.57% <0.00%> (-4.77%)`	⬇️
...hmanager/securityplugin/SecurityPluginSubject.java	`0.00% <0.00%> (ø)`
...pensearch/identity/authz/OpenSearchPermission.java	`0.00% <0.00%> (ø)`
...a/org/opensearch/identity/realm/InternalRealm.java	`68.00% <8.33%> (-17.00%)`	⬇️
...ty/src/main/java/org/opensearch/identity/User.java	`76.92% <57.14%> (-8.08%)`	⬇️
...ntity/authz/IndexNameExpressionResolverHolder.java	`71.42% <71.42%> (ø)`
...n/java/org/opensearch/identity/SecurityFilter.java	`60.71% <75.00%> (+3.89%)`	⬆️
...n/java/org/opensearch/identity/IdentityPlugin.java	`67.10% <100.00%> (+0.43%)`	⬆️
.../index/shard/IndexShardNotRecoveringException.java	`0.00% <0.00%> (-50.00%)`	⬇️
... and 457 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

github-actions · 2023-01-31T15:15:30Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/10311/
CommitID: 49e6b15
Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
Is the failure a flaky test unrelated to your change?

cwperks · 2023-01-31T15:55:30Z

The gradle check failure is unrelated to this PR:

Execution failed for task ':qa:mixed-cluster:v2.6.0#mixedClusterTest'.
> `cluster{:qa:mixed-cluster:v2.6.0}` failed to wait for cluster health yellow after 40 SECONDS
    IO error while waiting cluster
    500 Internal Server Error

peternied · 2023-01-31T17:36:38Z

sandbox/modules/identity/src/main/java/org/opensearch/identity/User.java

@@ -51,6 +53,9 @@ public User(final String username, final String bcryptHash, Map<String, String>
    @JsonProperty(value = "attributes")
    private Map<String, String> attributes = Collections.emptyMap();

+    @JsonProperty(value = "permissions")
+    private List<String> permissions = Collections.emptyList();


Not blocking this change: I think we should retrieve permissions from the store that @scrawfor99 has created

Agreed. This is added here to show a permissions check working end to end and the magic of permissions resolution (Subject -> List of Permissions) happens in doGetAuthorizationInfo of the InternalRealm. That method can be updated when the long-term permissions store is decided upon and implemented.

peternied

Getting this merged for speed

cwperks added 15 commits January 20, 2023 19:24

Connect dynamic config factory with internal realm

f22e04c

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

Remove example_internal_users.yml

f4f1bbd

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

Add to CHANGELOG

afeff5c

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

Create temp folder and file using permitted APIs

0fa0627

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

Remove apostrophe

51c0ef4

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

Add Before

d0bc4d6

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

Use java.nio

4614ef6

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

WIP on permission checking

e13f18c

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

Port IndexResolverReplacer

f035dbf

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

Create OpenSearchPermission and use glob matching

37cc783

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

Basic permissions checks

a5f5f46

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

Remove commons-collections

0f2962f

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

Merge branch 'feature/identity' into permissions-data-read

6c9eb35

Remove unused code

416c295

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

Add TODO

a6eddc0

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

cwperks commented Jan 26, 2023

View reviewed changes

.../identity/src/main/java/org/opensearch/identity/authz/IndexNameExpressionResolverHolder.java Show resolved Hide resolved

cwperks commented Jan 26, 2023

View reviewed changes

Add to CHANGELOG

7f9d16e

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

stephen-crawford reviewed Jan 26, 2023

View reviewed changes

hash to permissions

91ed20b

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

Add comment

310661d

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

stephen-crawford mentioned this pull request Jan 27, 2023

[Feature/Identity] Extend Shiro Permissions to allow for resolving permission indices #6024

Closed

6 tasks

Merge branch 'feature/identity' into permissions-data-read

8b16b01

stephen-crawford mentioned this pull request Jan 31, 2023

[Feature/Identity][WIP] Permission checking and grant resolution #6110

Closed

6 tasks

peternied reviewed Jan 31, 2023

View reviewed changes

peternied approved these changes Jan 31, 2023

View reviewed changes

peternied marked this pull request as ready for review January 31, 2023 17:40

peternied merged commit bdad5f9 into opensearch-project:feature/identity Jan 31, 2023

peternied mentioned this pull request Jan 31, 2023

[Feature/Identity] Basic permissions checking from Subject #5089

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature/Identity] Basic permissions check #6029

[Feature/Identity] Basic permissions check #6029

cwperks commented Jan 26, 2023

cwperks Jan 26, 2023

peternied Jan 31, 2023

cwperks Jan 26, 2023 •

edited

Loading

github-actions bot commented Jan 26, 2023

github-actions bot commented Jan 26, 2023

stephen-crawford Jan 26, 2023

cwperks Jan 26, 2023

stephen-crawford Jan 27, 2023

stephen-crawford Jan 26, 2023

cwperks Jan 26, 2023

stephen-crawford Jan 27, 2023

github-actions bot commented Jan 26, 2023

github-actions bot commented Jan 27, 2023

github-actions bot commented Jan 27, 2023

codecov-commenter commented Jan 27, 2023

github-actions bot commented Jan 31, 2023

cwperks commented Jan 31, 2023

peternied Jan 31, 2023

cwperks Jan 31, 2023

peternied left a comment

[Feature/Identity] Basic permissions check #6029

[Feature/Identity] Basic permissions check #6029

Conversation

cwperks commented Jan 26, 2023

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 26, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 26, 2023

Gradle Check (Jenkins) Run Completed with:

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

github-actions bot commented Jan 26, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 27, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 27, 2023

Gradle Check (Jenkins) Run Completed with:

codecov-commenter commented Jan 27, 2023

Codecov Report

github-actions bot commented Jan 31, 2023

Gradle Check (Jenkins) Run Completed with:

cwperks commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

cwperks Jan 26, 2023 •

edited

Loading