enable SNI by default

[carlhoerberg]

Enabling peer verification by default might be breaking things ppl, but enabling SNI shouldn't have any adverse effects.

[michaelklishin]

We've had a really long discussion about this for the Java client.

It's a usability and support load for the core team [1] vs. better security defaults. For Java client so far we have decided to not force peer verification if it's not enabled explicitly.

Since this is a breaking change that can affect a substantial percentage of users, we likely won't be able to ship it in v3.8.x. @lukebakken @dumbbell thoughts?

1. Unfortunately, no amount of effort poured into the TLS doc guide helps with the lack of understanding of TLS by most developers and subsequent requests for free support on the mailing list and here on GitHub

[carlhoerberg]

Ok, lets skip the peer verification, but I really don't see an issue with SNI, do you?

On Wed, 1 Apr 2020, 00:17 Michael Klishin, notifications@github.com wrote:

We've had a really long discussion about this for the Java client.

It's a usability and support load for the core team (unfortunately no amount of TLS doc guide https://www.rabbitmq.com/ssl.html content helps with the lack of understanding of TLS by most developers) vs. better security defaults. For Java client so far we have decided to not force peer verification if it's not enabled explicitly.

Since this is a breaking change that can affect a substantial percentage of users, we likely won't be able to ship it in v3.8.x. @lukebakken https://github.com/lukebakken @dumbbell https://github.com/dumbbell thoughts?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/rabbitmq/rabbitmq-erlang-client/pull/132#issuecomment-606906476, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABL6TTG4YUCMR4Y7B4Q3P3RKJTYVANCNFSM4LYBZ6GQ .

[carlhoerberg]

I reverted the peer verification change, only kept enabling SNI which shouldn't have any adverse effects for anyone, but is required if multiple rabbitmq servers are behind a loadbalancer/terminator.

[michaelklishin]

CI failures are legit: gmake ct-unit suite assertions were not updated.

[michaelklishin]

According to Erlang docs, SNI is enabled by default unless explicitly disabled.

This line seems to confirm it.

Why do we need to enable it explicitly?

[michaelklishin]

The following patch updates test assertions:

commit efeb6fc2e7c5da11d95d6d353cdd8d84e8a3802b (HEAD -> cloudamqp-tls-sni)
Author: Michael Klishin <michael@clojurewerkz.org>
Date:   Wed Apr 1 14:06:44 2020 +0300

    Update unit test assertions
---
 test/unit_SUITE.erl | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/test/unit_SUITE.erl b/test/unit_SUITE.erl
index d960e40..10a0046 100644
--- a/test/unit_SUITE.erl
+++ b/test/unit_SUITE.erl
@@ -142,7 +142,10 @@ amqp_uri_parsing(_Config) ->

     {ok, #amqp_params_network{host = "host1", ssl_options = TLSOpts1}} =
         amqp_uri:parse("amqps://host1/%2f?cacertfile=/path/to/cacertfile.pem"),
-    Exp1 = [{cacertfile,"/path/to/cacertfile.pem"}],
+    Exp1 = [
+        {cacertfile,"/path/to/cacertfile.pem"},
+        {server_name_indication,"host1"}
+    ],
     ?assertEqual(lists:usort(Exp1), lists:usort(TLSOpts1)),

     {ok, #amqp_params_network{host = "host2", ssl_options = TLSOpts2}} =
@@ -161,7 +164,8 @@ amqp_uri_parsing(_Config) ->
         amqp_uri:parse("amqps://host3/%2f?verify=verify_peer"
                        "&fail_if_no_peer_cert=true"),
     Exp3 = [{fail_if_no_peer_cert, true},
-            {verify, verify_peer}],
+            {verify, verify_peer},
+            {server_name_indication,"host3"}],
     ?assertEqual(lists:usort(Exp3), lists:usort(TLSOpts3)),

     {ok, #amqp_params_network{host = "host4", ssl_options = TLSOpts4}} =
@@ -172,7 +176,8 @@ amqp_uri_parsing(_Config) ->
     Exp4 = [{certfile,  "/path/to/certfile.pem"},
             {cacertfile,"/path/to/cacertfile.pem"},
             {password,  "topsecret"},
-            {depth,     5}],
+            {depth,     5},
+            {server_name_indication,"host4"}],
     ?assertEqual(lists:usort(Exp4), lists:usort(TLSOpts4)),

     {ok, #amqp_params_network{host = "host5", ssl_options = TLSOpts5}} =

[carlhoerberg]

I tested with a shovel in rabbitmq 3.8.3 and it doesn't send the SNI header, not sure where it gets disabled though.

[michaelklishin]

SNI won't be set for IP addresses, only hostnames.

[carlhoerberg]

i tested with hostname

[michaelklishin]

Backported to v3.8.x.