You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/01/13 20:35:55 UTC
[GitHub] [incubator-pinot] sajjad-moradi opened a new pull request #6440: Add authentication for write endpoints of Controller resources
sajjad-moradi opened a new pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440
## Description
- This PR modifies `AccessControl` interface to add methods for checking if the client has permission for Controller endpoints which modifies the state of the system either by modifying metadata or the actual data segments.
- Through the changes introduced in this PR, a malicious user can be prevented to change the state of the system. This will also prevents mistakes from normal users by constraining them to only modify the tables they own.
- The two new methods default to true for backward compatibility.
## Testing Done
Deployed locally and used a mock implementation of AccessControl interface. Some of the write endpoints like uploading segments and creating tenant, schema, and table were fully tested. All the other write endpoints were only tested to see if the injection of HttpHeaders actually happens and the validation part works properly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#discussion_r564035596
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -492,7 +496,9 @@ public Response startReplaceSegments(
public Response endReplaceSegments(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
@ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
- @ApiParam(value = "Segment lineage entry id returned by startReplaceSegments API") @QueryParam("segmentLineageEntryId") String segmentLineageEntryId) {
+ @ApiParam(value = "Segment lineage entry id returned by startReplaceSegments API") @QueryParam("segmentLineageEntryId") String segmentLineageEntryId,
+ @Context HttpHeaders httpHeaders) {
+ AccessControlUtils.validateWritePermission(httpHeaders, tableName, _accessControlFactory, LOGGER);
Review comment:
I understand semantically it's fine if we don't validate here, but to be consistent with other write endpoint maybe it's better to have access control here as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#discussion_r563012906
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -492,7 +496,9 @@ public Response startReplaceSegments(
public Response endReplaceSegments(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
@ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
- @ApiParam(value = "Segment lineage entry id returned by startReplaceSegments API") @QueryParam("segmentLineageEntryId") String segmentLineageEntryId) {
+ @ApiParam(value = "Segment lineage entry id returned by startReplaceSegments API") @QueryParam("segmentLineageEntryId") String segmentLineageEntryId,
+ @Context HttpHeaders httpHeaders) {
+ AccessControlUtils.validateWritePermission(httpHeaders, tableName, _accessControlFactory, LOGGER);
Review comment:
If we validated startReplacements(), I guess we may not need to validate endReplacement(), but it may not hurt I suppose.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-759753751
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=h1) Report
> Merging [#6440](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=desc) (8ab2c99) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.62%`.
> The diff coverage is `59.96%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6440 +/- ##
==========================================
- Coverage 66.44% 64.82% -1.63%
==========================================
Files 1075 1337 +262
Lines 54773 65681 +10908
Branches 8168 9578 +1410
==========================================
+ Hits 36396 42578 +6182
- Misses 15700 20055 +4355
- Partials 2677 3048 +371
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `64.82% <59.96%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <ø> (+9.52%)` | :arrow_up: |
| [...n/java/org/apache/pinot/client/BrokerResponse.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Jyb2tlclJlc3BvbnNlLmphdmE=) | `100.00% <ø> (ø)` | |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <ø> (-13.29%)` | :arrow_down: |
| [...n/java/org/apache/pinot/client/ExecutionStats.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0V4ZWN1dGlvblN0YXRzLmphdmE=) | `15.55% <ø> (ø)` | |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <ø> (-51.10%)` | :arrow_down: |
| [...n/java/org/apache/pinot/client/ResultSetGroup.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L1Jlc3VsdFNldEdyb3VwLmphdmE=) | `65.38% <ø> (+0.16%)` | :arrow_up: |
| ... and [1188 more](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=footer). Last update [bacaed2...8ab2c99](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io commented on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-759753751
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=h1) Report
> Merging [#6440](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=desc) (629f62e) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `22.34%`.
> The diff coverage is `38.41%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6440 +/- ##
===========================================
- Coverage 66.44% 44.10% -22.35%
===========================================
Files 1075 1322 +247
Lines 54773 64535 +9762
Branches 8168 9408 +1240
===========================================
- Hits 36396 28465 -7931
- Misses 15700 33703 +18003
+ Partials 2677 2367 -310
```
| Flag | Coverage Δ | |
|---|---|---|
| integration | `44.10% <38.41%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `100.00% <ø> (ø)` | |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `52.83% <0.00%> (-13.84%)` | :arrow_down: |
| [...org/apache/pinot/broker/queryquota/HitCounter.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9IaXRDb3VudGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...che/pinot/broker/queryquota/MaxHitRateTracker.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9NYXhIaXRSYXRlVHJhY2tlci5qYXZh) | `0.00% <0.00%> (ø)` | |
| [...ache/pinot/broker/queryquota/QueryQuotaEntity.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcXVlcnlxdW90YS9RdWVyeVF1b3RhRW50aXR5LmphdmE=) | `0.00% <0.00%> (-50.00%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ceselector/StrictReplicaGroupInstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL1N0cmljdFJlcGxpY2FHcm91cEluc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/TimeSegmentPruner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL1RpbWVTZWdtZW50UHJ1bmVyLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...roker/routing/segmentpruner/interval/Interval.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsLmphdmE=) | `0.00% <0.00%> (ø)` | |
| [...r/routing/segmentpruner/interval/IntervalTree.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9zZWdtZW50cHJ1bmVyL2ludGVydmFsL0ludGVydmFsVHJlZS5qYXZh) | `0.00% <0.00%> (ø)` | |
| ... and [1307 more](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=footer). Last update [3d24302...629f62e](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-765860188
> > So, we are still making the assumption that the client credentials are sent via http headers. I suppose that is OK, but maybe we should also add an interface that passes the client's identity (as received via https auth) to the auth interface we have? We do have https now.
>
> You mean mTLS identity? Authentication should be decoupled from protocol. Fair point to not assume http headers assumption.
Agreed about separating authentication and protocol, but you cannot do much authentication sending cleartext stuff back and forth. Anyone can impersonate easily. Even with https, if we have header based auth, it is easy to just insert some header and do what one wants to do.
In these cases, it translates to very weak (if any) authentication.
We used this as a first round in trusted environments. With cloud based deployments in place, I am not sure if this holds true all the time
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#discussion_r564806601
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,35 @@
/**
* Return whether the client has data access to the given table.
*
+ * Note: This method is only used for read access. It will be deprecated soon and its usage will be replaced by
+ * `hasAccess` method with AccessType.READ.
+ *
* @param httpHeaders Http headers
* @param tableName Name of the table to be accessed
* @return Whether the client has data access to the table
*/
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has permission to the given table
+ *
+ * @param accessType access type
+ * @param httpHeaders HTTP headers
+ * @param tableName Name of the table to be accessed
+ * @return whether the client has permission
+ */
+ default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String tableName) {
+ return true;
+ }
+
+ /**
+ * Return whether the client has permission to access the endpoints which are not table level
Review comment:
Sounds reasonable. Also adding the endpoint name will probably be helpful as it can be logged in AccessControl implementation.
@kishoreg since you originally proposed to use AccessType, what do you think of Subbu's suggestion?
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,35 @@
/**
* Return whether the client has data access to the given table.
*
+ * Note: This method is only used for read access. It will be deprecated soon and its usage will be replaced by
Review comment:
Will do.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] kishoreg commented on a change in pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
kishoreg commented on a change in pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#discussion_r557049026
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -35,4 +35,25 @@
* @return Whether the client has data access to the table
*/
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has write permission to the given table
+ *
+ * @param httpHeaders http headers
+ * @param tableName name of the table to be accessed
+ * @return whether the client has write permission
+ */
+ default boolean hasWritePermission(HttpHeaders httpHeaders, String tableName) {
Review comment:
can we do hasAccess(AccessType, HttpHeaders httpHeaders, String tableName) this will be more flexible and we can add more types later without having to change the interface
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#discussion_r564991028
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessType.java
##########
@@ -0,0 +1,24 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+public enum AccessType {
+ READ, WRITE
Review comment:
This goes with the other comment of passing the endpoint URI and the operation.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-759930512
> Great feature addition! This is long time due for Pinot. Thanks for investing your time on this area.
>
> Out of curiosity, have you considered implementing authorization declaratively at a servlet filter level rather than on each individual endpoints? Authorization and authentication being an orthogonal concern, I feel it would be less intrusive in the code base if basic CRUD rules could be defined across all endpoints rather than requiring a specific implementation on every endpoint.
At first I actually did consider declarative approach. I did a small POC in which I defined an AuthFilter and registered it to get called before execution of all endpoints. Then I added an annotation for the write endpoints and in the AuthFilter only performed authentication for the methods having the new annotation. That way adding authentication, which is a separate concern, was less intrusive and was all addressed in one place - AuthFilter class.
The reason I couldn't continue that route was that some of the endpoints are table specific and for them, table name is an input to the authentication procedure. Users need to be able to modify/delete their tables, but not other people's table. The problem here is that table name is a runtime parameter and in some methods it's a path parameter (or api parameter) and for some other methods like adding schema it's deep inside the json body of the post request. For these endpoints, extracting table name needs to happen within the endpoint and not doable in the AuthFilter. That's why I abandoned the aspect oriented way of defining annotation and AuthFilter and instead used the current approach which is more verbose and not that pretty.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] codecov-io edited a comment on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-759753751
# [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=h1) Report
> Merging [#6440](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=desc) (8578da4) into [master](https://codecov.io/gh/apache/incubator-pinot/commit/1beaab59b73f26c4e35f3b9bc856b03806cddf5a?el=desc) (1beaab5) will **decrease** coverage by `1.41%`.
> The diff coverage is `56.80%`.
[](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6440 +/- ##
==========================================
- Coverage 66.44% 65.03% -1.42%
==========================================
Files 1075 1323 +248
Lines 54773 64537 +9764
Branches 8168 9408 +1240
==========================================
+ Hits 36396 41974 +5578
- Misses 15700 19568 +3868
- Partials 2677 2995 +318
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `65.03% <56.80%> (?)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [...e/pinot/broker/api/resources/PinotBrokerDebug.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYXBpL3Jlc291cmNlcy9QaW5vdEJyb2tlckRlYnVnLmphdmE=) | `0.00% <0.00%> (-79.32%)` | :arrow_down: |
| [...ot/broker/broker/AllowAllAccessControlFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL0FsbG93QWxsQWNjZXNzQ29udHJvbEZhY3RvcnkuamF2YQ==) | `71.42% <ø> (-28.58%)` | :arrow_down: |
| [.../helix/BrokerUserDefinedMessageHandlerFactory.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jyb2tlclVzZXJEZWZpbmVkTWVzc2FnZUhhbmRsZXJGYWN0b3J5LmphdmE=) | `33.96% <0.00%> (-32.71%)` | :arrow_down: |
| [...ker/routing/instanceselector/InstanceSelector.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcm91dGluZy9pbnN0YW5jZXNlbGVjdG9yL0luc3RhbmNlU2VsZWN0b3IuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ava/org/apache/pinot/client/AbstractResultSet.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Fic3RyYWN0UmVzdWx0U2V0LmphdmE=) | `66.66% <0.00%> (+9.52%)` | :arrow_up: |
| [.../main/java/org/apache/pinot/client/Connection.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0Nvbm5lY3Rpb24uamF2YQ==) | `35.55% <0.00%> (-13.29%)` | :arrow_down: |
| [...inot/client/JsonAsyncHttpPinotClientTransport.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY2xpZW50cy9waW5vdC1qYXZhLWNsaWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY2xpZW50L0pzb25Bc3luY0h0dHBQaW5vdENsaWVudFRyYW5zcG9ydC5qYXZh) | `10.90% <0.00%> (-51.10%)` | :arrow_down: |
| [...not/common/assignment/InstancePartitionsUtils.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vYXNzaWdubWVudC9JbnN0YW5jZVBhcnRpdGlvbnNVdGlscy5qYXZh) | `73.80% <ø> (+0.63%)` | :arrow_up: |
| [...common/config/tuner/NoOpTableTableConfigTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL05vT3BUYWJsZVRhYmxlQ29uZmlnVHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| [...ot/common/config/tuner/RealTimeAutoIndexTuner.java](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3R1bmVyL1JlYWxUaW1lQXV0b0luZGV4VHVuZXIuamF2YQ==) | `100.00% <ø> (ø)` | |
| ... and [1161 more](https://codecov.io/gh/apache/incubator-pinot/pull/6440/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=footer). Last update [68fbb9c...8578da4](https://codecov.io/gh/apache/incubator-pinot/pull/6440?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] daniellavoie commented on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
daniellavoie commented on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-765859442
> So, we are still making the assumption that the client credentials are sent via http headers. I suppose that is OK, but maybe we should also add an interface that passes the client's identity (as received via https auth) to the auth interface we have? We do have https now.
You mean mTLS identity? Authentication should be decoupled from protocol. Fair point to not assume http headers assumption.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#discussion_r564699735
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,35 @@
/**
* Return whether the client has data access to the given table.
*
+ * Note: This method is only used for read access. It will be deprecated soon and its usage will be replaced by
Review comment:
You mean, this method will be removed soon, right? In that case, you should mark it deprecated, and leave it here for one release.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,35 @@
/**
* Return whether the client has data access to the given table.
*
+ * Note: This method is only used for read access. It will be deprecated soon and its usage will be replaced by
+ * `hasAccess` method with AccessType.READ.
+ *
* @param httpHeaders Http headers
* @param tableName Name of the table to be accessed
* @return Whether the client has data access to the table
*/
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has permission to the given table
+ *
+ * @param accessType access type
+ * @param httpHeaders HTTP headers
+ * @param tableName Name of the table to be accessed
+ * @return whether the client has permission
+ */
+ default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String tableName) {
+ return true;
+ }
+
+ /**
+ * Return whether the client has permission to access the endpoints which are not table level
Review comment:
It may be good to keep this API extensible, is to add the endpoint name (and version) and the operation (CRUD) in the API. The authenticator can then choose to (for example) allow adding a new table, but not modifying any existing table or schema. Or, allow adding segments to a table, but not deleting segments.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessType.java
##########
@@ -0,0 +1,24 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+public enum AccessType {
+ READ, WRITE
Review comment:
Should we have CREATE, READ, UPDATE, DELETE? May keep this extensible for the future.
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -0,0 +1,65 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.controller.api.resources.ControllerApplicationException;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.slf4j.Logger;
+
+
+public class AccessControlUtils {
+
+ public static void validateWritePermission(HttpHeaders httpHeaders, String tableName,
Review comment:
Please add javadocs for these methods. It looks like these are supposed to be used by REST api end points (since throw specific exceptions).
Also, if you can find a way to avoid static methods that will be super helpful. It will be useful to write unit tests where access control returns can be tested.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-767103431
> Don't you also need to change PinotQueryResource?
It already has access control validation under `getQueryResponse` method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#discussion_r564035596
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/resources/PinotSegmentUploadDownloadRestletResource.java
##########
@@ -492,7 +496,9 @@ public Response startReplaceSegments(
public Response endReplaceSegments(
@ApiParam(value = "Name of the table", required = true) @PathParam("tableName") String tableName,
@ApiParam(value = "OFFLINE|REALTIME") @QueryParam("type") String tableTypeStr,
- @ApiParam(value = "Segment lineage entry id returned by startReplaceSegments API") @QueryParam("segmentLineageEntryId") String segmentLineageEntryId) {
+ @ApiParam(value = "Segment lineage entry id returned by startReplaceSegments API") @QueryParam("segmentLineageEntryId") String segmentLineageEntryId,
+ @Context HttpHeaders httpHeaders) {
+ AccessControlUtils.validateWritePermission(httpHeaders, tableName, _accessControlFactory, LOGGER);
Review comment:
I understand semantically it's fine if we don't validate here, but to be consistent with other write endpoint maybe it's better to have access control here as well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-767103431
> Don't you also need to change PinotQueryResource?
It already has access control validation under `getQueryResponse` method.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-770111560
@mcvsubbu @daniellavoie @kishoreg I proposed a declarative approach for access control in a new RP https://github.com/apache/incubator-pinot/pull/6507
I tried to address all comments here. I'm closing this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] mcvsubbu commented on a change in pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
mcvsubbu commented on a change in pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#discussion_r564990881
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -30,9 +30,35 @@
/**
* Return whether the client has data access to the given table.
*
+ * Note: This method is only used for read access. It will be deprecated soon and its usage will be replaced by
+ * `hasAccess` method with AccessType.READ.
+ *
* @param httpHeaders Http headers
* @param tableName Name of the table to be accessed
* @return Whether the client has data access to the table
*/
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has permission to the given table
+ *
+ * @param accessType access type
+ * @param httpHeaders HTTP headers
+ * @param tableName Name of the table to be accessed
+ * @return whether the client has permission
+ */
+ default boolean hasAccess(AccessType accessType, HttpHeaders httpHeaders, String tableName) {
+ return true;
+ }
+
+ /**
+ * Return whether the client has permission to access the endpoints which are not table level
Review comment:
You may want to consider adding the endpoint URI instead of name/version.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#discussion_r564808321
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControlUtils.java
##########
@@ -0,0 +1,65 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pinot.controller.api.access;
+
+import javax.ws.rs.core.HttpHeaders;
+import javax.ws.rs.core.Response;
+import org.apache.pinot.controller.api.resources.ControllerApplicationException;
+import org.apache.pinot.spi.utils.builder.TableNameBuilder;
+import org.slf4j.Logger;
+
+
+public class AccessControlUtils {
+
+ public static void validateWritePermission(HttpHeaders httpHeaders, String tableName,
Review comment:
👍 👍 on your unit test point.
Will do both.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] daniellavoie commented on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
daniellavoie commented on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-765862374
> > > So, we are still making the assumption that the client credentials are sent via http headers. I suppose that is OK, but maybe we should also add an interface that passes the client's identity (as received via https auth) to the auth interface we have? We do have https now.
> >
> >
> > You mean mTLS identity? Authentication should be decoupled from protocol. Fair point to not assume http headers assumption.
>
> Agreed about separating authentication and protocol, but you cannot do much authentication sending cleartext stuff back and forth. Anyone can impersonate easily. Even with https, if we have header based auth, it is easy to just insert some header and do what one wants to do.
>
> In these cases, it translates to very weak (if any) authentication.
>
> We used this as a first round in trusted environments. With cloud based deployments in place, I am not sure if this holds true all the time
Inflight encryption and trusted identity management should not be mixed. Running "secured" authentication does requires a "secured" transport and a trusted identity provider. But I wouldn't discard the value of authentication without a secured transport since from a functional perspective, processing a request under a specific identity is valuable even though the channel is not secured (I'm thinking contextual user base request processing for dev and local use cases). The end game is you need a secured channel of course but these two aren't bound in my opinion. My takeaway is separation of concern and pluggable interfaces is paramount.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] daniellavoie commented on pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
daniellavoie commented on pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#issuecomment-759884459
Great feature addition! This is long time due for Pinot. Thanks for investing your time on this area.
Out of curiosity, have you considered implementing authorization declaratively at a servlet filter level rather than on each individual endpoints? Authorization and authentication being an orthogonal concern, I feel it would be less intrusive in the code base if basic CRUD rules could be defined across all endpoints rather than requiring a specific implementation on every endpoint.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi closed pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
sajjad-moradi closed pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] sajjad-moradi commented on a change in pull request #6440: Add authentication for write endpoints of Controller resources
Posted by GitBox <gi...@apache.org>.
sajjad-moradi commented on a change in pull request #6440:
URL: https://github.com/apache/incubator-pinot/pull/6440#discussion_r557049902
##########
File path: pinot-controller/src/main/java/org/apache/pinot/controller/api/access/AccessControl.java
##########
@@ -35,4 +35,25 @@
* @return Whether the client has data access to the table
*/
boolean hasDataAccess(HttpHeaders httpHeaders, String tableName);
+
+ /**
+ * Return whether the client has write permission to the given table
+ *
+ * @param httpHeaders http headers
+ * @param tableName name of the table to be accessed
+ * @return whether the client has write permission
+ */
+ default boolean hasWritePermission(HttpHeaders httpHeaders, String tableName) {
Review comment:
That's a good idea. I'll refactor.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org