Skip to content

[fix][broker] Fix passing incorrect authentication data #16201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 28, 2022

Conversation

nodece
Copy link
Member

@nodece nodece commented Jun 23, 2022

Signed-off-by: Zixuan Liu nodeces@gmail.com

Motivation

#16065 fixes the race condition issue, but introduces a new issue. This issue is triggered when the Proxy and Broker work together, when we use the proxy to request the broker to do lookup/subscribe/produce operation, the broker always uses the original authentication data for authorization, not proxy authentication data, which causes this issue.

Modification

  • Fix passing authentication data, differentiate between original auth data and connected auth data by avoid to use the getAuthenticationData(), this method name is easy to cause confusion and can not correctly get the authentication data

Verifying this change

  • Make sure that the change passes the CI checks.

Added unit test.

Documentation

  • doc-not-needed

@nodece nodece force-pushed the fix-broker-auth-data branch from 16fa583 to ff33ae5 Compare June 24, 2022 06:34
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 24, 2022
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proxy tests still succeed without the modifications on ServerCnx

@nodece
Copy link
Member Author

nodece commented Jun 24, 2022

The proxy tests still succeed without the modifications on ServerCnx

You can try this step:

  1. Clone this PR
  2. Open org.apache.pulsar.proxy.server.ProxyWithJwtAuthorizationTest
  3. Comment out the proxyConfig.setForwardAuthorizationCredentials(true); and conf.setAuthenticateOriginalAuthData(true);
  4. Run org.apache.pulsar.proxy.server.ProxyWithJwtAuthorizationTest

org.apache.pulsar.proxy.server.ProxyWithJwtAuthorizationTest#testProxyAuthorization and org.apache.pulsar.proxy.server.ProxyWithJwtAuthorizationTest#testProxyAuthorizationWithPrefixSubscriptionAuthMode will fail.

@BewareMyPower
Copy link
Contributor

Comment out the proxyConfig.setForwardAuthorizationCredentials(true); and conf.setAuthenticateOriginalAuthData(true);

So why did you add these two configs to tests?

image

In addition, it still succeeded.

@nodece
Copy link
Member Author

nodece commented Jun 24, 2022

