Merge branch 'gmcintire/security-audit' into 'main'

Add rate limiting to auth and API endpoints using Hammer

See merge request towerops/towerops!2

CLAUDE.md

@@ -120,6 +120,25 @@ All LiveViews, LiveComponents, and HTML modules automatically get these imports/
 - Gettext for translations
 - Verified routes with `~p` sigil
 
+### Rate Limiting
+
+Uses Hammer (ETS-backed) for rate limiting to prevent brute-force attacks.
+
+**Endpoints Protected:**
+- **Auth endpoints** (10 req/min per IP): `/users/log-in`, `/users/register`, `/users/reset-password`, TOTP verification, mobile auth QR flows
+- **API v1 endpoints** (1000 req/min per IP): `/api/v1/*` - relaxed for Terraform/automation
+- **Admin API endpoints**: Not rate limited (superuser only, different attack profile)
+
+**Implementation:**
+- Plug: `ToweropsWeb.Plugs.RateLimit` with `:auth` or `:api` type
+- Uses client's real IP from `X-Forwarded-For` or `X-Real-IP` headers (for proxy/load balancer)
+- Returns `429 Too Many Requests` with `Retry-After` header when limit exceeded
+
+**Configuration:**
+- Disabled in test environment via `config :towerops, :rate_limiting_enabled, false`
+- ETS backend configured in `config/config.exs`
+- Hammer process started in `application.ex` supervision tree
+
 ### Asset Pipeline
 
 - **Tailwind CSS v4**: Uses new `@import "tailwindcss"` syntax in `assets/css/app.css`, no `tailwind.config.js` needed

config/config.exs

@@ -20,6 +20,10 @@ config :esbuild,
     env: %{"NODE_PATH" => [Path.expand("../deps", __DIR__), Mix.Project.build_path()]}
   ]
 
+# Rate limiting with Hammer (ETS backend)
+config :hammer,
+  backend: {Hammer.Backend.ETS, [expiry_ms: to_timeout(minute: 10), cleanup_interval_ms: to_timeout(minute: 1)]}
+
 config :honeybadger,
   api_key: "hbp_xe5xMnpLZ2XJsXQJujoEkgCmiqCfwa0uYA3Y",
   environment_name: config_env(),

config/test.exs

@@ -61,4 +61,6 @@ config :towerops,
   ping_module: Towerops.Monitoring.PingMock,
   poller_module: Towerops.Snmp.PollerMock,
   snmp_adapter: Towerops.Snmp.SnmpMock,
-  sql_sandbox: true
+  sql_sandbox: true,
+  # Disable rate limiting in tests to avoid blocking concurrent test requests
+  rate_limiting_enabled: false

lib/towerops/application.ex

@@ -81,6 +81,8 @@ defmodule Towerops.Application do
         {Cluster.Supervisor, [topologies, [name: Towerops.ClusterSupervisor]]},
         ToweropsWeb.Telemetry,
         Towerops.Repo,
+        # Rate limiting backend (ETS-based, per-node)
+        {Hammer.Backend.ETS, [expiry_ms: to_timeout(minute: 10), cleanup_interval_ms: to_timeout(minute: 1)]},
         {Oban, Application.fetch_env!(:towerops, Oban)},
         {DNSCluster, query: Application.get_env(:towerops, :dns_cluster_query) || :ignore},
         pubsub_spec(),

lib/towerops_web/plugs/rate_limit.ex

