Updates the ssl option to add settings rather than replace them

As per issue #458, passing the `ssl` option completely overrides
all settings, rather than just overwriting the "sub"-setting that
was specified. Eg `ssl: [{:verions, [:'tlsv1.2']}]` disabled all
TLS settings rather than just requiring TLSv1.2.

This commit changes HTTPoison's behaviour to merge the `ssl` settings
with my best-guess at what the defaults are. Since it may be a breaking
change, it also adds a new `ssl_override` option that can be used by
anyone wishing to maintain the existing behaviour.

This resolves #458.

lib/httpoison.ex

@@ -28,7 +28,10 @@ defmodule HTTPoison.Request do
     * `:proxy_auth` - proxy authentication `{User, Password}` tuple
     * `:socks5_user`- socks5 username
     * `:socks5_pass`- socks5 password
-    * `:ssl` - SSL options supported by the `ssl` erlang module
+    * `:ssl` - SSL options supported by the `ssl` erlang module. SSL defaults will be used where options
+               are not specified.
+    * `:ssl_override` - if `:ssl` is specified, this option is ignored, otherwise it can be used to
+              completely override SSL settings.
     * `:follow_redirect` - a boolean that causes redirects to be followed, can cause a request to return
       a `MaybeRedirect` struct. See: HTTPoison.MaybeRedirect
     * `:max_redirect` - an integer denoting the maximum number of redirects to follow. Default is 5

lib/httpoison/base.ex

@@ -723,7 +723,15 @@ defmodule HTTPoison.Base do
     recv_timeout = Keyword.get(options, :recv_timeout)
     stream_to = Keyword.get(options, :stream_to)
     async = Keyword.get(options, :async)
-    ssl = Keyword.get(options, :ssl)
+
+    ssl =
+      if Keyword.get(options, :ssl) do
+        default_ssl_options()
+        |> Keyword.merge(Keyword.get(options, :ssl))
+      else
+        Keyword.get(options, :ssl_override)
+      end
+
     follow_redirect = Keyword.get(options, :follow_redirect)
     max_redirect = Keyword.get(options, :max_redirect)
 
@@ -789,6 +797,19 @@ defmodule HTTPoison.Base do
     hn_proxy_options
   end
 
+  defp default_ssl_options() do
+    [
+      {:versions, [:"tlsv1.2", :"tlsv1.3"]},
+      {:verify, :verify_peer},
+      {:cacertfile, :certifi.cacertfile()},
+      {:depth, 10},
+      {:customize_hostname_check,
+       [
+         match_fun: :public_key.pkix_verify_hostname_match_fun(:https)
+       ]}
+    ]
+  end
+
   defp check_no_proxy(nil, _) do
     # Don't bother to check no_proxy if there's no proxy to use anyway.
     nil

test/httpoison_base_test.exs

@@ -513,7 +513,7 @@ defmodule HTTPoisonBaseTest do
     expect(:hackney, :body, fn _, _ -> {:ok, "response"} end)
   end
 
