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

[Feature/Extensions] Implement PrivilegeEvaluator on REST Layer. #2751

Closed
1 task done
Tracked by #2590
DarshitChanpura opened this issue May 9, 2023 · 8 comments
Closed
1 task done
Tracked by #2590
Assignees
Labels
triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. v2.10.0 Issues targeting release v2.10.0

Comments

@DarshitChanpura
Copy link
Member

DarshitChanpura commented May 9, 2023

As part of the new REST API authorization flow, a new PrivilegesEvaluator is required to evaluate a user against request REST route.
This involves:

@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label May 9, 2023
@DarshitChanpura DarshitChanpura added this to the Extensions MVP Support milestone May 9, 2023
@DarshitChanpura DarshitChanpura changed the title Implement PrivilegeEvaluator on REST Layer. [Feature/Extensions] Implement PrivilegeEvaluator on REST Layer. May 9, 2023
@davidlago davidlago moved this to In Progress in Security for Extensions May 10, 2023
@cwperks cwperks 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 May 15, 2023
@cwperks
Copy link
Member

cwperks commented May 15, 2023

[Triage] @willyborankin Tagging you on this issue

@cwperks
Copy link
Member

cwperks commented May 15, 2023

@willyborankin I have a 3 PRs open across Security, core and the SDK to add a notion of a ProtectedRoute which is a named route. The PRs are still open so I'm not sure if the SDK team or core was comfortable with the approach, but it was trying to solve the problem of naming a REST action similar to a transport action to have a name to place in a role definition. Similar to the work you had done around the security APIs. opensearch-project/OpenSearch#6870

@willyborankin
Copy link
Collaborator

willyborankin commented May 16, 2023

I think to add such functionality (it doesn't matter how permissions will look like) no need to add yet another PrivilegesEvaluator. The main thing in this case just avoid of using

  cluster_permissions:
    - "*"

for that I added method:

boolean hasExplicitClusterPermissionPermission(String action);
so together with this method to add permissions check for REST calls you need at least 5 lines of code. @cwperks, @DarshitChanpura wdyt?

@cwperks
Copy link
Member

cwperks commented May 16, 2023

@willyborankin I think it may be necessary to add at least a separate PrivielegesEvaluator.evaluate method because the inputs to the method would be different on the REST-Layer opposed to the Transport-Layer.

On the Transport-Layer the existing PrivelegesEvaluator takes the input:

public PrivilegesEvaluatorResponse evaluate(final User user, String action0, final ActionRequest request, Task task, final Set<String> injectedRoles)

but the Task and ActionRequest would not be available on the REST-Layer. This PR introduces the concept of RestLayerPrivilegesEvaluator which uses a different method signature for evaluation.

public PrivilegesEvaluatorResponse evaluate(final User user, String action0)

It would be great to have a single place for evaluating privileges, but I'm not entirely sure if its possible yet because the ActionRequest is a type of TransportRequest.

The PR linked above works for evaluating privileges on routes mapped to a single name (like a cluster_permission), but does not address how plugins that define index_permissions would migrate to an extension. Since REST Request forwarding to an extension occurs in the REST-Layer the request needs to be blocked before its forwarded inside of SecurityRestFilter.wrap.

One of the goals with extensions is that all existing optional plugins (apart from security and job-scheduler) could be re-written as an extension and use the SDK. That would include supporting ISM and CCR which define index permissions:

index_management_full_access:
  reserved: true
  cluster_permissions:
    - "cluster:admin/opendistro/ism/*"
    - "cluster:admin/opendistro/rollup/*"
    - "cluster:admin/opendistro/transform/*"
    - "cluster:admin/opensearch/notifications/feature/publish"
  index_permissions:
    - index_patterns:
        - '*'
      allowed_actions:
        - 'indices:admin/opensearch/ism/*'

# Allows users to use all cross cluster replication functionality at leader cluster
cross_cluster_replication_leader_full_access:
  reserved: true
  index_permissions:
    - index_patterns:
        - '*'
      allowed_actions:
        - "indices:admin/plugins/replication/index/setup/validate"
        - "indices:data/read/plugins/replication/changes"
        - "indices:data/read/plugins/replication/file_chunk"

@davidlago davidlago added the v2.9.0 v2.9.0 label Jun 28, 2023
@DarshitChanpura DarshitChanpura added v2.10.0 Issues targeting release v2.10.0 and removed v2.9.0 v2.9.0 labels Jul 13, 2023
@DarshitChanpura
Copy link
Member Author

DarshitChanpura commented Jul 13, 2023

@cwperks @willyborankin The current implementation of NamedRoute allows declaring a unique name for a route and has option to test against cluster permissions. For plugins looking to transform to extensions, we have an option to add cluster_permissions as a set of legacy action names on the route. However, for index_permissions I think we have an option to not support as them in REST layer authz, since we are not removing Transport Layer authz (index-permissions are still going to be evaluated at Transport-layer for DLS/FLS and other index related checks). Please let me know if I'm misunderstanding something.

@DarshitChanpura
Copy link
Member Author

Closing this issue. Please feel free to open it if you think it needs more comments.

@github-project-automation github-project-automation bot moved this from In Progress to Done in Security for Extensions Jul 17, 2023
@peternied peternied reopened this Jul 17, 2023
@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Jul 17, 2023
@peternied
Copy link
Member

@DarshitChanpura Can we close those 'unchecked' items in the description before we close this issue?

@peternied peternied removed the untriaged Require the attention of the repository maintainers and may need to be prioritized label Jul 17, 2023
@DarshitChanpura
Copy link
Member Author

Done.

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

No branches or pull requests

5 participants