@@ -0,0 +1,101 @@
+defmodule ToweropsWeb.Plugs.RateLimit do
+  @moduledoc """
+  Rate limiting plug using Hammer to prevent brute-force attacks.
+
+  Applies different rate limits based on the type of endpoint:
+  - Auth endpoints (login, register, password reset): 10 requests per minute per IP
+  - API endpoints: 100 requests per minute per IP
+
+  Uses the client's real IP address from X-Forwarded-For or X-Real-IP headers
+  when behind a proxy/load balancer.
+  """
+
+  import Phoenix.Controller, only: [json: 2]
+  import Plug.Conn
+
+  require Logger
+
+  @auth_limit 10
+  @auth_period to_timeout(minute: 1)
+
+  @api_limit 1000
+  @api_period to_timeout(minute: 1)
+
+  @type limit_type :: :auth | :api
+
+  def init(opts) do
+    type = Keyword.get(opts, :type, :api)
+
+    if type not in [:auth, :api] do
+      raise ArgumentError, "RateLimit plug :type must be :auth or :api, got: #{inspect(type)}"
+    end
+
+    type
+  end
+
+  def call(conn, type) do
+    if rate_limiting_enabled?() do
+      check_rate_limit(conn, type)
+    else
+      conn
+    end
+  end
+
+  defp rate_limiting_enabled? do
+    Application.get_env(:towerops, :rate_limiting_enabled, true)
+  end
+
+  defp check_rate_limit(conn, type) do
+    remote_ip = get_remote_ip(conn)
+    bucket_key = "#{type}:#{remote_ip}"
+
+    {limit, period} = limits_for(type)
+
+    case Hammer.check_rate(bucket_key, period, limit) do
+      {:allow, _count} ->
+        conn
+
+      {:deny, retry_after} ->
+        Logger.warning("Rate limit exceeded for #{bucket_key}, retry_after: #{retry_after}ms")
+
+        conn
+        |> put_resp_header("retry-after", Integer.to_string(div(retry_after, 1000)))
+        |> put_status(:too_many_requests)
+        |> json(%{error: "Too many requests. Please try again later."})
+        |> halt()
+    end
+  end
+
+  defp limits_for(:auth), do: {@auth_limit, @auth_period}
+  defp limits_for(:api), do: {@api_limit, @api_period}
+
+  defp get_remote_ip(conn) do
+    # Try X-Forwarded-For first (set by Traefik and other proxies)
+    case get_req_header(conn, "x-forwarded-for") do
+      [forwarded | _] ->
+        forwarded
+        |> String.split(",")
+        |> List.first()
+        |> String.trim()
+
+      [] ->
+        get_fallback_ip(conn)
+    end
+  end
+
+  defp get_fallback_ip(conn) do
+    case get_req_header(conn, "x-real-ip") do
+      [real_ip | _] ->
+        real_ip
+
+      [] ->
+        format_remote_ip(conn.remote_ip)
+    end
+  end
+
+  defp format_remote_ip({a, b, c, d}), do: "#{a}.#{b}.#{c}.#{d}"
+
+  defp format_remote_ip({a, b, c, d, e, f, g, h}) do
+    Enum.map_join([a, b, c, d, e, f, g, h], ":", &Integer.to_string(&1, 16))
+  end
+end

lib/towerops_web/router.ex

@@ -9,6 +9,7 @@ defmodule ToweropsWeb.Router do
   import ToweropsWeb.UserAuth
 
   alias ToweropsWeb.Api.V1
+  alias ToweropsWeb.Plugs.RateLimit
 
   pipeline :browser do
     plug :accepts, ["html"]
@@ -53,6 +54,15 @@ defmodule ToweropsWeb.Router do
     plug ToweropsWeb.Plugs.ApiAuth
   end
 
+  # Rate limiting pipelines
+  pipeline :rate_limit_auth do
+    plug RateLimit, type: :auth
+  end
+
+  pipeline :rate_limit_api do
+    plug RateLimit, type: :api
+  end
+
   # Health check endpoint for Kubernetes probes (no authentication required)
   scope "/", ToweropsWeb do
     get "/health", HealthController, :index
@@ -80,16 +90,18 @@ defmodule ToweropsWeb.Router do
   # See ToweropsWeb.AgentChannel for implementation
 
   # Mobile Auth API routes (no authentication required for login flow)
+  # Rate limited to prevent brute-force attacks on QR token verification
   scope "/api/v1/mobile/auth", ToweropsWeb.Api do
