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

nodece · 2022-10-20T04:11:40Z

Motivation

There is a bug that cannot update the authentication data variable during the auth challenge. The broker has two authentication data of proxy and user, the broker just updates the authenticationData during the auth challenge, originalAuthData has been ignored, which causes the following problems:

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

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

Line 705 in 72e0445

this.authenticationData = authState.getAuthDataSource();

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

NOTE: When the proxy forwards the authentication data of the user's client to the broker, the broker can only refresh the user's authentication data, and ignore the proxy's authentication data.

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

Line 805 in 631cd02

    
           AuthenticationState authState = this.originalAuthState != null ? originalAuthState : this.authState;

Modifications

When a new authentication comes in, we must new an authentication state, don't use the old state
In org.apache.pulsar.broker.service.ServerCnx#doAuthentication
- Update authentication data variable
- Update authentication state variable
- Update role variable
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
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository: nodece#8

codecov-commenter · 2022-10-20T05:02:49Z

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%)`	⬆️

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%)`	⬆️
.../pulsar/broker/stats/BrokerOperabilityMetrics.java	`98.21% <ø> (+5.56%)`	⬆️
...g/apache/pulsar/compaction/CompactedTopicImpl.java	`69.28% <0.00%> (+58.57%)`	⬆️
...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%)`	⬇️
...java/org/apache/pulsar/io/jdbc/JdbcSinkConfig.java	`23.33% <0.00%> (-1.67%)`	⬇️
...main/java/org/apache/pulsar/io/jdbc/JdbcUtils.java	`0.00% <0.00%> (ø)`
... and 1132 more

gengmao · 2022-10-27T03:35:42Z

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

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

codelipenghui · 2022-10-27T05:25:39Z

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

-                        remoteAddress, authMethod, this.authRole, originalPrincipal);
-            }
+        CompletableFuture<AuthData> authFuture;
+        if (!authState.isComplete()) {


The authState can be null if state != State.Connected

The authState always non-null, you can see handleConnect().

I agree with @codelipenghui, I think we need to check if authState is null here, newAuthState is implemented in the custom plugin and its return value could be null

codelipenghui · 2022-10-27T05:28:54Z

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

+                }
+                if (state != State.Connected) {
+                    // First time authentication is done
+                    completeConnect(clientProtocolVersion, clientVersion, enableSubscriptionPatternEvaluation);


Looks like we can move out from the thenCompose()?

It seems unnecessary.

We have checked if the state is connected at the beginning. But we going to check the connection is not connected again here. If we can merge them, it will make the code more easy to read.

We have checked if the state is connected at the beginning.

The state == State.Connected means doing an authentication refresh operation.

The state != State.Connected means first authentication is done.

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

codelipenghui · 2022-10-27T13:40:25Z

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

                    }
+                    state = State.Connected;


And if we can reach here. The state should be Connected right? Why need to set to Connected again?

And we should avoid change the state in another thread. Here is executed by the callback thread for completing the authFuture.

And if we can reach here. The state should be Connected right? Why need to set to Connected again?

~~You are right.~~, For auth challenge multiple times, we still need to set to Conected, see line 764.

And we should avoid change the state in another thread. Here is executed by the callback thread for completing the authFuture.

Do you mean I should switch to Pulsar thread to change the state?

And we should avoid change the state in another thread. Here is executed by the callback thread for completing the authFuture.

This is safe, each connection has its own ServerCnx.java, don't warry.

I believe @codelipenghui is right here. We are updating the state from a callback which could be another thread, and that is not a safe operation here. Instead, I think we should remove all of the async references in this PR and just focus on fixing the underlying problem with authentication data. I already started work on implementing PIP 97, and I will follow up on this PR to replace the synchronous methods with asynchronous calls.

I would like to know why this is not a safe operation. Are you worried that some other handler will change this state?

I think we should remove all of the async references in this PR and just focus on fixing the underlying problem with authentication data.

Good point out.

michaeljmarshall

Thanks for your PR and explanation @nodece! I would like to clarify our authn/authz design before discussing the implementation of this change.

The authorization provider cannot get the correct authentication data.

Is authentication data supposed to dynamically change? 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.

The primary benefit of requiring the authentication data to remain static for the given session is that it means currently authorized tasks do not need to be re-authorized. This is relevant since we do not re-verify if a role has permission to produce to a topic when we re-authenticate the client's auth data.

I'll concede that revoked permission granted to a role is not handled by our authorization scheme, but that seems like a separate question.

The validity of the authentication data is not checked.

Why do we need to verify the originalAuthenticationData? I thought the point was that the authenticationData had to be authentic and had to be associated to a sufficiently powerful role, and then we "trusted" the originalAuthenticationData implicitly while using the originalAuthenticationData for authorization purposes.

Let me know what you think. Thanks!

nodece · 2022-10-28T04:57:10Z

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 · 2022-10-28T06:06:45Z

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 · 2022-10-28T07:51:43Z

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 · 2022-10-28T14:42:03Z

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:

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

Lines 728 to 734 in b0945d1

    
           if (!authRole.equals(newAuthRole)) { 
        
               log.warn("[{}] Principal cannot change during an authentication refresh expected={} got={}", 
        
                       remoteAddress, authRole, newAuthRole); 
        
               ctx.close(); 
        
           } else { 
        
               log.info("[{}] Refreshed authentication credentials for role {}", remoteAddress, authRole); 
        
           }

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 · 2022-10-28T14:50:00Z

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.

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

Lines 728 to 734 in b0945d1

    
           if (!authRole.equals(newAuthRole)) { 
        
               log.warn("[{}] Principal cannot change during an authentication refresh expected={} got={}", 
        
                       remoteAddress, authRole, newAuthRole); 
        
               ctx.close(); 
        
           } else { 
        
               log.info("[{}] Refreshed authentication credentials for role {}", remoteAddress, authRole); 
        
           }

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

Good catch, I can move this logic.

nodece · 2022-11-17T03:04:30Z

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

michaeljmarshall

Overall, I think that my initial criticism of this PR was based on a flawed understanding of the ServerCnx and how forwarding authData works. I just started work today on finishing PIP 97. I will review the code while completing that work, and that should help me give a more accurate review of this PR.

michaeljmarshall · 2023-01-10T20:43:48Z

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

-            }
+        CompletableFuture<AuthData> authFuture = CompletableFuture.completedFuture(null);
+        if (!useOriginalAuthState && !authState.isComplete()) {
+            authFuture = authState.authenticateAsync(clientData);


I am starting work on finishing PIP 97 today. I think it might make sense to remove this update to use authenticateAsync since it is not related to this PR.

This PR is to fix the correct persistence authentication state and data.

Do you want to remove which?

We must use the authenticateAsync, this is used to check the user permission.

I am saying we should not make the switch to be asynchronous in this PR. We should make the switch in a separate PR. I started work this week to complete the PIP 97, which introduced the async changes.

Ping @michaeljmarshall

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

michaeljmarshall

@nodece - I have some questions about the underlying problem, I left some questions in comments. Otherwise, I see that there are not any new tests to cover this change. Given that it is a bug fix, we should add a test.

michaeljmarshall · 2023-01-18T04:34:21Z

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

                    }
+                    state = State.Connected;


I believe @codelipenghui is right here. We are updating the state from a callback which could be another thread, and that is not a safe operation here. Instead, I think we should remove all of the async references in this PR and just focus on fixing the underlying problem with authentication data. I already started work on implementing PIP 97, and I will follow up on this PR to replace the synchronous methods with asynchronous calls.

michaeljmarshall · 2023-01-18T06:58:37Z

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

+        // credentials, but we only can new an authentication state, because some authentication data(TLS, SASL)
+        // based on outside service.


Can you explain this a bit more? In the case of TLS authentication, we are not able to refresh the originalAuthenticationState, that case seems unrelated.

Sorry, there is some confusion. For originalAuthentication, we don't call the authentication checks. Because there is some state in the AuthenticationState, which depends on outside service.

pulsar/pulsar-broker-auth-sasl/src/main/java/org/apache/pulsar/broker/authentication/SaslAuthenticationState.java

Lines 64 to 67 in 82237d3

@Override

public AuthData authenticate(AuthData authData) throws AuthenticationException {

return pulsarSaslServer.response(authData);

}

originalAuthentication belongs to the user's client by the proxy forwarded, see

pulsar/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyClientCnx.java

Lines 68 to 70 in f360807

return Commands.newConnect(authentication.getAuthMethodName(), authData, protocolVersion,

PulsarVersion.getVersion(), proxyToTargetBrokerAddress, clientAuthRole, clientAuthData,

clientAuthMethod);

For originalAuthentication, we don't call the authentication checks.

Isn't this a problem though? We aren't really authenticating the originalAuthData if we don't call the authenticate method and make sure authentication is "complete". In the ProxyConnectionToBroker case, we can send back AuthChallenge In the event that the proxy is forwarding authentication information, we can issue AuthChallenge responses. It might not work so easily in the ProxyLookupRequests state.

For the current design, the originalAuthData is credible by the proxy forwarding authentication data. This authentication data was completed in the proxy and then sent this authentication data to the broker, so we don't check the authentication data in the broker.

For ProxyLookupRequests state, see #17831. I forward the AuthChallenge command to the user's client by the ProxyClient and Proxy Server.

For the current design, the originalAuthData is credible by the proxy forwarding authentication data. This authentication data was completed in the proxy and then sent this authentication data to the broker, so we don't check the authentication data in the broker.

My primary objection is that the setting required to enter this code block is called isAuthenticateOriginalAuthData. Note that we are already verifying most auth data in the case that the newAuthState method actually authenticates the data. That leaves out cases like SASL which do not authentication on AuthenticationState initialization because it is a multi-stage auth. However, I've just realized that forwarding the authentication information will especially not work with SASL because the proxy only forwards the last AuthData that it receives:

pulsar/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java

Lines 426 to 440 in d6fcdb8

private void doAuthentication(AuthData clientData)

throws Exception {

AuthData brokerData = authState.authenticate(clientData);

// authentication has completed, will send newConnected command.

if (authState.isComplete()) {

clientAuthRole = authState.getAuthRole();

if (LOG.isDebugEnabled()) {

LOG.debug("[{}] Client successfully authenticated with {} role {}",

remoteAddress, authMethod, clientAuthRole);

}

// First connection

if (this.connectionPool == null || state == State.Connecting) {

// authentication has completed, will send newConnected command.

completeConnect(clientData);

This seems like a gap in the protocol where multi-staged authentication is not able to be properly processed through the proxy. However, if its a gap, that means it is something that could be improved without breaking anything.

One question is whether it would help to have a distinct stages where we first authenticate the proxy and then authenticate its original client (when configured to do so). That could help to open up the design and to make the protocol a bit clearer.

This seems like a gap in the protocol where multi-staged authentication is not able to be properly processed through the proxy. However, if its a gap, that means it is something that could be improved without breaking anything.

When using the multi-staged authentication, we need to set authenticateOriginalAuthData=false in the broker, and then it works fine, the broker just checks the proxy's authentication data.

One question is whether it would help to have a distinct stages where we first authenticate the proxy and then authenticate its original client (when configured to do so).

The protocol needs to be changed, distinguishing between client and proxy.

The authentication logic here is a bit confusing, but for the current design we need to give the correct configuration in our documentation.

When using the multi-staged authentication, we need to set authenticateOriginalAuthData=false in the broker, and then it works fine, the broker just checks the proxy's authentication data.

I think this supports my statement that there is a gap in the protocol.

michaeljmarshall · 2023-01-18T07:31:20Z

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

-            String newAuthRole = authState.getAuthRole();
-
-            // Refresh the auth data.
-            this.authenticationData = authState.getAuthDataSource();


I spent many hours working on the authentication framework code today. Below are some of my thoughts. They aren't necessarily my final opinion.

In the PR description, this line was identified as the root cause of the issue, but based on the description and on this PR, I think I see it as a symptom of a larger issue. We do not have a clear enough definition for the state transitions in the ServerCnx class, which is essentially a finite state machine.

I want to describe part of the problem with this line of code. On the first pass through, the this.authenticationData is set by getting it from this.authState.getAuthDataSource() because useOriginalAuthState is false. Then, on subsequent AuthResponse commands from the original client, the authenticationData is set by getting it from this.originalAuthState.getAuthDataSource() because useOriginalAuthState is true. That means the this.authenticationData is incorrectly updated.

As a note, the broker gets AuthResponse commands from the original client when the connection through the client is in state ProxyConnectionToBroker. As of #17831, the broker also gets an AuthResponse from the original client when the proxy is in state ProxyLookupRequests with forwardAuthorizationCredentials=true.

Otherwise, the proxy sends its own authentication information.

Here are some problems with the current solution:

The AuthResponse protocol message only has one field for AuthData, and there is no indication whether the AuthData is for the proxy or the original client. As a consequence, the broker does not know whether to update the authenticationData or the originalAuthData.

The implementation for getAuthDataSource does not always get the most recent authenticationDataSource See TokenAuthenticationState. (I am already working on fixing this.)

Open questions:

Can we just remove the originalAuthState and only keep track of one authState?

Is the AuthenticationState object meant to last the whole lifecycle of a given ServerCnx? In my mind, the answer is yes, but this PR says otherwise. If not, it feels odd that we have a "state" object.

All of your description is correct.

The AuthResponse protocol message only has one field for AuthData, and there is no indication whether the AuthData is for the proxy or the original client. As a consequence, the broker does not know whether to update the authenticationData or the originalAuthData.

I started PIP a few months ago, see #17517

Can we just remove the originalAuthState and only keep track of one authState?

No. We must check both the originalAuthState and authState, if you don't check, you will overstep your authority.

Is the AuthenticationState object meant to last the whole lifecycle of a given ServerCnx? In my mind, the answer is yes, but this PR says otherwise. If not, it feels odd that we have a "state" object.

When authentication data changes, we need to renew the authentication state, and then we always get the correct data from that.

"state" is originalAuthState or authState.

No. We must check both the originalAuthState and authState, if you don't check, you will overstep your authority.

I agree that we currently check authorization on the proxy role and the original principal as well as with the proxy's authenticationDataSource and the originalAuthenticationDataSource. However, I am not sure why that is necessary, and as we've discussed, this is not even done correctly for the auth data source.

What is the case that a client will overstep its authority?

When the proxy is forwarding authentication data, it seems sufficient to verify that the proxy's authentication data is valid for a proxy as a one time check during connection initialization. All subsequent auth challenges will go to the client anyway.

If the proxy is unable to forward authentication data, then the broker will only have the proxy's authentication data and the originalPrincipal.

However, I am not sure why that is necessary, and as we've discussed, this is not even done correctly for the auth data source.

For the current design, the authenticationDataSource is credible by the proxy forwarding authentication data. This authentication data was completed in the proxy and then sent this authentication data to the broker, so we don't check the authentication data in the broker.

When the proxy is forwarding authentication data, it seems sufficient to verify that the proxy's authentication data is valid for a proxy as a one time check during connection initialization. All subsequent auth challenges will go to the client anyway.

I thought the same thing, but the Proxy's role usually is a superuser, if we don't check the client's role, this is overstepping its authority, and the client can get the lookup data and topic data.

If we don't consider this case, I agree with you.

No. We must check both the originalAuthState and authState, if you don't check, you will overstep your authority.

I agree that we currently check authorization on the proxy role and the original principal as well as with the proxy's authenticationDataSource and the originalAuthenticationDataSource. However, I am not sure why that is necessary, and as we've discussed, this is not even done correctly for the auth data source.

What is the case that a client will overstep its authority?

When the proxy is forwarding authentication data, it seems sufficient to verify that the proxy's authentication data is valid for a proxy as a one time check during connection initialization. All subsequent auth challenges will go to the client anyway.

If the proxy is unable to forward authentication data, then the broker will only have the proxy's authentication data and the originalPrincipal.

Right.

I thought the same thing, but the Proxy's role usually is a superuser, if we don't check the client's role, this is overstepping its authority, and the client can get the lookup data and topic data.

If we don't consider this case, I agree with you.

Can you describe the attack vector? From my perspective, I don't see how a client would be able to assume the proxy's role.

When the proxy is forwarding authentication data, it seems sufficient to verify that the proxy's authentication data is valid for a proxy as a one time check during connection initialization. All subsequent auth challenges will go to the client anyway.

This is ProxyConnectionToBroker operation, we also consider ProxyLookupRequests logic.

Can you describe the attack vector? From my perspective, I don't see how a client would be able to assume the proxy's role.

Would you like to pass only the proxy's authentication data? If yes, that's going to cross the line.

In proxy, the connection logic between ProxyLookupRequests and ProxyConnectionToBroker is different.

When the proxy is forwarding authentication data, it seems sufficient to verify that the proxy's authentication data is valid for a proxy as a one time check during connection initialization. All subsequent auth challenges will go to the client anyway.

This is ProxyConnectionToBroker operation, we also consider ProxyLookupRequests logic.

After #17831, when a proxy connection is in state ProxyLookupRequests with forwardAuthorizationCredentials=true, the proxy connection responds to the AuthChallenge with the original client's auth data by forwarding the broker's challenge to the client. That is why I said the auth challenges go to the client in that case.

Would you like to pass only the proxy's authentication data? If yes, that's going to cross the line.

No, that's not my suggestion. I am suggesting that when the proxy sends both its authData and the client's authData, the broker needs to only verify the proxy's authData at the beginning and at no other time because in all of those cases, the broker's auth challenge is going back to the original client and the proxy's authData is never again updated. This is the case because the proxy only forwards the client's auth data when forwardAuthorizationCredentials=true. One issue with my proposed solution is that the solution introduced in #17831 is not present in older versions of the proxy, so we'd need a way to address those proxies.

Good explanation!

michaeljmarshall · 2023-01-18T07:32:29Z

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

+                    originalAuthState =
+                            originalAuthenticationProvider.newAuthState(clientData, remoteAddress, sslSession);
+                } else {
+                    authState = authenticationProvider.newAuthState(clientData, remoteAddress, sslSession);


This does not feel right based on the AuthenticationState interface, which provides hooks for calls to authenticate. Can you provide additional motivation for this change?

When authentication data changes, we need to renew the authentication state, and then we always get the correct data from that.

I think the underlying problem is that calling authenticate does not update the AuthenticationDataSource in our token authentication provider. One solution is to recreate an auth state object. However, this has not been the implicit contract in the ServerCnx for as long as we've had the AuthenticationState. The contract has been that the AuthenticationState lasts the whole life of the connection. I think we should update the TokenAuthenticationState so that authenticate updates the AuthenticationDataSource.

Right. We can fix built-in plugins easily, but we also need to consider third-party plugins.

I actually don't think third-party plugins are going to be a problem here. The AuthenticationState object has lived the whole life of the ServerCnx already, so updating the TokenAuthenticationState#authenticate implementation shouldn't be a problem. Or, when you say "third-party plugins", are you referencing to those plugins relying on the TokenAuthenticationState? Even so, I think it's a pretty easy change, and I'd even consider it a bug since the AuthenticationDataSource is static even though we try to update it in the ServerCnx.

I created #19282 to start a concrete discussion.

Third-party plugins are the custom plug-in from users. I worry that breaks the user's plugin here if we don't recreate AuthenticationState.

I worry that breaks the user's plugin here if we don't recreate AuthenticationState.

We don't recreate the auth state now. Is there a specific case that you're worried about breaking?

We don't recreate the auth state now. Is there a specific case that you're worried about breaking?

Sure.

nodece · 2023-01-20T03:51:48Z

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

michaeljmarshall · 2023-01-20T05:38:45Z

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: #19292. I need to do some other work before tests will pass on that PR, so it is only a draft.

nodece · 2023-02-16T16:36:16Z

Closed by #19519.

nodece self-assigned this Oct 20, 2022

nodece added the ready-to-test label Oct 20, 2022

nodece requested review from eolivelli, merlimat, michaeljmarshall, tuteng, codelipenghui, mattisonchao and Technoboy- October 20, 2022 04:14

nodece added the area/broker label Oct 20, 2022

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 20, 2022

codelipenghui added this to the 2.12.0 milestone Oct 25, 2022

codelipenghui added release/2.9.4 release/2.11.1 release/2.10.3 labels Oct 25, 2022

tuteng requested changes Oct 27, 2022

View reviewed changes

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

codelipenghui added the type/bug The PR fixed a bug or issue reported a bug label Oct 27, 2022

codelipenghui reviewed Oct 27, 2022

View reviewed changes

codelipenghui requested a review from tuteng October 27, 2022 06:58

nodece removed release/2.9.4 release/2.10.3 labels Oct 27, 2022

nodece force-pushed the fix-update-authentication-data branch from dd7d9c8 to be12f6d Compare October 27, 2022 09:04

nodece requested a review from codelipenghui October 27, 2022 09:06

codelipenghui reviewed Oct 27, 2022

View reviewed changes

michaeljmarshall requested changes Oct 28, 2022

View reviewed changes

nodece removed the ready-to-test label Nov 2, 2022

nodece force-pushed the fix-update-authentication-data branch 3 times, most recently from 2c87e7c to c60b95b Compare November 2, 2022 07:58

nodece requested review from codelipenghui and michaeljmarshall November 4, 2022 09:24

gengmao mentioned this pull request Dec 14, 2022

PulsarProxy does not always refresh authentication tokens received from client #10816

Closed

nodece force-pushed the fix-update-authentication-data branch from c60b95b to def9d39 Compare December 15, 2022 08:41

michaeljmarshall reviewed Jan 10, 2023

View reviewed changes

nodece added 2 commits January 12, 2023 16:30

[fix][broker] Fix update authentication data

b3eb409

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

Fix review

357035f

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

nodece force-pushed the fix-update-authentication-data branch from def9d39 to 357035f Compare January 12, 2023 08:36

Improve write

631cd02

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

nodece requested a review from michaeljmarshall January 12, 2023 08:44

michaeljmarshall reviewed Jan 18, 2023

View reviewed changes

michaeljmarshall mentioned this pull request Jan 19, 2023

[Bug] Proxy does not forward credentials correctly for multi-staged authentication #19291

Open

2 tasks

sijie mentioned this pull request Jan 20, 2023

ISSUE-19291: [Bug] Proxy does not forward credentials correctly for multi-staged authentication streamnative/pulsar-archived#5439

Open

2 tasks

nodece mentioned this pull request Feb 14, 2023

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

Merged

5 tasks

nodece closed this Feb 16, 2023

		// credentials, but we only can new an authentication state, because some authentication data(TLS, SASL)
		// based on outside service.

	@Override
	public AuthData authenticate(AuthData authData) throws AuthenticationException {
	return pulsarSaslServer.response(authData);
	}

	return Commands.newConnect(authentication.getAuthMethodName(), authData, protocolVersion,
	PulsarVersion.getVersion(), proxyToTargetBrokerAddress, clientAuthRole, clientAuthData,
	clientAuthMethod);

	private void doAuthentication(AuthData clientData)
	throws Exception {
	AuthData brokerData = authState.authenticate(clientData);
	// authentication has completed, will send newConnected command.
	if (authState.isComplete()) {
	clientAuthRole = authState.getAuthRole();
	if (LOG.isDebugEnabled()) {
	LOG.debug("[{}] Client successfully authenticated with {} role {}",
	remoteAddress, authMethod, clientAuthRole);
	}

	// First connection
	if (this.connectionPool == null \|\| state == State.Connecting) {
	// authentication has completed, will send newConnected command.
	completeConnect(clientData);

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

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

Conversation

nodece commented Oct 20, 2022 • edited

Motivation

Modifications

Verifying this change

Documentation

Matching PR in forked repository

codecov-commenter commented Oct 20, 2022 • edited

Codecov Report

gengmao commented Oct 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodece Oct 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeljmarshall left a comment • edited

Choose a reason for hiding this comment

nodece commented Oct 28, 2022 • edited

michaeljmarshall commented Oct 28, 2022

nodece commented Oct 28, 2022

michaeljmarshall commented Oct 28, 2022 • edited

nodece commented Oct 28, 2022

nodece commented Nov 17, 2022

michaeljmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeljmarshall Jan 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodece Jan 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodece Jan 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodece commented Jan 20, 2023

michaeljmarshall commented Jan 20, 2023

nodece commented Feb 16, 2023

nodece commented Oct 20, 2022 •

edited

codecov-commenter commented Oct 20, 2022 •

edited

nodece Oct 27, 2022 •

edited

michaeljmarshall left a comment •

edited

nodece commented Oct 28, 2022 •

edited

michaeljmarshall commented Oct 28, 2022 •

edited

michaeljmarshall Jan 18, 2023 •

edited

nodece Jan 18, 2023 •

edited

nodece Jan 19, 2023 •

edited