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/04/14 09:57:19 UTC
[GitHub] [pulsar] nodece opened a new pull request, #15173: [cleanup][client] Cleanup the creation ssl context
nodece opened a new pull request, #15173:
URL: https://github.com/apache/pulsar/pull/15173
Signed-off-by: Zixuan Liu <no...@gmail.com>
### Motivation
Remove duplicate code.
### Modifications
Packaging the create ssl context to ClientConfigurationData.
### Documentation
- [x] `no-need-doc`
--
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 #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#discussion_r851167523
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -114,58 +107,14 @@ public boolean keepAlive(InetSocketAddress remoteAddress, Request ahcRequest,
});
serviceNameResolver = new PulsarServiceNameResolver();
- if (conf != null && StringUtils.isNotBlank(conf.getServiceUrl())) {
+ if (StringUtils.isNotBlank(conf.getServiceUrl())) {
serviceNameResolver.updateServiceUrl(conf.getServiceUrl());
if (conf.getServiceUrl().startsWith("https://")) {
- // Set client key and certificate if available
- AuthenticationDataProvider authData = conf.getAuthentication().getAuthData();
-
if (conf.isUseKeyStoreTls()) {
- KeyStoreParams params = authData.hasDataForTls() ? authData.getTlsKeyStoreParams() : null;
-
- final SSLContext sslCtx = KeyStoreSSLContext.createClientSslContext(
- conf.getSslProvider(),
- params != null ? params.getKeyStoreType() : null,
- params != null ? params.getKeyStorePath() : null,
- params != null ? params.getKeyStorePassword() : null,
- conf.isTlsAllowInsecureConnection() || !conf.isTlsHostnameVerificationEnable(),
Review Comment:
The `isTlsHostnameVerificationEnable()` cannot be used here, this is incorrect behavior.
--
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 #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#issuecomment-1213634010
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] nodece commented on a diff in pull request #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#discussion_r851089768
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -114,58 +107,14 @@ public boolean keepAlive(InetSocketAddress remoteAddress, Request ahcRequest,
});
serviceNameResolver = new PulsarServiceNameResolver();
- if (conf != null && StringUtils.isNotBlank(conf.getServiceUrl())) {
+ if (StringUtils.isNotBlank(conf.getServiceUrl())) {
Review Comment:
It was always non-null.
--
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 #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#discussion_r851167523
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -114,58 +107,14 @@ public boolean keepAlive(InetSocketAddress remoteAddress, Request ahcRequest,
});
serviceNameResolver = new PulsarServiceNameResolver();
- if (conf != null && StringUtils.isNotBlank(conf.getServiceUrl())) {
+ if (StringUtils.isNotBlank(conf.getServiceUrl())) {
serviceNameResolver.updateServiceUrl(conf.getServiceUrl());
if (conf.getServiceUrl().startsWith("https://")) {
- // Set client key and certificate if available
- AuthenticationDataProvider authData = conf.getAuthentication().getAuthData();
-
if (conf.isUseKeyStoreTls()) {
- KeyStoreParams params = authData.hasDataForTls() ? authData.getTlsKeyStoreParams() : null;
-
- final SSLContext sslCtx = KeyStoreSSLContext.createClientSslContext(
- conf.getSslProvider(),
- params != null ? params.getKeyStoreType() : null,
- params != null ? params.getKeyStorePath() : null,
- params != null ? params.getKeyStorePassword() : null,
- conf.isTlsAllowInsecureConnection() || !conf.isTlsHostnameVerificationEnable(),
Review Comment:
The `isTlsHostnameVerificationEnable()` cannot used here.
When TrustManager is set, we can verify the tls host name.
--
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 #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#discussion_r851167523
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -114,58 +107,14 @@ public boolean keepAlive(InetSocketAddress remoteAddress, Request ahcRequest,
});
serviceNameResolver = new PulsarServiceNameResolver();
- if (conf != null && StringUtils.isNotBlank(conf.getServiceUrl())) {
+ if (StringUtils.isNotBlank(conf.getServiceUrl())) {
serviceNameResolver.updateServiceUrl(conf.getServiceUrl());
if (conf.getServiceUrl().startsWith("https://")) {
- // Set client key and certificate if available
- AuthenticationDataProvider authData = conf.getAuthentication().getAuthData();
-
if (conf.isUseKeyStoreTls()) {
- KeyStoreParams params = authData.hasDataForTls() ? authData.getTlsKeyStoreParams() : null;
-
- final SSLContext sslCtx = KeyStoreSSLContext.createClientSslContext(
- conf.getSslProvider(),
- params != null ? params.getKeyStoreType() : null,
- params != null ? params.getKeyStorePath() : null,
- params != null ? params.getKeyStorePassword() : null,
- conf.isTlsAllowInsecureConnection() || !conf.isTlsHostnameVerificationEnable(),
Review Comment:
The `isTlsHostnameVerificationEnable()` cannot be used here.
Only when `isTlsHostnameVerificationEnable()` is `true` and `isTlsAllowInsecureConnection()` is `false`, we can use secure trust manager, this is incorrect behavior.
--
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 closed pull request #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
nodece closed pull request #15173: [cleanup][client] Cleanup the creation ssl context
URL: https://github.com/apache/pulsar/pull/15173
--
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 #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#discussion_r851167523
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -114,58 +107,14 @@ public boolean keepAlive(InetSocketAddress remoteAddress, Request ahcRequest,
});
serviceNameResolver = new PulsarServiceNameResolver();
- if (conf != null && StringUtils.isNotBlank(conf.getServiceUrl())) {
+ if (StringUtils.isNotBlank(conf.getServiceUrl())) {
serviceNameResolver.updateServiceUrl(conf.getServiceUrl());
if (conf.getServiceUrl().startsWith("https://")) {
- // Set client key and certificate if available
- AuthenticationDataProvider authData = conf.getAuthentication().getAuthData();
-
if (conf.isUseKeyStoreTls()) {
- KeyStoreParams params = authData.hasDataForTls() ? authData.getTlsKeyStoreParams() : null;
-
- final SSLContext sslCtx = KeyStoreSSLContext.createClientSslContext(
- conf.getSslProvider(),
- params != null ? params.getKeyStoreType() : null,
- params != null ? params.getKeyStorePath() : null,
- params != null ? params.getKeyStorePassword() : null,
- conf.isTlsAllowInsecureConnection() || !conf.isTlsHostnameVerificationEnable(),
Review Comment:
The `isTlsHostnameVerificationEnable()` cannot be used here.
Only when `isTlsHostnameVerificationEnable()` is `true` and `isTlsAllowInsecureConnection()`is `false`, we can use secure trust manager, this is incorrect behavior.
--
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 #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#issuecomment-1159601634
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] nodece commented on pull request #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
nodece commented on PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#issuecomment-1183012821
Hi @michaeljmarshall, this CI has been passed, could you review this PR?
--
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 #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#discussion_r851167523
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -114,58 +107,14 @@ public boolean keepAlive(InetSocketAddress remoteAddress, Request ahcRequest,
});
serviceNameResolver = new PulsarServiceNameResolver();
- if (conf != null && StringUtils.isNotBlank(conf.getServiceUrl())) {
+ if (StringUtils.isNotBlank(conf.getServiceUrl())) {
serviceNameResolver.updateServiceUrl(conf.getServiceUrl());
if (conf.getServiceUrl().startsWith("https://")) {
- // Set client key and certificate if available
- AuthenticationDataProvider authData = conf.getAuthentication().getAuthData();
-
if (conf.isUseKeyStoreTls()) {
- KeyStoreParams params = authData.hasDataForTls() ? authData.getTlsKeyStoreParams() : null;
-
- final SSLContext sslCtx = KeyStoreSSLContext.createClientSslContext(
- conf.getSslProvider(),
- params != null ? params.getKeyStoreType() : null,
- params != null ? params.getKeyStorePath() : null,
- params != null ? params.getKeyStorePassword() : null,
- conf.isTlsAllowInsecureConnection() || !conf.isTlsHostnameVerificationEnable(),
Review Comment:
The `isTlsHostnameVerificationEnable()` cannot used here.
--
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 #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#issuecomment-1131011641
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] michaeljmarshall commented on pull request #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#issuecomment-1179472286
@nodece - thanks for your contribution. This PR has merge conflicts. Are you able to resolve those, and then I'll review this?
--
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] HQebupt commented on a diff in pull request #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
HQebupt commented on code in PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#discussion_r851010002
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -114,58 +107,14 @@ public boolean keepAlive(InetSocketAddress remoteAddress, Request ahcRequest,
});
serviceNameResolver = new PulsarServiceNameResolver();
- if (conf != null && StringUtils.isNotBlank(conf.getServiceUrl())) {
+ if (StringUtils.isNotBlank(conf.getServiceUrl())) {
Review Comment:
It seems that the` ClientConfigurationData conf` may be null in `PulsarAdmin`. Shall you keep the judgement to avoid NPE ?
--
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 #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#discussion_r851094552
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -114,58 +107,14 @@ public boolean keepAlive(InetSocketAddress remoteAddress, Request ahcRequest,
});
serviceNameResolver = new PulsarServiceNameResolver();
- if (conf != null && StringUtils.isNotBlank(conf.getServiceUrl())) {
+ if (StringUtils.isNotBlank(conf.getServiceUrl())) {
Review Comment:
@HQebupt
--
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 #15173: [cleanup][client] Cleanup the creation ssl context
Posted by GitBox <gi...@apache.org>.
nodece commented on code in PR #15173:
URL: https://github.com/apache/pulsar/pull/15173#discussion_r851167523
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/http/AsyncHttpConnector.java:
##########
@@ -114,58 +107,14 @@ public boolean keepAlive(InetSocketAddress remoteAddress, Request ahcRequest,
});
serviceNameResolver = new PulsarServiceNameResolver();
- if (conf != null && StringUtils.isNotBlank(conf.getServiceUrl())) {
+ if (StringUtils.isNotBlank(conf.getServiceUrl())) {
serviceNameResolver.updateServiceUrl(conf.getServiceUrl());
if (conf.getServiceUrl().startsWith("https://")) {
- // Set client key and certificate if available
- AuthenticationDataProvider authData = conf.getAuthentication().getAuthData();
-
if (conf.isUseKeyStoreTls()) {
- KeyStoreParams params = authData.hasDataForTls() ? authData.getTlsKeyStoreParams() : null;
-
- final SSLContext sslCtx = KeyStoreSSLContext.createClientSslContext(
- conf.getSslProvider(),
- params != null ? params.getKeyStoreType() : null,
- params != null ? params.getKeyStorePath() : null,
- params != null ? params.getKeyStorePassword() : null,
- conf.isTlsAllowInsecureConnection() || !conf.isTlsHostnameVerificationEnable(),
Review Comment:
The `isTlsHostnameVerificationEnable()` cannot used here`.
--
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