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

peternied · 2022-11-04T21:32:13Z

Description

Adds capability for subject to check if a permission is allowed or not and creates a mechanism for actions to describe the permissions associated with them.

Issues Resolved

Related Create a permissions check API for extensions to us with custom resources opensearch-sdk-java#40

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.

github-actions · 2022-11-04T21:35:54Z

Gradle Check (Jenkins) Run Completed with:

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

cwperks

Left some initial feedback. Thanks for putting this out there to get a conversation going around this topic.

cwperks · 2022-11-08T18:47:31Z

sandbox/libs/authn/src/main/java/org/opensearch/authn/Permission.java

+    private final String[] permissionChunks; 
+
+    public Permission(final String permission) {
+        this.permissionChunks = permission.split("\\.");


Can this be made into a constant. wdyt about PERMISSION_DELIMITER?

We should create another constructor, which accepts delimiter as an argument.

These permissions should have a consistent format, following @cwperks suggestion

server/src/main/java/org/opensearch/action/ActionRequest.java

server/src/main/java/org/opensearch/action/ActionType.java

server/src/main/java/org/opensearch/action/admin/indices/create/CreateIndexAction.java

sandbox/libs/authn/src/main/java/org/opensearch/authn/Subject.java

cwperks · 2022-11-08T19:03:23Z

server/src/main/java/org/opensearch/action/admin/indices/create/CreateIndexRequest.java

+    @Override
+    public List<String> requiredPermissions() {
+        return List.of(
+            "opensearch.engine.index.create",


This is a good place to start and should be expandable to index patterns. Are you thinking that all permissions in core be preceded with opensearch.engine and there will be a separate convention for actions added by plug-ins or modules?

FYI @shanilpa @jimishs.

For this specific request when it comes to indexing, its clearly part of the core/engine, or maybe the prefix should just be opensearch.indexing.index.create. Maybe of the nature [Source].[Category].[Object].[Action]?

I don't plan on iterating on this name for this pull request and we will need to follow up in the broader vNext permissions conversation.

Should these be loaded from a file or some other store? (Could be part of next iteration)

I looked into the relationship between permissions and actions and there is no good mechanism
to engage with actions in a way that produces a list of permissions.

Exploring the ActionModule there is a common system to register actions however this registration doesn't pass all of the information about action execution, it passes a limited version.

What I would recommend would be creating a new description for action related data that encompasses the action type, action request, action response, and the action handler. Building this into or around the ActionModule seems appropriate but this will need to be rolled out over time since it would be backwards incompatible to plugins. This is a good follow up item.

DarshitChanpura

Ty for this PR. Added a few comments.

DarshitChanpura · 2022-11-15T16:20:58Z

sandbox/libs/authn/src/main/java/org/opensearch/authn/Permission.java

+        this.permissionChunks = permission.split("\\.");
+    }
+
+    public boolean matches(final String permissionRequired) {


seems like we can leverage equals method here. Thoughts?

A user might have a grant represented as opensearch.indexing.index, and the checked permission will be opensearch.indexing.index.create. This isn't an equality check, but a check if the permission grant matches the permission checked.

As I'm writing this out, it might make sense to create different objects to represent the grant vs the permission them selves - I'll revisit this

DarshitChanpura · 2022-11-15T16:22:13Z

sandbox/libs/authn/src/main/java/org/opensearch/authn/Permission.java

+    private final String[] permissionChunks; 
+
+    public Permission(final String permission) {
+        this.permissionChunks = permission.split("\\.");


We should create another constructor, which accepts delimiter as an argument.

sandbox/libs/authn/src/main/java/org/opensearch/authn/Subject.java

DarshitChanpura · 2022-11-15T16:37:36Z

server/src/main/java/org/opensearch/action/admin/indices/create/CreateIndexRequest.java

+    @Override
+    public List<String> requiredPermissions() {
+        return List.of(
+            "opensearch.engine.index.create",


Should these be loaded from a file or some other store? (Could be part of next iteration)

DarshitChanpura · 2022-11-15T16:38:51Z

server/src/main/java/org/opensearch/action/support/TransportAction.java

+        if (unauthorizedException != null) {
+            listener.onFailure(unauthorizedException);
+            return;
+        }


very clean 👏

server/src/main/java/org/opensearch/identity/noop/NoopSubject.java

sandbox/libs/authn/src/main/java/org/opensearch/authn/Permission.java

Adds capability for subject to check if a permission is allowed or not and creates a mechanism for actions to describe the permissinos associated with them. Signed-off-by: Peter Nied <[email protected]>

Signed-off-by: Peter Nied <[email protected]>

DarshitChanpura · 2023-01-11T21:08:08Z

sandbox/libs/authn/src/main/java/org/opensearch/authn/Permission.java

+        try {
+            this.permissionChunks = permission.split(PERMISSION_DELIMITER);
+        } catch (Exception) {
+            throw new InvalidPermissionName(permission);
+        }


This change is related to the change suggestion below for checkIsValid method

Suggested change

try {

this.permissionChunks = permission.split(PERMISSION_DELIMITER);

} catch (Exception) {

throw new InvalidPermissionName(permission);

}

this.permissionChunks = splitIntoChunks(permission);

I did this to throw the specific type of exception rather than having a 'string' format issue be seen

okay, i was thinking it from reusability side of things. Maybe we can change definition of the constructor and the splitIntoChunks throws InvalidPermissionException. Thoughts?

DarshitChanpura · 2023-01-11T21:10:56Z

sandbox/libs/authn/src/main/java/org/opensearch/authn/Permission.java

+    public static void checkIsValid(final String permission) {
+        new Permission(permission);
+    }


We can introduce following for re-usability, instead of creating a new unused object everytime this method is called

Suggested change

public static void checkIsValid(final String permission) {

new Permission(permission);

}

private static String[] splitIntoChunks(final String permission){

try {

return permission.split(PERMISSION_DELIMITER);

} catch (Exception) {

throw new InvalidPermissionName(permission);

}

}

public static void checkIsValid(final String permission) {

splitIntoChunks(permission);

}

github-actions · 2023-01-11T22:00:43Z

Gradle Check (Jenkins) Run Completed with:

RESULT: TIMEOUT ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9310/
CommitID: ba6f5cb
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-11T22:01:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: TIMEOUT ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9311/
CommitID: 2237639
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: Peter Nied <[email protected]>

github-actions · 2023-01-11T22:03:15Z

Gradle Check (Jenkins) Run Completed with:

RESULT: null ❌
URL: null
CommitID: 18eea2e
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-11T22:14:58Z

Gradle Check (Jenkins) Run Completed with:

RESULT: TIMEOUT ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9312/
CommitID: 6e0eb0a
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-12T18:52:08Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9340/
CommitID: 18eea2e
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: Peter Nied <[email protected]>

github-actions · 2023-01-12T21:04:32Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9350/
CommitID: b595373
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: Peter Nied <[email protected]>

github-actions · 2023-01-12T21:13:34Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9351/
CommitID: 5845848
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: Peter Nied <[email protected]>

github-actions · 2023-01-12T21:25:07Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9352/
CommitID: 972e2ea
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: Peter Nied <[email protected]>

github-actions · 2023-01-12T22:18:25Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9357/
CommitID: 4e1e4a7
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: Peter Nied <[email protected]>

github-actions · 2023-01-13T00:40:16Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9362/
CommitID: bd0e0f8
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?

peternied · 2023-01-31T17:43:28Z

@cwperks already has trail blazed in this space #6029

@scrawfor99 this is a good reference for a rudimentary permission checking as applied in OpenSearch, Craig's PR above is a a way to see how shiro integration could handle some of that logic. I think we will need a blending of this and your changes we good forward

peternied requested review from a team and reta as code owners November 4, 2022 21:32

peternied changed the title ~~Basic permissions checking from Subject~~ [Feature/Identity] Basic permissions checking from Subject Nov 8, 2022

cwperks reviewed Nov 8, 2022

View reviewed changes

DarshitChanpura reviewed Nov 15, 2022

View reviewed changes

peternied added 2 commits January 11, 2023 19:06

Basic permissions checking from Subject

c42fbe1

Adds capability for subject to check if a permission is allowed or not and creates a mechanism for actions to describe the permissinos associated with them. Signed-off-by: Peter Nied <[email protected]>

Initial PR feedback

ba6f5cb

Signed-off-by: Peter Nied <[email protected]>

peternied force-pushed the check-permissions branch from 014adb4 to ba6f5cb Compare January 11, 2023 19:57

peternied requested review from anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, gbbafna, setiah, kartg, kotwanikunal, mch2, nknize, owaiskazi19, adnapibar, Rishikesh1159, ryanbogan, saratvemulapalli, shwetathareja, dreamer-89, tlfeng, VachaShah and xuezhou25 as code owners January 11, 2023 19:57

peternied added 2 commits January 11, 2023 19:58

Revert unneeded changse

2237639

Signed-off-by: Peter Nied <[email protected]>

Add permissions checks with logging

6e0eb0a

Signed-off-by: Peter Nied <[email protected]>

DarshitChanpura reviewed Jan 11, 2023

View reviewed changes

Fix basic build issue

18eea2e

Signed-off-by: Peter Nied <[email protected]>

Remove licenses that are no longer needed

b595373

Signed-off-by: Peter Nied <[email protected]>

Fix other licenses

5845848

Signed-off-by: Peter Nied <[email protected]>

Fix other licenses

972e2ea

Signed-off-by: Peter Nied <[email protected]>

Fix other licenses

4e1e4a7

Signed-off-by: Peter Nied <[email protected]>

Fix other licenses

bd0e0f8

Signed-off-by: Peter Nied <[email protected]>

peternied closed this Jan 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

peternied commented Nov 4, 2022

github-actions bot commented Nov 4, 2022

cwperks left a comment

cwperks Nov 8, 2022

DarshitChanpura Nov 15, 2022

peternied Jan 11, 2023

cwperks Nov 8, 2022

peternied Nov 8, 2022

DarshitChanpura Nov 15, 2022

peternied Jan 11, 2023

DarshitChanpura left a comment

DarshitChanpura Nov 15, 2022

peternied Nov 17, 2022

DarshitChanpura Nov 15, 2022

DarshitChanpura Nov 15, 2022

DarshitChanpura Nov 15, 2022

DarshitChanpura Jan 11, 2023 •

edited

Loading

peternied Jan 11, 2023

DarshitChanpura Jan 12, 2023

DarshitChanpura Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 11, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 12, 2023

github-actions bot commented Jan 13, 2023

peternied commented Jan 31, 2023

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

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

Conversation

peternied commented Nov 4, 2022

Description

Issues Resolved

Check List

github-actions bot commented Nov 4, 2022

Gradle Check (Jenkins) Run Completed with:

cwperks 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarshitChanpura 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

Choose a reason for hiding this comment

DarshitChanpura Jan 11, 2023 • 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

github-actions bot commented Jan 11, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 11, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 11, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 11, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 12, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 12, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 12, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 12, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 12, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 13, 2023

Gradle Check (Jenkins) Run Completed with:

peternied commented Jan 31, 2023

DarshitChanpura Jan 11, 2023 •

edited

Loading