[fix][authentication] Store the original authentication data #19519

nodece · 2023-02-14T16:13:51Z

Motivation

In the authentication:

With the proxy, the broker stores the client authentication to the originalAuthData and originalPrincipal, and stores the proxy authentication to the authenticationData and authRole.
Without the proxy, the broker stores the authentication to the authenticationData and authRole

When with the proxy, the broker only checks whether originalAuthData is expired. If true, the broker sends AuthChallenge to the client, then the client sends CommandAuthResponse.

In handleAuthResponse logic, the broker always stores the authentication to authenticationData and authRole, without considering the proxy case. When the authorization provider checks the role and authentication data, it is unmatched, this is incorrect behavior, so we need to distinguish whether have the proxy and then store the authentication data and role correctly.

More context: #18130

Modifications

Fix storing the authentication in the authChallengeSuccessCallback
Add MockMutableAuthenticationProvider and MockMutableAuthenticationState to refresh the role and datasource
Make MockAlwaysExpiredAuthenticationState extends MockMutableAuthenticationState to avoid the code duplication, and override the isExpired

Verifying this change

Make sure that the change passes the CI checks.

Added testRefreshOriginalPrincipalWithAuthDataForwardedFromProxy test

Documentation

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

Signed-off-by: Zixuan Liu <[email protected]>

michaeljmarshall

It looks like this PR is partially addressing #19332. It won't solve that issue because this PR doesn't address the fact that we cannot refresh both of the AuthData objects.

pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockAuthenticationProvider.java

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

michaeljmarshall · 2023-02-15T04:54:28Z

pulsar-broker/src/test/java/org/apache/pulsar/broker/service/ServerCnxTest.java

@@ -1030,6 +1030,55 @@ public void testVerifyAuthRoleAndAuthDataFromDirectConnectionBroker() throws Exc
                }));
    }

+    @Test
+    public void testRefreshOriginalPrincipalWithAuthDataForwardedFromProxy() throws Exception {


Note that the tests that comment about https://github.com/apache/pulsar/issues/19332 should fail because they make assertions on the current behavior.

For current design, it works fine.

I must not have written some of the assertions I thought I did. You're right that those tests all pass. It might be worth removing the comments that reference #19332 because your PR will make them incorrect.

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java

Signed-off-by: Zixuan Liu <[email protected]>

nodece · 2023-02-15T14:49:57Z

Hi @michaeljmarshall, I updated this PR.

pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockAuthenticationProvider.java

Signed-off-by: Zixuan Liu <[email protected]>

michaeljmarshall

LGTM, great work @nodece

nodece · 2023-02-16T07:31:52Z

/pulsarbot rerun-failure-checks

nodece · 2023-02-16T10:05:36Z

/pulsarbot rerun-failure-checks

Signed-off-by: Zixuan Liu <[email protected]>

michaeljmarshall · 2023-02-17T04:32:49Z

@nodece - I think this is a bug, so we can back port it to the older release branches, if you would like. However, I am finishing up cherry picking some of this PR's dependencies, so it will be easiest to delay cherry-picking by a day or two.

nodece · 2023-02-17T05:03:22Z

@michaeljmarshall Thank you for your work! I look forward to your contribution!

michaeljmarshall · 2023-02-27T19:56:22Z

@nodece - just so you know, I finished cherry picking my changes to the older branches.

…19519) Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit 2d90089) Signed-off-by: Zixuan Liu <[email protected]>

Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit 2d90089) Signed-off-by: Zixuan Liu <[email protected]>

…19519) Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit 2d90089) Signed-off-by: Zixuan Liu <[email protected]>

When #19519 was cherry-picked to branch-2.11, it did not implement the authenticate method in the MockMutableAuthenticationState, which led to several test failures in the ServerCnxTest class. This commit fixes those tests. Note that the issue is only in the test code.

michaeljmarshall · 2023-03-18T15:09:23Z

@nodece - just letting you know there was a minor issue when cherry picking this to branch-2.11. Some of the ServerCnxTest tests were failing. I fixed it with ad06fac.

…19519) Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit 2d90089)

[fix][authentication] Store the original authentication data

3883fcd

Signed-off-by: Zixuan Liu <[email protected]>

nodece requested review from michaeljmarshall, lhotari, codelipenghui, coderzc, mattisonchao, merlimat, poorbarcode and Technoboy- February 14, 2023 16:13

nodece self-assigned this Feb 14, 2023

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 14, 2023

nodece added this to the 3.0.0 milestone Feb 14, 2023

