Merge pull request #241 from borag/secret-safe

Harden secret handling in connection settings

Author: mmmries

lib/gnat.ex

@@ -51,6 +51,20 @@ defmodule Gnat do
   * `inbox_prefix` - Prefix to use for the message inbox of this connection
   * `no_responders` - Enable the no responders behavior (see `Gnat.request/4`)
   * `name` - The client name reported by NATS monitoring endpoints
+  * `nkey_seed` - The nkey seed used to sign the server's nonce: a zero-arity function that
+    returns the nkey seed as a binary. A binary is also accepted for backward compatibility, but
+    is not recommended, because the seed can then appear in supervisor crash reports.
+  * `username` - The user name for user/password authentication
+  * `password` - The password for user/password authentication: a zero-arity function that
+    returns the password as a binary. A binary is also accepted for backward compatibility, but
+    is not recommended, because the password can then appear in supervisor crash reports.
+  * `token` - The token for token authentication: a zero-arity function that returns the token
+    as a binary. A binary is also accepted for backward compatibility, but is not recommended,
+    because the token can then appear in supervisor crash reports.
+
+  A function is called while the CONNECT message is built, during each connection handshake.
+  The `nkey_seed`, `password` and `token` settings are redacted from the connection's process
+  status, its crash reports and the connection supervisor's log.
   """
   @type connection_settings :: %{
           optional(:connection_timeout) => non_neg_integer(),
@@ -62,7 +76,11 @@ defmodule Gnat do
           optional(:tcp_opts) => list(),
           optional(:tls) => boolean(),
           optional(:no_responders) => boolean(),
-          optional(:name) => binary()
+          optional(:name) => binary(),
+          optional(:nkey_seed) => binary() | (-> binary()),
+          optional(:username) => binary(),
+          optional(:password) => binary() | (-> binary()),
+          optional(:token) => binary() | (-> binary())
         }
 
   @typedoc """
@@ -468,6 +486,15 @@ defmodule Gnat do
     GenServer.call(name, :server_info)
   end
 
+  # GenServer declares format_status/1 from Elixir 1.17; OTP calls it on every supported version.
+  if Version.match?(System.version(), ">= 1.17.0"), do: @impl(GenServer)
+
+  def format_status(%{state: %{connection_settings: settings} = state} = status) do
+    %{status | state: %{state | connection_settings: Gnat.Handshake.redact_settings(settings)}}
+  end
+
+  def format_status(status), do: status
+
   @impl GenServer
   def init(connection_settings) do
     connection_settings = Map.merge(@default_connection_settings, connection_settings)

lib/gnat/connection_supervisor.ex

