Add error checking in nts controller

config/test.exs

@@ -18,3 +18,5 @@ config :logger, level: :warning
 
 # Initialize plugs at runtime for faster test compilation
 config :phoenix, :plug_init_mode, :runtime
+
+config :chronoscope, Chronoscope.NTS, behaviour: Chronoscope.NTS.BehaviourMock

lib/chronoscope/nts/key_establishment_client.ex

@@ -10,7 +10,7 @@ defmodule Chronoscope.NTS.KeyEstablishmentClient do
   def key_establishment(%{host: host, port: port}) do
     case ssl_connect(host, port) do
       {:ok, socket} -> perform_key_establishment(socket)
-      {:error, {:tls_alert, {:handshake_failure, error}}} -> {:error, String.trim("#{error}")}
+      {:error, {:tls_alert, {:handshake_failure, error}}} -> handshake_failure_message("#{error}")
       {:error, {:tls_alert, {:no_application_protocol, error}}} -> {:error, String.trim("#{error}")}
       {:error, :timeout} -> {:error, :timeout}
       {:error, error} -> {:error, inspect(error)}
@@ -55,4 +55,14 @@ defmodule Chronoscope.NTS.KeyEstablishmentClient do
         {:error, :timeout}
     end
   end
+
+  defp handshake_failure_message(error) do
+    cond do
+      error =~ ~r/\{bad_cert,hostname_check_failed\}$/ ->
+        {:error, "The certificate is NOT trusted. The name in the certificate does not match the expected."}
+
+      true ->
+        {:error, String.trim(error)}
+    end
+  end
 end

lib/chronoscope_web/controllers/api/v1/nts/key_establishment_controller.ex

@@ -5,13 +5,26 @@ defmodule ChronoscopeWeb.API.V1.NTS.KeyEstablishmentController do
 
   alias Chronoscope.NTS
 
-  @default_port "4460"
+  @default_port 4460
 
-  def get(conn, params) do
-    host = params["host"]
-    port = String.to_integer(params["port"] || @default_port)
+  def get(conn, %{"host" => _, "port" => port} = params) do
+    try do
+      handle_get(conn, Map.put(params, "port", String.to_integer(port)))
+    rescue
+      ArgumentError -> bad_request_response(conn, "invalid port")
+    end
+  end
 
-    case NTS.key_establishment(host, port) do
+  def get(conn, %{"host" => _} = params) do
+    handle_get(conn, Map.put(params, "port", @default_port))
+  end
+
+  def get(conn, _params) do
+    bad_request_response(conn, "missing host")
+  end
+
+  defp handle_get(conn, %{"host" => host, "port" => port}) when is_integer(port) and port > 0 and port < 65536 do
+    case nts_behaviour().key_establishment(host, port) do
       {:ok, response} ->
         json(conn, %{status: :ok, response: format_response(response)})
 
@@ -20,9 +33,23 @@ defmodule ChronoscopeWeb.API.V1.NTS.KeyEstablishmentController do
     end
   end
 
+  defp handle_get(conn, _params) do
+    bad_request_response(conn, "invalid port")
+  end
+
   defp format_response(response) do
     response
     |> Map.take([:aead_algorithms, :cert_expiration, :cookie_length, :cookies, :next_protocols, :port, :server])
     |> Map.update(:cookies, 0, &length/1)
   end
+
+  defp bad_request_response(conn, message) do
+    conn
+    |> put_status(:bad_request)
+    |> json(%{error: message})
+  end
+
+  defp nts_behaviour() do
+    Application.get_env(:chronoscope, NTS)[:behaviour] || NTS
+  end
 end

mix.exs

@@ -50,7 +50,8 @@ defmodule Chronoscope.MixProject do
       {:dns_cluster, "~> 0.1.1"},
       {:bandit, "~> 1.2"},
       {:tls_certificate_check, "~> 1.21"},
-      {:x509, "~> 0.8"}
+      {:x509, "~> 0.8"},
+      {:mox, "~> 1.1", only: :test}
     ]
   end
 

mix.lock