-  test "passing ssl option" do
+  test "passing ssl override option" do
     expect(:hackney, :request, fn :post,
                                   "http://localhost",
                                   [],
@@ -524,6 +524,34 @@ defmodule HTTPoisonBaseTest do
 
     expect(:hackney, :body, fn _, _ -> {:ok, "response"} end)
 
+    assert HTTPoison.post!("localhost", "body", [], ssl_override: [certfile: "certs/client.crt"]) ==
+             %HTTPoison.Response{
+               status_code: 200,
+               headers: "headers",
+               body: "response",
+               request_url: "http://localhost",
+               request: %HTTPoison.Request{
+                 body: "body",
+                 headers: [],
+                 method: :post,
+                 options: [ssl_override: [certfile: "certs/client.crt"]],
+                 params: %{},
+                 url: "http://localhost"
+               }
+             }
+  end
+
+  test "passing ssl option" do
+    expect(:hackney, :request, fn :post, "http://localhost", [], "body", [ssl_options: opts] ->
+      assert opts[:versions] == [:"tlsv1.2", :"tlsv1.3"]
+      assert opts[:verify] == :verify_peer
+      assert opts[:customize_hostname_check][:match_fun]
+      assert opts[:certfile] == "certs/client.crt"
+      {:ok, 200, "headers", :client}
+    end)
+
+    expect(:hackney, :body, fn _, _ -> {:ok, "response"} end)
+
     assert HTTPoison.post!("localhost", "body", [], ssl: [certfile: "certs/client.crt"]) ==
              %HTTPoison.Response{
                status_code: 200,

test/httpoison_test.exs

@@ -160,26 +160,6 @@ defmodule HTTPoisonTest do
     end)
   end
 
-  test "https scheme" do
-    httparrot_priv_dir = :code.priv_dir(:httparrot)
-    cacert_file = "#{httparrot_priv_dir}/ssl/server-ca.crt"
-    cert_file = "#{httparrot_priv_dir}/ssl/server.crt"
-    key_file = "#{httparrot_priv_dir}/ssl/server.key"
-
-    assert_response(
-      HTTPoison.get(
-        "https://localhost:8433/get",
-        [],
-        ssl: [cacertfile: cacert_file, keyfile: key_file, certfile: cert_file]
-      ),
-      fn response ->
-        assert Request.to_curl(response.request) ==
-                 {:ok,
-                  "curl --cert #{cert_file} --key #{key_file} --cacert #{cacert_file} -X GET https://localhost:8433/get"}
-      end
-    )
-  end
-
   test "http+unix scheme" do
     if Application.get_env(:httparrot, :unix_socket, false) do
       case {HTTParrot.unix_socket_supported?(), Application.fetch_env(:httparrot, :socket_path)} do
@@ -354,4 +334,127 @@ defmodule HTTPoisonTest do
     |> hd
     |> elem(1)
   end
+
+  describe "ssl config tests" do
+    test "https scheme" do
+      httparrot_priv_dir = :code.priv_dir(:httparrot)
+      cacert_file = "#{httparrot_priv_dir}/ssl/server-ca.crt"
+      cert_file = "#{httparrot_priv_dir}/ssl/server.crt"
+      key_file = "#{httparrot_priv_dir}/ssl/server.key"
+
+      assert_response(
+        HTTPoison.get(
+          "https://localhost:8433/get",
+          [],
+          ssl: [cacertfile: cacert_file, keyfile: key_file, certfile: cert_file]
+        ),
+        fn response ->
+          assert Request.to_curl(response.request) ==
+                   {:ok,
+                    "curl --cert #{cert_file} --key #{key_file} --cacert #{cacert_file} -X GET https://localhost:8433/get"}
+        end
+      )
+    end
+
+    test "expired certificate" do
+      assert {:error, %HTTPoison.Error{reason: {:tls_alert, {:certificate_expired, reason}}}} =
+               HTTPoison.get("https://expired.badssl.com/")
+
+      assert to_string(reason) =~ "Certificate Expired"
+      # TLS version should not matter
+      assert {:error, %HTTPoison.Error{reason: {:tls_alert, _}}} =
+               HTTPoison.get("https://expired.badssl.com/", [], ssl: [{:versions, [:"tlsv1.2"]}])
+
+      assert {:ok, _} =
+               HTTPoison.get("https://expired.badssl.com/", [], ssl: [{:verify, :verify_none}])
+
+      # Can be disabled via verify_fun
+      assert {:ok, _} =
+               HTTPoison.get("https://expired.badssl.com/", [],
+                 ssl: [
+                   verify_fun:
+                     {fn _, reason, state ->
+                        case reason do
+                          {:bad_cert, :cert_expired} -> {:valid, state}
+                          {:bad_cert, :unknown_ca} -> {:valid, state}
+                          {:extension, _} -> {:valid, state}
+                          :valid -> {:valid, state}
+                          :valid_peer -> {:valid, state}
+                          error -> {:fail, error}
+                        end
+                      end, []}
+                 ]
+               )
+    end
+
+    test "allows changing TLS1.0 settings" do
+      assert {:error,
+              %HTTPoison.Error{
+                reason: {:tls_alert, {:protocol_version, reason}}
+              }} =
+               HTTPoison.get("https://tls-v1-0.badssl.com:1010/", [],
+                 ssl: [{:versions, [:"tlsv1.2"]}]
+               )
+
+      assert to_string(reason) =~ "Protocol Version"
+
+      if :tlsv1 in :ssl.versions()[:available] do
+        assert {:ok, _} =
+                 HTTPoison.get("https://tls-v1-0.badssl.com:1010/", [],
+                   ssl: [
+                     {:versions, [:tlsv1]}
+                   ]
+                 )
+      end
+    end
+
+    test "allows changing TLS1.1 settings" do
+      assert {:error,
+              %HTTPoison.Error{
+                reason: {:tls_alert, {:protocol_version, reason}}
+              }} =
+               HTTPoison.get("https://tls-v1-1.badssl.com:1011/", [],
+                 ssl: [versions: [:"tlsv1.2", :tlsv1]]
+               )
+
+      assert to_string(reason) =~ "Protocol Version"
+
+      if :"tlsv1.1" in :ssl.versions()[:available] do
+        assert {:ok, _} =
+                 HTTPoison.get("https://tls-v1-1.badssl.com:1011/", [],
+                   ssl: [versions: [:"tlsv1.1"]]
+                 )
+      end
+    end
+
+    test "does support tls1.2" do
+      if :"tlsv1.2" in :ssl.versions()[:supported] do
+        assert {:ok, _} = HTTPoison.get("https://tls-v1-2.badssl.com:1012/", [])
+      end
+
+      if :"tlsv1.2" in :ssl.versions()[:available] do
+        assert {:ok, _} =
+                 HTTPoison.get("https://tls-v1-2.badssl.com:1012/", [],
+                   ssl: [versions: [:"tlsv1.2"]]
+                 )
+      end
+    end
+
+    test "invalid common name" do
+      assert {:error,
+              %HTTPoison.Error{
+                reason: {:tls_alert, {:handshake_failure, reason}}
+              }} = HTTPoison.get("https://wrong.host.badssl.com/")
+
+      assert to_string(reason) =~ ~r"hostname|altnames"
+
+      assert {:error,
+              %HTTPoison.Error{
+                reason: {:tls_alert, _}
+              }} =
+               HTTPoison.get("https://wrong.host.badssl.com/", [],
+                 ssl: [{:versions, [:"tlsv1.2"]}]
+               )
+    end
+  end
 end