michaeljmarshall commented on code in PR #18130: URL: https://github.com/apache/pulsar/pull/18130#discussion_r1080838475
########## 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. Review Comment: > 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: https://github.com/apache/pulsar/blob/d6fcdb8e1e192e0d9e2fbe6307d273b27ed2930f/pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java#L426-L440 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 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: commits-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org