These tests need to verify the originalPrincipal, we have to forward the client authentication data to the broker by proxy.

    /**
     * <pre>
     * It verifies jwt + Authentication + Authorization (client -> proxy -> broker).
     *
     * 1. client connects to proxy over jwt and pass auth-data
     * 2. proxy authenticate client and retrieve client-role
     *    and send it to broker as originalPrincipal over jwt
     * 3. client creates producer/consumer via proxy
     * 4. broker authorize producer/consumer create request using originalPrincipal
     *
     * </pre>
     */
    @Test
    public void testProxyAuthorization() throws Exception {

This is my test result:

image

@BewareMyPower
Copy link
Contributor

BewareMyPower commented Jun 24, 2022

I found the cause. It's because when I ran the tests, I have reverted your changes for ServerCnx.

Therefore, you must add another test to verify that when authenticaionEnabled and forwardAuthorizationCredentials are false, the authorization will succeed, which is unexpected. Otherwise it could be possible to introduce a regression in future because the tests would still pass if I reverted the changes for ServerCnx in this PR.

@nodece
Copy link
Member Author

nodece commented Jun 24, 2022

Your suggestion is great, I'll make a PR to do that.

@BewareMyPower
Copy link
Contributor

I'm still confused about why #16065 introduced the bug? I just reverted e6b12c6 and run the ProxyWithJwtAuthorizationTest,ProxyWithJwtAuthorizationTest passed. i.e. it's not a regression.

But this PR makes ProxyWithJwtAuthorizationTest require the following configs:

        conf.setAuthenticateOriginalAuthData(true);
        proxyConfig.setForwardAuthorizationCredentials(true);

It looks like a breaking change on these configs.

@nodece
Copy link
Member Author

nodece commented Jun 24, 2022

I'm still confused about why #16065 introduced the bug?

This bug is triggered only when the proxy is used, you can see this PR uses the getAuthenticationData() to get authentication data, when the proxy is used, it'll often use the proxy authentication data.

public AuthenticationDataSource getAuthenticationData() {
        return originalAuthData != null ? originalAuthData : authenticationData;
 }

The following is our test log:

2022-06-23T07:19:15,242+0000 [pulsar-io-4-1] DEBUG AuthorizationProviderOAuth - subject: client-role, scopes: [access], authzRoles: [
]
2022-06-23T07:19:15,242+0000 [pulsar-io-4-1] DEBUG org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider - Verify if role proxy-role is allowed to LOOKUP to topic per

The subject doesn't equal the role, it is incorrect.

I just reverted e6b12c6 and run the ProxyWithJwtAuthorizationTest,ProxyWithJwtAuthorizationTest passed. i.e. it's not a regression.

But this PR makes ProxyWithJwtAuthorizationTest require the following configs:

        conf.setAuthenticateOriginalAuthData(true);
        proxyConfig.setForwardAuthorizationCredentials(true);

It looks like a breaking change on these configs.

It was an accident, although the intermediate link of verification is incorrect, the test can still pass.

@nodece nodece force-pushed the fix-broker-auth-data branch 3 times, most recently from 95223ce to 6ba3293 Compare June 26, 2022 15:53
@nodece nodece requested a review from BewareMyPower June 26, 2022 16:03
@nodece nodece force-pushed the fix-broker-auth-data branch 2 times, most recently from f3b5671 to 3653e5c Compare June 27, 2022 03:33
@nodece
Copy link
Member Author

nodece commented Jun 27, 2022

@BewareMyPower Thank you for pointing out this problem, you are right, I made a wrong design in the code that made the test fail.

@codelipenghui
Copy link
Contributor

@merlimat Please help review this PR.

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@utahkay
Copy link
Contributor

utahkay commented Jun 28, 2022

Could this please be picked into 2.8, 2.9 and 2.10?

@codelipenghui codelipenghui added doc-not-needed Your PR changes do not impact docs and removed doc-not-needed Your PR changes do not impact docs labels Jun 28, 2022
@codelipenghui codelipenghui merged commit 936bbbc into apache:master Jun 28, 2022
codelipenghui pushed a commit that referenced this pull request Jun 28, 2022
### Motivation

#16065 fixes the race condition issue, but introduces a new issue. This issue is triggered when the Proxy and Broker work together, when we use the proxy to request the broker to do lookup/subscribe/produce operation, the broker always uses the original authentication data for authorization, not proxy authentication data, which causes this issue.

### Modification

- Fix passing authentication data, differentiate between original auth data and connected auth data by avoid to use the  `getAuthenticationData()`, this method name is easy to cause confusion and can not correctly get the authentication data

(cherry picked from commit 936bbbc)
@nodece
Copy link
Member Author

nodece commented Jun 29, 2022

Could this please be picked into 2.8, 2.9 and 2.10?

@codelipenghui Do you have this planning?

nodece added a commit to nodece/pulsar that referenced this pull request Jun 29, 2022
### Motivation

apache#16065 fixes the race condition issue, but introduces a new issue. This issue is triggered when the Proxy and Broker work together, when we use the proxy to request the broker to do lookup/subscribe/produce operation, the broker always uses the original authentication data for authorization, not proxy authentication data, which causes this issue.

### Modification

- Fix passing authentication data, differentiate between original auth data and connected auth data by avoid to use the  `getAuthenticationData()`, this method name is easy to cause confusion and can not correctly get the authentication data

(cherry picked from commit 936bbbc)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@mattisonchao mattisonchao added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Jul 2, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jul 4, 2022
### Motivation

apache#16065 fixes the race condition issue, but introduces a new issue. This issue is triggered when the Proxy and Broker work together, when we use the proxy to request the broker to do lookup/subscribe/produce operation, the broker always uses the original authentication data for authorization, not proxy authentication data, which causes this issue.

### Modification

- Fix passing authentication data, differentiate between original auth data and connected auth data by avoid to use the  `getAuthenticationData()`, this method name is easy to cause confusion and can not correctly get the authentication data

(cherry picked from commit 936bbbc)
(cherry picked from commit adf5ce7)
nodece added a commit to nodece/pulsar that referenced this pull request Jul 5, 2022
### Motivation

apache#16065 fixes the race condition issue, but introduces a new issue. This issue is triggered when the Proxy and Broker work together, when we use the proxy to request the broker to do lookup/subscribe/produce operation, the broker always uses the original authentication data for authorization, not proxy authentication data, which causes this issue.

### Modification

- Fix passing authentication data, differentiate between original auth data and connected auth data by avoid to use the  `getAuthenticationData()`, this method name is easy to cause confusion and can not correctly get the authentication data

(cherry picked from commit 936bbbc)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@Technoboy- Technoboy- added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Jul 5, 2022
Technoboy- pushed a commit that referenced this pull request Jul 5, 2022
)

