Accept URIs with scheme, add default SSL options for https, deprecate URLs without scheme (#357)

* Accept URIs with scheme, add default SSL options for https, deprecate URLs without scheme

This commit changes how addresses are handled by GRPC.Stub.connect/2.

First, by accepting URIs with scheme. Before, `http://localhost:50051" would
cause a crash.

Second, by using the URI scheme to automatically set a GRPC credential if no
GRPC credentials were provided. If we were to remove this part, we could at
least validate that a GRPC credential was provided when the scheme is https.

Finally, by deprecating receiving URLs without scheme, such as `localhost:50051`.
This isn't strictly necessary, but in my opinion makes things more consistent when
we want to support both local sockets and remote urls.

* Remove superflous guards

* Improve error message

* Update lib/grpc/stub.ex

* Compute default ssl option in compile_time

Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>

* format

* Fix mistake

* Add error when trying to use :cred with http

* Minor fixes/improvements

* Test for opening channels with ips, restructure channel test suite

* Fix typespec for connect/2

* Update lib/grpc/stub.ex

* Add scheme in all non-unix socket connect/2 calls

Makes all examples in the documentation include `http` or `https`.
Changes tests to avoid using deprecated code.

* Revert "Add scheme in all non-unix socket connect/2 calls"

This reverts commit ec02a94.

* Remove deprecation warning

* Apply suggestions from code review

---------

Co-authored-by: Paulo Valente <16843419+polvalente@users.noreply.github.com>

Author: v0idpwn

lib/grpc/stub.ex

@@ -131,17 +131,62 @@ defmodule GRPC.Stub do
   """
   @spec connect(String.t(), keyword()) :: {:ok, Channel.t()} | {:error, any()}
   def connect(addr, opts \\ []) when is_binary(addr) and is_list(opts) do
-    {host, port} =
-      case String.split(addr, ":") do
-        [host, port] -> {host, port}
-        [socket_path] -> {{:local, socket_path}, 0}
-      end
+    # This works because we only accept `http` and `https` schemes (allowlisted below explicitly)
+    # addresses like "localhost:1234" parse as if `localhost` is the scheme for URI, and this falls through to
+    # the base case. Accepting only `http/https` is a trait of `connect/3`.
+
+    case URI.parse(addr) do
+      %URI{scheme: @secure_scheme, host: host, port: port} ->
+        opts = Keyword.put_new_lazy(opts, :cred, &default_ssl_option/0)
+        connect(host, port, opts)
+
+      %URI{scheme: @insecure_scheme, host: host, port: port} ->
+        if opts[:cred] do
+          raise ArgumentError, "invalid option for insecure (http) address: :cred"
+        end
+
+        connect(host, port, opts)
+
+      # For compatibility with previous versions, we accept URIs in
+      # the "#{address}:#{port}" format
+      _ ->
+        case String.split(addr, ":") do
+          [socket_path] ->
+            connect({:local, socket_path}, 0, opts)
+
+          [address, port] ->
+            port = String.to_integer(port)
+            connect(address, port, opts)
+        end
+    end
+  end
 
-    connect(host, port, opts)
+  if {:module, CAStore} == Code.ensure_loaded(CAStore) do
+    defp default_ssl_option do
+      %GRPC.Credential{
+        ssl: [
+          verify: :verify_peer,
+          depth: 99,
+          cacert_file: CAStore.file_path()
+        ]
+      }
+    end
+  else
+    defp default_ssl_option do
+      raise """
+      no GRPC credentials provided. Please either:
+
+      - Pass the `:cred` option to `GRPC.Stub.connect/2,3`
+      - Add `:castore` to your list of dependencies in `mix.exs`
+      """
+    end
   end
 
-  @spec connect(String.t(), binary() | non_neg_integer(), keyword()) ::
-          {:ok, Channel.t()} | {:error, any()}
+  @spec connect(
+          String.t() | {:local, String.t()},
+          binary() | non_neg_integer(),
+          keyword()
+        ) :: {:ok, Channel.t()} | {:error, any()}
   def connect(host, port, opts) when is_binary(port) do
     connect(host, String.to_integer(port), opts)
   end

mix.exs

@@ -45,6 +45,7 @@ defmodule GRPC.Mixfile do
       {:gun, "~> 2.0"},
       {:jason, ">= 0.0.0", optional: true},
       {:cowlib, "~> 2.12"},
+      {:castore, "~> 0.1 or ~> 1.0", optional: true},
       {:protobuf, "~> 0.11"},
       {:protobuf_generate, "~> 0.1.1", only: [:dev, :test]},
       {:googleapis,

mix.lock

@@ -1,4 +1,5 @@
 %{
+  "castore": {:hex, :castore, "1.0.6", "ffc42f110ebfdafab0ea159cd43d31365fa0af0ce4a02ecebf1707ae619ee727", [:mix], [], "hexpm", "374c6e7ca752296be3d6780a6d5b922854ffcc74123da90f2f328996b962d33a"},
   "cowboy": {:hex, :cowboy, "2.11.0", "356bf784599cf6f2cdc6ad12fdcfb8413c2d35dab58404cf000e1feaed3f5645", [:make, :rebar3], [{:cowlib, "2.12.1", [hex: :cowlib, repo: "hexpm", optional: false]}, {:ranch, "1.8.0", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm", "0fa395437f1b0e104e0e00999f39d2ac5f4082ac5049b67a5b6d56ecc31b1403"},
   "cowlib": {:hex, :cowlib, "2.12.1", "a9fa9a625f1d2025fe6b462cb865881329b5caff8f1854d1cbc9f9533f00e1e1", [:make, :rebar3], [], "hexpm", "163b73f6367a7341b33c794c4e88e7dbfe6498ac42dcd69ef44c5bc5507c8db0"},
   "dialyxir": {:hex, :dialyxir, "1.4.3", "edd0124f358f0b9e95bfe53a9fcf806d615d8f838e2202a9f430d59566b6b53b", [:mix], [{:erlex, ">= 0.2.6", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "bf2cfb75cd5c5006bec30141b131663299c661a864ec7fbbc72dfa557487a986"},

test/grpc/channel_test.exs

@@ -3,20 +3,69 @@ defmodule GRPC.ChannelTest do
   alias GRPC.Test.ClientAdapter
   alias GRPC.Channel
 
-  test "connect/2 works for insecure" do
-    {:ok, channel} = GRPC.Stub.connect("10.1.0.0:50051", adapter: ClientAdapter)
-    assert %Channel{host: "10.1.0.0", port: 50051, scheme: "http", cred: nil} = channel
-  end
+  for {kind, addr} <- [{"ip", "10.0.0.1"}, {"hostname", "example.com"}] do
+    describe "connect/2 with http and #{kind}" do
+      test "works" do
+        {:ok, channel} =
+          GRPC.Stub.connect("http://#{unquote(addr)}:50051", adapter: ClientAdapter)
+
+        assert %Channel{host: unquote(addr), port: 50051, scheme: "http", cred: nil} = channel
+      end
+
+      test "errors if credential is provided" do
+        cred = %GRPC.Credential{ssl: []}
+
+        assert_raise ArgumentError, "invalid option for insecure (http) address: :cred", fn ->
+          GRPC.Stub.connect("http://#{unquote(addr)}:50051", adapter: ClientAdapter, cred: cred)
+        end
+      end
+    end
+
+    describe "connect/2 with https and #{kind}" do
+      test "sets default credential" do
+        {:ok, channel} =
+          GRPC.Stub.connect("https://#{unquote(addr)}:50051", adapter: ClientAdapter)
+
+        assert %Channel{host: unquote(addr), port: 50051, scheme: "https", cred: cred} = channel
+
+        assert Keyword.has_key?(cred.ssl, :verify)
+        assert Keyword.has_key?(cred.ssl, :depth)
+        assert Keyword.has_key?(cred.ssl, :cacert_file)
+      end
+
+      test "allows overriding default credentials" do
+        cred = %GRPC.Credential{ssl: []}
 
-  test "connect/2 works for ssl" do
-    cred = %{ssl: []}
-    {:ok, channel} = GRPC.Stub.connect("10.1.0.0:50051", adapter: ClientAdapter, cred: cred)
-    assert %Channel{host: "10.1.0.0", port: 50051, scheme: "https", cred: ^cred} = channel
+        {:ok, channel} =
+          GRPC.Stub.connect("https://#{unquote(addr)}:50051", adapter: ClientAdapter, cred: cred)
+
+        assert %Channel{host: unquote(addr), port: 50051, scheme: "https", cred: ^cred} = channel
+      end
+    end
+
+    describe "connect/2 with no scheme, #{kind} and" do
+      test "no cred uses http" do
+        {:ok, channel} = GRPC.Stub.connect("#{unquote(addr)}:50051", adapter: ClientAdapter)
+        assert %Channel{host: unquote(addr), port: 50051, scheme: "http", cred: nil} = channel
+      end
+
+      test "cred uses https" do
+        cred = %{ssl: []}
+
+        {:ok, channel} =
+          GRPC.Stub.connect("#{unquote(addr)}:50051", adapter: ClientAdapter, cred: cred)
+
+        assert %Channel{host: unquote(addr), port: 50051, scheme: "https", cred: ^cred} = channel
+      end
+    end
   end
 
   test "connect/2 allows setting default headers" do
     headers = [{"authorization", "Bearer TOKEN"}]
-    {:ok, channel} = GRPC.Stub.connect("10.1.0.0:50051", adapter: ClientAdapter, headers: headers)
-    assert %Channel{host: "10.1.0.0", port: 50051, headers: ^headers} = channel
+
+    {:ok, channel} =
+      GRPC.Stub.connect("http://10.0.0.1:50051", adapter: ClientAdapter, headers: headers)
+
+    assert %Channel{host: "10.0.0.1", port: 50051, headers: ^headers} = channel
   end
 end