search

opensearch-project / OpenSearch

🔎 Open source distributed and RESTful search engine.
https://opensearch.org/docs/latest/opensearch/index/
Apache License 2.0
9.7k stars 1.8k forks source link

Permissions granting #5858

Open peternied opened 1 year ago

peternied commented 1 year ago

Use case:

There are actions that given a principle can create, retrieve, update, delete the permissions grants it has.

Acceptance Criteria:

stephen-crawford commented 1 year ago

@peternied, @RyanL1997, @DarshitChanpura, and @cwperks, do you have any thoughts about the permission formatting requirements?

I am thinking of two options. We could make it so that you can do basically anything as a valid permission as long as it had the correct deliminator usage and then leave the real checking to the step where we are going to check that the permission matches the required permission. Or we could implement a way of making sure that the permissions are constrained to real permission items using a list of keywords etc. My one concern with the latter is that we would then offer fewer configuration options since if anyone wanted to modify the naming conventions of the permissions for some reason (maybe they wanted their logs to say mycluster.myindex.read they would need to be able to add things to the list of permission keywords. The first option does not require this but will either combine the matching check into part of the formatting check or duplicate the effort (yuck).

What does everyone think? I am leaning towards the second option but maybe there is another way of doing things I am overlooking.

EDIT: Can someone give me access to the Identity Project Board? I cannot assign this to myself for whatever reason even though I can drag the columns around and type in the expected days etc. Thank you!

cwperks commented 1 year ago

Jotting down some initial thoughts.

Action/Permissions should be broken down into 3 major categories:

Index Permissions

For index permissions, I like to draw analogies to SQL though there may not always be a 1-to-1 comparison

SQL Syntax: GRANT <permission> TO <database_principal > ON <table>.

stephen-crawford commented 1 year ago

Hi @cwperks, thank you for sharing this list. It is definitely helpful to see all the different actions we currently support when thinking about how to organize/name permissions in the new model. I think one of the major struggles with this is going to be condensing some of the permission names given the existing actions are so long.

As mentioned above, I had been thinking about something along the lines of .. but when the actions themselves have numerous parts it may not be as straightforward. I set up two interfaces and an abstract class for permissions, their handling and storage over in the linked draft.

peternied commented 1 year ago

https://github.com/opensearch-project/OpenSearch/issues/5889 is tracking name schema / campaigns so you could defer some of this as long as the implementation was flexible with whatever constraints we pick

stephen-crawford commented 1 year ago

5889 is tracking name schema / campaigns so you could defer some of this as long as the implementation was flexible with whatever constraints we pick

That sounds good to me. Right now, I am awaiting some more design feedback and looking at how to generalize the exceptions and logging for the two interfaces. I also need to implement admin checks for the operations.

stephen-crawford commented 1 year ago

This is all done with #6154 except the part that prevents granting a permission to a non-existant principal. Right now, I am not sure how we do that without having the principals all be stored in the cluster. I had thought that this is something we had wanted to avoid. I am in favor of not worrying about whether a principal exists or not when a grant occurs. Perhaps we will need to add a TTL for permissions in storage to flush out unused permissions (make it LRU queue that runs alongside the mapping), but either way I think more discussion is needed before I can implement a principal validation mechanism.

Maybe we can consider this issue resolved and move the principal validation aspect to its own issue?