You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/02/16 19:16:08 UTC
[GitHub] [activemq-artemis] sebthom opened a new pull request #3456: ARTEMIS-3117 Cache SSLContext in NettyAcceptor to mitigate performance
sebthom opened a new pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456
degradation in JDK 11 during TLS connection initialization.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#discussion_r577527749
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultOpenSSLContextFactory.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Copyright 2021 The Apache Software Foundation.
+ *
+ * Licensed 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.activemq.artemis.core.remoting.impl.ssl;
+
+import java.util.Map;
+
+import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
+import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
+import org.apache.activemq.artemis.spi.core.remoting.ssl.OpenSSLContextFactory;
+
+import io.netty.handler.ssl.SslContext;
+
+/**
+ * Default {@link OpenSSLContextFactory} for use in {@link NettyConnector} and NettyAcceptor.
+ */
+public class DefaultOpenSSLContextFactory implements OpenSSLContextFactory {
+
+ protected SSLSupport createSSLSupport(final Map<String, Object> config,
+ final String keystoreProvider, final String keystorePath, final String keystorePassword,
Review comment:
The SSLSupport class basically is such a config class, but it resides in the impl package.
Introduce something like an SSLContextConfig class on the API/SPI level would from my view be beneficial too, but it should then also used for the JDK SSLContextFactory, e.g. the signature should change from
```java
SSLContext getSSLContext(Map<String, Object> configuration,
String keystoreProvider, String keystorePath, String keystorePassword,
String truststoreProvider, String truststorePath, String truststorePassword,
String crlPath, String trustManagerFactoryPlugin, boolean trustAll) throws Exception;
```
to
```
SSLContext getSSLContext(SSLContextConfig config) throws Exception;
```
I can do this in a backward compatible way using default methods, but I would get the OK to do so from at least two reviewers. @jbertram WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#discussion_r577532185
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultOpenSSLContextFactory.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Copyright 2021 The Apache Software Foundation.
+ *
+ * Licensed 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.activemq.artemis.core.remoting.impl.ssl;
+
+import java.util.Map;
+
+import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
+import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
+import org.apache.activemq.artemis.spi.core.remoting.ssl.OpenSSLContextFactory;
+
+import io.netty.handler.ssl.SslContext;
+
+/**
+ * Default {@link OpenSSLContextFactory} for use in {@link NettyConnector} and NettyAcceptor.
+ */
+public class DefaultOpenSSLContextFactory implements OpenSSLContextFactory {
+
+ protected SSLSupport createSSLSupport(final Map<String, Object> config,
+ final String keystoreProvider, final String keystorePath, final String keystorePassword,
Review comment:
In the same go I would also like to remove the confusing SSL_CONTEXT_PROP_NAME "sslContext" property as it's usage does not seem to make much sense. Why would a caching SSL context factory be adviced to always return the same SSL context no matter the given input parameters (keystore/truststore etc) (which is currently how it is implemented in [CachingSSLContextFactory](https://github.com/apache/activemq-artemis/blob/master/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/CachingSSLContextFactory.java#L48)). @clebertsuconic WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on pull request #3456: ARTEMIS-3117 Cache SslContext in NettyAcceptor to mitigate performance
Posted by GitBox <gi...@apache.org>.
sebthom commented on pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#issuecomment-780085438
@jbertram If I create DefaultSSLContextFactory/CachingSSLContextFactory for OpenSSL which into package should I place them using which names? Alternatively I could extend the existing ContextFactories and provide an additional method that returns the Netty SslContext.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] franz1981 commented on a change in pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
franz1981 commented on a change in pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#discussion_r577496984
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultOpenSSLContextFactory.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Copyright 2021 The Apache Software Foundation.
+ *
+ * Licensed 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.activemq.artemis.core.remoting.impl.ssl;
+
+import java.util.Map;
+
+import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
+import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
+import org.apache.activemq.artemis.spi.core.remoting.ssl.OpenSSLContextFactory;
+
+import io.netty.handler.ssl.SslContext;
+
+/**
+ * Default {@link OpenSSLContextFactory} for use in {@link NettyConnector} and NettyAcceptor.
+ */
+public class DefaultOpenSSLContextFactory implements OpenSSLContextFactory {
+
+ protected SSLSupport createSSLSupport(final Map<String, Object> config,
+ final String keystoreProvider, final String keystorePath, final String keystorePassword,
Review comment:
why not creating a config class that would pack all these args in once?
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultOpenSSLContextFactory.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Copyright 2021 The Apache Software Foundation.
+ *
+ * Licensed 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.activemq.artemis.core.remoting.impl.ssl;
+
+import java.util.Map;
+
+import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
+import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
+import org.apache.activemq.artemis.spi.core.remoting.ssl.OpenSSLContextFactory;
+
+import io.netty.handler.ssl.SslContext;
+
+/**
+ * Default {@link OpenSSLContextFactory} for use in {@link NettyConnector} and NettyAcceptor.
+ */
+public class DefaultOpenSSLContextFactory implements OpenSSLContextFactory {
+
+ protected SSLSupport createSSLSupport(final Map<String, Object> config,
+ final String keystoreProvider, final String keystorePath, final String keystorePassword,
+ final String truststoreProvider, final String truststorePath, final String truststorePassword,
+ final String crlPath, final String trustManagerFactoryPlugin, final boolean trustAll
+ ) {
+ if (log.isDebugEnabled()) {
+ final String sep = System.getProperty("line.separator") + " ";
Review comment:
can be packed in a separate method to improve readability?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on a change in pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
jbertram commented on a change in pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#discussion_r578709081
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultOpenSSLContextFactory.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Copyright 2021 The Apache Software Foundation.
+ *
+ * Licensed 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.activemq.artemis.core.remoting.impl.ssl;
+
+import java.util.Map;
+
+import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
+import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
+import org.apache.activemq.artemis.spi.core.remoting.ssl.OpenSSLContextFactory;
+
+import io.netty.handler.ssl.SslContext;
+
+/**
+ * Default {@link OpenSSLContextFactory} for use in {@link NettyConnector} and NettyAcceptor.
+ */
+public class DefaultOpenSSLContextFactory implements OpenSSLContextFactory {
+
+ protected SSLSupport createSSLSupport(final Map<String, Object> config,
+ final String keystoreProvider, final String keystorePath, final String keystorePassword,
Review comment:
@sebthom, I think creating a config class is a good idea. I think you should close this and we can move forward with #3459.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#discussion_r577527749
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultOpenSSLContextFactory.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Copyright 2021 The Apache Software Foundation.
+ *
+ * Licensed 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.activemq.artemis.core.remoting.impl.ssl;
+
+import java.util.Map;
+
+import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
+import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
+import org.apache.activemq.artemis.spi.core.remoting.ssl.OpenSSLContextFactory;
+
+import io.netty.handler.ssl.SslContext;
+
+/**
+ * Default {@link OpenSSLContextFactory} for use in {@link NettyConnector} and NettyAcceptor.
+ */
+public class DefaultOpenSSLContextFactory implements OpenSSLContextFactory {
+
+ protected SSLSupport createSSLSupport(final Map<String, Object> config,
+ final String keystoreProvider, final String keystorePath, final String keystorePassword,
Review comment:
The SSLSupport class basically is such a config class, but it resides in the impl package.
Introduce something like an SSLContextConfig class on the API/SPI level would from my view be beneficial too, but it should then also be used for the JDK SSLContextFactory, e.g. the signature should change from
```java
SSLContext getSSLContext(Map<String, Object> configuration,
String keystoreProvider, String keystorePath, String keystorePassword,
String truststoreProvider, String truststorePath, String truststorePassword,
String crlPath, String trustManagerFactoryPlugin, boolean trustAll) throws Exception;
```
to
```
SSLContext getSSLContext(SSLContextConfig config) throws Exception;
```
I can do this in a backward compatible way using default methods, but I would get the OK to do so from at least two reviewers. @jbertram WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3456: ARTEMIS-3117 Cache SslContext in NettyAcceptor to mitigate performance
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#issuecomment-780091005
You can put them into `org.apache.activemq.artemis.core.remoting.impl.ssl` using something like `OpenSSLContextFactory` & `CachingOpenSSLContextFactory`. Extending the existing versions is tempting, but I'm hesitant to recommend that as it may get messy. In my opinion it makes sense to have multiple implementations given the pluggable nature of the factory.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom closed pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
sebthom closed pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram commented on pull request #3456: ARTEMIS-3117 Cache SslContext in NettyAcceptor to mitigate performance
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#issuecomment-780068689
I see a handful of issues with this PR:
1. It eliminates the use of `SSLContextFactoryProvider`.
2. It eliminates any option to use `javax.net.ssl.SSLContext` (i.e. the default JDK provider). It only uses `io.netty.handler.ssl.SslContext` (i.e. the OpenSSL provider).
IMO you should just change `org.apache.activemq.artemis.core.remoting.impl.netty.NettyAcceptor#loadOpenSslEngine` to use `SSLContextFactoryProvider` and then implement a caching OpenSSL SSL context factory like `org.apache.activemq.artemis.core.remoting.impl.ssl.CachingSSLContextFactory`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#issuecomment-780599627
> > checkstyle error on this PR.
>
> where do you see that? The Jobs are green and say `[INFO] You have 0 Checkstyle violations.`
>
> I am also running the checkstyle ruleset locally in Eclipse and don't see errors for the code I changed.
That is so weird.. for some reason my browser was showing an older version... sorry about that. ignore me.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] clebertsuconic commented on pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
clebertsuconic commented on pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#issuecomment-780591600
checkstyle error on this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#discussion_r577532185
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultOpenSSLContextFactory.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Copyright 2021 The Apache Software Foundation.
+ *
+ * Licensed 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.activemq.artemis.core.remoting.impl.ssl;
+
+import java.util.Map;
+
+import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
+import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
+import org.apache.activemq.artemis.spi.core.remoting.ssl.OpenSSLContextFactory;
+
+import io.netty.handler.ssl.SslContext;
+
+/**
+ * Default {@link OpenSSLContextFactory} for use in {@link NettyConnector} and NettyAcceptor.
+ */
+public class DefaultOpenSSLContextFactory implements OpenSSLContextFactory {
+
+ protected SSLSupport createSSLSupport(final Map<String, Object> config,
+ final String keystoreProvider, final String keystorePath, final String keystorePassword,
Review comment:
At the same time I would also like to remove the confusing SSL_CONTEXT_PROP_NAME "sslContext" property as it's usage does not seem to make much sense. Why would a caching SSL context factory be adviced to always return the same SSL context no matter the given input parameters (keystore/truststore etc) (which is currently how it is implemented in [CachingSSLContextFactory](https://github.com/apache/activemq-artemis/blob/master/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/CachingSSLContextFactory.java#L48)). @clebertsuconic WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
sebthom commented on pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#issuecomment-780594204
> checkstyle error on this PR.
where do you see that? The Jobs are green and say `[INFO] You have 0 Checkstyle violations.`
I am also running the checkstyle ruleset locally in Eclipse and don't see errors for the code I changed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] sebthom commented on a change in pull request #3456: ARTEMIS-3117 Provide CachingOpenSSLContextFactory
Posted by GitBox <gi...@apache.org>.
sebthom commented on a change in pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#discussion_r577532185
##########
File path: artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/DefaultOpenSSLContextFactory.java
##########
@@ -0,0 +1,95 @@
+/*
+ * Copyright 2021 The Apache Software Foundation.
+ *
+ * Licensed 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.activemq.artemis.core.remoting.impl.ssl;
+
+import java.util.Map;
+
+import org.apache.activemq.artemis.core.remoting.impl.netty.NettyConnector;
+import org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants;
+import org.apache.activemq.artemis.spi.core.remoting.ssl.OpenSSLContextFactory;
+
+import io.netty.handler.ssl.SslContext;
+
+/**
+ * Default {@link OpenSSLContextFactory} for use in {@link NettyConnector} and NettyAcceptor.
+ */
+public class DefaultOpenSSLContextFactory implements OpenSSLContextFactory {
+
+ protected SSLSupport createSSLSupport(final Map<String, Object> config,
+ final String keystoreProvider, final String keystorePath, final String keystorePassword,
Review comment:
In the same go I would also like to remove of the confusing SSL_CONTEXT_PROP_NAME "sslContext" property as it's usage does not seem to make much sense. Why would a caching SSL context factory be adviced to always return the same SSL context no matter the given input parameters (keystore/truststore etc) (which is currently how it is implemented in [CachingSSLContextFactory](https://github.com/apache/activemq-artemis/blob/master/artemis-core-client/src/main/java/org/apache/activemq/artemis/core/remoting/impl/ssl/CachingSSLContextFactory.java#L48)). @clebertsuconic WDYT?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [activemq-artemis] jbertram edited a comment on pull request #3456: ARTEMIS-3117 Cache SslContext in NettyAcceptor to mitigate performance
Posted by GitBox <gi...@apache.org>.
jbertram edited a comment on pull request #3456:
URL: https://github.com/apache/activemq-artemis/pull/3456#issuecomment-780068689
I see a couple of issues with this PR:
1. It eliminates the use of `SSLContextFactoryProvider`.
2. It eliminates any option to use `javax.net.ssl.SSLContext` (i.e. the default JDK provider). It only uses `io.netty.handler.ssl.SslContext` (i.e. the OpenSSL provider).
IMO you should just change `org.apache.activemq.artemis.core.remoting.impl.netty.NettyAcceptor#loadOpenSslEngine` to use `SSLContextFactoryProvider` and then implement a caching OpenSSL SSL context factory like `org.apache.activemq.artemis.core.remoting.impl.ssl.CachingSSLContextFactory`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org