[improve][broker] Authorize originalPrincipal when provided #19830

michaeljmarshall · 2023-03-16T05:49:01Z

Motivation

In #19455, I made a change to require that only a role in the broker's proxyRoles configuration could provide an original principal. Because this change can break certain installations on patch upgrades, a better temporary solution is to always authorize the originalPrincipal when present and to allow non proxy roles to supply the original principal.

Modifications

Update the AuthorizationService#isValidOriginalPrincipal to allow non proxy roles to supply an original principal. I added a log line in place of the previous failure.
Update all calls to ServiceConfiguration#getProxyRoles() to also check if the original principal is not blank. When it is not blank, we will authorize both roles. Note that this is consistent with the logic in the AuthorizationService#isValidOriginalPrincipal method that is already merged to master.
Update tests to allow for this new behavior.

Verifying this change

I modified existing tests to make this work, and I added a new test to cover mos of the relevant cases where we changed logic.

I plan to copy the new tests to master since they will be valuable there too.

Documentation

doc-required

I will follow up with docs for these proxy changes.

Matching PR in forked repository

PR in forked repository: in order to save time, we'll run tests directly in the pulsar repo.

...broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java

...er-common/src/test/java/org/apache/pulsar/broker/authorization/AuthorizationServiceTest.java

nodece

LGTM

(cherry picked from commit 6bc3530)

(cherry picked from commit 6bc3530) (cherry picked from commit 7f2ec42)

michaeljmarshall · 2023-03-17T16:15:39Z

I'm not sure it really makes sense to add the cherry-picked/branch-2.11 label, but our general workflow is to find PRs that are missing that label and have a release/2.11.x label, so adding the label simplifies the workflow for anyone filtering for Prs that need to be cherry picked.

coderzc · 2023-03-18T11:59:35Z

@michaeljmarshall
The brach 2.9 test failed after cherry-pick this PR. Can you take a look?
https://github.com/coderzc/pulsar/actions/runs/4453791274/jobs/7824321824?pr=38#step:9:903

michaeljmarshall · 2023-03-18T14:09:41Z

@coderzc good catch. The test is actually failing here too. It is in the "flaky test" batch and I didn't take a look. There are several ServerCnxTest tests failing. Taking a look now.

The ServerCnxTest class is run as part of the flaky tests, so #19830 was merged without noticing that the changes didn't actually pass tests. This commit fixes the test by changing the assertions to ensure that "correct" combinations of roles and original principals are verified.

The ServerCnxTest class is run as part of the flaky tests, so #19830 was merged without noticing that the changes didn't actually pass tests. This commit fixes the test by changing the assertions to ensure that "correct" combinations of roles and original principals are verified. (cherry picked from commit 13f4a0d)

michaeljmarshall · 2023-03-18T15:08:02Z

@coderzc - all branches ServerCnxTest classes are now passing.

…ior (#19845) Relates to: #19455 #19830 ### Motivation When I added the requirement for the proxy to use a role in the `proxyRoles` set, I didn't add a test that checked the negative case. This new test was first added in #19830 with one small difference. The goal of this test is to ensure that authorization of the client role and the original role is handled correctly. ### Modifications * Add new test class named `AuthorizationServiceTest`. We use `pass.proxy` and `fail.proxy` as proxy roles to simulate cases where the proxy's role passes and fails authorization, which is always possible. * Add new mock authorization provider named `MockAuthorizationProvider`. The logic is to let any role that starts with `pass` be considered authorized. ### Verifying this change This is a new test. It simply verifies the existing behavior to prevent future regressions. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: Skipping forked test since the new tests pass locally and there are no other modifications.

Anonymitaet · 2023-03-21T07:05:28Z

I will follow up with docs for these proxy changes.

Hi @michaeljmarshall any progress on the doc? Thanks!

…9830) (cherry picked from commit 6bc3530) (cherry picked from commit 7ea5f43)

The ServerCnxTest class is run as part of the flaky tests, so apache#19830 was merged without noticing that the changes didn't actually pass tests. This commit fixes the test by changing the assertions to ensure that "correct" combinations of roles and original principals are verified. (cherry picked from commit 13f4a0d) (cherry picked from commit 45f303c)

poorbarcode · 2023-03-31T16:53:45Z

Hi @michaeljmarshall

I saw that you pushed a commit into branch-2.10 instead of cherry-picking #19830. But there was a mistake that caused branch-master and branch-2.10 to be implemented differently, see: 7ea5f43#diff-73e921d897fa80c701149187144cdd4141c719040775e3b8258c5a8823a08bdcR198, and it makes an issue: The user who has not super role can not operate the tenant API even if authorization is disabled (but authentication is enabled).

You can reproduce this issue like this:

enabled authentication
disabled authorization
call pulsar-admin tenants list with the non-super role.

Same for branch-2.11.

Could you take a look?

michaeljmarshall · 2023-04-01T00:47:51Z

I saw that you pushed a commit into branch-2.10 instead of cherry-picking #19830.

