You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2021/12/15 13:06:21 UTC
[GitHub] [pulsar] kkkoder opened a new pull request #13339: [Issue 10816][Proxy] Refresh client auth token
kkkoder opened a new pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339
Fixes #10816
### Motivation
See #10816
### Modifications
Refresh client token on proxy (to lookup) in case when refresh token command have been received from broker.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change added tests and can be verified as follows:
- org.apache.pulsar.proxy.server.ProxyWithJwtAuthorizationTest#testRefreshClientToken
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (no)
- The public API: (no)
- The schema: (no)
- The default values of configurations: (no)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
Check the box below and label this PR (if you have committer privilege).
Need to update docs?
- [x] `no-need-doc`
Bug fix
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] kkkoder commented on pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
kkkoder commented on pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#issuecomment-995070186
> @kkkoder let me open a PR for checkstyle fix
Ok, do it.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] kkoderok commented on a change in pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
kkoderok commented on a change in pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#discussion_r772111599
##########
File path: pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyWithJwtAuthorizationTest.java
##########
@@ -47,6 +41,12 @@
import org.testng.annotations.Test;
import javax.crypto.SecretKey;
+import java.time.Duration;
+import java.util.*;
Review comment:
ок
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#issuecomment-1051438820
The pr had no activity for 30 days, mark with Stale label.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#issuecomment-994832897
@kkkoder I think you do too much checkstyle format in this PR. I would suggest to split them if needed
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a change in pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#discussion_r836999618
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java
##########
@@ -240,16 +253,23 @@ public void channelActive(ChannelHandlerContext ctx) throws Exception {
ctx.close();
}
});
+ } else {
+ log.warn("Error during handshake", th);
+ ctx.close();
+ }
+ });
}
- protected ByteBuf newConnectCommand() throws Exception {
+ protected CompletableFuture<ByteBuf> newConnectCommand() throws Exception {
// mutual authentication is to auth between `remoteHostName` and this client for this channel.
// each channel will have a mutual client/server pair, mutual client evaluateChallenge with init data,
// and return authData to server.
authenticationDataProvider = authentication.getAuthData(remoteHostName);
AuthData authData = authenticationDataProvider.authenticate(AuthData.INIT_AUTH_DATA);
- return Commands.newConnect(authentication.getAuthMethodName(), authData, this.protocolVersion,
- PulsarVersion.getVersion(), proxyToTargetBrokerAddress, null, null, null);
+ return CompletableFuture.completedFuture(
Review comment:
What's the point of using CompletableFuture here ? It seems there is no async operations.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#discussion_r837179928
##########
File path: pulsar-client/src/main/java/org/apache/pulsar/client/impl/ClientCnx.java
##########
@@ -240,16 +253,23 @@ public void channelActive(ChannelHandlerContext ctx) throws Exception {
ctx.close();
}
});
+ } else {
+ log.warn("Error during handshake", th);
+ ctx.close();
+ }
+ });
}
- protected ByteBuf newConnectCommand() throws Exception {
+ protected CompletableFuture<ByteBuf> newConnectCommand() throws Exception {
// mutual authentication is to auth between `remoteHostName` and this client for this channel.
// each channel will have a mutual client/server pair, mutual client evaluateChallenge with init data,
// and return authData to server.
authenticationDataProvider = authentication.getAuthData(remoteHostName);
AuthData authData = authenticationDataProvider.authenticate(AuthData.INIT_AUTH_DATA);
- return Commands.newConnect(authentication.getAuthMethodName(), authData, this.protocolVersion,
- PulsarVersion.getVersion(), proxyToTargetBrokerAddress, null, null, null);
+ return CompletableFuture.completedFuture(
Review comment:
good point
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyClientCnx.java
##########
@@ -20,43 +20,93 @@
import io.netty.buffer.ByteBuf;
import io.netty.channel.EventLoopGroup;
+import java.util.Arrays;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.Supplier;
import org.apache.pulsar.PulsarVersion;
import org.apache.pulsar.client.impl.ClientCnx;
import org.apache.pulsar.client.impl.conf.ClientConfigurationData;
import org.apache.pulsar.common.api.AuthData;
+import org.apache.pulsar.common.api.proto.CommandAuthChallenge;
import org.apache.pulsar.common.protocol.Commands;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
public class ProxyClientCnx extends ClientCnx {
- String clientAuthRole;
- AuthData clientAuthData;
- String clientAuthMethod;
- int protocolVersion;
+ private String clientAuthRole;
+ private String clientAuthMethod;
+ private int protocolVersion;
+ private boolean forwardAuthorizationCredentials;
+ private Supplier<CompletableFuture<AuthData>> clientAuthDataSupplier;
public ProxyClientCnx(ClientConfigurationData conf, EventLoopGroup eventLoopGroup, String clientAuthRole,
- AuthData clientAuthData, String clientAuthMethod, int protocolVersion) {
+ Supplier<CompletableFuture<AuthData>> clientAuthDataSupplier,
+ String clientAuthMethod, int protocolVersion, boolean forwardAuthorizationCredentials) {
super(conf, eventLoopGroup);
this.clientAuthRole = clientAuthRole;
- this.clientAuthData = clientAuthData;
this.clientAuthMethod = clientAuthMethod;
this.protocolVersion = protocolVersion;
+ this.forwardAuthorizationCredentials = forwardAuthorizationCredentials;
+ this.clientAuthDataSupplier = clientAuthDataSupplier;
}
@Override
- protected ByteBuf newConnectCommand() throws Exception {
- if (log.isDebugEnabled()) {
- log.debug("New Connection opened via ProxyClientCnx with params clientAuthRole = {},"
- + " clientAuthData = {}, clientAuthMethod = {}",
+ protected CompletableFuture<ByteBuf> newConnectCommand() throws Exception {
+ authenticationDataProvider = authentication.getAuthData(remoteHostName);
+ AuthData authData = authenticationDataProvider.authenticate(AuthData.INIT_AUTH_DATA);
+
+ return clientAuthDataSupplier.get().thenApply(clientAuthData -> {
+ if (log.isDebugEnabled()) {
+ log.debug("New Connection opened via ProxyClientCnx with params clientAuthRole = {},"
+ + " clientAuthData = {}, clientAuthMethod = {}",
clientAuthRole, clientAuthData, clientAuthMethod);
+ }
+
+ return Commands.newConnect(authentication.getAuthMethodName(), authData, this.protocolVersion,
+ PulsarVersion.getVersion(), proxyToTargetBrokerAddress, clientAuthRole, clientAuthData,
+ clientAuthMethod);
+ });
+ }
+
+ @Override
+ protected void handleAuthChallenge(CommandAuthChallenge authChallenge) {
+ boolean isRefresh = Arrays.equals(
+ AuthData.REFRESH_AUTH_DATA_BYTES,
+ authChallenge.getChallenge().getAuthData()
+ );
+
+ if (!forwardAuthorizationCredentials || !isRefresh) {
+ super.handleAuthChallenge(authChallenge);
+ return;
}
- authenticationDataProvider = authentication.getAuthData(remoteHostName);
- AuthData authData = authenticationDataProvider.authenticate(AuthData.INIT_AUTH_DATA);
- return Commands.newConnect(authentication.getAuthMethodName(), authData, this.protocolVersion,
- PulsarVersion.getVersion(), proxyToTargetBrokerAddress, clientAuthRole, clientAuthData,
- clientAuthMethod);
+ try {
+ clientAuthDataSupplier.get()
+ .thenAccept(authData -> sendAuthResponse(authData, clientAuthMethod));
+ } catch (Exception e) {
Review comment:
how is it possible to have an Exception here ?
in case of failure we should deal with the error and send a response or close the connection
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hsaputra commented on a change in pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
hsaputra commented on a change in pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#discussion_r836990591
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java
##########
@@ -439,11 +451,65 @@ ClientConfigurationData createClientConfiguration() throws UnsupportedAuthentica
private PulsarClientImpl createClient(final ClientConfigurationData clientConf, final AuthData clientAuthData,
final String clientAuthMethod, final int protocolVersion) throws PulsarClientException {
this.connectionPool = new ProxyConnectionPool(clientConf, service.getWorkerGroup(),
- () -> new ProxyClientCnx(clientConf, service.getWorkerGroup(), clientAuthRole, clientAuthData,
- clientAuthMethod, protocolVersion));
+ () -> new ProxyClientCnx(clientConf, service.getWorkerGroup(), clientAuthRole,
+ this::getOrRefreshClientAuthData, clientAuthMethod, protocolVersion,
+ service.getConfiguration().isForwardAuthorizationCredentials()));
return new PulsarClientImpl(clientConf, service.getWorkerGroup(), connectionPool, service.getTimer());
}
+ private void updateClientAuthData(AuthData clientData) {
+ this.clientAuthData = clientData;
+ authFutureList.getAndSet(Collections.emptyList())
+ .forEach(future -> future.complete(clientData));
+ }
+
+ private CompletableFuture<AuthData> getOrRefreshClientAuthData() {
Review comment:
Could you walk me through what this method suppose to do? Thanks
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#issuecomment-995332646
@kkkoder I have done in #13343, could you please pull master code, and undo the format, thanks :)
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sursingh commented on a change in pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
sursingh commented on a change in pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#discussion_r839797205
##########
File path: pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyWithJwtAuthorizationTest.java
##########
@@ -363,6 +374,72 @@ public void testProxyAuthorizationWithPrefixSubscriptionAuthMode() throws Except
log.info("-- Exiting {} test --", methodName);
}
+ @Test
+ void testRefreshClientToken() throws Exception {
+ log.info("-- Starting {} test --", methodName);
+
+ startProxy();
+ createAdminClient();
+
+ @SuppressWarnings("unchecked")
+ Supplier<String> tokenSupplier = Mockito.mock(Supplier.class);
+ when(tokenSupplier.get()).thenAnswer(answer -> createClientJwtToken(Duration.ofSeconds(1)));
+
+ PulsarClient proxyClient = PulsarClient.builder()
+ .serviceUrl(proxyService.getServiceUrl()).statsInterval(0, TimeUnit.SECONDS)
+ .authentication(AuthenticationFactory.token(tokenSupplier))
+ .operationTimeout(1000, TimeUnit.MILLISECONDS)
+ .build();
+
+ String namespaceName = "my-property/proxy-authorization/my-ns";
+ admin.clusters().createCluster("proxy-authorization", ClusterData.builder().serviceUrl(brokerUrl.toString()).build());
+ admin.tenants().createTenant("my-property",
+ new TenantInfoImpl(Sets.newHashSet("appid1", "appid2"), Sets.newHashSet("proxy-authorization")));
+ admin.namespaces().createNamespace(namespaceName);
+
+ admin.namespaces().grantPermissionOnNamespace(namespaceName, CLIENT_ROLE,
+ Sets.newHashSet(AuthAction.consume, AuthAction.produce));
+ log.info("-- Admin permissions {} ---", admin.namespaces().getPermissions(namespaceName));
+
+ Producer<byte[]> producer = proxyClient.newProducer(Schema.BYTES)
+ .topic("persistent://my-property/proxy-authorization/my-ns/my-topic1").create();
+
+ final int msgs = 10;
+ for (int i = 0; i < msgs; i++) {
+ String message = "my-message-" + i;
+ producer.send(message.getBytes());
+ }
+
+ //noinspection unchecked
+ clearInvocations(tokenSupplier);
+ Thread.sleep(3000);
+ verify(tokenSupplier, atLeastOnce()).get();
Review comment:
I don't think this test is correct. It is passing for us, even without the associated fix.
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on a change in pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#discussion_r771804713
##########
File path: pulsar-proxy/src/test/java/org/apache/pulsar/proxy/server/ProxyWithJwtAuthorizationTest.java
##########
@@ -47,6 +41,12 @@
import org.testng.annotations.Test;
import javax.crypto.SecretKey;
+import java.time.Duration;
+import java.util.*;
Review comment:
suggest not use star imports
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Shoothzj commented on pull request #13339: [Issue 10816][Proxy] Refresh client auth token
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #13339:
URL: https://github.com/apache/pulsar/pull/13339#issuecomment-994860835
@kkkoder let me open a PR for checkstyle fix
--
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-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org