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/01/13 11:19:53 UTC
[GitHub] [pulsar] nodece opened a new pull request #13740: [Broker] Fix config ciphers and protocols to web server
nodece opened a new pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740
Signed-off-by: Zixuan Liu <no...@gmail.com>
Fixes #13734
### Motivation
Pulsar doesn't config ciphers and protocols to the web server when `tlsEnabledWithKeyStore=false`.
### Modifications
- Add `JettySslContextFactory.class` supports create server SSL context
- Add the `tlsCiphers` and `tlsProtocols` checks when create web server/service
### Documentation
Need to update docs?
- [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 pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1053808043
> My question now is: is Conscript more secure? In this case I prefer it as default as we mostly care about production for security.
Maybe Conscript is more secure with JDK 11 in production OS such as CentOS, Debian, etc., but we can't just consider the production environment. For example, I want to use Pulsar in Win 8, it is not working.
> Security is better then being user friendly with people on old systems.
Software compatibility is more important than security. For example, some users want to test Pulsar with TLS in Win 7/8 or macOS 11.5.2, the Conscript cannot work, but the use JDK provider is working.
> If Conscript as default, it is looks does not add value then I am fine to change the default
I prefer to use the JDK provider as default, you can also see that there has been feedback from the issue.
--
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 edited a comment on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1053808043
> My question now is: is Conscript more secure? In this case I prefer it as default as we mostly care about production for security.
> Security is better then being user friendly with people on old systems.
Maybe Conscript is more secure with JDK 11 in production OS such as CentOS, Debian, etc., but we can't just consider the production environment. Software compatibility is more important than security. For example, some users want to test Pulsar with TLS in Win 7/8 or macOS 11.5.2, the Conscript cannot work, but the use JDK provider is working.
> If Conscript as default, it is looks does not add value then I am fine to change the default
I prefer to use the JDK provider as default, you can also see that there has been feedback from the issue.
--
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 #13740: [Broker] Fix config ciphers and protocols to web server
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1013049580
/pulsarbot run-failures-check
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1018635440
@lhotari Thanks for your review!
--
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 commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1018523385
Thanks @lhotari , I will remove the release labels for 2.8 and 2.9
--
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 removed a comment on pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece removed a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1063641230
/pulsarbot run-failure-checks
--
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 #13740: [Broker] Fix config ciphers and protocols to web server
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1013841318
/pulsarbot run-failure-checks
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r786391289
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/jetty/package-info.java
##########
@@ -0,0 +1,20 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pulsar.common.util.jetty;
Review comment:
better to have a new line end
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/jetty/JettySslContextFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.util.jetty;
+
+import java.util.Set;
+import javax.net.ssl.SSLContext;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.common.util.DefaultSslContextBuilder;
+import org.apache.pulsar.common.util.SslContextAutoRefreshBuilder;
+import org.apache.pulsar.common.util.keystoretls.NetSslContextBuilder;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+
+@Slf4j
+public class JettySslContextFactory {
+ public static SslContextFactory.Server createServerSslContextWithKeystore(String sslProviderString,
+ String keyStoreTypeString,
+ String keyStore,
+ String keyStorePassword,
+ boolean allowInsecureConnection,
+ String trustStoreTypeString,
+ String trustStore,
+ String trustStorePassword,
+ boolean requireTrustedClientCertOnConnect,
+ Set<String> ciphers,
+ Set<String> protocols,
+ long certRefreshInSec) {
+ NetSslContextBuilder sslCtxRefresher = new NetSslContextBuilder(
Review comment:
can we merge this util class with `org.apache.pulsar.common.util.keystoretls.org.apache.pulsar.common.util.keystoretls`.
They are functionally similar.
--
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 change in pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r786582424
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/jetty/JettySslContextFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.util.jetty;
+
+import java.util.Set;
+import javax.net.ssl.SSLContext;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.common.util.DefaultSslContextBuilder;
+import org.apache.pulsar.common.util.SslContextAutoRefreshBuilder;
+import org.apache.pulsar.common.util.keystoretls.NetSslContextBuilder;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+
+@Slf4j
+public class JettySslContextFactory {
+ public static SslContextFactory.Server createServerSslContextWithKeystore(String sslProviderString,
+ String keyStoreTypeString,
+ String keyStore,
+ String keyStorePassword,
+ boolean allowInsecureConnection,
+ String trustStoreTypeString,
+ String trustStore,
+ String trustStorePassword,
+ boolean requireTrustedClientCertOnConnect,
+ Set<String> ciphers,
+ Set<String> protocols,
+ long certRefreshInSec) {
+ NetSslContextBuilder sslCtxRefresher = new NetSslContextBuilder(
Review comment:
I think made a `jetty` package is better, which only provides create the SSL context for the Jetty server.
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1032226417
@mattisonchao Could you 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] nodece commented on a change in pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820470641
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java
##########
@@ -38,6 +38,7 @@
import lombok.Getter;
import lombok.extern.slf4j.Slf4j;
import org.apache.pulsar.common.util.SecurityUtility;
+import org.apache.pulsar.common.util.jetty.JettySslContextFactory;
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] lhotari commented on a change in pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820512363
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -518,51 +540,18 @@ private static void setupClientAuthentication(SslContextBuilder builder,
}
}
- public static SslContextFactory createSslContextFactory(boolean tlsAllowInsecureConnection,
- String tlsTrustCertsFilePath, String tlsCertificateFilePath, String tlsKeyFilePath,
- boolean tlsRequireTrustedClientCertOnConnect, boolean autoRefresh, long certRefreshInSec)
- throws GeneralSecurityException, SSLException, FileNotFoundException, IOException {
- SslContextFactory sslCtxFactory = null;
- if (autoRefresh) {
- sslCtxFactory = new SslContextFactoryWithAutoRefresh(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath, tlsRequireTrustedClientCertOnConnect, 0);
- } else {
- sslCtxFactory = new SslContextFactory();
- SSLContext sslCtx = createSslContext(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath);
- sslCtxFactory.setSslContext(sslCtx);
- }
- if (tlsRequireTrustedClientCertOnConnect) {
- sslCtxFactory.setNeedClientAuth(true);
- } else {
- sslCtxFactory.setWantClientAuth(true);
+ private static Provider resolveProvider(String providerName) throws NoSuchAlgorithmException {
+ if (StringUtils.isEmpty(providerName)) {
Review comment:
Another required change seems to be that `ServiceConfiguration.tlsProvider` and `ProxyConfiguration.tlsProvider` fields are initializated to `"Conscrypt"`.
--
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 removed a comment on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece removed a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1013893657
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1014315459
/pulsarbot run-failure-checks
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1015475463
/pulsarbot run-failure-checks
--
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 commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1020967997
ping @lhotari Please help review the PR again, 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] nodece commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1053591331
> Ghe attached issue reports that with modern Windows versions it works.
You are right, it can work on Windows 8/10, but cannot work on Windows 7 in Ghe report.
In my env, I use macOS 11.5.2, it also cannot work, it is unstable.
> I am not sure it is worth to change the default for obsolete clients, as far as there is a way to configure a different provider.
Keeping stable is important in this software, we can't be sure that users are always running Pulsar on the latest operating system, so we need to switch to JDK provider as default. If you want to use Conscrypt, you can set `tlsProvider=Conscrypt` as default in `broker.conf`.
--
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 #13740: [Broker] Fix config ciphers and protocols to web server
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1013893657
/pulsarbot run-failure-checks
--
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 edited a comment on pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060789065
@lhotari These comments have been fixed, if you have time please take a look again, 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] nodece commented on a change in pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820580098
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/service/WebSocketProxyConfiguration.java
##########
@@ -135,6 +135,21 @@
@Deprecated
private boolean tlsEnabled = false;
+ @FieldContext(doc = "Name for the SSL provider")
+ private String sslProvider;
Review comment:
Done.
--
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 #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060485638
> I'd assume that the tlsProvider field would be initialized to the default which is "Conscrypt" for all 3 classes: ServiceConfiguration, ProxyConfiguration and WebSocketProxyConfiguration
Apologize, I thought it would work just by modifying the `WebService.java`.
--
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 #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1063641518
/pulsarbot rerun-failure-checks
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1019104482
/pulsarbot rerun-failure-checks
--
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 change in pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820469653
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java
##########
@@ -109,17 +108,24 @@ public WebService(PulsarService pulsar) throws PulsarServerException {
config.getTlsTrustStore(),
config.getTlsTrustStorePassword(),
config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
+ config.getWebServiceTlsCiphers() != null ? config.getWebServiceTlsCiphers() :
+ config.getTlsCiphers(),
+ config.getWebServiceTlsProtocols() != null ? config.getWebServiceTlsProtocols() :
+ config.getTlsCiphers(),
Review comment:
Fixed.
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java
##########
@@ -109,17 +108,24 @@ public WebService(PulsarService pulsar) throws PulsarServerException {
config.getTlsTrustStore(),
config.getTlsTrustStorePassword(),
config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
+ config.getWebServiceTlsCiphers() != null ? config.getWebServiceTlsCiphers() :
+ config.getTlsCiphers(),
+ config.getWebServiceTlsProtocols() != null ? config.getWebServiceTlsProtocols() :
+ config.getTlsCiphers(),
config.getTlsCertRefreshCheckDurationSec()
);
} else {
- sslCtxFactory = SecurityUtility.createSslContextFactory(
+ sslCtxFactory = JettySslContextFactory.createServerSslContext(
+ config.getTlsProvider(),
config.isTlsAllowInsecureConnection(),
config.getTlsTrustCertsFilePath(),
config.getTlsCertificateFilePath(),
config.getTlsKeyFilePath(),
- config.isTlsRequireTrustedClientCertOnConnect(), true,
+ config.isTlsRequireTrustedClientCertOnConnect(),
+ config.getWebServiceTlsCiphers() != null ? config.getWebServiceTlsCiphers() :
+ config.getTlsCiphers(),
+ config.getWebServiceTlsProtocols() != null ? config.getWebServiceTlsProtocols() :
+ config.getTlsCiphers(),
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 edited a comment on pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060485638
> I'd assume that the tlsProvider field would be initialized to the default which is "Conscrypt" for all 3 classes: ServiceConfiguration, ProxyConfiguration and WebSocketProxyConfiguration
Apologize, these things has been 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 pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060291332
Covert to draft: Need to refactor this PR, keep using the Conscrypt as the SSL context provider.
--
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 change in pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820470961
##########
File path: pulsar-common/src/test/java/org/apache/pulsar/common/util/jetty/JettySslContextFactoryWithKeyStoreTest.java
##########
@@ -16,9 +16,21 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.pulsar.common.util.keystoretls;
+
+package org.apache.pulsar.common.util.jetty;
Review comment:
Fixed.
##########
File path: pulsar-common/src/test/java/org/apache/pulsar/common/util/jetty/JettySslContextFactoryTest.java
##########
@@ -0,0 +1,174 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pulsar.common.util.jetty;
+
+import com.google.common.io.Resources;
+import java.io.IOException;
+import java.security.GeneralSecurityException;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLHandshakeException;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.config.RegistryBuilder;
+import org.apache.http.conn.socket.ConnectionSocketFactory;
+import org.apache.http.conn.ssl.NoopHostnameVerifier;
+import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
+import org.apache.pulsar.common.util.SecurityUtility;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+import org.testng.annotations.Test;
+
+@Slf4j
+public class JettySslContextFactoryTest {
Review comment:
Fixed.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java
##########
@@ -339,7 +340,10 @@ public static SSLContext createClientSslContext(String keyStoreTypeString,
return keyStoreSSLContext.createSSLContext();
}
- // for web server. autoRefresh is default true.
+ /**
+ * for web server. autoRefresh is default true.
+ * @deprecated Use JettySslContextFactory.createServerSslContextWithKeystore instead.
+ */
public static SslContextFactory.Server createSslContextFactory(String sslProviderString,
String keyStoreTypeString,
String keyStore,
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] lhotari commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060300076
> Hi @lhotari, could you take a look this PR again?
@nodece I hope you would have replied to each of my earlier PR comments and confirming whether you made the change or not. Please adapt that style for addressing PR feedback in the future.
I'm sorry for the delay of the review. I'll take a look now.
--
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 #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060338693
@lhotari @eolivelli @codelipenghui Could you take a look this PR? Thanks for your time.
--
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] lhotari commented on a change in pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820520188
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -518,51 +540,18 @@ private static void setupClientAuthentication(SslContextBuilder builder,
}
}
- public static SslContextFactory createSslContextFactory(boolean tlsAllowInsecureConnection,
- String tlsTrustCertsFilePath, String tlsCertificateFilePath, String tlsKeyFilePath,
- boolean tlsRequireTrustedClientCertOnConnect, boolean autoRefresh, long certRefreshInSec)
- throws GeneralSecurityException, SSLException, FileNotFoundException, IOException {
- SslContextFactory sslCtxFactory = null;
- if (autoRefresh) {
- sslCtxFactory = new SslContextFactoryWithAutoRefresh(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath, tlsRequireTrustedClientCertOnConnect, 0);
- } else {
- sslCtxFactory = new SslContextFactory();
- SSLContext sslCtx = createSslContext(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath);
- sslCtxFactory.setSslContext(sslCtx);
- }
- if (tlsRequireTrustedClientCertOnConnect) {
- sslCtxFactory.setNeedClientAuth(true);
- } else {
- sslCtxFactory.setWantClientAuth(true);
+ private static Provider resolveProvider(String providerName) throws NoSuchAlgorithmException {
+ if (StringUtils.isEmpty(providerName)) {
Review comment:
Yes, it's intended to be used whenever we use Jetty. See #10372 for the original motivation.
I noticed that the "tlsProvider" field is missing from WebSocketProxyConfiguration class. I'd assume that the tlsProvider field would be set to "Conscrypt" for all 3 classes: ServiceConfiguration, ProxyConfiguration and WebSocketProxyConfiguration.
--
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 #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060789065
@lhotari These comments have been fixed, if you have time please take a look again.
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1014130077
/pulsarbot run-failure-checks
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r786391733
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/jetty/package-info.java
##########
@@ -0,0 +1,20 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pulsar.common.util.jetty;
Review comment:
just a advice
--
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 removed a comment on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece removed a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1015475463
/pulsarbot run-failure-checks
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060317777
@lhotari Thanks for your review, these review has been fixed. If I miss your comment, please let me know. Right now I need to keep the Conscrypt as default.
--
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 change in pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820514793
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -518,51 +540,18 @@ private static void setupClientAuthentication(SslContextBuilder builder,
}
}
- public static SslContextFactory createSslContextFactory(boolean tlsAllowInsecureConnection,
- String tlsTrustCertsFilePath, String tlsCertificateFilePath, String tlsKeyFilePath,
- boolean tlsRequireTrustedClientCertOnConnect, boolean autoRefresh, long certRefreshInSec)
- throws GeneralSecurityException, SSLException, FileNotFoundException, IOException {
- SslContextFactory sslCtxFactory = null;
- if (autoRefresh) {
- sslCtxFactory = new SslContextFactoryWithAutoRefresh(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath, tlsRequireTrustedClientCertOnConnect, 0);
- } else {
- sslCtxFactory = new SslContextFactory();
- SSLContext sslCtx = createSslContext(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath);
- sslCtxFactory.setSslContext(sslCtx);
- }
- if (tlsRequireTrustedClientCertOnConnect) {
- sslCtxFactory.setNeedClientAuth(true);
- } else {
- sslCtxFactory.setWantClientAuth(true);
+ private static Provider resolveProvider(String providerName) throws NoSuchAlgorithmException {
+ if (StringUtils.isEmpty(providerName)) {
Review comment:
Does only web service use the "Conscrypt" provider?
--
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 change in pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820470086
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/jetty/JettySslContextFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.util.jetty;
+
+import java.util.Set;
+import javax.net.ssl.SSLContext;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.common.util.DefaultSslContextBuilder;
+import org.apache.pulsar.common.util.SslContextAutoRefreshBuilder;
+import org.apache.pulsar.common.util.keystoretls.NetSslContextBuilder;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+
+@Slf4j
+public class JettySslContextFactory {
+ public static SslContextFactory.Server createServerSslContextWithKeystore(String sslProviderString,
+ String keyStoreTypeString,
+ String keyStore,
+ String keyStorePassword,
+ boolean allowInsecureConnection,
+ String trustStoreTypeString,
+ String trustStore,
+ String trustStorePassword,
+ boolean requireTrustedClientCertOnConnect,
+ Set<String> ciphers,
+ Set<String> protocols,
+ long certRefreshInSec) {
+ NetSslContextBuilder sslCtxRefresher = new NetSslContextBuilder(
+ sslProviderString,
+ keyStoreTypeString,
+ keyStore,
+ keyStorePassword,
+ allowInsecureConnection,
+ trustStoreTypeString,
+ trustStore,
+ trustStorePassword,
+ requireTrustedClientCertOnConnect,
+ certRefreshInSec);
+
+ return new Server(sslProviderString, sslCtxRefresher, requireTrustedClientCertOnConnect, ciphers, protocols);
+ }
+
+ public static SslContextFactory createServerSslContext(String sslProviderString, boolean tlsAllowInsecureConnection,
+ String tlsTrustCertsFilePath,
+ String tlsCertificateFilePath,
+ String tlsKeyFilePath,
+ boolean tlsRequireTrustedClientCertOnConnect,
+ Set<String> ciphers,
+ Set<String> protocols,
+ long certRefreshInSec) {
+ DefaultSslContextBuilder sslCtxRefresher =
+ new DefaultSslContextBuilder(tlsAllowInsecureConnection, tlsTrustCertsFilePath, tlsCertificateFilePath,
+ tlsKeyFilePath, tlsRequireTrustedClientCertOnConnect, certRefreshInSec, sslProviderString);
+
+ return new Server(sslProviderString, sslCtxRefresher, tlsRequireTrustedClientCertOnConnect, ciphers, protocols);
+ }
+
+ private static class Server extends SslContextFactory.Server {
+ private final SslContextAutoRefreshBuilder<SSLContext> sslCtxRefresher;
+
+ public Server(String sslProviderString, SslContextAutoRefreshBuilder<SSLContext> sslCtxRefresher,
+ boolean requireTrustedClientCertOnConnect, Set<String> ciphers, Set<String> protocols) {
+ super();
+ this.sslCtxRefresher = sslCtxRefresher;
+
+ if (ciphers != null && ciphers.size() > 0) {
+ this.setIncludeCipherSuites(ciphers.toArray(new String[0]));
+ }
+
+ if (protocols != null && protocols.size() > 0) {
+ this.setIncludeProtocols(protocols.toArray(new String[0]));
+ }
+
+ if (sslProviderString != null && !sslProviderString.equals("")) {
+ setProvider(sslProviderString);
+ }
+
+ if (requireTrustedClientCertOnConnect) {
+ this.setNeedClientAuth(true);
+ } else {
+ this.setWantClientAuth(true);
+ }
+ this.setTrustAll(true);
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] lhotari commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060307536
> I am +1 to this patch but I would keep Conscript as default
Yes, I think that Conscrypt should continue to be the default.
--
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] lhotari commented on a change in pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820547663
##########
File path: pulsar-websocket/src/main/java/org/apache/pulsar/websocket/service/WebSocketProxyConfiguration.java
##########
@@ -135,6 +135,21 @@
@Deprecated
private boolean tlsEnabled = false;
+ @FieldContext(doc = "Name for the SSL provider")
+ private String sslProvider;
Review comment:
this is called `tlsProvider` in other locations. Please rename.
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060321088
> @nodece Please create any new Jetty related classes in `pulsar-broker-common` module instead of `pulsar-common` module. Switch to use package name `org.apache.pulsar.jetty.tls` since these classes are about TLS support in Jetty. We should remove any references to Jetty modules in `pulsar-common` since that is a shared module also used on the client. Jetty shouldn't be pulled in to the client dependencies. Please remove the existing `jetty-util` dependency from `pulsar-common`. /cc @codelipenghui
>
> These changes should be only done in master branch. For maintenance branches (branch-2.9, branch-2.8), the workaround for the handshake issues on Mac could be to stick to TLSv1.2 protocol and remove TLSv1.3 from configured protocols. In TLSv1.3, the handshake works differently. More details in https://lists.apache.org/thread/30dzw87pxlthg7gops6zfy04pmw7fbt6 . TLSv1.3 also drops support for SHA-1 certificates and will refuse to handshake when certificates are invalid.
Thanks for your review, their comments have been 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 edited a comment on pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060485638
> I'd assume that the tlsProvider field would be initialized to the default which is "Conscrypt" for all 3 classes: ServiceConfiguration, ProxyConfiguration and WebSocketProxyConfiguration
Your mean that add `tlsProvder=Conscrypt` to these classes? If yes, these things has been fixed.
Then the CI failed when `tlsProvder=Conscrypt`, I need to fix the CI.
--
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] lhotari commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1018281333
@nodece Please create any new Jetty related classes in `pulsar-broker-common` module instead of `pulsar-common` module.
Switch to use package name `org.apache.pulsar.jetty.tls` since these classes are about TLS support in Jetty.
We should remove any references to Jetty modules in `pulsar-common` since that is a shared module also used on the client. Jetty shouldn't be pulled in to the client dependencies. Please remove the existing `jetty-util` dependency from `pulsar-common`. /cc @codelipenghui
These changes should be only done in master branch. For maintenance branches (branch-2.9, branch-2.8), the workaround for the handshake issues on Mac could be to stick to TLSv1.2 protocol and remove TLSv1.3 from configured protocols. In TLSv1.3, the handshake works differently. More details in https://lists.apache.org/thread/30dzw87pxlthg7gops6zfy04pmw7fbt6 . TLSv1.3 also drops support for SHA-1 certificates and will refuse to handshake when certificates are invalid.
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1051937084
@lhotari Could you help review this PR? Thanks for your time.
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1015552359
/pulsarbot rerun-failure-checks
--
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] lhotari commented on a change in pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820494537
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -518,51 +540,18 @@ private static void setupClientAuthentication(SslContextBuilder builder,
}
}
- public static SslContextFactory createSslContextFactory(boolean tlsAllowInsecureConnection,
- String tlsTrustCertsFilePath, String tlsCertificateFilePath, String tlsKeyFilePath,
- boolean tlsRequireTrustedClientCertOnConnect, boolean autoRefresh, long certRefreshInSec)
- throws GeneralSecurityException, SSLException, FileNotFoundException, IOException {
- SslContextFactory sslCtxFactory = null;
- if (autoRefresh) {
- sslCtxFactory = new SslContextFactoryWithAutoRefresh(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath, tlsRequireTrustedClientCertOnConnect, 0);
- } else {
- sslCtxFactory = new SslContextFactory();
- SSLContext sslCtx = createSslContext(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath);
- sslCtxFactory.setSslContext(sslCtx);
- }
- if (tlsRequireTrustedClientCertOnConnect) {
- sslCtxFactory.setNeedClientAuth(true);
- } else {
- sslCtxFactory.setWantClientAuth(true);
+ private static Provider resolveProvider(String providerName) throws NoSuchAlgorithmException {
+ if (StringUtils.isEmpty(providerName)) {
Review comment:
```suggestion
if (CONSCRYPT_PROVIDER != null && StringUtils.isEmpty(providerName)) {
```
`CONSCRYPT_PROVIDER` might be null if it cannot be loaded.
--
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] lhotari commented on a change in pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820525231
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -518,51 +540,18 @@ private static void setupClientAuthentication(SslContextBuilder builder,
}
}
- public static SslContextFactory createSslContextFactory(boolean tlsAllowInsecureConnection,
- String tlsTrustCertsFilePath, String tlsCertificateFilePath, String tlsKeyFilePath,
- boolean tlsRequireTrustedClientCertOnConnect, boolean autoRefresh, long certRefreshInSec)
- throws GeneralSecurityException, SSLException, FileNotFoundException, IOException {
- SslContextFactory sslCtxFactory = null;
- if (autoRefresh) {
- sslCtxFactory = new SslContextFactoryWithAutoRefresh(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath, tlsRequireTrustedClientCertOnConnect, 0);
- } else {
- sslCtxFactory = new SslContextFactory();
- SSLContext sslCtx = createSslContext(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath);
- sslCtxFactory.setSslContext(sslCtx);
- }
- if (tlsRequireTrustedClientCertOnConnect) {
- sslCtxFactory.setNeedClientAuth(true);
- } else {
- sslCtxFactory.setWantClientAuth(true);
+ private static Provider resolveProvider(String providerName) throws NoSuchAlgorithmException {
+ if (StringUtils.isEmpty(providerName)) {
Review comment:
After the most recent changes, shouldn't this condition be inverted ? `if (!StringUtils.isEmpty(providerName)) {`
--
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 change in pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820470253
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -300,27 +310,65 @@ public static SslContext createNettySslContextForServer(boolean allowInsecureCon
}
public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
- Certificate[] certificates, PrivateKey privateKey) throws GeneralSecurityException {
+ Certificate[] certificates, PrivateKey privateKey)
+ throws GeneralSecurityException {
+ return createSslContext(allowInsecureConnection, trustCertficates, certificates, privateKey, null);
+ }
+
+ public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
+ Certificate[] certificates, PrivateKey privateKey, String providerName)
+ throws GeneralSecurityException {
KeyStoreHolder ksh = new KeyStoreHolder();
TrustManager[] trustManagers = null;
KeyManager[] keyManagers = null;
+ Provider provider = null;
+ if (providerName != null && !providerName.equals("")) {
+ if (providerName.equalsIgnoreCase(CONSCRYPT_PROVIDER_NAME)) {
Review comment:
Fixed.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -300,27 +310,65 @@ public static SslContext createNettySslContextForServer(boolean allowInsecureCon
}
public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
- Certificate[] certificates, PrivateKey privateKey) throws GeneralSecurityException {
+ Certificate[] certificates, PrivateKey privateKey)
+ throws GeneralSecurityException {
+ return createSslContext(allowInsecureConnection, trustCertficates, certificates, privateKey, null);
+ }
+
+ public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
+ Certificate[] certificates, PrivateKey privateKey, String providerName)
+ throws GeneralSecurityException {
KeyStoreHolder ksh = new KeyStoreHolder();
TrustManager[] trustManagers = null;
KeyManager[] keyManagers = null;
+ Provider provider = null;
+ if (providerName != null && !providerName.equals("")) {
+ if (providerName.equalsIgnoreCase(CONSCRYPT_PROVIDER_NAME)) {
+ provider = CONSCRYPT_PROVIDER;
+ } else {
+ provider = Security.getProvider(providerName);
+ if (provider == null) {
+ log.warn("Cannot to get {} security provider, fallback to using JDK default security provider",
+ providerName);
+ provider = SSLContext.getDefault().getProvider();
+ }
+ }
Review comment:
Fixed.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/jetty/JettySslContextFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.util.jetty;
+
+import java.util.Set;
+import javax.net.ssl.SSLContext;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.common.util.DefaultSslContextBuilder;
+import org.apache.pulsar.common.util.SslContextAutoRefreshBuilder;
+import org.apache.pulsar.common.util.keystoretls.NetSslContextBuilder;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+
+@Slf4j
+public class JettySslContextFactory {
Review comment:
Fixed.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/jetty/package-info.java
##########
@@ -0,0 +1,19 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.util.jetty;
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 change in pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820469840
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -300,27 +310,65 @@ public static SslContext createNettySslContextForServer(boolean allowInsecureCon
}
public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
- Certificate[] certificates, PrivateKey privateKey) throws GeneralSecurityException {
+ Certificate[] certificates, PrivateKey privateKey)
+ throws GeneralSecurityException {
+ return createSslContext(allowInsecureConnection, trustCertficates, certificates, privateKey, null);
+ }
+
+ public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
+ Certificate[] certificates, PrivateKey privateKey, String providerName)
+ throws GeneralSecurityException {
KeyStoreHolder ksh = new KeyStoreHolder();
TrustManager[] trustManagers = null;
KeyManager[] keyManagers = null;
+ Provider provider = null;
+ if (providerName != null && !providerName.equals("")) {
+ if (providerName.equalsIgnoreCase(CONSCRYPT_PROVIDER_NAME)) {
+ provider = CONSCRYPT_PROVIDER;
+ } else {
+ provider = Security.getProvider(providerName);
+ if (provider == null) {
+ log.warn("Cannot to get {} security provider, fallback to using JDK default security provider",
+ providerName);
+ provider = SSLContext.getDefault().getProvider();
+ }
+ }
+ }
- trustManagers = setupTrustCerts(ksh, allowInsecureConnection, trustCertficates, CONSCRYPT_PROVIDER);
- keyManagers = setupKeyManager(ksh, privateKey, certificates);
+ trustManagers = setupTrustCerts(ksh, allowInsecureConnection, trustCertficates, provider);
+ keyManagers = setupKeyManager(ksh, privateKey, certificates, provider);
- SSLContext sslCtx = CONSCRYPT_PROVIDER != null ? SSLContext.getInstance("TLS", CONSCRYPT_PROVIDER)
+ SSLContext sslCtx = provider != null ? SSLContext.getInstance("TLS", provider)
: SSLContext.getInstance("TLS");
sslCtx.init(keyManagers, trustManagers, new SecureRandom());
- sslCtx.getDefaultSSLParameters();
return sslCtx;
}
- private static KeyManager[] setupKeyManager(KeyStoreHolder ksh, PrivateKey privateKey, Certificate[] certificates)
+ private static KeyManager[] setupKeyManager(KeyStoreHolder ksh, PrivateKey privateKey, Certificate[] certificates,
+ Provider provider)
throws KeyStoreException, NoSuchAlgorithmException, UnrecoverableKeyException {
KeyManager[] keyManagers = null;
+ String algorithm = KeyManagerFactory.getDefaultAlgorithm();
+
if (certificates != null && privateKey != null) {
ksh.setPrivateKey("private", privateKey, certificates);
- KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+ KeyManagerFactory kmf = null;
+
+ try {
+ kmf = provider != null ? KeyManagerFactory.getInstance(algorithm, provider) :
+ KeyManagerFactory.getInstance(algorithm);
+ } catch (NoSuchAlgorithmException e) {
+ if (provider != null) {
+ log.warn(
+ "Unable to get KeyManagerFactory instance for algorithm [{}] on provider [{}], using JDK "
+ + "default",
+ algorithm, provider.getName());
+ kmf = KeyManagerFactory.getInstance(algorithm);
+ } else {
+ throw e;
+ }
+ }
+
Review comment:
Fixed.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -348,9 +410,10 @@ public static SSLContext createSslContext(boolean allowInsecureConnection, Certi
}
trustManagers = tmf.getTrustManagers();
-
- for (TrustManager trustManager : trustManagers) {
- processConscryptTrustManager(trustManager);
+ if (securityProvider != null && securityProvider.getName().equalsIgnoreCase(CONSCRYPT_PROVIDER_NAME)) {
Review comment:
Fixed.
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/WebServer.java
##########
@@ -108,18 +107,24 @@ public WebServer(ProxyConfiguration config, AuthenticationService authentication
config.getTlsTrustStore(),
config.getTlsTrustStorePassword(),
config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
+ config.getWebServiceTlsCiphers() != null ? config.getWebServiceTlsCiphers() :
+ config.getTlsCiphers(),
+ config.getWebServiceTlsProtocols() != null ? config.getWebServiceTlsProtocols() :
+ config.getTlsCiphers(),
config.getTlsCertRefreshCheckDurationSec()
);
} else {
- sslCtxFactory = SecurityUtility.createSslContextFactory(
+ sslCtxFactory = JettySslContextFactory.createServerSslContext(
+ config.getTlsProvider(),
config.isTlsAllowInsecureConnection(),
config.getTlsTrustCertsFilePath(),
config.getTlsCertificateFilePath(),
config.getTlsKeyFilePath(),
config.isTlsRequireTrustedClientCertOnConnect(),
- true,
+ config.getWebServiceTlsCiphers() != null ? config.getWebServiceTlsCiphers() :
+ config.getTlsCiphers(),
+ config.getWebServiceTlsProtocols() != null ? config.getWebServiceTlsProtocols() :
+ config.getTlsCiphers(),
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 pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1063641230
/pulsarbot run-failure-checks
--
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 edited a comment on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1053213742
@eolivelli The root cause is the Conscrypt doesn't work on Windows 7 and macOS 11.5.2, but the use JDK provider is working, this issue is from the user feedface, see https://github.com/apache/pulsar/issues/13734.
--
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 pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1053547964
Ghe attached issue reports that with modern Windows versions it works.
I am not sure it is worth to change the default for obsolete clients, as far as there is a way to configure a different provider.
I am +1 to this patch but I would keep Conscript as default
--
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 edited a comment on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1053591331
> Ghe attached issue reports that with modern Windows versions it works.
You are right, it can work on Windows 10, but cannot work on Windows 7/8 in Ghe report.
In my env, I use macOS 11.5.2, it also cannot work, it is unstable.
> I am not sure it is worth to change the default for obsolete clients, as far as there is a way to configure a different provider.
Keeping stable is important in this software, we can't be sure that users are always running Pulsar on the latest operating system, so we need to switch to JDK provider as default. If you want to use Conscrypt, you can set `tlsProvider=Conscrypt` as default in `broker.conf`.
--
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 removed a comment on pull request #13740: [Broker] Fix config ciphers and protocols to web server
Posted by GitBox <gi...@apache.org>.
nodece removed a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1013049580
/pulsarbot run-failures-check
--
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 #13740: [Broker] Fix config ciphers and protocols to web server
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1013050173
/pulsarbot run-failure-checks
--
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] lhotari commented on a change in pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r789411080
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java
##########
@@ -109,17 +108,24 @@ public WebService(PulsarService pulsar) throws PulsarServerException {
config.getTlsTrustStore(),
config.getTlsTrustStorePassword(),
config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
+ config.getWebServiceTlsCiphers() != null ? config.getWebServiceTlsCiphers() :
+ config.getTlsCiphers(),
+ config.getWebServiceTlsProtocols() != null ? config.getWebServiceTlsProtocols() :
+ config.getTlsCiphers(),
Review comment:
typo
```suggestion
config.getTlsProtocols(),
```
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java
##########
@@ -109,17 +108,24 @@ public WebService(PulsarService pulsar) throws PulsarServerException {
config.getTlsTrustStore(),
config.getTlsTrustStorePassword(),
config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
+ config.getWebServiceTlsCiphers() != null ? config.getWebServiceTlsCiphers() :
+ config.getTlsCiphers(),
+ config.getWebServiceTlsProtocols() != null ? config.getWebServiceTlsProtocols() :
+ config.getTlsCiphers(),
config.getTlsCertRefreshCheckDurationSec()
);
} else {
- sslCtxFactory = SecurityUtility.createSslContextFactory(
+ sslCtxFactory = JettySslContextFactory.createServerSslContext(
+ config.getTlsProvider(),
config.isTlsAllowInsecureConnection(),
config.getTlsTrustCertsFilePath(),
config.getTlsCertificateFilePath(),
config.getTlsKeyFilePath(),
- config.isTlsRequireTrustedClientCertOnConnect(), true,
+ config.isTlsRequireTrustedClientCertOnConnect(),
+ config.getWebServiceTlsCiphers() != null ? config.getWebServiceTlsCiphers() :
+ config.getTlsCiphers(),
+ config.getWebServiceTlsProtocols() != null ? config.getWebServiceTlsProtocols() :
+ config.getTlsCiphers(),
Review comment:
```suggestion
config.getTlsProtocols(),
```
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/WebServer.java
##########
@@ -108,18 +107,24 @@ public WebServer(ProxyConfiguration config, AuthenticationService authentication
config.getTlsTrustStore(),
config.getTlsTrustStorePassword(),
config.isTlsRequireTrustedClientCertOnConnect(),
- config.getWebServiceTlsCiphers(),
- config.getWebServiceTlsProtocols(),
+ config.getWebServiceTlsCiphers() != null ? config.getWebServiceTlsCiphers() :
+ config.getTlsCiphers(),
+ config.getWebServiceTlsProtocols() != null ? config.getWebServiceTlsProtocols() :
+ config.getTlsCiphers(),
config.getTlsCertRefreshCheckDurationSec()
);
} else {
- sslCtxFactory = SecurityUtility.createSslContextFactory(
+ sslCtxFactory = JettySslContextFactory.createServerSslContext(
+ config.getTlsProvider(),
config.isTlsAllowInsecureConnection(),
config.getTlsTrustCertsFilePath(),
config.getTlsCertificateFilePath(),
config.getTlsKeyFilePath(),
config.isTlsRequireTrustedClientCertOnConnect(),
- true,
+ config.getWebServiceTlsCiphers() != null ? config.getWebServiceTlsCiphers() :
+ config.getTlsCiphers(),
+ config.getWebServiceTlsProtocols() != null ? config.getWebServiceTlsProtocols() :
+ config.getTlsCiphers(),
Review comment:
```suggestion
config.getTlsProtocols(),
```
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -300,27 +310,65 @@ public static SslContext createNettySslContextForServer(boolean allowInsecureCon
}
public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
- Certificate[] certificates, PrivateKey privateKey) throws GeneralSecurityException {
+ Certificate[] certificates, PrivateKey privateKey)
+ throws GeneralSecurityException {
+ return createSslContext(allowInsecureConnection, trustCertficates, certificates, privateKey, null);
+ }
+
+ public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
+ Certificate[] certificates, PrivateKey privateKey, String providerName)
+ throws GeneralSecurityException {
KeyStoreHolder ksh = new KeyStoreHolder();
TrustManager[] trustManagers = null;
KeyManager[] keyManagers = null;
+ Provider provider = null;
+ if (providerName != null && !providerName.equals("")) {
+ if (providerName.equalsIgnoreCase(CONSCRYPT_PROVIDER_NAME)) {
+ provider = CONSCRYPT_PROVIDER;
+ } else {
+ provider = Security.getProvider(providerName);
+ if (provider == null) {
+ log.warn("Cannot to get {} security provider, fallback to using JDK default security provider",
+ providerName);
+ provider = SSLContext.getDefault().getProvider();
+ }
+ }
+ }
- trustManagers = setupTrustCerts(ksh, allowInsecureConnection, trustCertficates, CONSCRYPT_PROVIDER);
- keyManagers = setupKeyManager(ksh, privateKey, certificates);
+ trustManagers = setupTrustCerts(ksh, allowInsecureConnection, trustCertficates, provider);
+ keyManagers = setupKeyManager(ksh, privateKey, certificates, provider);
- SSLContext sslCtx = CONSCRYPT_PROVIDER != null ? SSLContext.getInstance("TLS", CONSCRYPT_PROVIDER)
+ SSLContext sslCtx = provider != null ? SSLContext.getInstance("TLS", provider)
: SSLContext.getInstance("TLS");
sslCtx.init(keyManagers, trustManagers, new SecureRandom());
- sslCtx.getDefaultSSLParameters();
return sslCtx;
}
- private static KeyManager[] setupKeyManager(KeyStoreHolder ksh, PrivateKey privateKey, Certificate[] certificates)
+ private static KeyManager[] setupKeyManager(KeyStoreHolder ksh, PrivateKey privateKey, Certificate[] certificates,
+ Provider provider)
throws KeyStoreException, NoSuchAlgorithmException, UnrecoverableKeyException {
KeyManager[] keyManagers = null;
+ String algorithm = KeyManagerFactory.getDefaultAlgorithm();
+
if (certificates != null && privateKey != null) {
ksh.setPrivateKey("private", privateKey, certificates);
- KeyManagerFactory kmf = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
+ KeyManagerFactory kmf = null;
+
+ try {
+ kmf = provider != null ? KeyManagerFactory.getInstance(algorithm, provider) :
+ KeyManagerFactory.getInstance(algorithm);
+ } catch (NoSuchAlgorithmException e) {
+ if (provider != null) {
+ log.warn(
+ "Unable to get KeyManagerFactory instance for algorithm [{}] on provider [{}], using JDK "
+ + "default",
+ algorithm, provider.getName());
+ kmf = KeyManagerFactory.getInstance(algorithm);
+ } else {
+ throw e;
+ }
+ }
+
Review comment:
It is possible that the provider wasn't passed here intentionally. There were some quirks in switching to use Conscrypt in #10541. This might have been one of them. If you come across test failures, the changes in setupKeyManager might be causing them.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -300,27 +310,65 @@ public static SslContext createNettySslContextForServer(boolean allowInsecureCon
}
public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
- Certificate[] certificates, PrivateKey privateKey) throws GeneralSecurityException {
+ Certificate[] certificates, PrivateKey privateKey)
+ throws GeneralSecurityException {
+ return createSslContext(allowInsecureConnection, trustCertficates, certificates, privateKey, null);
+ }
+
+ public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
+ Certificate[] certificates, PrivateKey privateKey, String providerName)
+ throws GeneralSecurityException {
KeyStoreHolder ksh = new KeyStoreHolder();
TrustManager[] trustManagers = null;
KeyManager[] keyManagers = null;
+ Provider provider = null;
+ if (providerName != null && !providerName.equals("")) {
+ if (providerName.equalsIgnoreCase(CONSCRYPT_PROVIDER_NAME)) {
Review comment:
```suggestion
if (CONSCRYPT_PROVIDER != null && providerName.equalsIgnoreCase(CONSCRYPT_PROVIDER_NAME)) {
```
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/jetty/JettySslContextFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.util.jetty;
+
+import java.util.Set;
+import javax.net.ssl.SSLContext;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.common.util.DefaultSslContextBuilder;
+import org.apache.pulsar.common.util.SslContextAutoRefreshBuilder;
+import org.apache.pulsar.common.util.keystoretls.NetSslContextBuilder;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+
+@Slf4j
+public class JettySslContextFactory {
+ public static SslContextFactory.Server createServerSslContextWithKeystore(String sslProviderString,
+ String keyStoreTypeString,
+ String keyStore,
+ String keyStorePassword,
+ boolean allowInsecureConnection,
+ String trustStoreTypeString,
+ String trustStore,
+ String trustStorePassword,
+ boolean requireTrustedClientCertOnConnect,
+ Set<String> ciphers,
+ Set<String> protocols,
+ long certRefreshInSec) {
+ NetSslContextBuilder sslCtxRefresher = new NetSslContextBuilder(
+ sslProviderString,
+ keyStoreTypeString,
+ keyStore,
+ keyStorePassword,
+ allowInsecureConnection,
+ trustStoreTypeString,
+ trustStore,
+ trustStorePassword,
+ requireTrustedClientCertOnConnect,
+ certRefreshInSec);
+
+ return new Server(sslProviderString, sslCtxRefresher, requireTrustedClientCertOnConnect, ciphers, protocols);
+ }
+
+ public static SslContextFactory createServerSslContext(String sslProviderString, boolean tlsAllowInsecureConnection,
+ String tlsTrustCertsFilePath,
+ String tlsCertificateFilePath,
+ String tlsKeyFilePath,
+ boolean tlsRequireTrustedClientCertOnConnect,
+ Set<String> ciphers,
+ Set<String> protocols,
+ long certRefreshInSec) {
+ DefaultSslContextBuilder sslCtxRefresher =
+ new DefaultSslContextBuilder(tlsAllowInsecureConnection, tlsTrustCertsFilePath, tlsCertificateFilePath,
+ tlsKeyFilePath, tlsRequireTrustedClientCertOnConnect, certRefreshInSec, sslProviderString);
+
+ return new Server(sslProviderString, sslCtxRefresher, tlsRequireTrustedClientCertOnConnect, ciphers, protocols);
+ }
+
+ private static class Server extends SslContextFactory.Server {
+ private final SslContextAutoRefreshBuilder<SSLContext> sslCtxRefresher;
+
+ public Server(String sslProviderString, SslContextAutoRefreshBuilder<SSLContext> sslCtxRefresher,
+ boolean requireTrustedClientCertOnConnect, Set<String> ciphers, Set<String> protocols) {
+ super();
+ this.sslCtxRefresher = sslCtxRefresher;
+
+ if (ciphers != null && ciphers.size() > 0) {
+ this.setIncludeCipherSuites(ciphers.toArray(new String[0]));
+ }
+
+ if (protocols != null && protocols.size() > 0) {
+ this.setIncludeProtocols(protocols.toArray(new String[0]));
+ }
+
+ if (sslProviderString != null && !sslProviderString.equals("")) {
+ setProvider(sslProviderString);
+ }
+
+ if (requireTrustedClientCertOnConnect) {
+ this.setNeedClientAuth(true);
+ } else {
+ this.setWantClientAuth(true);
+ }
+ this.setTrustAll(true);
Review comment:
I can see that the original code had a possible security issue. I would assume that this would be the right thing to do here:
```suggestion
if (requireTrustedClientCertOnConnect) {
this.setNeedClientAuth(true);
this.setTrustAll(false);
} else {
this.setWantClientAuth(true);
this.setTrustAll(true);
}
```
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -300,27 +310,65 @@ public static SslContext createNettySslContextForServer(boolean allowInsecureCon
}
public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
- Certificate[] certificates, PrivateKey privateKey) throws GeneralSecurityException {
+ Certificate[] certificates, PrivateKey privateKey)
+ throws GeneralSecurityException {
+ return createSslContext(allowInsecureConnection, trustCertficates, certificates, privateKey, null);
+ }
+
+ public static SSLContext createSslContext(boolean allowInsecureConnection, Certificate[] trustCertficates,
+ Certificate[] certificates, PrivateKey privateKey, String providerName)
+ throws GeneralSecurityException {
KeyStoreHolder ksh = new KeyStoreHolder();
TrustManager[] trustManagers = null;
KeyManager[] keyManagers = null;
+ Provider provider = null;
+ if (providerName != null && !providerName.equals("")) {
+ if (providerName.equalsIgnoreCase(CONSCRYPT_PROVIDER_NAME)) {
+ provider = CONSCRYPT_PROVIDER;
+ } else {
+ provider = Security.getProvider(providerName);
+ if (provider == null) {
+ log.warn("Cannot to get {} security provider, fallback to using JDK default security provider",
+ providerName);
+ provider = SSLContext.getDefault().getProvider();
+ }
+ }
Review comment:
extract provider resolving logic to private static helper method `resolveProvider(String providerName)`, improves readability
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -348,9 +410,10 @@ public static SSLContext createSslContext(boolean allowInsecureConnection, Certi
}
trustManagers = tmf.getTrustManagers();
-
- for (TrustManager trustManager : trustManagers) {
- processConscryptTrustManager(trustManager);
+ if (securityProvider != null && securityProvider.getName().equalsIgnoreCase(CONSCRYPT_PROVIDER_NAME)) {
Review comment:
there's no need to add this check. `processConscryptTrustManager` contains the sufficient logic. A possible extra for-loop isn't a problem.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/jetty/package-info.java
##########
@@ -0,0 +1,19 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.util.jetty;
Review comment:
Please create this in `pulsar-broker-common` module instead of `pulsar-common` module.
Switch to use package name `org.apache.pulsar.jetty.tls` since these classes are about TLS support in Jetty.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/jetty/JettySslContextFactory.java
##########
@@ -0,0 +1,106 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.common.util.jetty;
+
+import java.util.Set;
+import javax.net.ssl.SSLContext;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.common.util.DefaultSslContextBuilder;
+import org.apache.pulsar.common.util.SslContextAutoRefreshBuilder;
+import org.apache.pulsar.common.util.keystoretls.NetSslContextBuilder;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+
+@Slf4j
+public class JettySslContextFactory {
Review comment:
Please create this in `pulsar-broker-common` module instead of `pulsar-common` module.
Switch to use package name `org.apache.pulsar.jetty.tls` since these classes are about TLS support in Jetty.
We should remove any references to Jetty modules in `pulsar-common` since that is shared module also used on the client. Jetty shouldn't be pulled in to the client dependencies.
##########
File path: pulsar-common/src/test/java/org/apache/pulsar/common/util/jetty/JettySslContextFactoryWithKeyStoreTest.java
##########
@@ -16,9 +16,21 @@
* specific language governing permissions and limitations
* under the License.
*/
-package org.apache.pulsar.common.util.keystoretls;
+
+package org.apache.pulsar.common.util.jetty;
Review comment:
Please move this to `pulsar-broker-common` module.
Switch to use package name `org.apache.pulsar.jetty.tls`.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java
##########
@@ -38,6 +38,7 @@
import lombok.Getter;
import lombok.extern.slf4j.Slf4j;
import org.apache.pulsar.common.util.SecurityUtility;
+import org.apache.pulsar.common.util.jetty.JettySslContextFactory;
Review comment:
Drop references to the Jetty support since pulsar-common is also used on the client. no need to have a deprecation cycle since this is not a public API for Pulsar.
##########
File path: pulsar-common/src/test/java/org/apache/pulsar/common/util/jetty/JettySslContextFactoryTest.java
##########
@@ -0,0 +1,174 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.pulsar.common.util.jetty;
+
+import com.google.common.io.Resources;
+import java.io.IOException;
+import java.security.GeneralSecurityException;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLHandshakeException;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.http.client.methods.HttpGet;
+import org.apache.http.config.RegistryBuilder;
+import org.apache.http.conn.socket.ConnectionSocketFactory;
+import org.apache.http.conn.ssl.NoopHostnameVerifier;
+import org.apache.http.conn.ssl.SSLConnectionSocketFactory;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.apache.http.impl.client.HttpClientBuilder;
+import org.apache.http.impl.client.HttpClients;
+import org.apache.http.impl.conn.PoolingHttpClientConnectionManager;
+import org.apache.pulsar.common.util.SecurityUtility;
+import org.eclipse.jetty.server.Server;
+import org.eclipse.jetty.server.ServerConnector;
+import org.eclipse.jetty.util.ssl.SslContextFactory;
+import org.testng.annotations.Test;
+
+@Slf4j
+public class JettySslContextFactoryTest {
Review comment:
Please create this in `pulsar-broker-common` module instead of `pulsar-common` module.
Switch to use package name `org.apache.pulsar.jetty.tls` since these classes are about TLS support in Jetty.
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/keystoretls/KeyStoreSSLContext.java
##########
@@ -339,7 +340,10 @@ public static SSLContext createClientSslContext(String keyStoreTypeString,
return keyStoreSSLContext.createSSLContext();
}
- // for web server. autoRefresh is default true.
+ /**
+ * for web server. autoRefresh is default true.
+ * @deprecated Use JettySslContextFactory.createServerSslContextWithKeystore instead.
+ */
public static SslContextFactory.Server createSslContextFactory(String sslProviderString,
String keyStoreTypeString,
String keyStore,
Review comment:
Please remove this method completely. It was there to be used in Jetty.
Drop references to the Jetty support since pulsar-common is also used on the client. no need to have a deprecation cycle since this is not a public API for Pulsar end users.
--
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 commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1020967997
ping @lhotari Please help review the PR again, 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] nodece edited a comment on pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060485638
> I'd assume that the tlsProvider field would be initialized to the default which is "Conscrypt" for all 3 classes: ServiceConfiguration, ProxyConfiguration and WebSocketProxyConfiguration
Your mean that add `tlsProvder=Conscrypt` to these classes? If yes, these things has been fixed.
Then the CI failed when `tlsProvder=Conscrypt`.
--
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 edited a comment on pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060485638
> I'd assume that the tlsProvider field would be initialized to the default which is "Conscrypt" for all 3 classes: ServiceConfiguration, ProxyConfiguration and WebSocketProxyConfiguration
Your mean that add `tlsProvder=Conscrypt` to these classes? If yes, these things has been 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] lhotari commented on pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060338820
> Thanks for your review, their comments have been fixed.
Thanks for replying to each 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] lhotari commented on pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1085409855
@nodece Please rebase the changes
--
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 pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1053674666
My question now is: is Conscript more secure? In this case I prefer it as default as we mostly care about production for security.
We can set the legacy provider in standalone.conf for developers and so help your usecase.
Security is better then being user friendly with people on old systems.
If Conscript does not add value then I am fine to change the default
@lhotari what do you think about Conscript? IIRC you introduced 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] nodece commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1053213742
@eolivelli The root cause is the Conscrypt doesn't work on Windows 7 and macOS 11.5.2, this issue is from the user feedface, see https://github.com/apache/pulsar/issues/13734.
--
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 edited a comment on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece edited a comment on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1053808043
> My question now is: is Conscript more secure? In this case I prefer it as default as we mostly care about production for security.
> Security is better then being user friendly with people on old systems.
Maybe Conscrypt is more secure with JDK 11 in production OS such as CentOS, Debian, etc., but we can't just consider the production environment. Software compatibility is more important than security. For example, some users want to test Pulsar with TLS in Win 7/8 or macOS 11.5.2, the Conscript cannot work, but the use JDK provider is working.
> If Conscript as default, it is looks does not add value then I am fine to change the default
I prefer to use the JDK provider as default, you can also see that there has been feedback from the issue.
--
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 change in pull request #13740: [Broker] Full-support set ssl provider, ciphers and protocols for web service
Posted by GitBox <gi...@apache.org>.
nodece commented on a change in pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#discussion_r820536092
##########
File path: pulsar-common/src/main/java/org/apache/pulsar/common/util/SecurityUtility.java
##########
@@ -518,51 +540,18 @@ private static void setupClientAuthentication(SslContextBuilder builder,
}
}
- public static SslContextFactory createSslContextFactory(boolean tlsAllowInsecureConnection,
- String tlsTrustCertsFilePath, String tlsCertificateFilePath, String tlsKeyFilePath,
- boolean tlsRequireTrustedClientCertOnConnect, boolean autoRefresh, long certRefreshInSec)
- throws GeneralSecurityException, SSLException, FileNotFoundException, IOException {
- SslContextFactory sslCtxFactory = null;
- if (autoRefresh) {
- sslCtxFactory = new SslContextFactoryWithAutoRefresh(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath, tlsRequireTrustedClientCertOnConnect, 0);
- } else {
- sslCtxFactory = new SslContextFactory();
- SSLContext sslCtx = createSslContext(tlsAllowInsecureConnection, tlsTrustCertsFilePath,
- tlsCertificateFilePath, tlsKeyFilePath);
- sslCtxFactory.setSslContext(sslCtx);
- }
- if (tlsRequireTrustedClientCertOnConnect) {
- sslCtxFactory.setNeedClientAuth(true);
- } else {
- sslCtxFactory.setWantClientAuth(true);
+ private static Provider resolveProvider(String providerName) throws NoSuchAlgorithmException {
+ if (StringUtils.isEmpty(providerName)) {
Review comment:
Good catch! These things has been 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] lhotari commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1060309827
@nodece Please reply to each of my previous PR review comments first. I'll continue the review once you have done that.
--
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 #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
nodece commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1031185302
Hi @lhotari, could you take a look this PR again?
--
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 commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1016518449
@lhotari Please help review this PR, it's related to https://github.com/apache/pulsar/pull/10541
--
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] lhotari commented on pull request #13740: [Security] Fix handshake failure on web service
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #13740:
URL: https://github.com/apache/pulsar/pull/13740#issuecomment-1018289934
Certificates signed with SHA-1 are not trusted in TLSv1.3 at all. It is common that many clients reject certificates signed with SHA-1 also for older TLS protocol versions.
When there are TLS handshake issue, It is necessary to verify the certificate signature algorithm, the certificate validity and that the client trusts 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