-
Notifications
You must be signed in to change notification settings - Fork 340
Description
Problem Statement
What syntax should extension permissions have and how should OpenSearch interact with them?
Action items decided on this issue:
- What syntax should be used?
- What parsing method should be used?
This is the least important design decision when dealing with permissions for extensions.
Permission syntax and parsing is the least important consideration when designing permissions for extensions. It is always possible to use the same permission syntax that OpenSearch does today. Namely, permissions follow a <action_type>:<action>/<filter> pattern. For example, indices:data/read/field_caps. A permission can be either index- or cluster-level. An index-level permission can be granted granularly. The index-level permissions allow for an optional resource pattern. For example, indices:admin/aliases can take an optional filter where the target resources are specified after the aliases action. Cluster-level permissions do not allow for this fine-tuning and can only be granted or not.
Because the existing syntax can be re-used, and there is already a system in place for parsing user permissions in this form, this topic should be revisited last.
$\textcolor{teal}{\textsf{What syntax should permissions have?}}$
When looking at permission syntax, there are three main options that come to mind.
I am in favor of redesigning permission syntax. While there is nothing inherently wrong with the current syntax, I would be interested in seeing permissions defined with either or <action_name>?param1=value1¶m2=value2.
- The first option for permission syntax is
<action_type>.<action>.<resource_pattern>. This option takes a similar approach to delimiting the permission string as the existing permission syntax does today. It improves upon the existing version by being more human-readable and simplifying the parsing logic. When looking at the current permission syntax, there is not a clear logic behind the delimiters. While the separation is helpful for parsing, the different characters used to split the string makes the code complicated and the output hard to read. This option shifts to a easier to read format with periods being used to separate each segment from the others.
| Pros | Cons |
|---|---|
| Easier to read | Have to resolve legacy format behind the scenes |
| Parsing logic can be largely reused | Superficial change |
| Simplifies code | Eventual deprecation can be a hassle |
What do code changes look like?
For either syntax change, it will be easiest to implement the new formatting by notifying extension makers of the change (so that they design their permissions in the expected format) and resolving both the legacy and new formats inside the PrivilegesEvaluator.java class of the Security Plugin. The class makes use of pattern matching as well as explicit string comparison, so the updated syntax will need to added to the appropriate location. If the legacy syntax is removed from the class, any legacy permissions will need to be converted to the new format prior to evaluation.
- The second option was suggested by @cwperks. It is just as complex as the current syntax but has the benefit of being in the same format as a URL. This would let us use some URL parsing libraries to process the strings for us. For example, the permission
indices/data/read/search?index-pattern=my-index*&index-pattern=other-index*could be parsed using a URL parser. Note that the:betweenindicesanddatahas been switched to a/.
Using a / makes more sense then the arbitrary : delimiter being used now. In a URL, the : is used to separate the scheme from the hostname, but it does not seem like it has any real purpose in the current syntax.
What do code changes look like?
See above code changes comment.
| Pros | Cons |
|---|---|
| Parsable by URL libraries | Still hard to read |
| Implicit meaning to each section | Not clear why we want the strings to be parsable by URL parsers |
| Using a library simplifies code | Eventual deprecation can be a hassle |
-
$\textcolor{lime}{\textsf{Keep the existing syntax.}}$ The final choice would be leaving the syntax the same. This option is safe since we already have the syntax and do not require any modifications. We can simply port the existing model over onto extensions with extensions standing-in for users. This option is best if we cannot make any syntactical changes or if we are only concerned with the fastest option.
| Pros | Cons |
|---|---|
| Fast and easy | Hard to read syntax |
| Avoids deprecation | Delimiters are arbitrary |
| Already parse user permissions so just need to convert over to extensions | May lock-into a bad system |
What do code changes look like?
There should not be any code changes outside of adding an 'extension' path to the permissions. In core, this will require adding the new actions for the extensions (or handling their injection). In the Security Plugin, changes can just be made to the PrivilegesEvaluator with the newly added action format.
$\textcolor{teal}{\textsf{How should permissions be parsed?}}$
This question should be addressed by the answer of the previous question. If the current permission syntax is used, then it makes the most sense to use the existing parsing mechanism as well. If the syntax is swapped to use only . as its delimiter, then modifying the existing parsing code to handle either form makes the most sense. Finally, it a URL format syntax is adopted, using a URL parser from a library makes the most sense.
What do code changes look like?
For handling permission parsing, changes will need to be made based on which syntax option is used. For a change in syntax, parsing logic will need to be added to read from whatever permissions storage is used. The logic should connect from the storage to the PrivilegesEvaluator. In the PrivilegesEvaluator, the permissions should be read and processed similar to how legacy permissions are handled. If no change in syntax is made, changes will only be required in the PrivilegesEvaluator. These changes will need to allow the existing framework to parse the new extension action type.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status