[fix][broker] Fix update authentication data

[nodece]

Motivation

In the Pulsar protocol, we always directly update the authenticationData during the auth challenge, originalAuthData has been ignored, which causes the following problems:

1. The authorization provider cannot get the correct authentication data.
2. The validity of the authentication data is not checked.

https://github.com/apache/pulsar/blob/72e044515d8dae3ce452818261b7328a4deb6e5f/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L705

This should affect the 2.9, 2.10, and 2.11 versions.

NOTE: When the proxy forwards the authentication data of the client to the broker, the broker can only refresh the forwarded authentication data.

Modifications

1. In org.apache.pulsar.broker.service.ServerCnx#doAuthentication, there is a need to get the authentication data from the AuthenticationState, which includes checking if the authentication is valid.
2. Using org.apache.pulsar.broker.authentication.AuthenticationState#authenticateAsync instead of org.apache.pulsar.broker.authentication.AuthenticationState#authenticate, this is safe.
3. When an authentication error occurs, throw details, so some tests need to be updated.

Verifying this change

The Pulsar test covers this change.

Documentation

• [ ] doc
• [ ] doc-required
• [x] doc-not-needed
• [ ] doc-complete

Matching PR in forked repository

PR in forked repository: https://github.com/nodece/pulsar/pull/8

[codecov-commenter]

Codecov Report

Merging #18130 (dd7d9c8) into master (6c65ca0) will increase coverage by 10.98%. The diff coverage is 49.28%.

@@              Coverage Diff              @@
##             master   #18130       +/-   ##
=============================================
+ Coverage     34.91%   45.90%   +10.98%     
- Complexity     5707    17633    +11926     
=============================================
  Files           607     1574      +967     
  Lines         53396   128475    +75079     
  Branches       5712    14132     +8420     
=============================================
+ Hits          18644    58977    +40333     
- Misses        32119    63410    +31291     
- Partials       2633     6088     +3455     

Flag	Coverage Δ
unittests	45.90% <49.28%> (+10.98%)	:arrow_up:

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

Impacted Files	Coverage Δ
...che/bookkeeper/mledger/impl/ManagedLedgerImpl.java	71.11% <0.00%> (ø)
...org/apache/pulsar/broker/ServiceConfiguration.java	52.06% <ø> (ø)
.../service/SystemTopicBasedTopicPoliciesService.java	62.97% <0.00%> (+11.38%)	:arrow_up:
.../pulsar/broker/stats/BrokerOperabilityMetrics.java	98.21% <ø> (+5.56%)	:arrow_up:
...g/apache/pulsar/compaction/CompactedTopicImpl.java	69.28% <0.00%> (+58.57%)	:arrow_up:
...va/org/apache/pulsar/client/impl/ConsumerBase.java	21.95% <0.00%> (ø)
...ache/pulsar/functions/utils/io/ConnectorUtils.java	0.00% <0.00%> (ø)
...va/org/apache/pulsar/io/jdbc/JdbcAbstractSink.java	4.16% <0.00%> (-0.03%)	:arrow_down:
...java/org/apache/pulsar/io/jdbc/JdbcSinkConfig.java	23.33% <0.00%> (-1.67%)	:arrow_down:
...main/java/org/apache/pulsar/io/jdbc/JdbcUtils.java	0.00% <0.00%> (ø)
... and 1132 more

[gengmao]

@codelipenghui @tuteng @merlimat can you please review this PR? Looks critical for token refreshing to work as expected with proxy.

[nodece]

Is authentication data supposed to dynamically change?

Yes, we can dynamically change the authentication data by auth challenge.

We require the role to stay the same across authentication refreshes, which implies to me that the rest of the data is supposed to be static. That might not be how it is used, in practice, though.

Usually, the authentication data need to change when the authentication data expired. Some authentication provider only care the authentication data, which didn't use a passing role by the Pulsar.

Why do we need to verify the originalAuthenticationData?

The originalAuthenticationData comes from the user client by the proxy forwarded, so we must verify that. When there is no proxy, we only check authenticationData, old code is right.

We have two authentication data, one from the proxy, and one from the client. When the authentication has an expired limit, the authentication flow is complex, and causes some authentication issues.

[michaeljmarshall]

Is authentication data supposed to dynamically change?

Yes, we can dynamically change the authentication data by auth challenge.

We require the role to stay the same across authentication refreshes, which implies to me that the rest of the data is supposed to be static. That might not be how it is used, in practice, though.

Usually, the authentication data need to change when the authentication data expired. Some authentication provider only care the authentication data, which didn't use a passing role by the Pulsar.

My point is that the authorization related data should not change. Auth refresh requires that some portion of the auth data changes, but because we verify that the role is the same, we are essentially requiring that the authorization related infor remains the same.

Why do we need to verify the originalAuthenticationData?

The originalAuthenticationData comes from the user client by the proxy forwarded, so we must verify that. When there is no proxy, we only check authenticationData, old code is right.

My understanding is that the proxy verifies the authenticity of the originalAuthenticationData (it is only authenticationData in the proxy). Then, the broker verifies the authenticationData and trusts the originalAuthenticationData because it is supplied by the proxy role.

We have two authentication data, one from the proxy, and one from the client. When the authentication has an expired limit, the authentication flow is complex, and causes some authentication issues.

It seems like your concern is not just the authentication flow, but how it interacts with authorization. Is that correct?

[nodece]

It seems like your concern is not just the authentication flow, but how it interacts with authorization. Is that correct?

Right! For the default authentication provider, which doesn't care about the authentication data.

But when we implement a customize authentication provider, I depend on the authentication data, but the Pulsar cannot pass the correct authentication data to this authentication provider.

[michaeljmarshall]

But when we implement a customize authentication provider, I depend on the authentication data, but the Pulsar cannot pass the correct authentication data to this authentication provider.

Makes sense. My point is that we close the connection when the role changes:

https://github.com/apache/pulsar/blob/b0945d1d45d1e911d24151a23cf284e476203ba7/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L728-L734

That logic was put in place before the logic to pass the authData to the AuthorizationProvider. I am not sure that we should change this paradigm.

[nodece]

Makes sense. My point is that we close the connection when the role changes.

It looks like we cannot update the role. Let us continue to keep this logic.

https://github.com/apache/pulsar/blob/b0945d1d45d1e911d24151a23cf284e476203ba7/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java#L728-L734

That logic was put in place before the logic to pass the authData to the AuthorizationProvider.

Good catch, I can move this logic.

[nodece]

@codelipenghui @michaeljmarshall @Technoboy- Could you review this PR? Thanks for your time!

[nodece]

This PR is a little big, so maybe I should split this PR.

[michaeljmarshall]

This PR is a little big, so maybe I should split this PR.

Sounds good to me. As far as making things asynchronous, I am happy to follow up on that work. I already have this draft for the ProxyConnection: https://github.com/apache/pulsar/pull/19292. I need to do some other work before tests will pass on that PR, so it is only a draft.

[nodece]

Closed by #19519.