feat: check ip against allow list (#240)

* feat: check ip against allow list * feat: add allow_list field to tenant * fix: cleanup * fix: handle more responses from :inet.peername * fix: bump version * fix: info contains both a tenant and a user * fix: filter CIDRs in one pass * fix: add field to OpenAPI spec * fix: cleanup * fix: nit * fix: fail closed on bad addresses * fix: filter_cidrs will now handle any addr type * feat: validate ranges in allow_list and respond with bad ranges found * fix: remove "translate error" text for nicer error messages * fix: render more fields * fix: use except with Jason.Encoder * fix: better error message * fix: error message * fix: Telem.client_join when address not allowed * feat: check allow_list in NativeHandler * fix: add allow_list field to docs * fix: open API spec allow_list type and examples * fix: more concise spec Co-authored-by: Stas <[email protected]> * fix: handle ssl ports too * fix: simplify spec --------- Co-authored-by: Stas <[email protected]>
supabase · Jan 3, 2024 · da316ff · da316ff
1 parent f0bb3aa
commit da316ff
Show file tree

Hide file tree

Showing 17 changed files with 210 additions and 55 deletions.
diff --git a/VERSION b/VERSION
@@ -1 +1 @@
-1.0.12
+1.1.0
diff --git a/docs/configuration/tenants.md b/docs/configuration/tenants.md
@@ -39,3 +39,5 @@ All `tenant` fields and their types are defined in the `Supavisor.Tenants.Tenant
 `client_idle_timeout` - the maximum duration of an idle client connection
 
 `default_pool_strategy` - the default strategy when pulling available connections from the pool queue
+
+`allow_list` - a list of CIDR ranges which are allowed to connect
diff --git a/lib/supavisor/client_handler.ex b/lib/supavisor/client_handler.ex
@@ -173,28 +173,40 @@ defmodule Supavisor.ClientHandler do
 
         Registry.register(Supavisor.Registry.TenantClients, id, [])
 
-        if info.tenant.enforce_ssl and !data.ssl do
-          Logger.error("Tenant is not allowed to connect without SSL, user #{user}")
+        {:ok, addr} = HH.addr_from_sock(sock)
 
-          :ok = HH.send_error(sock, "XX000", "SSL connection is required")
-          Telem.client_join(:fail, data.id)
-          {:stop, :normal}
-        else
-          new_data = update_user_data(data, info, user, id)
+        cond do
+          info.tenant.enforce_ssl and !data.ssl ->
+            Logger.error("Tenant is not allowed to connect without SSL, user #{user}")
 
-          case auth_secrets(info, user) do
-            {:ok, auth_secrets} ->
-              Logger.debug("Authentication method: #{inspect(auth_secrets)}")
+            :ok = HH.send_error(sock, "XX000", "SSL connection is required")
+            Telem.client_join(:fail, id)
+            {:stop, :normal}
 
-              {:keep_state, new_data, {:next_event, :internal, {:handle, auth_secrets}}}
+          HH.filter_cidrs(info.tenant.allow_list, addr) == [] ->
+            message = "Address not in tenant allow_list: " <> inspect(addr)
+            Logger.error(message)
+            :ok = HH.send_error(sock, "XX000", message)
 
-            {:error, reason} ->
-              Logger.error("Authentication auth_secrets error: #{inspect(reason)}")
+            Telem.client_join(:fail, id)
+            {:stop, :normal}
 
-              :ok = HH.send_error(sock, "XX000", "Authentication error")
-              Telem.client_join(:fail, data.id)
-              {:stop, :normal}
-          end
+          true ->
+            new_data = update_user_data(data, info, user, id)
+
+            case auth_secrets(info, user) do
+              {:ok, auth_secrets} ->
+                Logger.debug("Authentication method: #{inspect(auth_secrets)}")
+
+                {:keep_state, new_data, {:next_event, :internal, {:handle, auth_secrets}}}
+
+              {:error, reason} ->
+                Logger.error("Authentication auth_secrets error: #{inspect(reason)}")
+
+                :ok = HH.send_error(sock, "XX000", "Authentication error")
+                Telem.client_join(:fail, id)
+                {:stop, :normal}
+            end
         end
 
       {:error, reason} ->

diff --git a/lib/supavisor/handler_helpers.ex b/lib/supavisor/handler_helpers.ex
@@ -126,4 +126,68 @@ defmodule Supavisor.HandlerHelpers do
     :ok = sock_send(sock, msg)
     :ok = sock_close(sock)
   end
+
+  @doc """
+  Takes an allow list of CIDR ranges and filtres them for ranges which contain the address
+  to test.
+
+  If the IP address of the socket is not found an empty list is returned.
+
+  ## Examples
+
+    iex> Supavisor.HandlerHelpers.filter_cidrs(["0.0.0.0/0", "::/0"], {127, 0, 0, 1})
+    ["0.0.0.0/0"]
+
+    iex> Supavisor.HandlerHelpers.filter_cidrs(["71.209.249.38/32"], {71, 209, 249, 39})
+    []
+
+    iex> Supavisor.HandlerHelpers.filter_cidrs(["0.0.0.0/0", "::/0"], {8193, 3512, 34211, 0, 0, 35374, 880, 29492})
+    ["::/0"]
+
+    iex> Supavisor.HandlerHelpers.filter_cidrs(["0.0.0.0/0", "::/0"], :error)
+    []
+
+  """
+
+  @spec filter_cidrs(list(), :inet.ip_address() | any()) :: list()
+  def filter_cidrs(allow_list, addr) when is_list(allow_list) and is_tuple(addr) do
+    for range <- allow_list,
+        range |> InetCidr.parse() |> InetCidr.contains?(addr) do
+      range
+    end
+  end
+
+  def filter_cidrs(allow_list, _addr) when is_list(allow_list) do
+    []
+  end
+
+  @spec addr_from_sock(S.sock()) :: {:ok, :inet.ip_address()} | :error
+  def addr_from_sock({:gen_tcp, port}) do
+    case :inet.peername(port) do
+      {:ok, {:local, _}} ->
+        :error
+
+      {:ok, {:undefined, _}} ->
+        :error
+
+      {:ok, {:unspec, _}} ->
+        :error
+
+      {:ok, {addr, _port}} ->
+        {:ok, addr}
+
+      {:error, _} ->
+        :error
+    end
+  end
+
+  def addr_from_sock({:ssl, port}) do
+    case :ssl.peername(port) do
+      {:ok, {addr, _port}} ->
+        {:ok, addr}
+
+      {:error, _} ->
+        :error
+    end
+  end
 end
diff --git a/lib/supavisor/monitoring/telem.ex b/lib/supavisor/monitoring/telem.ex
@@ -49,7 +49,7 @@ defmodule Supavisor.Monitoring.Telem do
   end
 
   @spec client_join(:ok | :fail, S.id()) :: :ok
-  def client_join(status, {tenant, user, mode}) do
+  def client_join(status, {{_, tenant}, user, mode}) do
     :telemetry.execute(
       [:supavisor, :client, :joins, status],
       %{},

diff --git a/lib/supavisor/native_handler.ex b/lib/supavisor/native_handler.ex
@@ -137,7 +137,7 @@ defmodule Supavisor.NativeHandler do
     Logger.metadata(project: external_id, user: user, mode: "native")
 
     case Tenants.get_tenant_cache(external_id, sni_hostname) do
-      %{db_host: host, db_port: port, external_id: ext_id} ->
+      %{db_host: host, db_port: port, external_id: ext_id} = tenant ->
         id = Supavisor.id(ext_id, user, :native, :native)
         Registry.register(Supavisor.Registry.TenantClients, id, [])
 
@@ -152,14 +152,23 @@ defmodule Supavisor.NativeHandler do
         ip_ver = H.detect_ip_version(host)
         host = String.to_charlist(host)
 
-        case connect_local(host, port, payload, ip_ver, state.ssl) do
-          {:ok, db_sock} ->
-            auth = %{host: host, port: port, ip_ver: ip_ver}
-            {:noreply, %{state | db_sock: db_sock, db_auth: auth}}
+        {:ok, addr} = HH.addr_from_sock(sock)
 
-          {:error, reason} ->
-            Logger.error("Error connecting to tenant db: #{inspect(reason)}")
-            {:stop, :normal, state}
+        unless HH.filter_cidrs(tenant.allow_list, addr) == [] do
+          case connect_local(host, port, payload, ip_ver, state.ssl) do
+            {:ok, db_sock} ->
+              auth = %{host: host, port: port, ip_ver: ip_ver}
+              {:noreply, %{state | db_sock: db_sock, db_auth: auth}}
+
+            {:error, reason} ->
+              Logger.error("Error connecting to tenant db: #{inspect(reason)}")
+              {:stop, :normal, state}
+          end
+        else
+          message = "Address not in tenant allow_list: " <> inspect(addr)
+          Logger.error(message)
+          :ok = HH.send_error(sock, "XX000", message)
+          {:stop, :normal, state}
         end
 
       _ ->

diff --git a/lib/supavisor/tenants/tenant.ex b/lib/supavisor/tenants/tenant.ex
@@ -10,6 +10,8 @@ defmodule Supavisor.Tenants.Tenant do
   @primary_key {:id, :binary_id, autogenerate: true}
   @schema_prefix "_supavisor"
 
+  @derive {Jason.Encoder, except: [:upstream_tls_ca, :__meta__]}
+
   schema "tenants" do
     field(:db_host, :string)
     field(:db_port, :integer)
@@ -29,6 +31,7 @@ defmodule Supavisor.Tenants.Tenant do
     field(:client_idle_timeout, :integer, default: 0)
     field(:client_heartbeat_interval, :integer, default: 60)
     field(:default_pool_strategy, Ecto.Enum, values: [:fifo, :lifo], default: :fifo)
+    field(:allow_list, {:array, :string}, default: ["0.0.0.0/0", "::/0"])
 
     has_many(:users, User,
       foreign_key: :tenant_external_id,
@@ -61,7 +64,8 @@ defmodule Supavisor.Tenants.Tenant do
       :default_max_clients,
       :client_idle_timeout,
       :client_heartbeat_interval,
-      :default_pool_strategy
+      :default_pool_strategy,
+      :allow_list
     ])
     |> check_constraint(:upstream_ssl, name: :upstream_constraints, prefix: "_supavisor")
     |> check_constraint(:upstream_verify, name: :upstream_constraints, prefix: "_supavisor")
@@ -71,9 +75,60 @@ defmodule Supavisor.Tenants.Tenant do
       :db_host,
       :db_port,
       :db_database,
-      :require_user
+      :require_user,
+      :allow_list
     ])
+    |> validate_allow_list()
     |> unique_constraint([:external_id])
     |> cast_assoc(:users, with: &User.changeset/2)
   end
+
+  @doc """
+  Validates CIDRs passed in allow_list field parse correctly.
+
+  ## Examples
+
+    iex> changeset =
+    iex> Ecto.Changeset.change(%Supavisor.Tenants.Tenant{}, %{allow_list: ["0.0.0.0"]})
+    iex> |> Supavisor.Tenants.Tenant.validate_allow_list()
+    iex> changeset.errors
+    [allow_list: {"Invalid CIDR range: 0.0.0.0", []}]
+
+    iex> changeset =
+    iex> Ecto.Changeset.change(%Supavisor.Tenants.Tenant{}, %{allow_list: ["0.0.0.0/0", "::/0"]})
+    iex> |> Supavisor.Tenants.Tenant.validate_allow_list()
+    iex> changeset.errors
+    []
+
+    iex> changeset =
+    iex> Ecto.Changeset.change(%Supavisor.Tenants.Tenant{}, %{allow_list: ["0.0.0.0/0", "foo", "bar"]})
+    iex> |> Supavisor.Tenants.Tenant.validate_allow_list()
+    iex> changeset.errors
+    [{:allow_list, {"Invalid CIDR range: foo", []}}, {:allow_list, {"Invalid CIDR range: bar", []}}]
+
+    iex> changeset =
+    iex> Ecto.Changeset.change(%Supavisor.Tenants.Tenant{}, %{allow_list: ["0.0.0.0/0   "]})
+    iex> |> Supavisor.Tenants.Tenant.validate_allow_list()
+    iex> changeset.errors
+    []
+  """
+
+  @spec validate_allow_list(Ecto.Changeset.t()) :: Ecto.Changeset.t()
+  def validate_allow_list(changeset) do
+    validate_change(changeset, :allow_list, fn :allow_list, value when is_list(value) ->
+      for range <- value, !valid_range?(range) do
+        {:allow_list, "Invalid CIDR range: #{range}"}
+      end
+    end)
+  end
+
+  defp valid_range?(range) do
+    try do
+      InetCidr.parse(range)
+      true
+    rescue
+      _e ->
+        false
+    end
+  end
 end
diff --git a/lib/supavisor_web/open_api_schemas.ex b/lib/supavisor_web/open_api_schemas.ex
@@ -46,7 +46,8 @@ defmodule SupavisorWeb.OpenApiSchemas do
         max_clients: 25_000,
         mode_type: "transaction",
         inserted_at: "2023-03-27T12:00:00Z",
-        updated_at: "2023-03-27T12:00:00Z"
+        updated_at: "2023-03-27T12:00:00Z",
+        allow_list: ["0.0.0.0/0", "::/0"]
       }
     })
 
@@ -93,6 +94,7 @@ defmodule SupavisorWeb.OpenApiSchemas do
         db_database: "postgres",
         inserted_at: "2023-03-27T12:00:00Z",
         updated_at: "2023-03-27T12:00:00Z",
+        allow_list: ["0.0.0.0/0", "::/0"],
         users: [
           %{
             id: "b1024a4c-4eb4-4c64-8f49-c8a46c2b2e16",
@@ -150,7 +152,12 @@ defmodule SupavisorWeb.OpenApiSchemas do
             },
             users: %Schema{type: :array, items: User},
             inserted_at: %Schema{type: :string, format: :date_time, readOnly: true},
-            updated_at: %Schema{type: :string, format: :date_time, readOnly: true}
+            updated_at: %Schema{type: :string, format: :date_time, readOnly: true},
+            allow_list: %Schema{
+              type: :array,
+              description: "List of CIDR addresses",
+              default: ["0.0.0.0/0", "::/0"]
+            }
           },
           required: [
             :db_host,
@@ -166,6 +173,7 @@ defmodule SupavisorWeb.OpenApiSchemas do
             ip_version: "auto",
             enforce_ssl: false,
             require_user: true,
+            allow_list: ["0.0.0.0/0", "::/0"],
             users: [
               %{
                 db_user: "postgres",

diff --git a/lib/supavisor_web/views/error_helpers.ex b/lib/supavisor_web/views/error_helpers.ex
@@ -21,6 +21,6 @@ defmodule SupavisorWeb.ErrorHelpers do
   Translates an error message using gettext.
   """
   def translate_error({msg, opts}) do
-    "translate_error #{msg} #{inspect(opts)}"
+    "#{msg} #{inspect(opts)}"
   end
 end
diff --git a/lib/supavisor_web/views/tenant_view.ex b/lib/supavisor_web/views/tenant_view.ex
@@ -12,26 +12,7 @@ defmodule SupavisorWeb.TenantView do
   end
 
   def render("tenant.json", %{tenant: tenant}) do
-    %{
-      id: tenant.id,
-      external_id: tenant.external_id,
-      db_host: tenant.db_host,
-      db_port: tenant.db_port,
-      db_database: tenant.db_database,
-      ip_version: tenant.ip_version,
-      upstream_ssl: tenant.upstream_ssl,
-      upstream_verify: tenant.upstream_verify,
-      enforce_ssl: tenant.enforce_ssl,
-      require_user: tenant.require_user,
-      auth_query: tenant.auth_query,
-      sni_hostname: tenant.sni_hostname,
-      default_pool_size: tenant.default_pool_size,
-      default_max_clients: tenant.default_max_clients,
-      client_idle_timeout: tenant.client_idle_timeout,
-      client_heartbeat_interval: tenant.client_heartbeat_interval,
-      default_pool_strategy: tenant.default_pool_strategy,
-      users: render_many(tenant.users, UserView, "user.json")
-    }
+    %{tenant | users: render_many(tenant.users, UserView, "user.json")}
   end
 
   def render("error.json", %{error: reason}) do

diff --git a/mix.exs b/mix.exs
@@ -60,6 +60,7 @@ defmodule Supavisor.MixProject do
       {:logflare_logger_backend, github: "Logflare/logflare_logger_backend", tag: "v0.11.4"},
       {:distillery, "~> 2.1"},
       {:cachex, "~> 3.6"},
+      {:inet_cidr, "~> 1.0.0"},
 
       # pooller
       {:poolboy, "~> 1.5.2"},

diff --git a/mix.lock b/mix.lock
@@ -25,6 +25,7 @@
   "file_system": {:hex, :file_system, "0.2.10", "fb082005a9cd1711c05b5248710f8826b02d7d1784e7c3451f9c1231d4fc162d", [:mix], [], "hexpm", "41195edbfb562a593726eda3b3e8b103a309b733ad25f3d642ba49696bf715dc"},
   "finch": {:hex, :finch, "0.16.0", "40733f02c89f94a112518071c0a91fe86069560f5dbdb39f9150042f44dcfb1a", [:mix], [{:castore, "~> 0.1 or ~> 1.0", [hex: :castore, repo: "hexpm", optional: false]}, {:mime, "~> 1.0 or ~> 2.0", [hex: :mime, repo: "hexpm", optional: false]}, {:mint, "~> 1.3", [hex: :mint, repo: "hexpm", optional: false]}, {:nimble_options, "~> 0.4 or ~> 1.0", [hex: :nimble_options, repo: "hexpm", optional: false]}, {:nimble_pool, "~> 0.2.6 or ~> 1.0", [hex: :nimble_pool, repo: "hexpm", optional: false]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "f660174c4d519e5fec629016054d60edd822cdfe2b7270836739ac2f97735ec5"},
   "hpax": {:hex, :hpax, "0.1.2", "09a75600d9d8bbd064cdd741f21fc06fc1f4cf3d0fcc335e5aa19be1a7235c84", [:mix], [], "hexpm", "2c87843d5a23f5f16748ebe77969880e29809580efdaccd615cd3bed628a8c13"},
+  "inet_cidr": {:hex, :inet_cidr, "1.0.4", "a05744ab7c221ca8e395c926c3919a821eb512e8f36547c062f62c4ca0cf3d6e", [:mix], [], "hexpm", "64a2d30189704ae41ca7dbdd587f5291db5d1dda1414e0774c29ffc81088c1bc"},
   "jason": {:hex, :jason, "1.4.1", "af1504e35f629ddcdd6addb3513c3853991f694921b1b9368b0bd32beb9f1b63", [:mix], [{:decimal, "~> 1.0 or ~> 2.0", [hex: :decimal, repo: "hexpm", optional: true]}], "hexpm", "fbb01ecdfd565b56261302f7e1fcc27c4fb8f32d56eab74db621fc154604a7a1"},
   "joken": {:hex, :joken, "2.5.0", "09be497d804b8115eb6f07615cef2e60c2a1008fb89dc0aef0d4c4b4609b99aa", [:mix], [{:jose, "~> 1.11.2", [hex: :jose, repo: "hexpm", optional: false]}], "hexpm", "22b25c89617c5ed8ca7b31026340a25ea0f9ca7160f9706b79be9ed81fdf74e7"},
   "jose": {:hex, :jose, "1.11.6", "613fda82552128aa6fb804682e3a616f4bc15565a048dabd05b1ebd5827ed965", [:mix, :rebar3], [], "hexpm", "6275cb75504f9c1e60eeacb771adfeee4905a9e182103aa59b53fed651ff9738"},

diff --git a/priv/repo/migrations/20231229010413_add_tenant_allow_list.exs b/priv/repo/migrations/20231229010413_add_tenant_allow_list.exs
@@ -0,0 +1,9 @@
+defmodule Supavisor.Repo.Migrations.AddTenantAllowList do
+  use Ecto.Migration
+
+  def change do
+    alter table("tenants", prefix: "_supavisor") do
+      add(:allow_list, {:array, :string}, null: false, default: ["0.0.0.0/0", "::/0"])
+    end
+  end
+end
diff --git a/test/supavisor/handler_helpers_test.exs b/test/supavisor/handler_helpers_test.exs
@@ -0,0 +1,4 @@
+defmodule Supavisor.HandlerHelpersTest do
+  use ExUnit.Case
+  doctest Supavisor.HandlerHelpers
+end
diff --git a/test/supavisor/tenants_tenant_test.exs b/test/supavisor/tenants_tenant_test.exs
@@ -0,0 +1,4 @@
+defmodule Supavisor.Tenants.TenantTest do
+  use ExUnit.Case
+  doctest Supavisor.Tenants.Tenant
+end
Original file line number	Diff line number	Diff line change
Expand Up		@@ -39,3 +39,5 @@ All `tenant` fields and their types are defined in the `Supavisor.Tenants.Tenant
		`client_idle_timeout` - the maximum duration of an idle client connection

		`default_pool_strategy` - the default strategy when pulling available connections from the pool queue

		`allow_list` - a list of CIDR ranges which are allowed to connect