[fix][broker] Fix tenant admin authorization bug. by dragonls · Pull Request #20068 · apache/pulsar

dragonls · 2023-04-11T04:35:30Z

Fixes #20066

Motivation

Fixes the bug that producers/consumers will all disconnect while using tenant admin to produce/consume.

The root cause is that the permission check logic is not the same bewteen org.apache.pulsar.broker.service.ServerCnx and org.apache.pulsar.broker.service.persistent.PersistentTopic#onPoliciesUpdate.

In org.apache.pulsar.broker.service.ServerCnx, while handleProducer and handleSubscribe, the permission check will go to isTopicOperationAllowed and finally be processed by org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider#allowTopicOperationAsync, which will validateTenantAdminAccess. Everything is fine.

In org.apache.pulsar.broker.service.persistent.PersistentTopic#onPoliciesUpdate, it checks the permission by producer.checkPermissionsAsync and consumer.checkPermissionsAsync. Let's take the processing logic of the producer as an example. It will be processed by org.apache.pulsar.broker.authorization.AuthorizationService#canProduceAsync and finally org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider#canProduceAsync. The tenant admin can not pass the check.

Modifications

Update org.apache.pulsar.broker.authorization.AuthorizationProvider#canProduceAsync to org.apache.pulsar.broker.authorization.AuthorizationProvider#allowTopicOperationAsync in org.apache.pulsar.broker.authorization.AuthorizationService#canProduceAsync. So do org.apache.pulsar.broker.authorization.AuthorizationService#canConsumeAsync and org.apache.pulsar.broker.authorization.AuthorizationService#canLookupAsync

Verifying this change

Make sure that the change passes the CI checks.

This change added tests and can be verified as follows:

Added and fixed the test in org.apache.pulsar.broker.auth.AuthorizationTest#simple
Added and fixed the test in org.apache.pulsar.websocket.proxy.ProxyAuthorizationTest

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository: dragonls#7

dragonls · 2023-04-12T07:03:29Z

In fact, there are many places in the current code that check the permission of super user or tenant admin:

org.apache.pulsar.broker.authorization.AuthorizationProvider#isSuperUser in org.apache.pulsar.broker.authorization.AuthorizationService, including functions canProduceAsync, canConsumeAsync, canLookupAsync. No tenant admin check here.
org.apache.pulsar.broker.authorization.AuthorizationProvider#isSuperUserOrAdmin in org.apache.pulsar.broker.authorization.AuthorizationService, including function allowSinkOpsAsync, allowSourceOpsAsync, allowFunctionOpsAsync.
After this PR, org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider#validateTenantAdminAccess in org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider, including functions canProduceAsync, canConsumeAsync. No tenant admin check in allowTheSpecifiedActionOpsAsync.

Maybe we need to uniformly add/update permission check wherever we need. And also remove some reduplicated check, for example:

validateTenantAdminAccess in org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider#allowTopicOperationAsync
isSuperUser in org.apache.pulsar.broker.authorization.AuthorizationService, including functions canProduceAsync, canConsumeAsync, canLookupAsync.

I am just afaid that there are still some authorization bug not found.

What do you think? @Technoboy- @nodece

nodece · 2023-04-12T08:42:44Z

I know these cases, I want to deprecate the canProduceAsync, canConsumeAsync, and canLookupAsync, use the allowTopicOperationAsync instead.

dragonls · 2023-04-12T08:55:02Z

I know these cases, I want to deprecate the canProduceAsync, canConsumeAsync, and canLookupAsync, use the allowTopicOperationAsync instead.

Sounds good. I can do this after this PR merged.

Technoboy- · 2023-04-12T09:08:04Z

I know these cases, I want to deprecate the canProduceAsync, canConsumeAsync, and canLookupAsync, use the allowTopicOperationAsync instead.

Sounds good. I can do this after this PR merged.

Yes, this will keep the logic the same. But we'd better discuss it in the dev mail list first.

dragonls · 2023-04-12T09:14:44Z

Make sense. I also find some function in org.apache.pulsar.broker.authorization.AuthorizationService marked deprecated.

Maybe we can do the same thing.

nodece · 2023-04-12T09:28:30Z

Thank @dragonls for your contribution, I made #19461 to that before.

dragonls · 2023-04-12T12:04:04Z

/pulsarbot rerun-failure-checks

codecov-commenter · 2023-04-12T14:07:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.96%. Comparing base (dc1cdac) to head (44bf9fe).
⚠️ Report is 2644 commits behind head on master.

Additional details and impacted files

@@              Coverage Diff              @@
##             master   #20068       +/-   ##
=============================================
+ Coverage     24.26%   72.96%   +48.69%     
- Complexity      294    31823    +31529     
=============================================
  Files          1609     1868      +259     
  Lines        125669   138332    +12663     
  Branches      13707    15220     +1513     