-    pipe_through :api
+    pipe_through [:api, :rate_limit_auth]
 
     post "/qr/verify", MobileAuthController, :verify_qr_token
     post "/qr/complete", MobileAuthController, :complete_qr_login
   end
 
   # Mobile API routes (requires mobile authentication)
+  # Rate limited to prevent API abuse
   scope "/api/v1/mobile", ToweropsWeb.Api do
-    pipe_through :mobile_api
+    pipe_through [:mobile_api, :rate_limit_api]
 
     # Auth session management
     get "/auth/session", MobileAuthController, :get_session
@@ -104,8 +116,9 @@ defmodule ToweropsWeb.Router do
   end
 
   # API v1 routes (requires API token authentication)
+  # Rate limited to prevent API abuse
   scope "/api/v1", V1 do
-    pipe_through :api_v1
+    pipe_through [:api_v1, :rate_limit_api]
 
     resources "/sites", SitesController, except: [:new, :edit]
     resources "/devices", DevicesController, except: [:new, :edit]
@@ -157,9 +170,10 @@ defmodule ToweropsWeb.Router do
   end
 
   ## Authentication routes
+  ## Rate limited to prevent brute-force attacks
 
   scope "/", ToweropsWeb do
-    pipe_through [:browser, :redirect_if_user_is_authenticated]
+    pipe_through [:browser, :redirect_if_user_is_authenticated, :rate_limit_auth]
 
     live "/users/register", UserRegistrationLive, :new
   end
@@ -171,7 +185,7 @@ defmodule ToweropsWeb.Router do
   end
 
   scope "/", ToweropsWeb do
-    pipe_through [:browser]
+    pipe_through [:browser, :rate_limit_auth]
 
     get "/users/log-in", UserSessionController, :new
     get "/users/log-in/totp", UserSessionController, :totp_verification_form

mix.exs

@@ -85,7 +85,8 @@ defmodule Towerops.MixProject do
       {:dialyxir, "~> 1.4", only: [:dev, :test], runtime: false},
       {:credo, "~> 1.7", only: [:dev, :test], runtime: false},
       {:sobelow, "~> 0.14.1", only: [:dev, :test], runtime: false},
-      {:mix_audit, "~> 2.1", only: [:dev, :test], runtime: false}
+      {:mix_audit, "~> 2.1", only: [:dev, :test], runtime: false},
+      {:hammer, "~> 6.2"}
     ]
   end
 

mix.lock

@@ -25,6 +25,7 @@
   "gen_smtp": {:hex, :gen_smtp, "1.3.0", "62c3d91f0dcf6ce9db71bcb6881d7ad0d1d834c7f38c13fa8e952f4104a8442e", [:rebar3], [{:ranch, ">= 1.8.0", [hex: :ranch, repo: "hexpm", optional: false]}], "hexpm", "0b73fbf069864ecbce02fe653b16d3f35fd889d0fdd4e14527675565c39d84e6"},
   "gettext": {:hex, :gettext, "1.0.2", "5457e1fd3f4abe47b0e13ff85086aabae760497a3497909b8473e0acee57673b", [:mix], [{:expo, "~> 0.5.1 or ~> 1.0", [hex: :expo, repo: "hexpm", optional: false]}], "hexpm", "eab805501886802071ad290714515c8c4a17196ea76e5afc9d06ca85fb1bfeb3"},
   "hackney": {:hex, :hackney, "1.25.0", "390e9b83f31e5b325b9f43b76e1a785cbdb69b5b6cd4e079aa67835ded046867", [:rebar3], [{:certifi, "~> 2.15.0", [hex: :certifi, repo: "hexpm", optional: false]}, {:idna, "~> 6.1.0", [hex: :idna, repo: "hexpm", optional: false]}, {:metrics, "~> 1.0.0", [hex: :metrics, repo: "hexpm", optional: false]}, {:mimerl, "~> 1.4", [hex: :mimerl, repo: "hexpm", optional: false]}, {:parse_trans, "3.4.1", [hex: :parse_trans, repo: "hexpm", optional: false]}, {:ssl_verify_fun, "~> 1.1.0", [hex: :ssl_verify_fun, repo: "hexpm", optional: false]}, {:unicode_util_compat, "~> 0.7.1", [hex: :unicode_util_compat, repo: "hexpm", optional: false]}], "hexpm", "7209bfd75fd1f42467211ff8f59ea74d6f2a9e81cbcee95a56711ee79fd6b1d4"},
