James 3502, add ssl support #307

andrevka · 2021-02-25T07:21:42Z

https://issues.apache.org/jira/browse/JAMES-3502?filter=-2

…on.java

…on.SSLConfiguration

…hentication)

…ord for key password as well

…ctionFactory

chibenwa

Nice work!

Please don't forget to update src/site/xdoc/server/config-rabbitmq.xml as well!

chibenwa · 2021-02-25T08:47:10Z

...-common/rabbitmq/src/main/java/org/apache/james/backends/rabbitmq/RabbitMQConfiguration.java


 import org.apache.commons.configuration2.Configuration;

 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;

+import static org.apache.james.backends.rabbitmq.RabbitMQConfiguration.SSLConfiguration.*;
+import static org.apache.james.backends.rabbitmq.RabbitMQConfiguration.SSLConfiguration.SSLValidationStrategy.*;


Please avoid wildcards, this will likely not pass the checkstyle.

removed, applied checkstyle

chibenwa · 2021-02-25T08:47:52Z

...-common/rabbitmq/src/main/java/org/apache/james/backends/rabbitmq/RabbitMQConfiguration.java

+
+        static class Builder {
+
+            interface RequireSSLStrategyTrustStore {


Use @FunctionalInterface annotation where relevant.

chibenwa · 2021-02-25T08:48:57Z

...-common/rabbitmq/src/main/java/org/apache/james/backends/rabbitmq/RabbitMQConfiguration.java

+        }
+
+        public enum HostNameVerifier {
+            DEFAULT,


DEFAUT does not describe a behaviour. Can we find terms to express what this does?

what do you think of SUBJECT_ALTERNATIVE_NAME_VERIFIER ?

chibenwa · 2021-02-25T08:50:17Z

...mon/rabbitmq/src/main/java/org/apache/james/backends/rabbitmq/RabbitMQConnectionFactory.java

@@ -54,18 +69,85 @@ private ConnectionFactory from(RabbitMQConfiguration rabbitMQConfiguration) {
            connectionFactory.setUsername(rabbitMQConfiguration.getManagementCredentials().getUser());
            connectionFactory.setPassword(String.valueOf(rabbitMQConfiguration.getManagementCredentials().getPassword()));

+            if (configuration.useSsl()) setupSslConfiguration(connectionFactory);


You miss a {} block after this if, the code style will not like it ;-)

chibenwa · 2021-02-25T08:53:18Z

docs/modules/servers/pages/distributed/configure/rabbitmq.adoc

+
+| ssl.keystore.password
+| Configure the keystore password. If configured then "ssl.keystore" must also be configured,
+Optional string, defaults to empty string


Is there some cases when it should be specified?

client certificate authentication?

Sorry, I missread. That's ok.

chibenwa · 2021-02-25T08:53:26Z

docs/modules/servers/pages/distributed/configure/rabbitmq.adoc

+
+| ssl.keystore
+| Points to the keystore(PKCS12) used for verifying rabbitmq connection. If configured then "ssl.keystore.password" must also be configured,
+Optional string, defaults to empty string


Is there some cases when it should be specified?

client certificate authentication?

Sorry, I missread. That's ok.

chibenwa · 2021-02-25T08:53:37Z

docs/modules/servers/pages/distributed/configure/rabbitmq.adoc

+
+| ssl.hostname.verifier
+| Configure host name verification. Possible options are default and accept_any_hostname
+Optional string, defaults to default


What does default do?

fixed 3c0c552

chibenwa · 2021-02-25T08:53:43Z

docs/modules/servers/pages/distributed/configure/rabbitmq.adoc

+
+| ssl.validation.strategy
+| Configure the validation strategy used for rabbitmq connections. Possible values are default, ignore and override.
+Optional string, defaults to default


What does default do?

fixed 3c0c552

chibenwa · 2021-02-25T09:00:18Z

backends-common/rabbitmq/pom.xml

+            <groupId>org.apache.httpcomponents</groupId>
+            <artifactId>httpcore</artifactId>
+            <version>4.4.5</version>
+        </dependency>


Can you quickly explain why we need these dependencies? At first glance it seems unrelated to AMQP...

httpcore, has useful class SSLContextBuilder
httpclient, is needed for configuring mangement api client for org.apache.http.conn.ssl.DefaultHostnameVerifier

…documentation

chibenwa · 2021-03-01T01:12:31Z

Would other contributors like to have a look at this work?

It is 🍏 .

Arsnael

Good job overall thanks, just a few minor comments on my part :)

Arsnael · 2021-03-01T04:02:43Z

...-common/rabbitmq/src/main/java/org/apache/james/backends/rabbitmq/RabbitMQConfiguration.java

        ManagementCredentials managementCredentials = ManagementCredentials.from(configuration);
        return builder()
            .amqpUri(amqpUri)
            .managementUri(managementUri)
            .managementCredentials(managementCredentials)
+            .useSsl(useSsl)
+            .sslConfiguration(sslTrustConfiguration(configuration))
+            .sslConfiguration(sslTrustConfiguration(configuration))


duplicated line here

Arsnael · 2021-03-01T04:15:49Z

backends-common/rabbitmq/pom.xml

+        <dependency>
+            <groupId>org.apache.httpcomponents</groupId>
+            <artifactId>httpclient</artifactId>
+            <version>4.5.13</version>


We already have that artifact with the same version declared in the root pom file, so you can omit the version here

Arsnael · 2021-03-01T04:17:19Z

backends-common/rabbitmq/pom.xml

+        <dependency>
+            <groupId>org.apache.httpcomponents</groupId>
+            <artifactId>httpcore</artifactId>
+            <version>4.4.5</version>


I can see we have an httpcore artifact declared in james-server-jmap module as well, in a different version (4.4.14). Would be good to try centralizing the use of the artifact in the root pom and bump it here for rabbitmq maybe?

Arsnael · 2021-03-03T07:09:48Z

Merged

andreasvaldma added 13 commits February 23, 2021 19:26

JAMES-3502, add rabbitmq tls support

fc942b8

JAMES-3502, add rabbitmq tls support in connection factory

d2e1abf

JAMES-3502, update equals and hashCode methods on RabbitMQConfigurati…

a1fe208

…on.java

JAMES-3502, update equals and hashCode methods on RabbitMQConfigurati…

686105f

…on.SSLConfiguration

JAMES-3502, set keystore Optional.empty() as default (client cert aut…

90d8482

…hentication)

JAMES-3502, move exception catching to top level, use key store passw…

193c938

…ord for key password as well

JAMES-3502, add ssl tests for RabbitMQConfiguration and RabbitMqConne…

5707b46

…ctionFactory

JAMES-3502, add configuration key "useSslManagement"

16389c9

JAMES-3502, refactor, add more tests

469d21e

JAMES-3502, add tls support in RabbitMQManagementAPI.java

cabee16

JAMES-3502, update documentation for rabbitmq configuration keys.

2bdeedd

JAMES-3502, rabbitmqmanagementapi refactor

a30d665

JAMES-3502, update RabbitMQConfiguration documentation

0a5c078

chibenwa reviewed Feb 25, 2021

View reviewed changes

andreasvaldma added 5 commits February 25, 2021 11:44

JAMES-3502, fix checkstyle

3ee2612

JAMES-3502, add documentation in config-rabbitmq.xml

45f98a4

JAMES-3502, add functional interface annotations

62bd3ee

JAMES-3502, update ssl.keystore documentation

02606ab

JAMES-3502, update ssl.hostname.verifier and ssl.validation.strategy …

3c0c552

…documentation

chibenwa approved these changes Feb 25, 2021

View reviewed changes

Arsnael reviewed Mar 1, 2021

View reviewed changes

andreasvaldma added 3 commits March 1, 2021 14:54

JAMES-3502, remove duplicate line, refactor method name

4c2ecd2

JAMES-3502, use the version in root pom

50b6372

JAMES-3502, use the version in root pom for httpcore library

c0461e2

chibenwa approved these changes Mar 2, 2021

View reviewed changes

Arsnael approved these changes Mar 3, 2021

View reviewed changes

Arsnael added the waiting merge label Mar 3, 2021

Arsnael closed this Mar 3, 2021

Arsnael mentioned this pull request Sep 26, 2024

JAMES-4072 Support Redis TLS/SSL connection #2418

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

James 3502, add ssl support #307

James 3502, add ssl support #307

andrevka commented Feb 25, 2021

chibenwa left a comment

chibenwa Feb 25, 2021

andrevka Feb 25, 2021

chibenwa Feb 25, 2021

andrevka Feb 25, 2021

chibenwa Feb 25, 2021

andrevka Feb 25, 2021 •

edited

Loading

chibenwa Feb 25, 2021

chibenwa Feb 25, 2021

andrevka Feb 25, 2021

chibenwa Feb 25, 2021

chibenwa Feb 25, 2021

andrevka Feb 25, 2021

chibenwa Feb 25, 2021 •

edited

Loading

chibenwa Feb 25, 2021

andrevka Feb 25, 2021

chibenwa Feb 25, 2021

andrevka Feb 25, 2021

chibenwa Feb 25, 2021

andrevka Feb 25, 2021

chibenwa commented Mar 1, 2021

Arsnael left a comment

Arsnael Mar 1, 2021

Arsnael Mar 1, 2021

Arsnael Mar 1, 2021 •

edited

Loading

Arsnael commented Mar 3, 2021


		static class Builder {

		interface RequireSSLStrategyTrustStore {

James 3502, add ssl support #307

James 3502, add ssl support #307

Conversation

andrevka commented Feb 25, 2021

chibenwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrevka Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa Feb 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chibenwa commented Mar 1, 2021

Arsnael left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Arsnael Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Arsnael commented Mar 3, 2021

andrevka Feb 25, 2021 •

edited

Loading

chibenwa Feb 25, 2021 •

edited

Loading

Arsnael Mar 1, 2021 •

edited

Loading