=============================================
+ Hits          30490   100930    +70440     
+ Misses        90689    29390    -61299     
- Partials       4490     8012     +3522

Flag	Coverage Δ
inttests	`24.13% <0.00%> (-0.13%)`	⬇️
systests	`24.78% <0.00%> (?)`
unittests	`72.25% <100.00%> (?)`

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

Files with missing lines	Coverage Δ
...sar/broker/authorization/AuthorizationService.java	`59.75% <100.00%> (+54.55%)`	⬆️

... and 1590 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

michaeljmarshall

I don't think we want to change the canProduceAsync and canConsumeAsync methods. Instead, we can change the calling code. Let me know what you think.

nodece

~~I expect the test to fail. You also need to modify the org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider#allowTopicOperationAsync to avoid a dead loop.~~

@dragonls, Sorry for providing an incorrect review.

dragonls · 2023-04-17T12:59:50Z

I expect the test to fail. You also need to modify the org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider#allowTopicOperationAsync to avoid a dead loop.

I didn't find dead loop. Can you be more specific?

nodece · 2023-04-17T15:42:24Z

I expect the test to fail. You also need to modify the org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider#allowTopicOperationAsync to avoid a dead loop.

I didn't find dead loop. Can you be more specific?

Sorry that I provide an incorrect review.

nodece

LGTM

nodece · 2023-04-18T14:16:50Z

/pulsarbot rerun-failure-checks

Updated

nodece · 2023-04-19T15:40:19Z

Thank @dragonls for your contribution!

…)" This reverts commit fc17c1d.

michaeljmarshall · 2023-04-19T16:12:47Z

@nodece @dragonls - I don't think we should have taken this direction with the PR. I propose an alternative here: #20142.

…ache#20068)"" This reverts commit bc3346a.

…0068)" This reverts commit fc17c1d.

…20142) Fixes: #20066 ### Motivation In #20068 we changed the way that the `AuthorizationService` is implemented. I think we should change the `Consumer` and the `Producer` logic to call the correct `AuthorizationService` method. Given that our goal is to deprecate the `AuthorizationService` methods for `canProduce` and `canConsume`, this change helps us move in the right direction. ### Modifications * Update `Producer` and `Consumer` in broker to call the `AuthorizationService#allowTopicOperationAsync` method. ### Verifying this change This change is trivial. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: Skipping PR as I ran tests locally.

…#20143) This reverts commit fc17c1d. ### Motivation In #20068 we changed the way that the `AuthorizationService` is implemented. I think this approach could have unintended consequences. Instead, I think we should change the `Consumer` and the `Producer` logic to call the correct `AuthorizationService` method. I propose an update to the `Consumer` and `Producer` logic here #20142. Given that our goal is to deprecate the `AuthorizationService` methods for `canProduce` and `canConsume`, I think we should not change their implementations. ### Modifications * Revert #20068 ### Verifying this change This change is trivial. It removes certain test changes that were only made to make the previous PR work. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: Skipping PR as I ran tests locally.

dragonls · 2023-04-20T06:29:34Z

@nodece @dragonls - I don't think we should have taken this direction with the PR. I propose an alternative here: #20142.

OK. I looked into all relative discussion, the right direction seems to be:

All superUser/admin check should be moved into AuthorizationService, which keep AuthorizationProvider clean and more focused on the inner check.
Outer callers, such as Producer/Consumer, shoud call AuthorizationService.allowTopicOperationAsync instead of canProduceAsync/canConsumerAsync/canLookupAsync. The canProduceAsync/canConsumerAsync/canLookupAsync in AuthorizationProvider might be deprecated in the future.

poorbarcode · 2023-05-07T23:03:27Z

@michaeljmarshall

To avoid conflicts, I cherry-picked all three patches into branch 3.1.

Co-authored-by: druidliu <druidliu@tencent.com> (cherry picked from commit fc17c1d)

…20142) Fixes: #20066 ### Motivation In #20068 we changed the way that the `AuthorizationService` is implemented. I think we should change the `Consumer` and the `Producer` logic to call the correct `AuthorizationService` method. Given that our goal is to deprecate the `AuthorizationService` methods for `canProduce` and `canConsume`, this change helps us move in the right direction. ### Modifications * Update `Producer` and `Consumer` in broker to call the `AuthorizationService#allowTopicOperationAsync` method. ### Verifying this change This change is trivial. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: Skipping PR as I ran tests locally. (cherry picked from commit dc5e497)

