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] Should request be pass through at Transport Layer once it is authorized at REST layer? #2867

Closed
Tracked by #2590
DarshitChanpura opened this issue Jun 15, 2023 · 11 comments
Assignees
Labels
triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@DarshitChanpura
Copy link
Member

DarshitChanpura commented Jun 15, 2023

In the new model, we have moved authorization checks at REST layer. This alludes to a bigger question: "Should the REST authorized requests be passed through at Transport Layer?" and it applies to plugins that require Transport Layer interaction.

This question came up when trying to test an actual plugin endpoint by modifying it use the new route naming model.

Possible Answers:

  1. YES. The request can be marked at "already authorized" and can be marked as pass through by adding a config constant in thread context. (This will happen only for cluster_permissions. Everything )
    By doing so, we eliminate the need to double check permissions. This will allow supporting new permission model, without making any modifications at Transport layer.

  2. NO. The request should still be evaluated at Transport Layer, even though it was marked as already authorized. This will act as a safety net.
    Implementing this requires adding an extra check for permission evaluation at Transport Layer that checks for these new permissions in the list of cluster_permissions defined. This may also slightly affect performance.

@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Jun 15, 2023
@DarshitChanpura
Copy link
Member Author

@cwperks I know you had some thoughts on this. Feel free to edit the description and/or add comments here.

@cwperks
Copy link
Member

cwperks commented Jun 15, 2023

IMO it should be since the request is authorized - there's no need to pass through 2 gates.

Let's say we support a new AD permission-ing scheme where the user can be permitted to use AD either through having a role with the corresponding transport action names or with a new NamedRoute scheme.

Anomaly Read Only role with transport action names:

anomaly_read_access_transport_layer_authz:
  reserved: true
  cluster_permissions:
    - 'cluster:admin/opendistro/ad/detector/info'
    - 'cluster:admin/opendistro/ad/detector/search'
    - 'cluster:admin/opendistro/ad/detectors/get'
    - 'cluster:admin/opendistro/ad/result/search'
    - 'cluster:admin/opendistro/ad/tasks/search'
    - 'cluster:admin/opendistro/ad/detector/validate'
    - 'cluster:admin/opendistro/ad/result/topAnomalies'

vs. the same role, but with a hypothetical new naming scheme:

anomaly_read_access_rest_layer_authz:
  reserved: true
  cluster_permissions:
    - 'ad:detector/info'
    - 'ad:detector/search'
    - 'ad:detectors/get'
    - 'ad:result/search'
    - 'ad:tasks/search'
    - 'ad:detector/validate'
    - 'ad:result/topAnomalies'

Either should work to allow a user to use AD.


I have been mulling over a problem about how to fallback to transport layer authz if rest layer authz fails and here's what I am currently thinking. If REST-layer authz fails I don't think it should fail the request, I think it should move onto transport layer authz to see if the user is permitted to perform the transport action(s) run by the REST handler.

REST Layer authz would be a convenient way for new plugin developers to provide authorization to their API endpoints without necessarily having a transport action.

For plugins that would like to convert to using a new naming scheme they still may want to support the old naming scheme as well so in effect a user either needs to have a cluster permission that matches the route name or a cluster permission that matches the transport action name.


For the flow:

  1. User has access to use API
  • User is mapped to anomaly_read_access_rest_layer_authz
  • User calls on /_plugins/anomaly/detector/_search
  • User passed authz check on REST layer
  • Transport layer authz check is skipped since user has access to the endpoint
  1. User has access to the transport action
  • User is mapped to anomaly_read_access_transport_layer_auth
  • User calls on /_plugins/anomaly/detector/_search
  • User fails authz check on REST layer (do not fail early here)
  • Authz check on transport layer is still run since rest layer authz failed - if user passes then permission is granted

@peternied
Copy link
Member

NO. The request should still be evaluated at Transport Layer, even though it was marked as already authorized. This will act as a safety net.

I vote for no. We can always be less restrictive over time without worry of CVEs, doesn't work the other way around.

There are special cases in the permissions evaluator for all kinds of areas, mostly from core, but I would rather not run fewer permissions checks on the chance we are missing a DLS/FLS check or masking, or a REST action that issues a transport action of a different name. The tradeoff is in latency while potentially performing multiple AuthZ checks and the user will already be cached in the thread context - seems like that should be fast, no?

@davidlago
Copy link

I vote for no.

Having a default pass on a failing authz layer sounds like a bad security design. It should be the opposite... every authz layer is an opportunity to fail calls in different ways, not failing but still passing it along to the next layer.

The small latency tradeoff is one I'm very comfortable paying for in the name of more confidence in our authz checks.

@cwperks
Copy link
Member

cwperks commented Jun 16, 2023

Given the responses here I am going to try going through the plugin dev experience to see if its possible to create an API without a transport action. If APIs are required to have a transport action associated with them then I don't think REST-Layer authz would provide much value since a user would both need to be permissioned to use the REST endpoint and the Transport action. I know its been brought up before why its necessary to write a Transport-action for a REST API Handler and I'm not sure if it is or not. I will try this out today and see if its possible to write a REST handler without a Transport action.

Edit: I believe it is possible and the security plugin is a prime example. See https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java

@stephen-crawford
Copy link
Contributor

[Triage] Thank you for setting up this question @DarshitChanpura. Going to assign this to you.

@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 Jun 19, 2023
@DarshitChanpura
Copy link
Member Author

I believe it is possible and the security plugin is a prime example.

It is possible as an admin to by pass transport layer authz for all admin APIs. If a non-admin user tries to access them, the request would fail.

For this design, rest layer authz will act as a duplicate check for plugins (in-process), and a necessary check for extensions (out-of-process) to allow/prevent user from accessing certain APIs. If we mark all requests authorized from REST layer as pass through we might run into edge cases and will require thorough testing. By adding this on top of existing authz flow, we are adding an additional layer of security at a very small cost of performance.

For transport action permissions defined using the new naming convention <shortName>:<parent-action>/<action> (i.e. ad:detector/info) we'll add an extra check in PrivilegesEvaluator which checks against these to ensure that authz doesn't fail.

@DarshitChanpura
Copy link
Member Author

DarshitChanpura commented Jul 13, 2023

Based on the maintainer inputs: Requests will be evaluated at Transport Layer even if they are authorized at REST Layer.

@peternied peternied reopened this Aug 4, 2023
@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Aug 4, 2023
@peternied
Copy link
Member

There is an outstanding concern for a scenario where the route for a REST request is misconfigured and the request is authorized. In the case we had previously considered performing authorization at the REST layer AND at the Transport layer.

Based on the last comment there has been a change in design. If we are bypassing transport layer how will we handle this conflicting authorization rules?

@cwperks cwperks removed the untriaged Require the attention of the repository maintainers and may need to be prioritized label Aug 7, 2023
@cwperks
Copy link
Member

cwperks commented Aug 7, 2023

@peternied Can you follow-up with action items to mark this issue as closed?

@peternied
Copy link
Member

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.
Projects
None yet
Development

No branches or pull requests

5 participants