[Feature/Identity] Basic Permission Handling #5909

stephen-crawford · 2023-01-17T19:49:12Z

Signed-off-by: Stephen Crawford [email protected]

Description

Adds classes and interfaces to define the basic functionality of permission granting and storage.

The classes include:

Permission which defines what the permission object is
PermissionStore which is an interface that defines the functionality required by a permission storage solution
PermissionStorage which is a basic HashMap implementation of a PermissionStore
PermissionFactory which is used to create Permission instances and check their formatting.

Testing and further functionality (matching) to be added in a follow-up PR.

Issues Resolved

Will eventually resolve: #5858

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: Stephen Crawford <[email protected]>

stephen-crawford · 2023-01-17T19:49:44Z

@opensearch-project/security would love some feedback on this. Thank you!

github-actions · 2023-01-17T20:15:34Z

Gradle Check (Jenkins) Run Completed with:

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

How do we want to define proper formatting for the permissions strings?

Existing permissions from the security plugin are a hierarchy of A.B.C.D.E... which follow the naming scheme of actions. Using this . deiminated system would follow suite, but the actual delimiter could vary, maybe this would create a clear line between old and new, what do you think?

From AWS policy examples [link] they use a colon delimiter in namespaces of permisisons, iam:GetAccountPasswordPolicy.

From Shiro's permissions model [link] : is used separate between the permissions name, the action name, an the resource printer:print:lp7200.

How do we want to access the "correct" permission for use in order to execute the matching/comparison?

Access, could you provide more details about what this means to you? No matter how the permission is stored, e.g. it could be a trie the most important concern is being able to quickly check the permission of a given principal, 'Find'. Insertion/Update/Delete will occur less frequently and can be less efficient if needed.

Do we want to have a permission 'update' option? I am not sure what this would be used for but is there a need for an alternative to delete and add if you would want to turn a permission that was already granted on/off?

How do you think you would 'address' the update? Seems like each permissions representation should have unique identifier like a GUID that could be specifically deleted/modified. Modify can also be supported with a separate delete/create actions, how would you want to call these APIs?

peternied · 2023-01-18T16:02:20Z

sandbox/libs/authn/src/main/java/org/opensearch/authn/internal/InternalSubject.java