I'm not sure what you mean here. This PR (#19830) was merged to branch-2.11, and the cherry pick was based exactly on this PR's commit (6bc3530).

But there was a mistake that caused branch-master and branch-2.10 to be implemented differently

That was not a mistake. master is implemented differently because it requires that only a proxy role can supply the original principal.

The user who has not super role can not operate the tenant API even if authorization is disabled (but authentication is enabled).

I can see in the code why this is happening. It must just be a case that wasn't covered by any tests. I can try to put up a fix soon.

You can reproduce this issue like this:

* enabled authentication

* disabled authorization

* call `pulsar-admin tenants list` with the non-super role.

You also need to connect through a pulsar proxy for this to actually be an issue because the X-Original-Principal header is the real reason you're having an issue.

Let me know if you have any questions.

michaeljmarshall · 2023-04-01T06:23:28Z

@poorbarcode - this should solve it: #19989. We might want to stop the current 2.10 and 2.11 releases, depending on how important it is for users to run with authn but without authz.

### Motivation In #19455, I added a requirement that only the proxy role could supply an original principal. That check is only supposed to apply when the broker has authorization enabled. However, in one case, that was not the case. This PR does a check and returns early when authorization is not enabled in the broker. See #19830 (comment) for additional motivation. ### Modifications * Update the `PulsarWebResource#validateSuperUserAccessAsync` to only validate when authentication and authorization are enabled in the configuration. ### Verifying this change This is a trivial change. It'd be good to add tests, but I didn't include them here because this is a somewhat urgent fix. There was one test that broke because of this change, so there is at least some existing coverage. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: michaeljmarshall#39

### Motivation In #19455, I added a requirement that only the proxy role could supply an original principal. That check is only supposed to apply when the broker has authorization enabled. However, in one case, that was not the case. This PR does a check and returns early when authorization is not enabled in the broker. See #19830 (comment) for additional motivation. ### Modifications * Update the `PulsarWebResource#validateSuperUserAccessAsync` to only validate when authentication and authorization are enabled in the configuration. ### Verifying this change This is a trivial change. It'd be good to add tests, but I didn't include them here because this is a somewhat urgent fix. There was one test that broke because of this change, so there is at least some existing coverage. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: michaeljmarshall#39 (cherry picked from commit 1a6c28d)

In #19455, I added a requirement that only the proxy role could supply an original principal. That check is only supposed to apply when the broker has authorization enabled. However, in one case, that was not the case. This PR does a check and returns early when authorization is not enabled in the broker. See #19830 (comment) for additional motivation. * Update the `PulsarWebResource#validateSuperUserAccessAsync` to only validate when authentication and authorization are enabled in the configuration. This is a trivial change. It'd be good to add tests, but I didn't include them here because this is a somewhat urgent fix. There was one test that broke because of this change, so there is at least some existing coverage. - [x] `doc-not-needed` PR in forked repository: michaeljmarshall#39 (cherry picked from commit 1a6c28d)

In #19455, I added a requirement that only the proxy role could supply an original principal. That check is only supposed to apply when the broker has authorization enabled. However, in one case, that was not the case. This PR does a check and returns early when authorization is not enabled in the broker. See #19830 (comment) for additional motivation. * Update the `PulsarWebResource#validateSuperUserAccessAsync` to only validate when authentication and authorization are enabled in the configuration. This is a trivial change. It'd be good to add tests, but I didn't include them here because this is a somewhat urgent fix. There was one test that broke because of this change, so there is at least some existing coverage. - [x] `doc-not-needed` PR in forked repository: michaeljmarshall#39 (cherry picked from commit 1a6c28d) (cherry picked from commit 36f0db5)

…pache#19830)" This reverts commit 0c365e2. This commit was made as a concession because some were concerned that it might break implementations. My primary argument was that those implementations were at risk, so they were okay to "break". Therefore, I am reverting this on our fork. It is unlikely for there to be many conflicts because of this diversion.