…#20143) This reverts commit fc17c1d. ### Motivation In #20068 we changed the way that the `AuthorizationService` is implemented. I think this approach could have unintended consequences. Instead, I think we should change the `Consumer` and the `Producer` logic to call the correct `AuthorizationService` method. I propose an update to the `Consumer` and `Producer` logic here #20142. Given that our goal is to deprecate the `AuthorizationService` methods for `canProduce` and `canConsume`, I think we should not change their implementations. ### Modifications * Revert #20068 ### Verifying this change This change is trivial. It removes certain test changes that were only made to make the previous PR work. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: Skipping PR as I ran tests locally. (cherry picked from commit 00dc7a0)

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Apr 11, 2023

[fix][broker] Fix tenant admin authorization bug.

78d265f

dragonls force-pushed the master-fix-tenant-admin-auth branch from 093e668 to 78d265f Compare April 11, 2023 12:48

nodece reviewed Apr 11, 2023

View reviewed changes

Comment thread ...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated

Technoboy- reviewed Apr 12, 2023

View reviewed changes

Comment thread ...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated

druidliu added 2 commits April 12, 2023 11:43

Optimize.

65c7c40

Fix test cases.

2895b67

nodece assigned dragonls Apr 12, 2023

nodece added the ready-to-test label Apr 12, 2023

Technoboy- closed this Apr 12, 2023

Technoboy- reopened this Apr 12, 2023

michaeljmarshall previously requested changes Apr 12, 2023

View reviewed changes

Comment thread ...common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java Outdated

Optimize.

a0caa98

nodece requested changes Apr 17, 2023

View reviewed changes

Fix test error.

6d4052e

dragonls requested a review from michaeljmarshall April 18, 2023 07:13

Fix AuthorizationProducerConsumerTest.

44bf9fe

nodece approved these changes Apr 18, 2023

View reviewed changes

nodece requested a review from Technoboy- April 18, 2023 09:36

nodece merged commit fc17c1d into apache:master Apr 19, 2023

nodece added this to the 3.1.0 milestone Apr 19, 2023

michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Apr 19, 2023

Revert "[fix][broker] Fix tenant admin authorization bug. (apache#20068…

bc3346a

…)" This reverts commit fc17c1d.

michaeljmarshall mentioned this pull request Apr 19, 2023

[fix][broker] Producer/Consumer should call allowTopicOperationAsync #20142

Merged

1 task

michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Apr 19, 2023

Revert "Revert "[fix][broker] Fix tenant admin authorization bug. (ap…

f757419

…ache#20068)"" This reverts commit bc3346a.

michaeljmarshall added a commit to michaeljmarshall/pulsar that referenced this pull request Apr 19, 2023

[revert] "[fix][broker] Fix tenant admin authorization bug. (apache#2…

6c85d15

…0068)" This reverts commit fc17c1d.

michaeljmarshall mentioned this pull request Apr 19, 2023

[revert] "[fix][broker] Fix tenant admin authorization bug. (#20068)" #20143

Merged

1 task

dragonls mentioned this pull request Apr 20, 2023

[fix][broker] Fix auth test cases. #20151

Open

15 tasks

dragonls deleted the master-fix-tenant-admin-auth branch April 20, 2023 08:56

poorbarcode added release/3.0.1 cherry-picked/branch-3.0 labels May 7, 2023

poorbarcode pushed a commit that referenced this pull request May 7, 2023

[fix][broker] Fix tenant admin authorization bug. (#20068)

4c5f9d2

Co-authored-by: druidliu <druidliu@tencent.com> (cherry picked from commit fc17c1d)

RobertIndie added the area/broker label Aug 7, 2023

Conversation

dragonls commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

Uh oh!

Uh oh!

Uh oh!

dragonls commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodece commented Apr 12, 2023

Uh oh!

dragonls commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Technoboy- commented Apr 12, 2023

Uh oh!

dragonls commented Apr 12, 2023

Uh oh!

nodece commented Apr 12, 2023

Uh oh!

dragonls commented Apr 12, 2023

Uh oh!

codecov-commenter commented Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

michaeljmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodece left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dragonls commented Apr 17, 2023

Uh oh!

nodece commented Apr 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodece left a comment

Choose a reason for hiding this comment

Uh oh!

nodece commented Apr 18, 2023

Uh oh!

nodece commented Apr 19, 2023

Uh oh!

michaeljmarshall commented Apr 19, 2023

Uh oh!

dragonls commented Apr 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poorbarcode commented May 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dragonls commented Apr 11, 2023 •

edited

Loading

dragonls commented Apr 12, 2023 •

edited

Loading

dragonls commented Apr 12, 2023 •

edited

Loading

codecov-commenter commented Apr 12, 2023 •

edited

Loading

nodece left a comment •

edited

Loading

nodece commented Apr 17, 2023 •

edited

Loading

dragonls commented Apr 20, 2023 •

edited

Loading

poorbarcode commented May 7, 2023 •

edited

Loading