nodece added the area/authn label Feb 14, 2023

michaeljmarshall reviewed Feb 15, 2023

View reviewed changes

pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java Outdated Show resolved Hide resolved

michaeljmarshall mentioned this pull request Feb 15, 2023

Simplify authentication state management in Broker's ServerCnx class when networking with proxy #19332

Open

2 tasks

Improve test and authentication logic

03bcc4b

Signed-off-by: Zixuan Liu <[email protected]>

nodece requested a review from michaeljmarshall February 15, 2023 14:50

michaeljmarshall reviewed Feb 15, 2023

View reviewed changes

pulsar-broker/src/test/java/org/apache/pulsar/broker/auth/MockAuthenticationProvider.java Outdated Show resolved Hide resolved

Revert MockAuthenticationProvider

72d5ed5

Signed-off-by: Zixuan Liu <[email protected]>

nodece requested a review from michaeljmarshall February 16, 2023 03:19

michaeljmarshall approved these changes Feb 16, 2023

View reviewed changes

nodece added the ready-to-test label Feb 16, 2023

Fix test

6cf7aae

Signed-off-by: Zixuan Liu <[email protected]>

nodece merged commit 2d90089 into apache:master Feb 16, 2023

nodece mentioned this pull request Feb 16, 2023

[fix][broker] Fix update authentication data #18130

Closed

4 tasks

nodece added a commit that referenced this pull request Mar 13, 2023

[fix][authentication] Store the original authentication data (#19519)

58aa8da

Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit 2d90089) Signed-off-by: Zixuan Liu <[email protected]>

nodece mentioned this pull request Mar 13, 2023

[fix][authentication][branch-2.10] Store the original authentication data #19794

Closed

4 tasks

nodece added cherry-picked/branch-2.11 release/2.11.1 labels Mar 13, 2023

This was referenced Mar 13, 2023

[fix][authentication][branch-2.9] Store the original authentication data #19795

Closed

[fix][authentication][branch-2.8] Store the original authentication data #19796

Closed

michaeljmarshall pushed a commit to michaeljmarshall/pulsar that referenced this pull request Apr 20, 2023

[fix][authentication] Store the original authentication data (apache#…

6b07892

…19519) Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit 2d90089)

gaoran10 pushed a commit to gaoran10/pulsar that referenced this pull request Dec 2, 2023

[fix][authentication] Store the original authentication data (apache#…

d767999

…19519) Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit 2d90089)

This was referenced Dec 2, 2023

[fix][cherry-pick] Update original auth data after auth data refresh gaoran10/pulsar#30

Open

[branch-2.10][authentication] Update original auth data after auth data refresh #21660

Merged

gaoran10 pushed a commit to gaoran10/pulsar that referenced this pull request Dec 8, 2023

[fix][authentication] Store the original authentication data (apache#…

d3e8e2c

…19519) Signed-off-by: Zixuan Liu <[email protected]> (cherry picked from commit 2d90089)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix][authentication] Store the original authentication data #19519

[fix][authentication] Store the original authentication data #19519

nodece commented Feb 14, 2023 •

edited

Loading

michaeljmarshall left a comment

michaeljmarshall Feb 15, 2023

nodece Feb 15, 2023

michaeljmarshall Feb 15, 2023

nodece commented Feb 15, 2023

michaeljmarshall left a comment

nodece commented Feb 16, 2023

nodece commented Feb 16, 2023

michaeljmarshall commented Feb 17, 2023

nodece commented Feb 17, 2023

michaeljmarshall commented Feb 27, 2023

michaeljmarshall commented Mar 18, 2023

[fix][authentication] Store the original authentication data #19519

[fix][authentication] Store the original authentication data #19519

Conversation

nodece commented Feb 14, 2023 • edited Loading

Motivation

Modifications

Verifying this change

Documentation

michaeljmarshall left a comment

Choose a reason for hiding this comment

michaeljmarshall Feb 15, 2023

Choose a reason for hiding this comment

nodece Feb 15, 2023

Choose a reason for hiding this comment

michaeljmarshall Feb 15, 2023

Choose a reason for hiding this comment

nodece commented Feb 15, 2023

michaeljmarshall left a comment

Choose a reason for hiding this comment

nodece commented Feb 16, 2023

nodece commented Feb 16, 2023

michaeljmarshall commented Feb 17, 2023

nodece commented Feb 17, 2023

michaeljmarshall commented Feb 27, 2023

michaeljmarshall commented Mar 18, 2023

nodece commented Feb 14, 2023 •

edited

Loading