nodece commented on code in PR #18130:
URL: https://github.com/apache/pulsar/pull/18130#discussion_r1080774793
##########
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:
> 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.
--
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]
