You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/09/01 05:34:38 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request #11872: [Broker] Add Authz Check Before Completing Policy Operation
michaeljmarshall opened a new pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872
### Motivation
There are several calls in the v2/PeristentTopics endpoint class that are supposed to require authorization but never verify that authorization. These endpoints are authenticated, just not checked against the broker's authorization service.
### Modifications
* Update the `preValidation` method in the `PersistentTopicsBase` class to run the `validatePoliciesReadOnlyAccess()` for all write operations and to run an appropriate `validateTopicPolicyOperation(topicName, policyName, policyOperation)` check for all methods.
* When reading through these changes, please make sure that the `PolicyName` and the `PolicyOperation` make sense for the method.
* Remove the few pre-existing authz checks to prevent duplicate checks for authz.
* Add new `PolicyName` for `MAX_MESSAGE_SIZE`.
### Verifying this change
I verified this bug against a pulsar 2.8.0 broker. The broker authenticates the user, but does not check authorization.
I am not sure of the "right" way to test this change. It appears that we are missing _many_ tests for this part of the code, and even for the underlying `AuthorizationService` and `PulsarAuthorizationProvider` classes. Given the `validateTopicPolicyOperation` method has been in use for a while by a few other methods, this seems like a safe and straightforward change to make. I'd prefer to add more generic tests later, if that's okay.
### Does this pull request potentially affect one of the following parts:
This change will introduce authorization checks where there were none before. It is a correction in the behavior, as these checks were supposed to take place already.
### Documentation
I don't believe any documentation is required, as this only corrects behavior. It would be good to include this fix in the release notes.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #11872: [Broker] Update Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-1058886633
The pr had no activity for 30 days, mark with Stale label.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11872: [Broker] Update Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-929658562
@315157973 and @KannarFr - what is the right path forward here? This PR is at least helpful in adding tests that assert failure when a call is made with insufficient authorization. Do we need some interface implemented or should I close this PR and open a new one with the just the tests? The current implementation actually throws a 401, not a 403 as the annotations indicate, so those might need updating too. Thanks.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11872: [Broker] Update Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-929658562
@315157973 and @KannarFr - what is the right path forward here? This PR is at least helpful in adding tests that assert failure when a call is made with insufficient authorization. Do we need some interface implemented or should I close this PR and open a new one with the just the tests? The current implementation actually throws a 401, not a 403 as the annotations indicate, so those might need updating too. Thanks.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11872: [Broker] Add Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-909913704
@sijie, @merlimat, @codelipenghui, @eolivelli, @KannarFr - PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11872: [Broker] Add Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-909934951
I would add at least end to end tests
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #11872: [Broker] Update Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-931142779
@codelipenghui PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #11872: [Broker] Update Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-911529021
> > It seems that PolicyOperation is not handled in PulsarAuthorizationProvider#allowTopicPolicyOperationAsync, you add these PolicyOperation will eventually be ignored. This feature looks unfinished.
>
> Correct. The default authZ provider is not using it. I don't think it's unfinished: it's just about missing granularity in the pulsar internal permissions management.
>
> FYI [biscuit-pulsar ](https://github.com/CleverCloud/biscuit-pulsar/blob/master/src/main/java/com/clevercloud/biscuitpulsar/AuthorizationProviderBiscuit.java#L460) (an external authN/authZ provider) uses it.
We need a default implementation class
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #11872: [Broker] Update Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-911770413
Sure, but there is currently no permissions management on topics, correct?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #11872: [Broker] Add Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-909934951
I would add at least end to end tests
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] KannarFr commented on pull request #11872: [Broker] Update Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
KannarFr commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-911354125
> It seems that PolicyOperation is not handled in PulsarAuthorizationProvider#allowTopicPolicyOperationAsync, you add these PolicyOperation will eventually be ignored. This feature looks unfinished.
Correct. The default authZ provider is not using it. I don't think it's unfinished: it's just about missing granularity in the pulsar internal permissions management.
FYI [biscuit-pulsar ](https://github.com/CleverCloud/biscuit-pulsar/blob/master/src/main/java/com/clevercloud/biscuitpulsar/AuthorizationProviderBiscuit.java#L460) (an external authN/authZ provider) uses it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11872: [Broker] Add Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-910828509
@gaoran10 and @eolivelli - thank you for your feedback. In writing the tests, I discovered that my initial comments were overstated. There is an authorization check in the `preValidation` method. The check comes when the `getPartitionedTopicMetadataAsync` is called from the `preValidation` method. In `getPartitionedTopicMetadataAsync`, we run the following check:
https://github.com/apache/pulsar/blob/d57c1d29ffeee7a6f17a77e5d198892c6d1cdd27/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/AdminResource.java#L445
This check however does not use the `allowTopicPolicyOperationAsync` method from the `AuthorizationProvider` interface. It instead only verifies that a given role can `lookup` a given topic, and if a role can do that, then they can modify that topic's policies.
It looks like the `allowTopicPolicyOperationAsync` is a recently added method. Now that we have this dedicated method, I think we should use it. Further, note that all of the endpoints I modified are annotated with the following:
```java
@ApiResponse(code = 403, message = "Don't have admin permission")
```
This implies to me that the original design was to only allow modification of the topic policy endpoints by tenant admins, not just roles with produce/consume permission.
Please let me know what you think. Thanks!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11872: [Broker] Add Authz Check Before Completing Topic Level Policy Operation
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11872:
URL: https://github.com/apache/pulsar/pull/11872#issuecomment-909913704
@sijie, @merlimat, @codelipenghui, @eolivelli, @KannarFr - PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org