-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Rbac impl #11016
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
Rbac impl #11016
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 56 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Jackie-Jiang
left a comment
There was a problem hiding this 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
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java
Outdated
Show resolved
Hide resolved
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/auth/RBACAuthorization.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/auth/RBACAuthorization.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/auth/RBACAuthorization.java
Outdated
Show resolved
Hide resolved
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
Outdated
Show resolved
Hide resolved
pinot-broker/src/main/java/org/apache/pinot/broker/api/AccessControl.java
Outdated
Show resolved
Hide resolved
|
Do you have a design doc that can be linked to the PR? Or add more descriptions |
pinot-core/src/main/java/org/apache/pinot/core/auth/RBACAuthUtils.java
Outdated
Show resolved
Hide resolved
pinot-broker/src/main/java/org/apache/pinot/broker/broker/AuthenticationFilter.java
Outdated
Show resolved
Hide resolved
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
Outdated
Show resolved
Hide resolved
8258b8e to
ec7b94a
Compare
9d2db40 to
d18617b
Compare
… RBAC annotations (RBACAuthorization, ManualAuthorization).
…r checking authorization
…, and modified broker and controller request filters to process the annotation and pass the access checks to access control objects.
…ll possible actions
…cessControlFactory implementations
Jackie-Jiang
left a comment
There was a problem hiding this 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.
...n-tests/src/main/java/org/apache/pinot/broker/api/resources/BrokerEchoWithAutoDiscovery.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/auth/RBACAuthUtils.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/auth/Actions.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/auth/Actions.java
Outdated
Show resolved
Hide resolved
...test/java/org/apache/pinot/controller/api/extraresources/PinotDummyExtraRestletResource.java
Outdated
Show resolved
Hide resolved
pinot-broker/src/main/java/org/apache/pinot/broker/broker/AuthenticationFilter.java
Outdated
Show resolved
Hide resolved
pinot-broker/src/main/java/org/apache/pinot/broker/broker/AuthenticationFilter.java
Outdated
Show resolved
Hide resolved
…ication in controller and broker request filters
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly good
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java
Outdated
Show resolved
Hide resolved
...n-tests/src/main/java/org/apache/pinot/broker/api/resources/BrokerEchoWithAutoDiscovery.java
Outdated
Show resolved
Hide resolved
pinot-broker/src/main/java/org/apache/pinot/broker/api/resources/PinotBrokerDebug.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/auth/FineGrainedAuthUtils.java
Outdated
Show resolved
Hide resolved
...ller/src/main/java/org/apache/pinot/controller/api/resources/PinotControllerHealthCheck.java
Outdated
Show resolved
Hide resolved
...n/java/org/apache/pinot/controller/api/resources/PinotInstanceAssignmentRestletResource.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java
Outdated
Show resolved
Hide resolved
...er/src/main/java/org/apache/pinot/controller/api/resources/PinotInstanceRestletResource.java
Outdated
Show resolved
Hide resolved
...ller/src/main/java/org/apache/pinot/controller/api/resources/PinotSchemaRestletResource.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/auth/Actions.java
Outdated
Show resolved
Hide resolved
Jackie-Jiang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise
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:
How is fine grain authorization implemented?
There are 3 attributes required for granular authorization check:
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:
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.
The implementation builds upon existing AccessControl interface for Broker and Controller APIs by adding two methods: