Revert "Explicitly disable server name indication"

[nalundgaard]

Reverts #12.

It appears that the server_name_indication is not actually categorically added in OTP 20, it's only added when you connect to a Hostname that isn't an IP address. The wrinkle appears to be that Erlangs 20.0.x and 20.1.x didn't understand that string IPs, e.g., "127.0.0.1" are not Hostnames, despite that the documentation that says this (in ssl docs):

{server_name_indication, HostName :: hostname()}
Specify the hostname to be used in TLS Server Name Indication extension. If not specified it will default to the Host argument of connect/[3,4] unless it is of type inet:ipaddress().

The HostName will also be used in the hostname verification of the peer certificate using public_key:pkix_verify_hostname/2.

{server_name_indication, disable}
Prevents the Server Name Indication extension from being sent and disables the hostname verification check public_key:pkix_verify_hostname/2

So we end up with stupid errors like failed: {options,{{server_name_indication,"127.0.0.1"}}}.

It looks to me like this was introduced in Erlang 20.0-rc2: erlang/otp/commit/e9b0dbb4, "ssl: Add hostname check of server certificate."

It appears that it was then fixed in Erlang OTP 20.2: erlang/otp/commit/78a9a09a, "Stop checking DNS name for SNI."

As @RaimoNiskanen said in the comment there (in bold below):

RFC 6066, Section 3: Currently, the only server names supported are
DNS hostnames
...
But the definition seems very diffuse, so let all strings through
and leave it up to public_key to decide...

Diffuse indeed.

[jadeallenx]

👍 LGTM

[motobob]

i have a suspicion this failing test never worked as it's just skipped on native Mac but fails same way in my docker

[motobob]

DO NOT MERGE until stuff is above 20.3.X

[motobob]

@nalundgaard i'm going to rebase this and merge after verifying with S3/DDB and somme things other

[nalundgaard]

I can rebase it today.

[akontsevoy]

@nalundgaard Could we not perform certificate CN/SAN validation without depending on SNI? Hackney does it, IIRC, by means of https://github.com/deadtrickster/ssl_verify_fun.erl . That's what we actually need for security. Whereas SNI is (AFAIK) a nearly useless feature, it's needed only in cases where there's a single IP/host that needs to present multiple server certificates, which (AFAIK) we don't use, so it's right to keep it disabled by default.

[nalundgaard]

@akontsevoy The current state is that it's categorically off no matter what (overrides any client setting), as a "quick and dirty" workaround for a bug where it was broken on the client side in Erlang 20.1 (fixed in 20.3). That in not desirable; the default behavior is preferred (use it automatically as appropriate, assuming the client implementation isn't broken). This HTTP client is used by a wide array of people, and AWS definitely does use SNI in some of their services, as far as I am aware. I prefer to leave this to the client to set as appropriate, and those that want to turn it off may find #19 helpful. What do you think?

[akontsevoy]

OK, if by default it's on (as in curl/OpenSSL), maybe we should leave the default in place and let the user disable it if they need instead of force-disabling it for them (and compromising security of the connection in the process by diabling CN/SAN validation). I don't understand why (according to OTP docs) disabling SNI also disables certificate name validation, but I guess that's not a decision for us to make.