…ior (apache#19845) Relates to: apache#19455 apache#19830 ### Motivation When I added the requirement for the proxy to use a role in the `proxyRoles` set, I didn't add a test that checked the negative case. This new test was first added in apache#19830 with one small difference. The goal of this test is to ensure that authorization of the client role and the original role is handled correctly. ### Modifications * Add new test class named `AuthorizationServiceTest`. We use `pass.proxy` and `fail.proxy` as proxy roles to simulate cases where the proxy's role passes and fails authorization, which is always possible. * Add new mock authorization provider named `MockAuthorizationProvider`. The logic is to let any role that starts with `pass` be considered authorized. ### Verifying this change This is a new test. It simply verifies the existing behavior to prevent future regressions. ### Documentation - [x] `doc-not-needed` ### Matching PR in forked repository PR in forked repository: Skipping forked test since the new tests pass locally and there are no other modifications. (cherry picked from commit c6de57c)

This reverts commit 6a9a22c.

…#19989) In apache#19455, I added a requirement that only the proxy role could supply an original principal. That check is only supposed to apply when the broker has authorization enabled. However, in one case, that was not the case. This PR does a check and returns early when authorization is not enabled in the broker. See apache#19830 (comment) for additional motivation. * Update the `PulsarWebResource#validateSuperUserAccessAsync` to only validate when authentication and authorization are enabled in the configuration. This is a trivial change. It'd be good to add tests, but I didn't include them here because this is a somewhat urgent fix. There was one test that broke because of this change, so there is at least some existing coverage. - [x] `doc-not-needed` PR in forked repository: michaeljmarshall#39 (cherry picked from commit 1a6c28d) (cherry picked from commit 36f0db5)

[improve][broker] Authorize originalPrincipal when provided

2cdfde6

michaeljmarshall added area/broker doc-required Your PR changes impact docs and you will update later. release/2.8.5 release/2.11.1 ready-to-test release/2.9.5 release/2.10.4 area/authz labels Mar 16, 2023

michaeljmarshall requested review from lhotari, hangc0276, Technoboy-, eolivelli, codelipenghui, nodece and mattisonchao March 16, 2023 05:49

michaeljmarshall self-assigned this Mar 16, 2023

This was referenced Mar 16, 2023

[Branch-2.11] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455) #19820

Closed

[Branch-2.10] Revert [improve][broker] Require authRole is proxyRole to set originalPrincipal(#19455) #19824

Closed

mattisonchao approved these changes Mar 16, 2023

View reviewed changes

Technoboy- approved these changes Mar 16, 2023

View reviewed changes

hangc0276 approved these changes Mar 16, 2023

View reviewed changes

codelipenghui approved these changes Mar 16, 2023

View reviewed changes

coderzc approved these changes Mar 16, 2023

View reviewed changes

mattisonchao reviewed Mar 16, 2023

View reviewed changes

...broker-common/src/main/java/org/apache/pulsar/broker/authorization/AuthorizationService.java Show resolved Hide resolved

nodece reviewed Mar 16, 2023

View reviewed changes

...er-common/src/test/java/org/apache/pulsar/broker/authorization/AuthorizationServiceTest.java Show resolved Hide resolved

michaeljmarshall mentioned this pull request Mar 16, 2023

[improve][broker] Test AuthorizationService to cover proxyRoles behavior #19845

Merged

1 task

eolivelli approved these changes Mar 17, 2023

View reviewed changes

mattisonchao approved these changes Mar 17, 2023

View reviewed changes

nodece approved these changes Mar 17, 2023

View reviewed changes

michaeljmarshall added the cherry-picked/branch-2.10 label Mar 17, 2023

michaeljmarshall added a commit that referenced this pull request Mar 17, 2023

[improve][broker] Authorize originalPrincipal when provided (#19830)

7f2ec42

(cherry picked from commit 6bc3530)

michaeljmarshall added a commit that referenced this pull request Mar 17, 2023

[improve][broker] Authorize originalPrincipal when provided (#19830)

4dc5e20

(cherry picked from commit 6bc3530) (cherry picked from commit 7f2ec42)

michaeljmarshall added cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.11 labels Mar 17, 2023

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Mar 29, 2023

[improve][broker] Authorize originalPrincipal when provided (apache#1…

0c365e2

…9830) (cherry picked from commit 6bc3530) (cherry picked from commit 7ea5f43)

michaeljmarshall mentioned this pull request Apr 1, 2023

[fix][broker] Only validate superuser access if authz enabled #19989

Merged

1 task

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

Revert "[test] Fix ServerCnxTest failing after merge of apache#19830"

ecbcb81

This reverts commit 6a9a22c.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[improve][broker] Authorize originalPrincipal when provided #19830

[improve][broker] Authorize originalPrincipal when provided #19830

michaeljmarshall commented Mar 16, 2023

nodece left a comment

michaeljmarshall commented Mar 17, 2023

coderzc commented Mar 18, 2023

michaeljmarshall commented Mar 18, 2023

michaeljmarshall commented Mar 18, 2023

Anonymitaet commented Mar 21, 2023

poorbarcode commented Mar 31, 2023 •

edited

Loading

michaeljmarshall commented Apr 1, 2023

michaeljmarshall commented Apr 1, 2023

[improve][broker] Authorize originalPrincipal when provided #19830

[improve][broker] Authorize originalPrincipal when provided #19830

Conversation

michaeljmarshall commented Mar 16, 2023

Motivation

Modifications

Verifying this change

Documentation

Matching PR in forked repository

nodece left a comment

Choose a reason for hiding this comment

michaeljmarshall commented Mar 17, 2023

coderzc commented Mar 18, 2023

michaeljmarshall commented Mar 18, 2023

michaeljmarshall commented Mar 18, 2023

Anonymitaet commented Mar 21, 2023

poorbarcode commented Mar 31, 2023 • edited Loading

michaeljmarshall commented Apr 1, 2023

michaeljmarshall commented Apr 1, 2023

poorbarcode commented Mar 31, 2023 •

edited

Loading