nodece commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1073268889
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -707,73 +709,96 @@ private void completeConnect(int clientProtoVersion,
String clientVersion, boole
}
// According to auth result, send newConnected or newAuthChallenge command.
- private State doAuthentication(AuthData clientData,
- int clientProtocolVersion,
- String clientVersion) throws Exception {
-
+ private CompletableFuture<Void> doAuthenticationAsync(AuthData clientData,
int clientProtocolVersion,
+ String
clientVersion) {
// The original auth state can only be set on subsequent auth attempts
(and only
// in presence of a proxy and if the proxy is forwarding the
credentials).
// In this case, the re-validation needs to be done against the
original client
- // credentials.
- boolean useOriginalAuthState = (originalAuthState != null);
- AuthenticationState authState = useOriginalAuthState ?
originalAuthState : this.authState;
- String authRole = useOriginalAuthState ? originalPrincipal :
this.authRole;
- AuthData brokerData = authState.authenticate(clientData);
-
- if (log.isDebugEnabled()) {
- log.debug("Authenticate using original auth state : {}, role =
{}", useOriginalAuthState, authRole);
- }
+ // credentials, but we only can new an authentication state, because
some authentication data(TLS, SASL)
+ // based on outside service.
+ // If we can get the role from the authentication sate, the global
variable need to be updated.
- if (authState.isComplete()) {
- // Authentication has completed. It was either:
- // 1. the 1st time the authentication process was done, in which
case we'll send
- // a `CommandConnected` response
- // 2. an authentication refresh, in which case we need to refresh
authenticationData
-
- String newAuthRole = authState.getAuthRole();
-
- // Refresh the auth data.
- this.authenticationData = authState.getAuthDataSource();
Review Comment:
All of your description is correct.
> 1. 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
https://github.com/apache/pulsar/issues/17517
> 2. 1. 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.
> 2. 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
