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 2022/05/17 10:21:52 UTC
[GitHub] [pulsar] nodece opened a new pull request, #15634: [PIP-158][improve][client] Split client TLS transport encryption from authentication
nodece opened a new pull request, #15634:
URL: https://github.com/apache/pulsar/pull/15634
Fixes #15289
### Motivation
The client supports TLS transport encryption and TLS authentication, this code so like:
```java
PulsarClient client = PulsarClient.builder()
.serviceUrl("pulsar+ssl://localhost:6651")
.tlsTrustCertsFilePath("/path/to/cacert.pem")
.authentication(AuthenticationTls.class.getName(), authParams)
.build()
```
We are using AuthenticationTls to perform the TLS transport encryption and TLS authentication, this causes an issue that cannot use other authentication with TLS transport encryption, and also made our confusion if we use TLS transport encryption by setting `authentication`.
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [x] `doc-required`
(Your PR needs to update docs and you will update later)
--
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] codelipenghui merged pull request #15634: [PIP-158][improve][client] Split client TLS transport encryption from authentication
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #15634:
URL: https://github.com/apache/pulsar/pull/15634
--
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] nodece commented on a diff in pull request #15634: [PIP-158][improve][client] Split client TLS transport encryption from authentication
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15634:
URL: https://github.com/apache/pulsar/pull/15634#discussion_r909511411
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/NettyClientSslContextRefresher.java:
##########
@@ -46,6 +46,8 @@ public class NettyClientSslContextRefresher extends SslContextAutoRefreshBuilder
public NettyClientSslContextRefresher(SslProvider sslProvider, boolean allowInsecure,
String trustCertsFilePath,
+ String certsFilePath,
+ String keyFilePath,
Review Comment:
This PR just improves the client SDK, we have the built-in client/admin in the broker/proxy, these also need this improvement.
--
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] BewareMyPower commented on a diff in pull request #15634: [PIP-158][improve][client] Split client TLS transport encryption from authentication
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15634:
URL: https://github.com/apache/pulsar/pull/15634#discussion_r909554405
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/NettyClientSslContextRefresher.java:
##########
@@ -46,6 +46,8 @@ public class NettyClientSslContextRefresher extends SslContextAutoRefreshBuilder
public NettyClientSslContextRefresher(SslProvider sslProvider, boolean allowInsecure,
String trustCertsFilePath,
+ String certsFilePath,
+ String keyFilePath,
Review Comment:
But it looks like the built-in client or admin doesn't pass any non-null arguments to these two arguments as well. Why do they need this two extra parameters? I mean, all references pass `null` arguments, they are meaningless.
--
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] nodece commented on a diff in pull request #15634: [PIP-158][improve][client] Split client TLS transport encryption from authentication
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15634:
URL: https://github.com/apache/pulsar/pull/15634#discussion_r910143761
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/NettyClientSslContextRefresher.java:
##########
@@ -46,6 +46,8 @@ public class NettyClientSslContextRefresher extends SslContextAutoRefreshBuilder
public NettyClientSslContextRefresher(SslProvider sslProvider, boolean allowInsecure,
String trustCertsFilePath,
+ String certsFilePath,
+ String keyFilePath,
Review Comment:
Fixed.
--
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] nodece commented on a diff in pull request #15634: [PIP-158][improve][client] Split client TLS transport encryption from authentication
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15634:
URL: https://github.com/apache/pulsar/pull/15634#discussion_r909511913
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/KeyStoreTlsProducerConsumerTestWithAuthTest.java:
##########
@@ -92,6 +101,12 @@ protected void internalSetUpForBroker() {
conf.setAuthorizationEnabled(true);
Set<String> providers = new HashSet<>();
providers.add(AuthenticationProviderTls.class.getName());
+
+ Properties properties = new Properties();
+ properties.setProperty("tokenSecretKey", JWT_SECRET_PATH);
Review Comment:
Great catch!
--
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] BewareMyPower commented on a diff in pull request #15634: [PIP-158][improve][client] Split client TLS transport encryption from authentication
Posted by GitBox <gi...@apache.org>.
BewareMyPower commented on code in PR #15634:
URL: https://github.com/apache/pulsar/pull/15634#discussion_r909404624
##########
pulsar-common/src/main/java/org/apache/pulsar/common/util/NettyClientSslContextRefresher.java:
##########
@@ -46,6 +46,8 @@ public class NettyClientSslContextRefresher extends SslContextAutoRefreshBuilder
public NettyClientSslContextRefresher(SslProvider sslProvider, boolean allowInsecure,
String trustCertsFilePath,
+ String certsFilePath,
+ String keyFilePath,
Review Comment:
These two new parameters always accept `null` in all references, could you explain why did you add them?
##########
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/KeyStoreTlsProducerConsumerTestWithAuthTest.java:
##########
@@ -92,6 +101,12 @@ protected void internalSetUpForBroker() {
conf.setAuthorizationEnabled(true);
Set<String> providers = new HashSet<>();
providers.add(AuthenticationProviderTls.class.getName());
+
+ Properties properties = new Properties();
+ properties.setProperty("tokenSecretKey", JWT_SECRET_PATH);
Review Comment:
Instead of specifying a path of a manually generated secret key file, you can just generate the key in code.
```java
SecretKey secretKey = AuthTokenUtils.createSecretKey(SignatureAlgorithm.HS256);
Properties properties = new Properties();
properties.setProperty("tokenSecretKey", AuthTokenUtils.encodeKeyBase64(secretKey));
```
And the `CLIENTUSER_TOKEN` can be replaced by a non-final field like:
```java
userToken = AuthTokenUtils.createToken(secretKey, "user", Optional.empty())
```
--
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