+  "hammer": {:hex, :hammer, "6.2.1", "5ae9c33e3dceaeb42de0db46bf505bd9c35f259c8defb03390cd7556fea67ee2", [:mix], [{:poolboy, "~> 1.5", [hex: :poolboy, repo: "hexpm", optional: false]}], "hexpm", "b9476d0c13883d2dc0cc72e786bac6ac28911fba7cc2e04b70ce6a6d9c4b2bdc"},
   "heroicons": {:git, "https://github.com/tailwindlabs/heroicons.git", "0435d4ca364a608cc75e2f8683d374e55abbae26", [tag: "v2.2.0", sparse: "optimized", depth: 1]},
   "honeybadger": {:hex, :honeybadger, "0.24.1", "13ffe56b4d148649c8fbb0e091fefecc5d8e8eb7ade684b6900085a947d741d5", [:mix], [{:ecto, ">= 2.0.0", [hex: :ecto, repo: "hexpm", optional: true]}, {:hackney, "~> 1.1", [hex: :hackney, repo: "hexpm", optional: true]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}, {:phoenix, ">= 1.0.0 and < 2.0.0", [hex: :phoenix, repo: "hexpm", optional: true]}, {:plug, ">= 1.0.0 and < 2.0.0", [hex: :plug, repo: "hexpm", optional: true]}, {:process_tree, "~> 0.2.1", [hex: :process_tree, repo: "hexpm", optional: false]}, {:req, "~> 0.5.0", [hex: :req, repo: "hexpm", optional: true]}, {:telemetry, "~> 0.4 or ~> 1.0", [hex: :telemetry, repo: "hexpm", optional: false]}], "hexpm", "0c97d5a82c42298b9935dbc0a7e3c14372c8f55257f603828258ef9f7e0da892"},
   "hpax": {:hex, :hpax, "1.0.3", "ed67ef51ad4df91e75cc6a1494f851850c0bd98ebc0be6e81b026e765ee535aa", [:mix], [], "hexpm", "8eab6e1cfa8d5918c2ce4ba43588e894af35dbd8e91e6e55c817bca5847df34a"},

test/towerops_web/plugs/rate_limit_test.exs

@@ -0,0 +1,163 @@
+defmodule ToweropsWeb.Plugs.RateLimitTest do
+  use ToweropsWeb.ConnCase, async: false
+
+  alias ToweropsWeb.Plugs.RateLimit
+
+  # Use async: false because we're testing Hammer state
+
+  setup do
+    # Enable rate limiting for this test module (disabled by default in test env)
+    Application.put_env(:towerops, :rate_limiting_enabled, true)
+
+    on_exit(fn ->
+      # Restore default test config (disabled)
+      Application.put_env(:towerops, :rate_limiting_enabled, false)
+    end)
+
+    :ok
+  end
+
+  describe "init/1" do
+    test "accepts :auth type" do
+      assert :auth = RateLimit.init(type: :auth)
+    end
+
+    test "accepts :api type" do
+      assert :api = RateLimit.init(type: :api)
+    end
+
+    test "defaults to :api type" do
+      assert :api = RateLimit.init([])
+    end
+
+    test "raises on invalid type" do
+      assert_raise ArgumentError, ~r/must be :auth or :api/, fn ->
+        RateLimit.init(type: :invalid)
+      end
+    end
+  end
+
+  describe "call/2 with :auth type" do
+    test "allows requests under the limit", %{conn: conn} do
+      # Use a unique IP for this test
+      unique_ip = "10.0.0.#{:rand.uniform(255)}"
+
+      conn =
+        conn
+        |> put_req_header("x-forwarded-for", unique_ip)
+        |> RateLimit.call(:auth)
+
+      refute conn.halted
+    end
+
+    test "blocks requests over the limit", %{conn: conn} do
+      # Use a unique IP for this test
+      unique_ip = "10.1.0.#{:rand.uniform(255)}"
+
+      # Make 10 allowed requests (auth limit is 10)
+      Enum.each(1..10, fn _ ->
+        conn =
+          build_conn()
+          |> put_req_header("x-forwarded-for", unique_ip)
+          |> RateLimit.call(:auth)
+
+        refute conn.halted
+      end)
+
+      # 11th request should be blocked
+      conn =
+        conn
+        |> put_req_header("x-forwarded-for", unique_ip)
+        |> RateLimit.call(:auth)
+
+      assert conn.halted
+      assert conn.status == 429
+      assert get_resp_header(conn, "retry-after") != []
+
+      body = Jason.decode!(conn.resp_body)
+      assert body["error"] =~ "Too many requests"
+    end
+  end
+
+  describe "call/2 with :api type" do
+    test "allows requests under the limit", %{conn: conn} do
+      unique_ip = "10.2.0.#{:rand.uniform(255)}"
+
+      conn =
+        conn
+        |> put_req_header("x-forwarded-for", unique_ip)
+        |> RateLimit.call(:api)
+
+      refute conn.halted
+    end
+
+    test "allows higher limit than auth", %{conn: conn} do
+      unique_ip = "10.3.0.#{:rand.uniform(255)}"
+
+      # Make 50 requests - should all be allowed (API limit is 100)
+      Enum.each(1..50, fn _ ->
+        conn =
+          build_conn()
+          |> put_req_header("x-forwarded-for", unique_ip)
+          |> RateLimit.call(:api)
+
+        refute conn.halted
+      end)
+    end
+  end
+
+  describe "IP extraction" do
+    test "uses X-Forwarded-For header first", %{conn: conn} do
+      unique_ip = "192.168.1.#{:rand.uniform(255)}"
+
+      # Exhaust the limit for this IP
+      Enum.each(1..10, fn _ ->
+        build_conn()
+        |> put_req_header("x-forwarded-for", unique_ip)
+        |> RateLimit.call(:auth)
+      end)
+
+      # Different IP should still work
+      different_ip = "192.168.2.#{:rand.uniform(255)}"
+
+      conn =
+        conn
+        |> put_req_header("x-forwarded-for", different_ip)
+        |> RateLimit.call(:auth)
+
+      refute conn.halted
+    end
+
+    test "handles comma-separated X-Forwarded-For", %{conn: conn} do
+      unique_ip = "203.0.113.#{:rand.uniform(255)}"
+
+      # X-Forwarded-For can have multiple IPs (client, proxy1, proxy2)
+      conn =
+        conn
+        |> put_req_header("x-forwarded-for", "#{unique_ip}, 10.0.0.1, 10.0.0.2")
+        |> RateLimit.call(:auth)
+
+      refute conn.halted
+    end
+
+    test "falls back to X-Real-IP", %{conn: conn} do
+      unique_ip = "198.51.100.#{:rand.uniform(255)}"
+
+      conn =
+        conn
+        |> put_req_header("x-real-ip", unique_ip)
+        |> RateLimit.call(:auth)
+
+      refute conn.halted
+    end
+
+    test "falls back to remote_ip when no headers", %{conn: conn} do
+      # No forwarding headers, uses conn.remote_ip (127.0.0.1)
+      # Since other tests might have used this IP, we just verify the plug works
+      # by checking it doesn't crash - actual rate limiting is tested elsewhere
+      result = RateLimit.call(conn, :auth)
+      # Either allowed or blocked is fine - we're testing the IP extraction path
+      assert is_struct(result, Plug.Conn)
+    end
+  end
+end