@@ -13,6 +13,7 @@
   "jason": {:hex, :jason, "1.4.1", "af1504e35f629ddcdd6addb3513c3853991f694921b1b9368b0bd32beb9f1b63", [:mix], [{:decimal, "~> 1.0 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "fbb01ecdfd565b56261302f7e1fcc27c4fb8f32d56eab74db621fc154604a7a1"},
   "mime": {:hex, :mime, "2.0.5", "dc34c8efd439abe6ae0343edbb8556f4d63f178594894720607772a041b04b02", [:mix], [], "hexpm", "da0d64a365c45bc9935cc5c8a7fc5e49a0e0f9932a761c55d6c52b142780a05c"},
   "mint": {:hex, :mint, "1.5.2", "4805e059f96028948870d23d7783613b7e6b0e2fb4e98d720383852a760067fd", [:mix], [{:castore, "~> 0.1.0 or ~> 1.0", [hex: :castore, repo: "hexpm", optional: true]}, {:hpax, "~> 0.1.1", [hex: :hpax, repo: "hexpm", optional: false]}], "hexpm", "d77d9e9ce4eb35941907f1d3df38d8f750c357865353e21d335bdcdf6d892a02"},
+  "mox": {:hex, :mox, "1.1.0", "0f5e399649ce9ab7602f72e718305c0f9cdc351190f72844599545e4996af73c", [:mix], [], "hexpm", "d44474c50be02d5b72131070281a5d3895c0e7a95c780e90bc0cfe712f633a13"},
   "nimble_options": {:hex, :nimble_options, "1.1.0", "3b31a57ede9cb1502071fade751ab0c7b8dbe75a9a4c2b5bbb0943a690b63172", [:mix], [], "hexpm", "8bbbb3941af3ca9acc7835f5655ea062111c9c27bcac53e004460dfd19008a99"},
   "nimble_pool": {:hex, :nimble_pool, "1.0.0", "5eb82705d138f4dd4423f69ceb19ac667b3b492ae570c9f5c900bb3d2f50a847", [:mix], [], "hexpm", "80be3b882d2d351882256087078e1b1952a28bf98d0a287be87e4a24a710b67a"},
   "phoenix": {:hex, :phoenix, "1.7.11", "1d88fc6b05ab0c735b250932c4e6e33bfa1c186f76dcf623d8dd52f07d6379c7", [:mix], [{:castore, ">= 0.0.0", [hex: :castore, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: true]}, {:phoenix_pubsub, "~> 2.1", [hex: :phoenix_pubsub, repo: "hexpm", optional: false]}, {:phoenix_template, "~> 1.0", [hex: :phoenix_template, repo: "hexpm", optional: false]}, {:phoenix_view, "~> 2.0", [hex: :phoenix_view, repo: "hexpm", optional: true]}, {:plug, "~> 1.14", [hex: :plug, repo: "hexpm", optional: false]}, {:plug_cowboy, "~> 2.7", [hex: :plug_cowboy, repo: "hexpm", optional: true]}, {:plug_crypto, "~> 1.2 or ~> 2.0", [hex: :plug_crypto, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}, {:websock_adapter, "~> 0.5.3", [hex: :websock_adapter, repo: "hexpm", optional: false]}], "hexpm", "b1ec57f2e40316b306708fe59b92a16b9f6f4bf50ccfa41aa8c7feb79e0ec02a"},

test/chronoscope_web/controllers/api/v1/nts/key_establishment_controller_test.exs

@@ -0,0 +1,81 @@
+defmodule ChronoscopeWeb.API.V1.NTS.KeyEstablishmentControllerTest do
+  use ChronoscopeWeb.ConnCase, async: true
+
+  import Mox
+
+  setup :verify_on_exit!
+
+  test "requires a host name", %{conn: conn} do
+    response =
+      conn
+      |> get(~p"/api/v1/nts/key-establishment")
+      |> json_response(400)
+
+    assert %{"error" => "missing host"} == response
+  end
+
+  test "uses the given port number", %{conn: conn} do
+    Chronoscope.NTS.BehaviourMock
+    |> expect(:key_establishment, fn "localhost", 4461 -> {:ok, %{status: :ok}} end)
+
+    response =
+      conn
+      |> get(~p"/api/v1/nts/key-establishment?host=localhost&port=4461")
+      |> json_response(200)
+
+    assert %{"status" => "ok", "response" => %{"cookies" => 0}} == response
+  end
+
+  test "handles an out of range port number", %{conn: conn} do
+    response =
+      conn
+      |> get(~p"/api/v1/nts/key-establishment?host=localhost&port=65536")
+      |> json_response(400)
+
+    assert %{"error" => "invalid port"} == response
+  end
+
+  test "handles a negative port number", %{conn: conn} do
+    response =
+      conn
+      |> get(~p"/api/v1/nts/key-establishment?host=localhost&port=-4460")
+      |> json_response(400)
+
+    assert %{"error" => "invalid port"} == response
+  end
+
+  test "handles a bad port number", %{conn: conn} do
+    response =
+      conn
+      |> get(~p"/api/v1/nts/key-establishment?host=localhost&port=AA60")
+      |> json_response(400)
+
+    assert %{"error" => "invalid port"} == response
+  end
+
+  test "returns an empty successful response", %{conn: conn} do
+    Chronoscope.NTS.BehaviourMock
+    |> expect(:key_establishment, fn "localhost", 4460 -> {:ok, %{status: :ok}} end)
+
+    response =
+      conn
+      |> get(~p"/api/v1/nts/key-establishment?host=localhost")
+      |> json_response(200)
+
+    assert %{"status" => "ok", "response" => %{"cookies" => 0}} == response
+  end
+
+  test "returns a full successful response", %{conn: conn} do
+    Chronoscope.NTS.BehaviourMock
+    |> expect(:key_establishment, fn "localhost", 4460 ->
+      {:ok, %{cookies: [[], [], []], cookie_length: 300}}
+    end)
+
+    response =
+      conn
+      |> get(~p"/api/v1/nts/key-establishment?host=localhost")
+      |> json_response(200)
+
+    assert %{"status" => "ok", "response" => %{"cookies" => 3, "cookie_length" => 300}} == response
+  end
+end

test/support/mocks.ex

@@ -0,0 +1 @@
+Mox.defmock(Chronoscope.NTS.BehaviourMock, for: Chronoscope.NTS.Behaviour)