From ee863033eb997826781edf3292a0e26941e50cde Mon Sep 17 00:00:00 2001 From: Mike Cifelli Date: Sat, 30 Mar 2024 11:32:28 -0400 Subject: [PATCH] Add error checking in nts controller --- config/test.exs | 2 + .../nts/key_establishment_client.ex | 12 ++- .../v1/nts/key_establishment_controller.ex | 37 +++++++-- mix.exs | 3 +- mix.lock | 1 + .../nts/key_establishment_controller_test.exs | 81 +++++++++++++++++++ test/support/mocks.ex | 1 + 7 files changed, 130 insertions(+), 7 deletions(-) create mode 100644 test/chronoscope_web/controllers/api/v1/nts/key_establishment_controller_test.exs create mode 100644 test/support/mocks.ex diff --git a/config/test.exs b/config/test.exs index d28018b..2b2f47e 100644 --- a/config/test.exs +++ b/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 diff --git a/lib/chronoscope/nts/key_establishment_client.ex b/lib/chronoscope/nts/key_establishment_client.ex index a410b2e..67b606d 100644 --- a/lib/chronoscope/nts/key_establishment_client.ex +++ b/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 diff --git a/lib/chronoscope_web/controllers/api/v1/nts/key_establishment_controller.ex b/lib/chronoscope_web/controllers/api/v1/nts/key_establishment_controller.ex index 5a3d27f..8baf4d1 100644 --- a/lib/chronoscope_web/controllers/api/v1/nts/key_establishment_controller.ex +++ b/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 diff --git a/mix.exs b/mix.exs index c9b931f..99b1d72 100644 --- a/mix.exs +++ b/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 diff --git a/mix.lock b/mix.lock index dba61ce..21c528f 100644 --- a/mix.lock +++ b/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"}, diff --git a/test/chronoscope_web/controllers/api/v1/nts/key_establishment_controller_test.exs b/test/chronoscope_web/controllers/api/v1/nts/key_establishment_controller_test.exs new file mode 100644 index 0000000..416dd5b --- /dev/null +++ b/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 diff --git a/test/support/mocks.ex b/test/support/mocks.ex new file mode 100644 index 0000000..e3911a0 --- /dev/null +++ b/test/support/mocks.ex @@ -0,0 +1 @@ +Mox.defmock(Chronoscope.NTS.BehaviourMock, for: Chronoscope.NTS.Behaviour)