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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
