Fixed authentication flow via Pulsar Proxy #1707
Conversation
|
@jai1 This doesn't seem to me a super-critical error to stop the current RC. I think we can bump to 2.0.1 where we'll get all fixes (and we should do soon). |
|
@merlimat - ok changed the milestone to 2.0.1, can you review this PR |
| @Override | ||
| protected ByteBuf newConnectCommand() throws PulsarClientException { | ||
| if (log.isDebugEnabled()) { | ||
| log.info( |
| "New Connection opened via ProxyClientCnx with params clientAuthRole = {}, clientAuthData = {}, clientAuthMethod = {}", | ||
| clientAuthRole, clientAuthData, clientAuthMethod); | ||
| } | ||
| String authData = ""; |
There was a problem hiding this comment.
can we initialize with null?
| try { | ||
| client.close(); | ||
| } catch (PulsarClientException e) { | ||
| LOG.error("Unable to clode pulsar client - {}", client); |
There was a problem hiding this comment.
type close and add e.getMessage() in log
| this.clientAuthData = authData; | ||
| this.clientAuthMethod = authMethod; | ||
| } | ||
| createClient(clientConf, this.clientAuthData, this.clientAuthMethod); |
There was a problem hiding this comment.
as name suggests. can we create and return client in createClient(): this.client = createClient(..)
There was a problem hiding this comment.
also we don't want to share proxy-to-broker connection when service.getConfiguration().forwardAuthorizationCredentials() is enabled. else, we can always share proxy-to-broker connection for lookup.
so, should we create a new client only in case of service.getConfiguration().forwardAuthorizationCredentials()=true else we can always share it.?
| proxyService.close(); | ||
| } | ||
|
|
||
| private void createAdminClient() throws PulsarClientException { |
There was a problem hiding this comment.
createAdminClient() name seems misleading as we are not closing admin and we may think it's leaking. so, probably we can rename it as updateAdmin()?
| } | ||
| } | ||
|
|
||
| private int webServicePort; |
| @@ -65,15 +60,10 @@ | |||
| private ZooKeeperClientFactory zkClientFactory = null; | |||
|
|
|||
| private final EventLoopGroup acceptorGroup; | |||
| private final EventLoopGroup workerGroup; | |||
| final EventLoopGroup workerGroup; | |||
Currently, the broker/proxy authenticates the proxy/client when the connection is established (via
Server.handleConnectorProxyConnection.handleConnect) after that we cache theauthRolewe extract. The connection persists long after theauthDataexpires since we use the cachedauthRolethen onwards.In #1169 in order to preserve the Connection pool in ProxyLookupHandler, I changed the proxy code to cache the client
authDataand forward it to authenticate duringlookupsandgetPartitionMetaDatawhat I failed to see was that if the clientauthDataexpires broker will reject the lookups, since we are sending old ClientauthData.The fix for this is that for lookups instead of maintaining a single connection pool in proxy we maintain one connection pool per client connection and authenticate both the client and the proxy only when a new connection is created.