Skip to content

Conversation

@soumitra-st
Copy link
Contributor

@soumitra-st soumitra-st commented Jun 30, 2023

Support for fine grain authorization in Pinot

This PR adds support for fine grain authorization for all REST APIs in Pinot.

What is fine grain authorization?

Essentially, every API access can be thought of performing some "action" on a specific resource id "targetId" of type "TargetType". Hence before executing the REST API, the implementation validates if the caller has required access to perform the action on the specific resource, and proceeds if has access, else returns 403 error.

How can it benefit Pinot users?

This framework provides ability to give access to perform one action on a specific table, or build higher level user profiles to give access to a set of actions. For instance, one user (or group or users) can be configured to execute SQL queries only, and other user can be allowed to perform admin actions for a specifc table. One such higher level access control in the industry is role-based access control (RBAC). RBAC restricts system access based on a person's role within an organization. Through RBAC, one can enforce granualr as well as broad policies.

Example of roles:

  1. Cluster admin - allowed to perform any action on the cluster
  2. Table admin - allowed to create table, drop table, rebalance etc. on one or a set of tables
  3. Table reader - allowed to run SQL queries on one or a set of tables

How is fine grain authorization implemented?

There are 3 attributes required for granular authorization check:

  1. Action being performed
  2. Id of the resource on which the action is performed
  3. Type of the resource on which the action is performed

Action and type of the resource is known statically for an API. However, the id of the resource (table name) can be in the path or query parameters of the API, or in the payload. For example: "/tables/{tableName}/instancePartitions" API has the tableName in the path parameter, and "/sql" has the table name in JSON payload. There are two annotations used for these scenarios:

  1. Authorize - annotate the APIs to get the targetId from path or query params. The annotation has 3 elements:
    a. targetType - type of the target resource (Cluster or Table)
    b. paramName - path or query parameter to use to get the id or the resource
    c. action - to validate on the specific resource
    Request filter grabs the details from Authorize annotation to perform the validation.
  2. ManualAuthorization - no authorization check by the request filter, and the check is performed in the API implementation once the resource id is available.

The implementation builds upon existing AccessControl interface for Broker and Controller APIs by adding two methods:

  1. hasAccess - to check authorization using target resource type, resource id, and action
  2. defaultAccess - this method is called by request filter if an API is neither annotated with Authorize nor ManualAuthorization to ensure no API calls are going though without any check.

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2023

Codecov Report

Merging #11016 (e629beb) into master (4dd2bb5) will decrease coverage by 0.01%.
Report is 27 commits behind head on master.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #11016      +/-   ##
==========================================
- Coverage    0.11%    0.11%   -0.01%     
==========================================
  Files        2208     2222      +14     
  Lines      118484   119221     +737     
  Branches    17923    18046     +123     
==========================================
  Hits          137      137              
- Misses     118327   119064     +737     
  Partials       20       20              
Flag Coverage Δ
integration1temurin11 0.00% <0.00%> (ø)
integration1temurin17 ?
integration1temurin20 0.00% <0.00%> (ø)
integration2temurin11 ?
integration2temurin17 ?
integration2temurin20 0.00% <0.00%> (?)
unittests1temurin11 0.00% <0.00%> (ø)
unittests1temurin17 0.00% <0.00%> (ø)
unittests1temurin20 ?
unittests2temurin11 0.11% <0.00%> (-0.01%) ⬇️
unittests2temurin17 0.11% <0.00%> (-0.01%) ⬇️
unittests2temurin20 0.11% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...ava/org/apache/pinot/broker/api/AccessControl.java 0.00% <ø> (ø)
...ot/broker/api/resources/PinotBrokerAppConfigs.java 0.00% <ø> (ø)
...e/pinot/broker/api/resources/PinotBrokerDebug.java 0.00% <0.00%> (ø)
...t/broker/api/resources/PinotBrokerHealthCheck.java 0.00% <ø> (ø)
.../pinot/broker/api/resources/PinotBrokerLogger.java 0.00% <ø> (ø)
...pinot/broker/api/resources/PinotBrokerRouting.java 0.00% <ø> (ø)
...pinot/broker/api/resources/PinotClientRequest.java 0.00% <0.00%> (ø)
...ache/pinot/broker/broker/AuthenticationFilter.java 0.00% <0.00%> (ø)
...e/pinot/broker/broker/helix/BaseBrokerStarter.java 0.00% <0.00%> (ø)
...roker/requesthandler/BaseBrokerRequestHandler.java 0.00% <0.00%> (ø)
... and 43 more

... and 56 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also take a look at the failed test

@Jackie-Jiang
Copy link
Contributor

Do you have a design doc that can be linked to the PR? Or add more descriptions

@soumitra-st soumitra-st force-pushed the rbac-impl branch 2 times, most recently from 8258b8e to ec7b94a Compare July 8, 2023 23:50
@soumitra-st soumitra-st force-pushed the rbac-impl branch 4 times, most recently from 9d2db40 to d18617b Compare July 17, 2023 20:32
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good. Let's go over the actions again and make sure they are following some standard format so that we don't need to change them again.

…ication in controller and broker request filters
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly good

Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM otherwise

@Jackie-Jiang Jackie-Jiang merged commit 61dcea6 into apache:master Jul 27, 2023
s0nskar pushed a commit to s0nskar/pinot that referenced this pull request Aug 10, 2023
@Jackie-Jiang Jackie-Jiang added the release-notes Referenced by PRs that need attention when compiling the next release notes label Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature release-notes Referenced by PRs that need attention when compiling the next release notes security

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants