From b6808857ce8a178a97f3fd904ccef511d87e61e2 Mon Sep 17 00:00:00 2001 From: Tom Crossland Date: Thu, 9 Jul 2020 15:53:15 +0200 Subject: [PATCH 1/6] fix: edge case verification failure --- lib/openid_connect.ex | 3 +++ test/openid_connect_test.exs | 38 ++++++++++++++++++++++++++++++++---- 2 files changed, 37 insertions(+), 4 deletions(-) diff --git a/lib/openid_connect.ex b/lib/openid_connect.ex index 4e46d64..a532724 100644 --- a/lib/openid_connect.ex +++ b/lib/openid_connect.ex @@ -179,6 +179,9 @@ defmodule OpenIDConnect do {false, _claims, _jwk} -> {:error, :verify, "verification failed"} + + _ -> + {:error, :verify, "verification error"} end end diff --git a/test/openid_connect_test.exs b/test/openid_connect_test.exs index 2f4770f..b964560 100644 --- a/test/openid_connect_test.exs +++ b/test/openid_connect_test.exs @@ -36,10 +36,14 @@ defmodule OpenIDConnectTest do ] HTTPClientMock - |> expect(:get, fn "https://accounts.google.com/.well-known/openid-configuration", _headers, _opts -> + |> expect(:get, fn "https://accounts.google.com/.well-known/openid-configuration", + _headers, + _opts -> @google_document end) - |> expect(:get, fn "https://www.googleapis.com/oauth2/v3/certs", _headers, _opts -> @google_certs end) + |> expect(:get, fn "https://www.googleapis.com/oauth2/v3/certs", _headers, _opts -> + @google_certs + end) expected_document = @google_document @@ -107,7 +111,9 @@ defmodule OpenIDConnectTest do ] HTTPClientMock - |> expect(:get, fn "https://accounts.google.com/.well-known/openid-configuration", _headers, _opts -> + |> expect(:get, fn "https://accounts.google.com/.well-known/openid-configuration", + _headers, + _opts -> @google_document end) |> expect(:get, fn "https://www.googleapis.com/oauth2/v3/certs", _headers, _opts -> @@ -124,7 +130,9 @@ defmodule OpenIDConnectTest do ] HTTPClientMock - |> expect(:get, fn "https://accounts.google.com/.well-known/openid-configuration", _headers, _opts -> + |> expect(:get, fn "https://accounts.google.com/.well-known/openid-configuration", + _headers, + _opts -> @google_document end) |> expect(:get, fn "https://www.googleapis.com/oauth2/v3/certs", _headers, _opts -> @@ -494,6 +502,28 @@ defmodule OpenIDConnectTest do GenServer.stop(pid) end end + + test "fails when verification fails due to token manipulation" do + {:ok, pid} = GenServer.start_link(MockWorker, [], name: :openid_connect) + + try do + {jwk, []} = Code.eval_file("test/fixtures/rsa/jwk1.exs") + :ok = GenServer.call(pid, {:put, :jwk, JOSE.JWK.from(jwk)}) + + claims = %{"email" => "brian@example.com"} + + {_alg, token} = + jwk + |> JOSE.JWK.from() + |> JOSE.JWS.sign(Jason.encode!(claims), %{"alg" => "RS256"}) + |> JOSE.JWS.compact() + + result = OpenIDConnect.verify(:google, token <> " :)") + assert result == {:error, :verify, "verification error"} + after + GenServer.stop(pid) + end + end end defp set_jose_json_lib(_) do From fb1478aa0412d36709ad6aac3c4bd2926794eed1 Mon Sep 17 00:00:00 2001 From: Tom Crossland Date: Mon, 3 Aug 2020 11:22:46 +0200 Subject: [PATCH 2/6] #24: worker was dying if update_documents failed --- lib/openid_connect/worker.ex | 43 +++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/lib/openid_connect/worker.ex b/lib/openid_connect/worker.ex index 1b73484..b93ac48 100644 --- a/lib/openid_connect/worker.ex +++ b/lib/openid_connect/worker.ex @@ -9,6 +9,8 @@ defmodule OpenIDConnect.Worker do @refresh_time 60 * 60 * 1000 + require Logger + def start_link(provider_configs, name \\ :openid_connect) do GenServer.start_link(__MODULE__, provider_configs, name: name) end @@ -20,8 +22,8 @@ defmodule OpenIDConnect.Worker do def init(provider_configs) do state = Enum.into(provider_configs, %{}, fn {provider, config} -> - documents = update_documents(provider, config) - {provider, %{config: config, documents: documents}} + Process.send_after(self(), {:update_documents, provider}, 0) + {provider, %{config: config, documents: nil}} end) {:ok, state} @@ -43,23 +45,34 @@ defmodule OpenIDConnect.Worker do end def handle_info({:update_documents, provider}, state) do - config = get_in(state, [provider, :config]) - documents = update_documents(provider, config) - - state = put_in(state, [provider, :documents], documents) - - {:noreply, state} + with config <- get_in(state, [provider, :config]), + {:ok, documents} <- update_documents(provider, config) do + {:noreply, put_in(state, [provider, :documents], documents)} + else + _ -> {:noreply, state} + end end defp update_documents(provider, config) do - {:ok, %{remaining_lifetime: remaining_lifetime}} = - {:ok, documents} = OpenIDConnect.update_documents(config) - - refresh_time = time_until_next_refresh(remaining_lifetime) - - Process.send_after(self(), {:update_documents, provider}, refresh_time) + with {:ok, documents} <- OpenIDConnect.update_documents(config), + remaining_lifetime <- Map.get(documents, :remaining_lifetime), + refresh_time <- time_until_next_refresh(remaining_lifetime) do + Process.send_after(self(), {:update_documents, provider}, refresh_time) + {:ok, documents} + else + {:error, :update_documents, reason} = error -> + Logger.warn("Failed to update documents for provider #{provider}: #{message(reason)}") + Process.send_after(self(), {:update_documents, provider}, @refresh_time) + error + end + end - documents + defp message(reason) do + if Exception.exception?(reason) do + Exception.message(reason) + else + "#{inspect(reason)}" + end end defp time_until_next_refresh(nil), do: @refresh_time From 2b9bba5e345fb377f5f44484eaa275eb82624119 Mon Sep 17 00:00:00 2001 From: Tom Crossland Date: Mon, 3 Aug 2020 11:58:57 +0200 Subject: [PATCH 3/6] fix: tests failed as update_documents was deferred from Worker.init --- test/openid_connect/worker_test.exs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/openid_connect/worker_test.exs b/test/openid_connect/worker_test.exs index 281d3a9..e4388f0 100644 --- a/test/openid_connect/worker_test.exs +++ b/test/openid_connect/worker_test.exs @@ -19,7 +19,7 @@ defmodule OpenIDConnect.WorkerTest do config = Application.get_env(:openid_connect, :providers) - {:ok, pid} = start_supervised({OpenIDConnect.Worker, config}) + {:ok, pid} = start_worker(config) state = :sys.get_state(pid) @@ -43,10 +43,9 @@ defmodule OpenIDConnect.WorkerTest do test "worker can respond to a call for the config" do mock_http_requests() - config = Application.get_env(:openid_connect, :providers) - {:ok, pid} = start_supervised({OpenIDConnect.Worker, config}) + {:ok, pid} = start_worker(config) google_config = GenServer.call(pid, {:config, :google}) @@ -58,8 +57,7 @@ defmodule OpenIDConnect.WorkerTest do config = Application.get_env(:openid_connect, :providers) - {:ok, pid} = start_supervised({OpenIDConnect.Worker, config}) - + {:ok, pid} = start_worker(config) discovery_document = GenServer.call(pid, {:discovery_document, :google}) expected_document = @@ -77,7 +75,7 @@ defmodule OpenIDConnect.WorkerTest do config = Application.get_env(:openid_connect, :providers) - {:ok, pid} = start_supervised({OpenIDConnect.Worker, config}) + {:ok, pid} = start_worker(config) jwk = GenServer.call(pid, {:jwk, :google}) @@ -98,4 +96,10 @@ defmodule OpenIDConnect.WorkerTest do end) |> expect(:get, fn "https://www.googleapis.com/oauth2/v3/certs", _headers, _opts -> @google_certs end) end + + defp start_worker(config) do + {:ok, pid} = start_supervised({OpenIDConnect.Worker, config}) + Process.sleep(10) # allow :update_documents to run + {:ok, pid} + end end From 82fea92acf2043e2c9751c0c53331f7a8fedbd95 Mon Sep 17 00:00:00 2001 From: Tom Crossland Date: Mon, 3 Aug 2020 18:53:06 +0200 Subject: [PATCH 4/6] refactor: avoid process sleep in tests --- lib/openid_connect/worker.ex | 23 +++++++++++++++++++---- test/openid_connect/worker_test.exs | 4 ++-- 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/lib/openid_connect/worker.ex b/lib/openid_connect/worker.ex index b93ac48..11d377e 100644 --- a/lib/openid_connect/worker.ex +++ b/lib/openid_connect/worker.ex @@ -11,21 +11,31 @@ defmodule OpenIDConnect.Worker do require Logger - def start_link(provider_configs, name \\ :openid_connect) do - GenServer.start_link(__MODULE__, provider_configs, name: name) + def start_link(options, name \\ :openid_connect) + + def start_link({provider_configs, notify_pid}, name) do + GenServer.start_link(__MODULE__, {provider_configs, notify_pid}, name: name) + end + + def start_link(provider_configs, name) do + start_link({provider_configs, nil}, name) end - def init(:ignore) do + def init({:ignore, _pid}) do :ignore end - def init(provider_configs) do + def init({provider_configs, notify_pid}) do state = Enum.into(provider_configs, %{}, fn {provider, config} -> Process.send_after(self(), {:update_documents, provider}, 0) {provider, %{config: config, documents: nil}} end) + unless is_nil(notify_pid) do + Process.send_after(self(), {:notify, notify_pid}, 1) + end + {:ok, state} end @@ -44,6 +54,11 @@ defmodule OpenIDConnect.Worker do {:reply, config, state} end + def handle_info({:notify, pid}, state) do + Process.send(pid, :ready, []) + {:noreply, state} + end + def handle_info({:update_documents, provider}, state) do with config <- get_in(state, [provider, :config]), {:ok, documents} <- update_documents(provider, config) do diff --git a/test/openid_connect/worker_test.exs b/test/openid_connect/worker_test.exs index e4388f0..78fc1fa 100644 --- a/test/openid_connect/worker_test.exs +++ b/test/openid_connect/worker_test.exs @@ -98,8 +98,8 @@ defmodule OpenIDConnect.WorkerTest do end defp start_worker(config) do - {:ok, pid} = start_supervised({OpenIDConnect.Worker, config}) - Process.sleep(10) # allow :update_documents to run + {:ok, pid} = start_supervised({OpenIDConnect.Worker, {config, self()}}) + assert_receive :ready {:ok, pid} end end From 049995253524e82ded81108fad09021a1197381b Mon Sep 17 00:00:00 2001 From: Tom Crossland Date: Mon, 3 Aug 2020 19:05:23 +0200 Subject: [PATCH 5/6] refactor: schedule notification of test pid in init --- lib/openid_connect/worker.ex | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/lib/openid_connect/worker.ex b/lib/openid_connect/worker.ex index 11d377e..2ba02f6 100644 --- a/lib/openid_connect/worker.ex +++ b/lib/openid_connect/worker.ex @@ -13,29 +13,27 @@ defmodule OpenIDConnect.Worker do def start_link(options, name \\ :openid_connect) - def start_link({provider_configs, notify_pid}, name) do - GenServer.start_link(__MODULE__, {provider_configs, notify_pid}, name: name) + def start_link(config, name) do + GenServer.start_link(__MODULE__, config, name: name) end - def start_link(provider_configs, name) do - start_link({provider_configs, nil}, name) + def init(:ignore) do + :ignore end - def init({:ignore, _pid}) do - :ignore + def init({provider_configs, pid}) when is_pid(pid) do + {:ok, state} = init(provider_configs) + Process.send_after(pid, :ready, 1) + {:ok, state} end - def init({provider_configs, notify_pid}) do + def init(provider_configs) do state = Enum.into(provider_configs, %{}, fn {provider, config} -> Process.send_after(self(), {:update_documents, provider}, 0) {provider, %{config: config, documents: nil}} end) - unless is_nil(notify_pid) do - Process.send_after(self(), {:notify, notify_pid}, 1) - end - {:ok, state} end @@ -54,11 +52,6 @@ defmodule OpenIDConnect.Worker do {:reply, config, state} end - def handle_info({:notify, pid}, state) do - Process.send(pid, :ready, []) - {:noreply, state} - end - def handle_info({:update_documents, provider}, state) do with config <- get_in(state, [provider, :config]), {:ok, documents} <- update_documents(provider, config) do From cfb424ef5e524f4be6cd25e1d79e1c2e0269aae0 Mon Sep 17 00:00:00 2001 From: Tom Crossland Date: Mon, 3 Aug 2020 19:16:10 +0200 Subject: [PATCH 6/6] test: test case for dns failure --- test/openid_connect/worker_test.exs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/openid_connect/worker_test.exs b/test/openid_connect/worker_test.exs index 78fc1fa..abf8ade 100644 --- a/test/openid_connect/worker_test.exs +++ b/test/openid_connect/worker_test.exs @@ -89,6 +89,14 @@ defmodule OpenIDConnect.WorkerTest do assert expected_jwk == jwk end + test "worker doesn't die if dns fails" do + mock_nxdomain_error() + + config = Application.get_env(:openid_connect, :providers) + + assert {:ok, _} = start_worker(config) + end + defp mock_http_requests do HTTPClientMock |> expect(:get, fn "https://accounts.google.com/.well-known/openid-configuration", _headers, _opts -> @@ -97,6 +105,11 @@ defmodule OpenIDConnect.WorkerTest do |> expect(:get, fn "https://www.googleapis.com/oauth2/v3/certs", _headers, _opts -> @google_certs end) end + defp mock_nxdomain_error do + HTTPClientMock + |> expect(:get, fn _, _, _ -> {:error, %HTTPoison.Error{id: nil, reason: :nxdomain}} end) + end + defp start_worker(config) do {:ok, pid} = start_supervised({OpenIDConnect.Worker, {config, self()}}) assert_receive :ready