Merge pull request #239 from ericmj/ericmj/credential-redaction

Extend credential redaction in connection diagnostics

Author: mmmries

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## 1.18.0
 
+* **Behavior change:** `Gnat.start_link/2` now returns `{:error, reason}` when the TLS upgrade, the header support check or sending `CONNECT` fails, instead of crashing during initialization. TLS alerts come back as-is (for example `{:error, {:tls_alert, {:certificate_expired, _}}}`), so `Gnat.ConnectionSupervisor` logs why the connection failed. Invalid `ssl_opts` return `{:error, {:options, {option, :redacted}}}` because `:ssl` echoes the option value, which can be a key or password.
 * **Behavior change:** `Gnat.Jetstream.API.KV.put_value/5`, `delete_key/4`, and `purge_key/4` now validate the JetStream publish acknowledgement instead of reporting `:ok` for any reply. They share the same ack validation as `create_key/5`: a rejected write returns `{:error, reason}` with the server's error map, and a malformed or unrelated ack returns `{:error, :invalid_publish_ack}`. Previously a server-side rejection (for example a `put_value/5` larger than the bucket's `max_value_size`) was silently reported as `:ok`. See #235
 * **Behavior change:** `Gnat.Jetstream.API.KV.create_key/5` now only creates a key when one doesn't already exist. Previously it behaved like `put_value/5` and silently overwrote existing keys, and it returned `:ok` even when the server rejected the write. This aligns the library with the NATS server and the official clients: the publish is conditional on the key's current revision (`Nats-Expected-Last-Subject-Sequence`), so concurrent creators can't overwrite each other, and keys that were deleted or purged can be recreated. If you relied on `create_key` overwriting an existing key, use `put_value/5` instead. Thanks to @ericmj for #234
   * Returns `{:error, :key_exists}` when a live value already exists for the key.

lib/gnat.ex

@@ -63,8 +63,9 @@ defmodule Gnat do
     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.
+  The `nkey_seed`, `password`, `token`, `jwt` and `ssl_opts` settings are redacted from
+  formatted process status, crash reports and connection supervisor logs. See
+  `start_link/2` for the limits of diagnostic redaction.
   """
   @type connection_settings :: %{
           optional(:connection_timeout) => non_neg_integer(),
@@ -188,6 +189,13 @@ defmodule Gnat do
   The `:inbox_prefix` must be a binary without spaces, tabs, carriage returns or
   line feeds. Invalid values raise `ArgumentError` before connecting.
 
+  Credentials are redacted from connection configuration logs and formatted OTP
+  status and crash reports. Exceptions raised during connection initialization
+  report their class and stack locations, with details and arguments redacted.
+  Raw debugging interfaces still have access to the original settings, including
+  `:sys.get_state/1`, tracing, and the raw debug history returned by
+  `:sys.get_status/1` when `:sys.log/2` is enabled.
+
   The final `opts` argument will be passed to the `GenServer.start_link` call so you can pass things like `[name: :gnat_connection]`.
   """
   @spec start_link(connection_settings(), keyword()) :: GenServer.on_start()
@@ -486,14 +494,8 @@ 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 {:format_status, 1} in GenServer.behaviour_info(:callbacks)
+  def format_status(status), do: Gnat.Redaction.redact(status)
 
   @impl GenServer
   def init(connection_settings) do
@@ -526,6 +528,9 @@ defmodule Gnat do
       {:error, reason} ->
         {:stop, reason}
     end
+  rescue
+    exception ->
+      Gnat.Redaction.reraise_connection_error(exception, __STACKTRACE__)
   end
 
   @impl GenServer

lib/gnat/connection_supervisor.ex

@@ -19,7 +19,11 @@ defmodule Gnat.ConnectionSupervisor do
   }
   ```
 
-  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.
+  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.
+
+  Credentials are redacted from connection configuration logs and formatted OTP
+  status and crash reports. The raw debugging limitations described in
+  `Gnat.start_link/2` also apply to this process.
 
   To use this in your supervision tree add an entry like this:
 
@@ -41,6 +45,9 @@ defmodule Gnat.ConnectionSupervisor do
     GenServer.start_link(__MODULE__, settings, options)
   end
 
+  @impl {:format_status, 1} in GenServer.behaviour_info(:callbacks)
+  def format_status(status), do: Gnat.Redaction.redact(status)
+
   @impl GenServer
   def init(options) do
     state = %{
@@ -53,19 +60,22 @@ defmodule Gnat.ConnectionSupervisor do
     Process.flag(:trap_exit, true)
     send(self(), :attempt_connection)
     {:ok, state}
+  rescue
+    exception ->
+      Gnat.Redaction.reraise_connection_error(exception, __STACKTRACE__)
   end
 
   @impl GenServer
   def handle_info(:attempt_connection, state) do
     connection_config = random_connection_config(state)
-    Logger.debug("connecting to #{inspect(Gnat.Handshake.redact_settings(connection_config))}")
+    Logger.debug("connecting to #{inspect(Gnat.Redaction.redact(connection_config))}")
 
     case Gnat.start_link(connection_config, name: state.name) do
       {:ok, gnat} ->
         {:noreply, %{state | gnat: gnat}}
 
       {:error, err} ->
-        Logger.error("failed to connect #{inspect(err)}")
+        Logger.error("failed to connect #{inspect(Gnat.Redaction.redact(err))}")
         Process.send_after(self(), :attempt_connection, state.backoff_period)
         {:noreply, %{state | gnat: nil}}
     end
@@ -79,27 +89,19 @@ defmodule Gnat.ConnectionSupervisor do
   end
 
   def handle_info({:EXIT, _pid, reason}, state) do
-    Logger.error("connection failed #{inspect(reason)}")
+    Logger.error("connection failed #{inspect(Gnat.Redaction.redact(reason))}")
     send(self(), :attempt_connection)
     {:noreply, state}
   end
 
   def handle_info(msg, state) do
-    Logger.error("#{__MODULE__} received unexpected message #{inspect(msg)}")
-    {:noreply, state}
-  end
+    Logger.error(
+      "#{__MODULE__} received unexpected message #{inspect(Gnat.Redaction.redact(msg))}"
+    )
 
-  # 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}}
+    {:noreply, state}
   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,20 +15,6 @@ 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
 
@@ -43,10 +29,15 @@ defmodule Gnat.Handshake do
     receive do
       {:tcp, ^tcp, operation} ->
         {_, [{:info, server_settings}]} = Parsec.parse(Parsec.new(), operation)
-        {:ok, socket} = upgrade_connection(tcp, user_settings)
-        settings = negotiate_settings(server_settings, user_settings)
-        :ok = send_connect(user_settings, settings, socket)
-        {:ok, socket, server_settings}
+
+        # Expected failures are returned rather than raised, because exceptions raised during
+        # initialization are redacted and would hide the reason from operators.
+        with :ok <- check_headers(server_settings, user_settings),
+             {:ok, socket} <- upgrade_connection(tcp, user_settings),
+             settings = negotiate_settings(server_settings, user_settings),
+             :ok <- send_connect(user_settings, settings, socket) do
+          {:ok, socket, server_settings}
+        end
     after
       1000 ->
         {:error, "timed out waiting for info"}
@@ -114,10 +105,6 @@ defmodule Gnat.Handshake do
     end
   end
 
-  defp negotiate_headers(_settings, _server, %{headers: true} = _user) do
-    raise "NATS Server does not support headers, but your connection settings specify header support"
-  end
-
   defp negotiate_headers(settings, _server, _user) do
     settings
   end
@@ -140,10 +127,33 @@ defmodule Gnat.Handshake do
     settings
   end
 
+  defp check_headers(%{headers: true} = _server, _user), do: :ok
+
+  defp check_headers(_server, %{headers: true} = _user) do
+    {:error,
+     "NATS Server does not support headers, but your connection settings specify header support"}
+  end
+
+  defp check_headers(_server, _user), do: :ok
+
   defp upgrade_connection(tcp, %{tls: true, ssl_opts: opts}) do
     :ok = :inet.setopts(tcp, active: true)
-    :ssl.connect(tcp, opts, 1_000)
+
+    case :ssl.connect(tcp, opts, 1_000) do
+      {:ok, socket} -> {:ok, socket}
+      {:error, reason} -> {:error, tls_error(reason)}
+    end
   end
 
   defp upgrade_connection(tcp, _settings), do: {:ok, tcp}
+
+  # :ssl echoes the offending value in option errors, which can be a private key, password
+  # or callback state, so only the option name is kept. Alerts such as an expired or untrusted
+  # certificate are returned as-is.
+  defp tls_error({:options, detail}) when is_tuple(detail) and is_atom(elem(detail, 0)) do
+    {:options, {elem(detail, 0), :redacted}}
+  end
+
+  defp tls_error({:options, _detail}), do: {:options, :redacted}
+  defp tls_error(reason), do: reason
 end

lib/gnat/redaction.ex

@@ -0,0 +1,34 @@
+defmodule Gnat.Redaction do
+  @moduledoc false
+
+  # TLS options can contain private keys, passwords and callback state.
+  @credential_keys [:password, :token, :jwt, :nkey_seed, :ssl_opts]
+
+  def redact(term) when is_map(term) do
+    :maps.map(fn key, value -> redact_value(key, value) end, term)
+  end
+
+  def redact({key, _value}) when key in @credential_keys, do: {key, :redacted}
+
+  def redact(term) when is_tuple(term) do
+    term |> Tuple.to_list() |> redact() |> List.to_tuple()
+  end
+
+  def redact([head | tail]), do: [redact(head) | redact(tail)]
+  def redact(term), do: term
+
+  def reraise_connection_error(exception, stacktrace) do
+    message = "connection initialization raised #{inspect(exception.__struct__)}: [REDACTED]"
+    reraise RuntimeError, [message: message], redact_stacktrace(stacktrace)
+  end
+
+  defp redact_stacktrace(stacktrace) do
+    Enum.map(stacktrace, fn {module, function, args_or_arity, location} ->
+      arity = if is_list(args_or_arity), do: length(args_or_arity), else: args_or_arity
+      {module, function, arity, Keyword.take(location, [:file, :line])}
+    end)
+  end
+
+  defp redact_value(key, _value) when key in @credential_keys, do: :redacted
+  defp redact_value(_key, value), do: redact(value)
+end