@@ -22,8 +25,12 @@
 public class InternalSubject implements Subject {
    private final org.apache.shiro.subject.Subject shiroSubject;

+    public HashMap<String, ArrayList<Permission>> grantedPermissions; // Not sure how we plan to store the permissions


To encapsulate the permissions, can we change this into an instance of the PermissionsHandler class? Maybe permissions storage/store is a more appropriate name

Currently the PermissionHandler is a static class so there is only one instance of it operating on numerous InternalSubjects and providing control of the different permissions each subject may have. This can be switched so that the PermissionHandler is not static and then this attribute could be defined as an instance if that is what you prefer.

peternied · 2023-01-18T16:05:50Z

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

+    /**
+     * Check that the permission required matches the permission available
+     */
+    public static boolean matchPermission(final Permission permission, final String permissionRequired) {


This seems like a properly of a permissions grant, it can match a required permissions, why not have it on the permissions object?

I separated this from the Permission object because I thought it would be best to keep any multi-permission functionality inside the PermissionHandler class. I felt that any instance of a permission should only be aware of itself and not be concerned with logic dealing with multiple permissions or their own correctness outside of syntax. If this function were part of the Permission class it would imply that the permissions were aware of a 'correct' alternative to themselves if they were malformed.

I can swap this to be inside the Permissions class if this is preferred.

peternied · 2023-01-18T16:07:36Z

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

+        try {
+            this.permissionString = permission;
+            this.permissionChunks = permission.split(PERMISSION_DELIMITER);
+            if (!permissionIsValidFormat()) {


This uses side effects to store information into the class and then read it later. Seems unneeded when a validator function could accept a string and accept/reject it

I agree with this analysis, take a read https://softwareengineering.stackexchange.com/questions/15269/why-are-side-effects-considered-evil-in-functional-programming

I can move this outside of the constructor. I think that the valid format call would be best placed outside of the Permission class as a result however. The intention is to make it so that invalid permissions are never fully created. Which is why the formatting check is called inside the constructor since it will throw an exception on the incorrect formatting and then the Permission that is invalid will never be created in the first place.

If we remove this from the constructor I think we should then move the formatting check into the handler class so that the handler could first check that a permission is correctly formatted and then assuming it is create it. It cannot do this if the function stays a method of the permission class since you would need to construct the permission before checking it.

Note; if you don't want to throw an exception in a ctor you could always make the constructor private, but then expose a static createInstance(...) that effectively calls the constructor after verifying the object, or use a builder pattern link

Sounds good.

peternied · 2023-01-18T16:11:37Z

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

+import java.util.HashMap;
+import java.util.Objects;
+
+public class PermissionHandler {


It would help to start with an interface to address what are the inputs and outputs of this system. With the current implementation it isn't as clear if the subject controls the data store or if it is managed by a 'swappable' system. I think the latter would be more flexible in the long term.

That makes sense. I will make an interface and then implement it.

github-actions · 2023-01-18T16:33:41Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9535/
CommitID: ec41e1e
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-18T18:08:08Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9544/
CommitID: 20ef97e
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-18T20:36:20Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9552/
CommitID: 2ecc923
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-18T22:16:44Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9562/
CommitID: 1d984b2
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-19T16:55:18Z

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

@@ -8,12 +8,26 @@

 package org.opensearch.authn;

+/**
+ * A generic interface for what a permission handler should implement


These comments are not very useful, they do not justify why one would use this interface or how it could be implemented in differernt ways

Here are some open ended questions to consider that might help inform your design.

Why/Why not separate permissions where a permission is stored from the subject?

What is the relationship between the actions¹ that are being permissioned vs the resources vs the subject/principals they are assigned to?

When you lookup permissions, subjects can represent multiple principals, when is there relationship projected within the data or dynamically by performing multiple lookups?

How could this interface support multiple storage implementation is it flexible enough for different kinds of usages, e.g. an in-memory LRU cache vs stored in a OpenSearch index

Footnotes

I called this 'name' before which is too generic, action name is more appropriate distinction - welcome to modify this ↩

I feel that the permissions should be stored in association to the operating subject to prevent extremely large instances of storage for clusters with many different subjects. Unless you are attaching a database structure, you would not want some data structure containing a doubly nested mapping of Subject : (Resource: Permission List).

The subjects are granted permissions to perform actions on a resource.

I do not know that we have a strong enough differentiation between a subject and principles at this point to be able to answer this yet. I think I would need further context on the concrete difference between the two and how we want to separate them. Maybe this is my confusion but I am not sure about this yet.

The interface just defines the three required operations you would need for the the handler. So you could implement the interface as pretty much any structure with as long as there is a method for PUT, GET, DELETE of permissions.

I hope this helps give some context to what I am thinking. These are all good questions and I will continue to keep these in mind as we go forward.

…ions class, extend permissions to make permission class Signed-off-by: Stephen Crawford <[email protected]>

stephen-crawford · 2023-01-19T21:15:46Z

I started over from scratch. @opensearch-project/security please let me know what you all think.

I am currently looking at implementing the permission handler but have not yet determined how to resolve the Principal to its targeted Subject. The java object includes a implies(Subject) method but that requires you pass the Subject. A way to use this would be to have a global Subject store but I do not know if there are ramifications of this design that need further discussion.

github-actions · 2023-01-19T21:29:46Z

Gradle Check (Jenkins) Run Completed with:

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

Nice job decomposing the high level spaces, added some comments for your consideration.

peternied · 2023-01-19T21:28:18Z

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

+    /**
+     * This function needs to be able to resolve a java Principal to an associated java Subject and return the associated Subject or throw an error if none corresponds
+     */
+    public Subject resolvePrincipal(Principal principal);


How do you think this would be used? I don't think principals can be turned into subjects without being about to login to the subject, what do you think of this notion?

I am still not convinced on needing this. I put it here just in case though, and was imagining we could basically keep a short-lived subject store and then use that to reverse look up the principals associated with each subject. Then you could use the implies(Subject) method to find which Subject a Principal corresponded with. This would let you expend a bit of time on the front to actually grant the permissions all to a Subject using a single Principal. So all my permissions would be under scrawfor99 instead of some being under that and some being under steecraw for instance. We would need to store a pointer to each Subject however and make OpenSearch manage these. They could of course be any given Principal but you would just want to be able to track when a new Principal was used to refer to the same Subject.

The other option which I prefer is this: Toss the entire resolution step and never resolve a Principal to a Subject. Instead you could just track the permissions granted to principals as Strings or another Object. Then when a Subject tries to take an action you would go through its associated Principals and look for permissions that match what it wants to do to one of its Principals. This is pretty similar to the current Role system in Security and may work nicely.

Toss the entire resolution step and never resolve a Principal to a Subject.

This option has my vote, 👍

peternied · 2023-01-19T21:37:54Z

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

+     * This function grants a permission to a subject when provided the permission and a principal resolvable to a subject
+     * A principal should never be able to grant a permission to itself and implementations should ensure that only valid Subjects are able to execute this operation.
+     */
+    public void putPermission(Permission permission, Principal principal);


When a permission gets added it might be useful to assign the permission an identifier so that delete permissions could delete by identifier rather than the whole permission object, that ID could be returned via this function.

If you don't have the identifier we will need a way to handle granting/removing identical permissions

In REST APIs this often looks like:

Request

POST /.../permission { permission: { permissionString: "opensearch.indexing.index.create", }, principal: "johnny rocket" }

Response

201 CREATED { id: "d2479857-6d7e-449d-b0c5-78d1bbfd1792" permission: { permissionString: "opensearch.indexing.index.create", }, principal: "johnny rocket" }

That sounds good to me!

peternied · 2023-01-19T21:40:49Z

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

+    String[] QUALIFIED_OPERATIONS;
+
+    // An array of the available resources which a permission can grant some operation to act upon.
+    String[] QUALIFIED_RESOURCES;


I really like the notion of having required qualifications, how do you think we can handle enforcing permissions that do not follow this schema?

For enforcing/validating the structure of permissions, I was thinking we would need to make it so that each schema has either a translator to interact with a common API or has their own equivalent. I am not really sure what would be helpful for permissions that are things other than strings. I think this can be made more general once the number and order of parts (see below about the number of parts in the permissions) is decided.

peternied · 2023-01-19T21:43:40Z

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

+     * Assumes that the permission is formatted as <principal>.<resource>.<action>
+     * The principal should already be verified before the permission is created.
+     */
+    public boolean permissionIsValidFormat() {


computationally this string manipulation is kind of expensive, what do you think about changing the storage of the permissionString into its composite parts?

That sounds good to me and would let you do just 1:1 comparison with the same time complexity as the set logic. Great call!

peternied · 2023-01-19T21:46:01Z

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

+/**
+ * An extension of the abstract Permission class which uses String-object Permissions.
+ *
+ * Example "opensearch.indexing.index.create"


Should they always be in 4 parts, want to name each of these parts? Checkin with @shanilpa for his thoughts

I was personally leaning towards 3 parts but I believe the example in the comment is a current way we do actions using the Security plugin. I will check with @shanilpa like you suggest.

…ts both; defines Permission object and extends Signed-off-by: Stephen Crawford <[email protected]>

github-actions · 2023-01-20T21:10:25Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9680/
CommitID: 94de566
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: Stephen Crawford <[email protected]>

github-actions · 2023-01-23T18:02:16Z

Gradle Check (Jenkins) Run Completed with:

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

stephen-crawford · 2023-01-23T20:04:37Z

Update: This is still not the right direction so I will be re-designing things.

Signed-off-by: Stephen Crawford <[email protected]>

github-actions · 2023-01-24T15:06:43Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9817/
CommitID: 000c138
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/PermissionStore.java

peternied · 2023-01-24T19:48:46Z

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

+    abstract void Permission(String permission);
+
+    abstract boolean isValidFormat();


Lets break this apart, create a PermissionFactory, which has a method public Permission createPermission(String(s)...), this method would enforce the rules. This prevents 'new' permissions from deviating from our preferred format, then lets create another method createLegacyPermissions(String) that doesn't enforce the rules so we can easily identify and fix them later. This eliminates the need for a isValidFormat that would only be used on creation.

This allows both paths to be followed, we still get objects that are a Permission, but codifies the old vs new naming scheme.

Permission would have the resource/action, in the follow up PR you could consider if 'match' or other methods would be added that are relevant to the check permissions scenarios.

Gotcha, I will make these changes.

Do you want me to try to add match now or should I wait for another PR? What about test cases etc.? I can combine them or separate things out if you want this to be the abstracts classes/interfaces and have concrete implementations in a follow-up.

Do you want me to try to add match now or should I wait for another PR

I would say wait for the next PR so you can cluster those design elements together in there usage. Your call

Sounds good.

Signed-off-by: Stephen Crawford <[email protected]>

github-actions · 2023-01-24T22:10:53Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9845/
CommitID: 77288e8
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/AbstractPermission.java

peternied

The design is looking really close, just a couple more tweaks

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

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

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

Signed-off-by: Stephen Crawford <[email protected]>

github-actions · 2023-01-25T15:19:27Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/9890/
CommitID: 2034009
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-27T22:52:45Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/10084/
CommitID: 83316c2
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-30T19:14:25Z

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

+     * This function creates a standard permission instance. It includes checking that the permission that is being created
+     * is properly formatted.
+     */
+    public Permission createPermission(String permissionString) {


For a future PR; there is a schema for these new permissions which involves include period delineation action namespace/names and a resource. Rather than only accept a 'string' which's contract is 'you need to understand how the permission is shaped and I'll throw an exception if you got it wrong, instead you could change the interface to make it easier to get the parts right, e.g. the signature could be createPermissions(final String namespace, final String resourceName, final String actionVerb, final string resourceFilter)

peternied · 2023-01-30T19:14:58Z

@scrawfor99 When you are ready for this to be merged let me know and I'll get it into the feature branch

stephen-crawford · 2023-01-30T19:40:30Z

@peternied I think this is good to go. I am working on a different branch based off these changes but it will be a separate PR. Thank you!

Basic Permission Handling

1708158

Signed-off-by: Stephen Crawford <[email protected]>

peternied requested changes Jan 18, 2023

View reviewed changes

stephen-crawford mentioned this pull request Jan 18, 2023

Permissions granting #5858

Open

peternied reviewed Jan 19, 2023

View reviewed changes

stephen-crawford force-pushed the permissions-granting branch from 1d984b2 to 24cb9bd Compare January 19, 2023 21:15

Set up basic interface for PermissionHandler, define abstract permiss…

24cb9bd

…ions class, extend permissions to make permission class Signed-off-by: Stephen Crawford <[email protected]>

peternied reviewed Jan 19, 2023

View reviewed changes

Adds interface for PermissionHandler and PermissionStore and implemen…

94de566

…ts both; defines Permission object and extends Signed-off-by: Stephen Crawford <[email protected]>

Add regex parsing

03b6e6e

Signed-off-by: Stephen Crawford <[email protected]>

Remove permission handler

000c138

Signed-off-by: Stephen Crawford <[email protected]>

peternied reviewed Jan 24, 2023

View reviewed changes

Added permission factory, swap to generic list

77288e8

Signed-off-by: Stephen Crawford <[email protected]>

peternied reviewed Jan 24, 2023

View reviewed changes

sandbox/libs/authn/src/main/java/org/opensearch/authn/AbstractPermission.java Outdated Show resolved Hide resolved

peternied reviewed Jan 24, 2023

View reviewed changes

Change permission naming structure

2034009

Signed-off-by: Stephen Crawford <[email protected]>

peternied reviewed Jan 30, 2023

View reviewed changes

peternied approved these changes Jan 30, 2023

View reviewed changes

peternied marked this pull request as ready for review January 30, 2023 21:35

peternied merged commit c88f550 into opensearch-project:feature/identity Jan 30, 2023

stephen-crawford deleted the permissions-granting branch March 29, 2023 15:16

		abstract void Permission(String permission);

		abstract boolean isValidFormat();

[Feature/Identity] Basic Permission Handling #5909

[Feature/Identity] Basic Permission Handling #5909

Conversation

stephen-crawford commented Jan 17, 2023 • edited Loading

Description

Issues Resolved

Check List

stephen-crawford commented Jan 17, 2023

github-actions bot commented Jan 17, 2023

Gradle Check (Jenkins) Run Completed with:

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

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

stephen-crawford Jan 18, 2023 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 18, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 18, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 18, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 18, 2023

Gradle Check (Jenkins) Run Completed with:

peternied Jan 19, 2023 • edited Loading

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

stephen-crawford commented Jan 19, 2023 • edited Loading

github-actions bot commented Jan 19, 2023

Gradle Check (Jenkins) Run Completed with:

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

Request

Response

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephen-crawford Jan 19, 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

Choose a reason for hiding this comment

github-actions bot commented Jan 20, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 23, 2023

Gradle Check (Jenkins) Run Completed with:

stephen-crawford commented Jan 23, 2023

github-actions bot commented Jan 24, 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

github-actions bot commented Jan 24, 2023

Gradle Check (Jenkins) Run Completed with:

peternied left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 25, 2023

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jan 27, 2023

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

peternied commented Jan 30, 2023

stephen-crawford commented Jan 30, 2023 • edited Loading

stephen-crawford commented Jan 17, 2023 •

edited

Loading

stephen-crawford Jan 18, 2023 •

edited

Loading

peternied Jan 19, 2023 •

edited

Loading

stephen-crawford commented Jan 19, 2023 •

edited

Loading

stephen-crawford Jan 19, 2023 •

edited

Loading

stephen-crawford commented Jan 30, 2023 •

edited

Loading