### Motivation

#16065 fixes the race condition issue, but introduces a new issue. This issue is triggered when the Proxy and Broker work together, when we use the proxy to request the broker to do lookup/subscribe/produce operation, the broker always uses the original authentication data for authorization, not proxy authentication data, which causes this issue.

### Modification

- Fix passing authentication data, differentiate between original auth data and connected auth data by avoid to use the  `getAuthenticationData()`, this method name is easy to cause confusion and can not correctly get the authentication data

(cherry picked from commit 936bbbc)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@codelipenghui
Copy link
Contributor

@nodece Could you please open a PR for branch-2.8

nodece added a commit to nodece/pulsar that referenced this pull request Jul 28, 2022
### Motivation

apache#16065 fixes the race condition issue, but introduces a new issue. This issue is triggered when the Proxy and Broker work together, when we use the proxy to request the broker to do lookup/subscribe/produce operation, the broker always uses the original authentication data for authorization, not proxy authentication data, which causes this issue.

### Modification

- Fix passing authentication data, differentiate between original auth data and connected auth data by avoid to use the  `getAuthenticationData()`, this method name is easy to cause confusion and can not correctly get the authentication data

(cherry picked from commit 936bbbc)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece added a commit to nodece/pulsar that referenced this pull request Jul 29, 2022
### Motivation

apache#16065 fixes the race condition issue, but introduces a new issue. This issue is triggered when the Proxy and Broker work together, when we use the proxy to request the broker to do lookup/subscribe/produce operation, the broker always uses the original authentication data for authorization, not proxy authentication data, which causes this issue.

### Modification

- Fix passing authentication data, differentiate between original auth data and connected auth data by avoid to use the  `getAuthenticationData()`, this method name is easy to cause confusion and can not correctly get the authentication data

(cherry picked from commit 936bbbc)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
nodece added a commit to nodece/pulsar that referenced this pull request Aug 1, 2022
### Motivation

apache#16065 fixes the race condition issue, but introduces a new issue. This issue is triggered when the Proxy and Broker work together, when we use the proxy to request the broker to do lookup/subscribe/produce operation, the broker always uses the original authentication data for authorization, not proxy authentication data, which causes this issue.

### Modification

- Fix passing authentication data, differentiate between original auth data and connected auth data by avoid to use the  `getAuthenticationData()`, this method name is easy to cause confusion and can not correctly get the authentication data

(cherry picked from commit 936bbbc)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
BewareMyPower pushed a commit that referenced this pull request Aug 2, 2022
)

### Motivation

#16065 fixes the race condition issue, but introduces a new issue. This issue is triggered when the Proxy and Broker work together, when we use the proxy to request the broker to do lookup/subscribe/produce operation, the broker always uses the original authentication data for authorization, not proxy authentication data, which causes this issue.

### Modification

- Fix passing authentication data, differentiate between original auth data and connected auth data by avoid to use the  `getAuthenticationData()`, this method name is easy to cause confusion and can not correctly get the authentication data

(cherry picked from commit 936bbbc)
Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@nodece
Copy link
Member Author

nodece commented Aug 2, 2022

@nodece Could you please open a PR for branch-2.8

See #16840.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants