feat: raise Pigeon.ConfigError when booting invalid configurations (#161

)

* feat: raise Pigeon.ConfigError when booting invalid configurations

* fix: pass doctests

* test: config raising with invalid configuration

* fix: parse {app, path} to nil if file does not actually exist

* feat: raise Pigeon.ConfigError for ADM (#162)

* feat: proper APNS config validation + error redaction

* fix: proper error tuple for APNS :key

* fix: redact FCM :key

* fix: handle APNS Config vs JWTConfig slightly differently

Config needs the actual result of the pem decode for certs/keys
whereas JWTConfig uses the raw text. Each now decode them in
their own modules instead of ConfigParser.

* fix: remove old APNS config validation check

The old nil-presence logic screws JWTConfig connections
with the new config validation updates.

* fix: update secrets.tar.enc

* ci: remove some old travis variables

* chore: bump version to 1.5.0

* chore: bump minimum Elixir version to 1.6

This allows us to remove the compile warning about using the
old `Enum.chunk/4`.

Co-authored-by: Andrew Shu <[email protected]>

.gitignore

@@ -11,3 +11,4 @@ secrets.tar
 scratchpad.txt
 .DS_Store
 .iex.exs
+*.swp

.travis.yml

@@ -16,7 +16,3 @@ before_install:
 - tar xvf secrets.tar
 script:
 - MIX_ENV=test mix do deps.get, compile, coveralls.travis
-env:
-  global:
-  - secure: kd6KF3eMgtaWIQbd/MG8m8L2cR3fqxmgRHWng4fqxRwPH45hP4IVLf2cQctGA9mm6nOV5wrNtys1fQzYPuXAKA/LAYJ/pc66ISdEjKjNpqxIGRCCJsB6lZft5XPgpAJTClwKGpFvEd7cLtz2ir4PBN0JjDyWtMTKHXarKzn7cOQNpRypwc5wWU1dmuqSsNNjr2hk3HUAwSNqB8W960Zqd39lqo0m+/tlq3L8LghcFlibn8XFNlzAmERpUpERkJZPbRplgx+ofigATGaC9Tzuc04WWLUthsPyBJkli2tYs/8Yd1tCjAYz90Tde0E1/Mzj6b4/LW4AGXrb4I5pKZrkkDw0gjqJ1Buu4U2d/TGZc+wU13BD5icpjq7zx8ZZ08N3XmQ6Y2ydd5p9MwCtUx5Dg2d9ybvdeCI1kj4HVcYAxqoBmf12od3fRZ+IwzbpUIvFn8R05wlvhA8nSLGloUceag3qUHJLD/tJOj9GRgR8ubw6hAQz7w7uWJhq3XSvHvdtYw2tfc9pGpjloA2gOhaStHrjJGFpzMFPITCEcQF1B7KraY/FPxMyvQL70l9RhQZ11F+56uwZ9oZFEQeyr5Tr6mPISyp1enz50HjwRrVy4pw5hOaTOCxA1dxrstWPhdKHOyYNtZDMFopn8yAIGzPE3z6UQ4bABqIlz8tpEEy13Eg=
-  - secure: f7LJkl/HyS/YghhADhVSNZjOsYkzuWXL9kqYAqjTxzB4g/2HPxkyjGjtlUHv82MvjvoIs7icDNjIveVf0ZP/is5t5T88HXfobYIW1H4hexXbN94oLJTaehdlkv0Fdi9Rfm7sBBH+tyd9HwS25R9MMSndRkxcnr78huFP8iWgRFFF4dDHyh8Dozzo+tHbrPPJtt8iJPz1Fs8U3nMQOdwHj8cA3fZXg65S7IzeavGAnkxtUDV22FgJd0PLD2i2RHAXAxjpmjLY3GIZDquSAO9DWJGElNRphg91nIT726vylm6IZ0JQ0H13hXbVfD4akYYb9ipL8CS/FzZexArtd1cg/nJKSz4G6AJyfByw0KqJwUgaPhvhlCeG8RDse4CH9dH7d8ODbUpxkcvh3Z31EUECY/2BC7Ma4ieLC4VAMBBgc21XxMPklt5z+wW8v2mblVudTSIRnXLL3QiiM2SHV0PLHyH7TPGiKU92J5HrhT3cKkrUMwcePLb6GUBKg6tSiF52DEiiGXWDWbj3rMEnwRbvAa5QkHNhVLwre1tsOt8nCX6cK3rY5fygR3W7lfzMQGMs/XzMdMaqFamJ0YRqUMh2msL0SdzltqFsXlS80JWS8U3zM1IlS448/qmhHzDjyV5mW/bd5iC7uDPQ/5rJfSaicfS1pNNp83Ufg7byNVaIbL4=

CHANGELOG.md

@@ -1,11 +1,27 @@
 # Changelog
 
+## v1.5.0
+* Bumped minimum Elixir version to 1.6
+* Raise `Pigeon.ConfigError` when booting invalid config structs. 
+  See below for validated keys and error types.
+* `APNS.JWTConfig` now validates key p8 content before connecting.
+* Relaxed `gen_stage` dependency to allow `~> 1.0`
+
+Validated config keys:
+- `ADM.Config` - `:client_id`, `:client_secret`
+- `APNS.Config` - `:cert`, `:key`
+- `APNS.JWTConfig` - `:team_id`, `:key`, `:key_identifier`
+- `FCM.Config` - `:key`
+
+Possible error values:
+  - `{:error, {:invalid, value}}`
+  - `{:error, {:nofile, value}}`
+
 ## v1.4.0
 * `apns-push-type` header support for iOS 13. An additional `:push_type` key has been
   added to the `APNS.Notification` struct.
 
 ## v1.3.2
-
 * Document workers configuration for run-time configuration of push workers.
 * Modify run-time configuration of push workers so that multiple (or no)
   workers may be returned by the startup configuration.

README.md

@@ -13,7 +13,7 @@ Add pigeon and kadabra as `mix.exs` dependencies:
 ```elixir
 def deps do
   [
-    {:pigeon, "~> 1.4.0"},
+    {:pigeon, "~> 1.5.0"},
     {:kadabra, "~> 0.4.4"}
   ]
 end

lib/pigeon/adm/config.ex

@@ -55,4 +55,25 @@ defmodule Pigeon.ADM.Config do
   end
 
   defp valid_item?(item), do: is_binary(item) and String.length(item) > 0
+
+  @spec validate!(any) :: :ok
+  def validate!(config) do
+    if valid?(config) do
+      :ok
+    else
+      raise Pigeon.ConfigError,
+        reason: "attempted to start without valid client id and secret",
+        config: redact(config)
+    end
+  end
+
+  defp redact(config) do
+    [:client_id, :client_secret]
+    |> Enum.reduce(config, fn k, acc ->
+      case Map.get(acc, k) do
+        nil -> acc
+        _ -> Map.put(acc, k, "[FILTERED]")
+      end
+    end)
+  end
 end

lib/pigeon/adm/worker.ex

@@ -25,6 +25,7 @@ defmodule Pigeon.ADM.Worker do
   def init({:ok, config}), do: initialize_worker(config)
 
   def initialize_worker(config) do
+    Pigeon.ADM.Config.validate!(config)
     {:ok,
      %{
        config: config,

lib/pigeon/apns/config.ex

@@ -116,17 +116,37 @@ defmodule Pigeon.APNS.Config do
     %__MODULE__{
       name: opts[:name],
       reconnect: Keyword.get(opts, :reconnect, false),
-      cert: ConfigParser.cert(opts[:cert]),
+      cert: cert(opts[:cert]),
       certfile: ConfigParser.file_path(opts[:cert]),
-      key: ConfigParser.key(opts[:key]),
+      key: key(opts[:key]),
       keyfile: ConfigParser.file_path(opts[:key]),
       uri: Keyword.get(opts, :uri, ConfigParser.uri_for_mode(opts[:mode])),
       port: Keyword.get(opts, :port, 443),
       ping_period: Keyword.get(opts, :ping_period, 600_000)
     }
+    |> ConfigParser.strip_errors(:cert, :certfile)
+    |> ConfigParser.strip_errors(:key, :keyfile)
   end
 
   def new(name) when is_atom(name), do: ConfigParser.parse(name)
+
+  defp cert(bin) when is_binary(bin) do
+    case :public_key.pem_decode(bin) do
+      [{:Certificate, cert, _}] -> cert
+      _ -> {:error, {:invalid, bin}}
+    end
+  end
+
+  defp cert(other), do: {:error, {:invalid, other}}
+
+  defp key(bin) when is_binary(bin) do
+    case :public_key.pem_decode(bin) do
+      [{:RSAPrivateKey, key, _}] -> {:RSAPrivateKey, key}
+      _ -> {:error, {:invalid, bin}}
+    end
+  end
+
+  defp key(other), do: {:error, {:invalid, other}}
 end
 
 defimpl Pigeon.Configurable, for: Pigeon.APNS.Config do
@@ -166,12 +186,32 @@ defimpl Pigeon.Configurable, for: Pigeon.APNS.Config do
 
   defdelegate close(config), to: Shared
 
-  def connect_socket_options(%{cert: nil, certfile: nil}) do
-    {:error, :invalid_config}
+  def validate!(config) do
+    case config do
+      %{cert: {:error, _}, certfile: {:error, _}} ->
+        raise Pigeon.ConfigError,
+          reason: "attempted to start without valid certificate",
+          config: redact(config)
+
+      %{key: {:error, _}, keyfile: {:error, _}} ->
+        raise Pigeon.ConfigError,
+          reason: "attempted to start without valid key",
+          config: redact(config)
+
+      _ ->
+        :ok
+    end
   end
 
-  def connect_socket_options(%{key: nil, keyfile: nil}) do
-    {:error, :invalid_config}
+  defp redact(config) do
+    [:cert, :key]
+    |> Enum.reduce(config, fn key, acc ->
+      case Map.get(acc, key) do
+        bin when is_binary(bin) -> Map.put(acc, key, "[FILTERED]")
+        {:RSAPrivateKey, _bin} -> Map.put(acc, key, "[FILTERED]")
+        _ -> acc
+      end
+    end)
   end
 
   def connect_socket_options(config) do

lib/pigeon/apns/config_parser.ex

@@ -24,7 +24,7 @@ defmodule Pigeon.APNS.ConfigParser do
   @spec parse(atom | config_opts) :: config | {:error, :invalid_config}
   def parse(opts) when is_list(opts) do
     case config_type(Enum.into(opts, %{})) do
-      :error -> raise "invalid apns configuration #{inspect(opts)}"
+      :error -> raise Pigeon.ConfigError, reason: "configuration is invalid", config: opts
       type -> type.new(opts)
     end
   end
@@ -42,47 +42,32 @@ defmodule Pigeon.APNS.ConfigParser do
   defp config_type(_else), do: :error
 
   @doc false
-  def file_path(nil), do: nil
-
   def file_path(path) when is_binary(path) do
-    path
-    |> Path.expand()
-    |> validate_file_path!()
+    if :filelib.is_file(path) do
+      Path.expand(path)
+    else
+      {:error, {:nofile, path}}
+    end
   end
 
   def file_path({app_name, path}) when is_atom(app_name) do
     path
     |> Path.expand(:code.priv_dir(app_name))
-    |> validate_file_path!()
+    |> file_path()
   end
 
-  @doc false
-  def cert({_app_name, _path}), do: nil
-  def cert(nil), do: nil
-
-  def cert(bin) do
-    case :public_key.pem_decode(bin) do
-      [{:Certificate, cert, _}] -> cert
-      _ -> nil
-    end
-  end
+  def file_path(other), do: {:error, {:nofile, other}}
 
   @doc false
-  def key({_app_name, _path}), do: nil
-  def key(nil), do: nil
-
-  def key(bin) do
-    case :public_key.pem_decode(bin) do
-      [{:RSAPrivateKey, key, _}] -> {:RSAPrivateKey, key}
-      _ -> nil
+  def strip_errors(config, key1, key2) do
+    case {Map.get(config, key1), Map.get(config, key2)} do
+      {{:error, _}, {:error, _}} -> config
+      {{:error, _}, _} -> Map.put(config, key1, nil)
+      {_, {:error, _}} -> Map.put(config, key2, nil)
     end
   end
 
   def uri_for_mode(:dev), do: @apns_development_api_uri
   def uri_for_mode(:prod), do: @apns_production_api_uri
   def uri_for_mode(_else), do: nil
-
-  defp validate_file_path!(path) do
-    if :filelib.is_file(path), do: path, else: raise("file not found: #{path}")
-  end
 end

lib/pigeon/apns/jwt_config.ex

@@ -101,7 +101,6 @@ defmodule Pigeon.APNS.JWTConfig do
       ...> )
       %Pigeon.APNS.JWTConfig{uri: "api.push.apple.com", name: :test,
       team_id: "DEF1234567", key_identifier: "ABC1234567", 
-      key: "test/support/AuthKey.p8-mock",
       keyfile: Path.expand("test/support/AuthKey.p8-mock"),
       ping_period: 300000, port: 2197, reconnect: false}
 
@@ -118,14 +117,24 @@ defmodule Pigeon.APNS.JWTConfig do
       uri: Keyword.get(opts, :uri, ConfigParser.uri_for_mode(opts[:mode])),
       port: Keyword.get(opts, :port, 443),
       ping_period: Keyword.get(opts, :ping_period, 600_000),
-      key: opts[:key],
+      key: key(opts[:key]),
       keyfile: ConfigParser.file_path(opts[:key]),
       key_identifier: Keyword.get(opts, :key_identifier),
       team_id: Keyword.get(opts, :team_id)
     }
+    |> ConfigParser.strip_errors(:key, :keyfile)
   end
 
   def new(name) when is_atom(name), do: ConfigParser.parse(name)
+
+  defp key(bin) when is_binary(bin) do
+    case :public_key.pem_decode(bin) do
+      [] -> {:error, {:invalid, bin}}
+      [{_, _, _}] -> bin
+    end
+  end
+
+  defp key(other), do: {:error, {:invalid, other}}
 end
 
 defimpl Pigeon.Configurable, for: Pigeon.APNS.JWTConfig do
@@ -176,8 +185,37 @@ defimpl Pigeon.Configurable, for: Pigeon.APNS.JWTConfig do
 
   defdelegate close(config), to: Shared
 
-  def connect_socket_options(%{key: nil}) do
-    {:error, :invalid_config}
+  def validate!(config) do
+    case config do
+      %{team_id: nil} ->
+        raise Pigeon.ConfigError,
+          reason: "attempted to start without valid team_id",
+          config: redact(config)
+
+      %{key_identifier: nil} ->
+        raise Pigeon.ConfigError,
+          reason: "attempted to start without valid key_identifier",
+          config: redact(config)
+
+      %{key: {:error, _}, keyfile: {:error, _}} ->
+        raise Pigeon.ConfigError,
+          reason: "attempted to start without valid key",
+          config: redact(config)
+
+      _ ->
+        :ok
+    end
+  end
+
+  defp redact(config) do
+    [:key, :keyfile]
+    |> Enum.reduce(config, fn key, acc ->
+      case Map.get(acc, key) do
+        bin when is_binary(bin) -> Map.put(acc, key, "[FILTERED]")
+        {:RSAPrivateKey, _bin} -> Map.put(acc, key, "[FILTERED]")
+        _ -> acc
+      end
+    end)
   end
 
   def connect_socket_options(%{key: _jwt_key} = config) do
@@ -194,8 +232,6 @@ defimpl Pigeon.Configurable, for: Pigeon.APNS.JWTConfig do
   end
 
   @spec put_bearer_token(Config.headers(), JWTConfig.t()) :: Config.headers()
-  defp put_bearer_token(headers, %{key: nil}), do: headers
-
   defp put_bearer_token(headers, config) do
     token_storage_key = config.key_identifier <> ":" <> config.team_id
     {timestamp, saved_token} = Pigeon.APNS.Token.get(token_storage_key)

lib/pigeon/config_error.ex

@@ -0,0 +1,14 @@
+defmodule Pigeon.ConfigError do
+  defexception reason: nil, config: nil
+
+  @impl true
+  def message(%{config: config, reason: reason}) do
+    """
+    #{reason}
+
+    The following configuration was given: 
+
+    #{inspect(config, pretty: true)}
+    """
+  end
+end

lib/pigeon/configurable.ex

@@ -52,4 +52,7 @@ defprotocol Pigeon.Configurable do
   def schedule_ping(config)
 
   def close(config)
+
+  @spec validate!(any) :: :ok
+  def validate!(config)
 end

lib/pigeon/fcm/config.ex

@@ -150,6 +150,22 @@ defimpl Pigeon.Configurable, for: Pigeon.FCM.Config do
   def close(_config) do
   end
 
+  def validate!(%{key: key}) when is_binary(key) do
+    :ok
+  end
+
+  def validate!(config) do
+    raise Pigeon.ConfigError,
+      reason: "attempted to start without valid key",
+      config: redact(config)
+  end
+
+  defp redact(%{key: key} = config) when is_binary(key) do
+    Map.put(config, :key, "[FILTERED]")
+  end
+
+  defp redact(config), do: config
+
   # no on_response callback, ignore
   def parse_result(_, _, nil, _notif), do: :ok
 

lib/pigeon/fcm/notification.ex

@@ -143,19 +143,11 @@ defmodule Pigeon.FCM.Notification do
 
   def new(reg_ids, notification, data) do
     reg_ids
-    |> chunk(@chunk_size, @chunk_size, [])
+    |> Enum.chunk_every(@chunk_size, @chunk_size, [])
     |> Enum.map(&new(&1, notification, data))
     |> List.flatten()
   end
 
-  defp chunk(collection, chunk_size, step, padding) do
-    if Kernel.function_exported?(Enum, :chunk_every, 4) do
-      Enum.chunk_every(collection, chunk_size, step, padding)
-    else
-      Enum.chunk(collection, chunk_size, step, padding)
-    end
-  end
-
   @doc """
   Updates `"data"` key in push payload.
 

lib/pigeon/worker.ex

@@ -41,6 +41,7 @@ defmodule Pigeon.Worker do
   end
 
   def init({:ok, config}) do
+    Configurable.validate!(config)
     state = %Worker{config: config}
     {:producer, state}
   end