From d100372e5596010cd9a54d0f3536956c678d7dd9 Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Sun, 16 Feb 2025 23:12:27 +1300 Subject: [PATCH 1/3] Add SSL validation support for `certs_keys` The `certs_keys` config key was added recently(ish) to the Erlang SSL stack. https://www.erlang.org/doc/apps/ssl/ssl.html#t:cert_key_conf/0 --- lib/plug/ssl.ex | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/lib/plug/ssl.ex b/lib/plug/ssl.ex index af21406a..13e31270 100644 --- a/lib/plug/ssl.ex +++ b/lib/plug/ssl.ex @@ -169,6 +169,7 @@ defmodule Plug.SSL do |> check_for_missing_keys() |> validate_ciphers() |> normalize_ssl_files() + |> normalize_certs_keys_ssl_files() |> convert_to_charlist() |> set_secure_defaults() |> configure_managed_tls() @@ -179,14 +180,15 @@ defmodule Plug.SSL do end defp check_for_missing_keys(options) do + has_certs_keys? = List.keymember?(options, :certs_keys, 0) has_sni? = List.keymember?(options, :sni_hosts, 0) or List.keymember?(options, :sni_fun, 0) has_key? = List.keymember?(options, :key, 0) or List.keymember?(options, :keyfile, 0) has_cert? = List.keymember?(options, :cert, 0) or List.keymember?(options, :certfile, 0) cond do has_sni? -> options - not has_key? -> fail("missing option :key/:keyfile") - not has_cert? -> fail("missing option :cert/:certfile") + not (has_key? or has_certs_keys?) -> fail("missing option :key/:keyfile/:certs_keys") + not (has_cert? or has_certs_keys?) -> fail("missing option :cert/:certfile/:certs_keys") true -> options end end @@ -196,6 +198,22 @@ defmodule Plug.SSL do Enum.reduce(ssl_files, options, &normalize_ssl_file(&1, &2)) end + defp normalize_certs_keys_ssl_files(options) do + if certs_keys = options[:certs_keys] do + ssl_files = [:keyfile, :certfile] + + updated_certs_keys = + Enum.map(certs_keys, fn cert_key -> + Enum.reduce(ssl_files, Map.to_list(cert_key), &normalize_ssl_file(&1, &2)) + |> Map.new() + end) + + List.keystore(options, :certs_keys, 0, {:certs_keys, updated_certs_keys}) + else + options + end + end + defp normalize_ssl_file(key, options) do value = options[key] From fb2e2baabd13b081abc12b0a86a470cb9ffa5e93 Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Mon, 17 Feb 2025 00:38:01 +1300 Subject: [PATCH 2/3] Simplify the Keyword list updating logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: José Valim --- lib/plug/ssl.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/plug/ssl.ex b/lib/plug/ssl.ex index 13e31270..78557af1 100644 --- a/lib/plug/ssl.ex +++ b/lib/plug/ssl.ex @@ -208,7 +208,7 @@ defmodule Plug.SSL do |> Map.new() end) - List.keystore(options, :certs_keys, 0, {:certs_keys, updated_certs_keys}) + Keyword.put(options, :certs_keys, updated_certs_keys) else options end From 17dcf4dfcd3b3299a4eca660e92f19b1a294ad23 Mon Sep 17 00:00:00 2001 From: Josh Kalderimis Date: Mon, 17 Feb 2025 10:42:57 +1300 Subject: [PATCH 3/3] Add tests for a handful of `keyfile`/`certfile` usecases --- lib/plug/ssl.ex | 20 +++++++----- test/plug/ssl_test.exs | 72 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/lib/plug/ssl.ex b/lib/plug/ssl.ex index 78557af1..a1f49875 100644 --- a/lib/plug/ssl.ex +++ b/lib/plug/ssl.ex @@ -195,7 +195,7 @@ defmodule Plug.SSL do defp normalize_ssl_files(options) do ssl_files = [:keyfile, :certfile, :cacertfile, :dhfile] - Enum.reduce(ssl_files, options, &normalize_ssl_file(&1, &2)) + Enum.reduce(ssl_files, options, &normalize_ssl_file(&1, &2, options[:otp_app])) end defp normalize_certs_keys_ssl_files(options) do @@ -204,17 +204,21 @@ defmodule Plug.SSL do updated_certs_keys = Enum.map(certs_keys, fn cert_key -> - Enum.reduce(ssl_files, Map.to_list(cert_key), &normalize_ssl_file(&1, &2)) + Enum.reduce( + ssl_files, + Map.to_list(cert_key), + &normalize_ssl_file(&1, &2, options[:otp_app]) + ) |> Map.new() end) - Keyword.put(options, :certs_keys, updated_certs_keys) + List.keystore(options, :certs_keys, 0, {:certs_keys, updated_certs_keys}) else options end end - defp normalize_ssl_file(key, options) do + defp normalize_ssl_file(key, options, otp_app) do value = options[key] cond do @@ -225,7 +229,7 @@ defmodule Plug.SSL do put_ssl_file(options, key, value) true -> - put_ssl_file(options, key, Path.expand(value, otp_app(options))) + put_ssl_file(options, key, Path.expand(value, resolve_otp_app(otp_app))) end end @@ -243,9 +247,9 @@ defmodule Plug.SSL do List.keystore(options, key, 0, {key, value}) end - defp otp_app(options) do - if app = options[:otp_app] do - Application.app_dir(app) + defp resolve_otp_app(otp_app) do + if otp_app do + Application.app_dir(otp_app) else fail("the :otp_app option is required when setting relative SSL certfiles") end diff --git a/test/plug/ssl_test.exs b/test/plug/ssl_test.exs index f86e379e..d35dcc21 100644 --- a/test/plug/ssl_test.exs +++ b/test/plug/ssl_test.exs @@ -4,6 +4,17 @@ defmodule Plug.SSLTest do import Plug.Conn describe "configure" do + # make sure some dummy files used for the keyfile and certfile + # tests are removed after each test. + setup do + on_exit(fn -> + File.rm("_build/test/lib/plug/abcdef") + File.rm("_build/test/lib/plug/ghijkl") + end) + + [] + end + import Plug.SSL, only: [configure: 1] test "sets secure_renegotiate and reuse_sessions to true depending on the version" do @@ -97,6 +108,67 @@ defmodule Plug.SSLTest do assert {:cert, "ghijkl"} in opts end + test "fails to configure if keyfile and certfile aren't absolute paths and otp_app is missing" do + assert {:error, message} = configure([:inet6, keyfile: "abcdef", certfile: "ghijkl"]) + assert message == "the :otp_app option is required when setting relative SSL certfiles" + end + + test "fails to configure if the keyfile doesn't exist" do + assert {:error, message} = + configure([:inet6, keyfile: "abcdef", certfile: "ghijkl", otp_app: :plug]) + + assert message =~ + ":keyfile either does not exist, or the application does not have permission to access it" + end + + test "fails to configure if the certfile doesn't exist" do + File.touch("_build/test/lib/plug/abcdef") + + assert {:error, message} = + configure([:inet6, keyfile: "abcdef", certfile: "ghijkl", otp_app: :plug]) + + assert message =~ + ":certfile either does not exist, or the application does not have permission to access it" + end + + test "expands the paths to the keyfile and certfile using the otp_app" do + File.touch("_build/test/lib/plug/abcdef") + File.touch("_build/test/lib/plug/ghijkl") + + assert {:ok, opts} = + configure([:inet6, keyfile: "abcdef", certfile: "ghijkl", otp_app: :plug]) + + assert to_string(opts[:keyfile]) =~ "_build/test/lib/plug/abcdef" + assert to_string(opts[:certfile]) =~ "_build/test/lib/plug/ghijkl" + end + + test "supports the certs_keys ssl config option" do + assert {:ok, opts} = + configure([:inet6, certs_keys: [%{key: "abcdef", cert: "ghijkl"}]]) + + assert :inet6 in opts + assert opts[:certs_keys] == [%{key: "abcdef", cert: "ghijkl"}] + end + + test "expands the paths for keyfile and certfile in the certs_keys ssl config option" do + File.touch("_build/test/lib/plug/abcdef") + File.touch("_build/test/lib/plug/ghijkl") + + assert {:ok, opts} = + configure([ + :inet6, + certs_keys: [%{keyfile: "abcdef", certfile: "ghijkl"}], + otp_app: :plug + ]) + + assert :inet6 in opts + + [%{keyfile: keyfile, certfile: certfile}] = opts[:certs_keys] + + assert to_string(keyfile) =~ "_build/test/lib/plug/abcdef" + assert to_string(certfile) =~ "_build/test/lib/plug/ghijkl" + end + test "errors when an invalid cipher is given" do assert configure(key: "abcdef", cert: "ghijkl", cipher_suite: :unknown) == {:error, "unknown :cipher_suite named :unknown"}