@@ -19,7 +19,7 @@ defmodule Gnat.ConnectionSupervisor do
   }
   ```
 
-  The connection settings can specify all of the same values that you pass to `Gnat.start_link/1`. Each time a connection is attempted we will use one of the provided connection settings to open the connection. This is a simplistic way of load balancing your connections across a cluster of nats nodes and allowing failover to other nodes in the cluster if one goes down.
+  The connection settings can specify all of the same values that you pass to `Gnat.start_link/1`. Their secrets are redacted from this process's status and crash reports. Each time a connection is attempted we will use one of the provided connection settings to open the connection. This is a simplistic way of load balancing your connections across a cluster of nats nodes and allowing failover to other nodes in the cluster if one goes down.
 
   To use this in your supervision tree add an entry like this:
 
@@ -58,7 +58,7 @@ defmodule Gnat.ConnectionSupervisor do
   @impl GenServer
   def handle_info(:attempt_connection, state) do
     connection_config = random_connection_config(state)
-    Logger.debug("connecting to #{inspect(connection_config)}")
+    Logger.debug("connecting to #{inspect(Gnat.Handshake.redact_settings(connection_config))}")
 
     case Gnat.start_link(connection_config, name: state.name) do
       {:ok, gnat} ->
@@ -89,6 +89,17 @@ defmodule Gnat.ConnectionSupervisor do
     {:noreply, state}
   end
 
+  # GenServer declares format_status/1 from Elixir 1.17; OTP calls it on every supported version.
+  if Version.match?(System.version(), ">= 1.17.0"), do: @impl(GenServer)
+
+  def format_status(%{state: %{connection_settings: settings} = state} = status)
+      when is_list(settings) do
+    redacted = Enum.map(settings, &Gnat.Handshake.redact_settings/1)
+    %{status | state: %{state | connection_settings: redacted}}
+  end
+
+  def format_status(status), do: status
+
   defp random_connection_config(%{connection_settings: connection_settings}) do
     connection_settings |> Enum.random()
   end

lib/gnat/handshake.ex

@@ -15,6 +15,20 @@ defmodule Gnat.Handshake do
     end
   end
 
+  @secret_settings [:nkey_seed, :password, :token]
+
+  @doc """
+  Replaces the secrets in connection settings so they can be shown in process status and
+  crash reports.
+  """
+  def redact_settings(settings) when is_map(settings) do
+    Enum.reduce(@secret_settings, settings, fn key, acc ->
+      if Map.has_key?(acc, key), do: Map.put(acc, key, :redacted), else: acc
+    end)
+  end
+
+  def redact_settings(settings), do: settings
+
   def negotiate_settings(server_settings, user_settings) do
     auth_required = server_settings[:auth_required] || user_settings[:auth_required] || false
 
@@ -53,11 +67,11 @@ defmodule Gnat.Handshake do
          %{username: username, password: password} = _user,
          true = _auth_required
        ) do
-    Map.merge(settings, %{user: username, pass: password})
+    Map.merge(settings, %{user: username, pass: resolve_secret(password)})
   end
 
   defp negotiate_auth(settings, _server, %{token: token} = _user, true = _auth_required) do
-    Map.merge(settings, %{auth_token: token})
+    Map.merge(settings, %{auth_token: resolve_secret(token)})
   end
 
   defp negotiate_auth(
@@ -66,7 +80,7 @@ defmodule Gnat.Handshake do
          %{nkey_seed: seed, jwt: jwt} = _user,
          true = _auth_required
        ) do
-    {:ok, nkey} = NKEYS.from_seed(seed)
+    {:ok, nkey} = seed |> resolve_secret() |> NKEYS.from_seed()
     signature = NKEYS.sign(nkey, nonce) |> Base.url_encode64() |> String.replace("=", "")
 
     Map.merge(settings, %{sig: signature, protocol: 1, jwt: jwt})
@@ -78,7 +92,7 @@ defmodule Gnat.Handshake do
          %{nkey_seed: seed} = _user,
          true = _auth_required
        ) do
-    {:ok, nkey} = NKEYS.from_seed(seed)
+    {:ok, nkey} = seed |> resolve_secret() |> NKEYS.from_seed()
     signature = NKEYS.sign(nkey, nonce) |> Base.url_encode64() |> String.replace("=", "")
     public = NKEYS.public_nkey(nkey)
 
@@ -89,6 +103,9 @@ defmodule Gnat.Handshake do
     settings
   end
 
+  defp resolve_secret(secret) when is_function(secret, 0), do: secret.()
+  defp resolve_secret(secret), do: secret
+
   defp negotiate_headers(settings, %{headers: true} = _server, user_settings) do
     if Map.get(user_settings, :headers, true) do
       Map.put(settings, :headers, true)

test/gnat/connection_supervisor_test.exs

@@ -0,0 +1,52 @@
+defmodule Gnat.ConnectionSupervisorTest do
+  use ExUnit.Case, async: true
+  @moduletag :capture_log
+
+  @nkey_seed "test/fixtures/nkey_seed" |> File.read!() |> String.trim()
+
+  test "connection secrets are redacted from the process status" do
+    secrets = %{password: "supervisor-password", token: "supervisor-token", nkey_seed: @nkey_seed}
+
+    settings = %{
+      name: :connection_supervisor_redaction_test,
+      backoff_period: 60_000,
+      connection_settings: [Map.merge(%{host: "127.0.0.1", port: 1}, secrets)]
+    }
+
+    {:ok, supervisor} = Gnat.ConnectionSupervisor.start_link(settings)
+    status = inspect(:sys.get_status(supervisor), limit: :infinity, printable_limit: :infinity)
+
+    assert status =~ "connection_settings"
+    Enum.each(Map.values(secrets), fn secret -> refute status =~ secret end)
+    GenServer.stop(supervisor)
+  end
+
+  test "connection settings are logged with their secrets redacted when connecting" do
+    import ExUnit.CaptureLog
+    secrets = %{password: "logged-password", token: "logged-token", nkey_seed: @nkey_seed}
+
+    settings = %{
+      name: :connection_supervisor_logging_test,
+      backoff_period: 60_000,
+      connection_settings: [
+        Map.merge(%{host: "127.0.0.1", port: 1, username: "logged-user"}, secrets)
+      ]
+    }
+
+    log =
+      capture_log([level: :debug], fn ->
+        {:ok, supervisor} = Gnat.ConnectionSupervisor.start_link(settings)
+        # the first connection attempt has been handled once the supervisor answers
+        :sys.get_state(supervisor)
+        GenServer.stop(supervisor)
+      end)
+
+    assert log =~ "connecting to"
+    assert log =~ ~s(username: "logged-user")
+    assert log =~ ~s(host: "127.0.0.1")
+    assert log =~ "password: :redacted"
+    assert log =~ "token: :redacted"
+    assert log =~ "nkey_seed: :redacted"
+    Enum.each(Map.values(secrets), fn secret -> refute log =~ secret end)
+  end
+end

test/gnat/handshake_test.exs

@@ -69,6 +69,31 @@ defmodule Gnat.HandshakeTest do
       assert result[:protocol] == 1
     end
 
+    test "accepts an nkey seed function" do
+      seed = "SUAIBDPBAUTWCWBKIO6XHQNINK5FWJW4OHLXC3HQ2KFE4PEJUA44CNHTC4"
+      server_settings = %{nonce: "test-nonce-value", auth_required: true}
+
+      from_binary = Handshake.negotiate_settings(server_settings, %{nkey_seed: seed})
+      from_function = Handshake.negotiate_settings(server_settings, %{nkey_seed: fn -> seed end})
+
+      assert from_function == from_binary
+      assert Map.has_key?(from_function, :sig)
+      assert Map.has_key?(from_function, :nkey)
+    end
+
+    test "accepts password and token functions" do
+      server_settings = %{auth_required: true}
+
+      assert %{user: "test", pass: "secret"} =
+               Handshake.negotiate_settings(server_settings, %{
+                 username: "test",
+                 password: fn -> "secret" end
+               })
+
+      assert %{auth_token: "my-secret-token"} =
+               Handshake.negotiate_settings(server_settings, %{token: fn -> "my-secret-token" end})
+    end
+
     test "works with JWT+nkey authentication when client forces auth" do
       nonce = "test-nonce-value"
       server_settings = %{nonce: nonce}

test/gnat_test.exs

@@ -37,6 +37,36 @@ defmodule GnatTest do
     :ok = Gnat.stop(pid)
   end
 
+  @tag :multi_server
+  test "connect to a server with user/pass authentication and a password function" do
+    connection_settings = %{
+      host: "localhost",
+      port: 4223,
+      tcp_opts: [:binary],
+      username: "bob",
+      password: fn -> "alice" end
+    }
+
+    {:ok, pid} = Gnat.start_link(connection_settings)
+    assert Process.alive?(pid)
+    :ok = Gnat.stop(pid)
+  end
+
+  @tag :multi_server
+  test "connect to a server with token authentication and a token function" do
+    connection_settings = %{
+      host: "localhost",
+      port: 4226,
+      tcp_opts: [:binary],
+      token: fn -> "SpecialToken" end,
+      auth_required: true
+    }
+
+    {:ok, pid} = Gnat.start_link(connection_settings)
+    assert Process.alive?(pid)
+    :ok = Gnat.stop(pid)
+  end
+
   @tag :multi_server
   test "connect to a server with token authentication" do
     connection_settings = %{
@@ -85,6 +115,17 @@ defmodule GnatTest do
     assert Gnat.stop(gnat) == :ok
   end
 
+  @tag :multi_server
+  test "connect to a server which requires nkeys with a seed function" do
+    connection_settings = %{
+      port: 4227,
+      nkey_seed: fn -> File.read!("test/fixtures/nkey_seed") end
+    }
+
+    {:ok, gnat} = Gnat.start_link(connection_settings)
+    assert Gnat.stop(gnat) == :ok
+  end
+
   test "subscribe to topic and receive a message" do
     {:ok, pid} = Gnat.start_link()
     {:ok, _ref} = Gnat.sub(pid, self(), "test")
@@ -362,6 +403,34 @@ defmodule GnatTest do
     :ok = Gnat.stop(gnat)
   end
 
+  test "connection secrets are redacted from the process status" do
+    secrets = %{password: "status-password", token: "status-token", nkey_seed: nkey_seed()}
+    {:ok, gnat} = Gnat.start_link(secrets)
+
+    status = inspect(:sys.get_status(gnat), limit: :infinity, printable_limit: :infinity)
+
+    assert status =~ "connection_settings"
+    Enum.each(Map.values(secrets), fn secret -> refute status =~ secret end)
+    assert Gnat.stop(gnat) == :ok
+  end
+
+  test "connection secrets are redacted from crash reports" do
+    import ExUnit.CaptureLog
+    Process.flag(:trap_exit, true)
+    secrets = %{password: "report-password", token: "report-token", nkey_seed: nkey_seed()}
+    {:ok, gnat} = Gnat.start_link(secrets)
+
+    log =
+      capture_log(fn ->
+        GenServer.stop(gnat, :crash_report_test)
+        assert_receive {:EXIT, ^gnat, :crash_report_test}
+      end)
+
+    assert log =~ "crash_report_test"
+    assert log =~ "connection_settings"
+    Enum.each(Map.values(secrets), fn secret -> refute log =~ secret end)
+  end
+
   test "connection timeout" do
     start = System.monotonic_time(:millisecond)
     connection_settings = %{host: ~c"169.33.33.33", connection_timeout: 200}
@@ -386,4 +455,6 @@ defmodule GnatTest do
     assert Map.has_key?(info, :version)
     assert is_binary(info.version)
   end
+
+  defp nkey_seed, do: "test/fixtures/nkey_seed" |> File.read!() |> String.trim()
 end