Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Question] What syntax should extension permissions have and how should they be parsed? #2565

Closed
2 tasks done
Tracked by #2596
stephen-crawford opened this issue Mar 17, 2023 · 4 comments
Closed
2 tasks done
Tracked by #2596
Assignees
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@stephen-crawford
Copy link
Contributor

stephen-crawford commented Mar 17, 2023

Problem Statement

What syntax should extension permissions have and how should OpenSearch interact with them?

$\textcolor{orange}{\textsf{Orange text is a header for a decision that is not made }}$
$\textcolor{teal}{\textsf{Teal text is a header for a decision that is made }}$
$\textcolor{violet}{\textsf{Violet text is the recommended option }}$
$\textcolor{lime}{\textsf{Lime text is the chosen option }}$

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&param2=value2.

  1. 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.

  1. 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 : between indices and data has 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
  1. $\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.

$\textcolor{lime}{\textsf{Use the existing system with slight changes}}$

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.

@stephen-crawford stephen-crawford added enhancement New feature or request untriaged Require the attention of the repository maintainers and may need to be prioritized labels Mar 17, 2023
@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Mar 20, 2023
@peternied
Copy link
Member

peternied commented Mar 22, 2023

I think 'permissions for extensions' needs a little refinement. From an administrator of a cluster, when an extension is installed they want to allow/deny the extension to be able to do certain things. We have a kind of language for this in the 'permissions' that can be assigned to roles and I think the kinds of allow/deny actions for extensions might be broader and distinct from how we talk about user scenarios.

  1. There is an idea that has been floating around service account I think that has 'classical' permissions associated with it, the same kinds of granularity as a user as defined by the roles they are mapped. For an administrator's mental model - I think service account permissions look very similar to users.

  2. I think there is another concept that we could flush out here around what an extension can be allowed/denied. E.g. Can the extension read any data at all - as a high-level 'scope / capability / application permission' might be something we should add to give administrators a clear sense of control that is easier to see at a glance. I think this would be ingrained in the installation/registration flow. The extension should be able to hint at what it needs to work, and then it should ask what it actually got so it can provide feedback if it isn't as useful or usable at all.

I think this document could expand on the second concept and the scenarios we want to support / not support for extensions. Maybe the service account fits here or in another issue, what do you think?

@cwperks
Copy link
Member

cwperks commented Mar 22, 2023

@peternied I would be hesitant to introduce another way of defining read/write permissions and instead allow the extension developer to define a set of permissions that would make the extension work best and allow the admin of the cluster to accept the default or modify before accepting.

I see a couple of different ideas that I'd like to separate. 1. Cluster admin needs to be presented with a list of how the extension extensions OpenSearch and the cluster admin can accept or reject, 2: Assuming the cluster admin says yes, then the cluster admin should be empowered to further restrict how the installed extension can interact with the cluster

Imagine the following installation steps:

  1. What does a cluster administrator see when first choosing to install an extension. i.e. For AD this could be:
This extension would like to:

1. Add REST Handlers
2. Run jobs asynchronously on behalf of users
3. Create system indices (.anomaly-detection*)
4. Create roles to use the extension
5. Trigger alerts

Do you want to continue installing the extension (y/n)?
  1. Assuming the cluster admin accepts the prompt above, then I imagine them moving to the next step of the prompt which would be to define how the extension can interact with the cluster and for that I imagine something similar to a roles definition.
For best use of this extension, the extension recommends the following policy:

anomaly_detection_extension_policy:
  description: "Anomaly Detection Extension Policy"
  cluster_permissions:
    - "cluster_monitor"
  index_permissions:
    - index_patterns:
        - "logs-*"
      allowed_actions:
        - "read"

During this prompt the admin would be able to edit the policy before its created and at this stage the service account/role for the account can be created. A service account token could also be generated for extensions that want system indices and the token could be shared with the extension to permit it to use the system index.

  1. There may be some additional setup for TLS before the installation is complete

@stephen-crawford
Copy link
Contributor Author

Hi @peternied, thank you for taking the time to review this document.

For details about service accounts, you can find a discussion question here. Specifically, under "How should extensions be tracked or managed?" I agree that service accounts are very similar to user accounts.

For your second point, I think this is where we differ in view a little. I am not sure that we require a second "type" or permission for extensions. In what I have been doing so far, I have yet to see a scenario where a second type of permission would be required. For the example you provided about reading, I am not sure why the extension could not just provide the explicit permissions it required in the traditional format. Then if the administrator did not want the extension to have that access, they could deny the installation. You can find details about this over here under "How should extensions get starting permissions?"

Let me know if I have not addressed your concerns.

@stephen-crawford
Copy link
Contributor Author

[Closing]

  1. What syntax should be used?
    We will keep the existing permission syntax structure.
  2. What parsing method should be used?
    We will use the existing parsing structure with minimal changes to parse the extension permission type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
Status: Done
Development

No branches or pull requests

3 participants