diff --git a/README.md b/README.md index 9811d40..80602ab 100644 --- a/README.md +++ b/README.md @@ -513,6 +513,7 @@ curl -H "Authorization: Bearer $TOKEN" http://localhost:4041/ | `MALACHI_DASHBOARD_AUTH_RATE_WINDOW_MS` | `60000` | Rate limit window (1 minute) | | `MALACHI_DASHBOARD_CORS_ENABLED` | `false` | Enable CORS for `/metrics` and `/stream` | | `MALACHI_DASHBOARD_CORS_ORIGINS` | `*` | Allowed CORS origins (comma-separated) | +| `MALACHI_DASHBOARD_SECURE_COOKIE` | `false` | Mark the session cookie `Secure`. Set it only behind a TLS-terminating proxy: the dashboard listener itself serves plain HTTP, and a browser refuses to store a `Secure` cookie from a plain-HTTP origin, which makes login fail with no error. | | `MALACHI_DASHBOARD_CSP` | (default) | Custom Content-Security-Policy | | `MALACHI_HSTS_ENABLED` | `true` | Enable HTTP Strict Transport Security | | `MALACHI_HSTS_MAX_AGE` | `31536000` | HSTS max-age (1 year) | diff --git a/config/runtime.exs b/config/runtime.exs index b641740..c058ec7 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -334,6 +334,12 @@ config :malachi, end), dashboard_auth_rate_limit: parse_int.(System.get_env("MALACHI_DASHBOARD_AUTH_RATE_LIMIT"), 10), dashboard_auth_rate_window_ms: parse_int.(System.get_env("MALACHI_DASHBOARD_AUTH_RATE_WINDOW_MS"), 60_000), + # Whether the session cookie is marked Secure. Off by default because `Malachi.Dashboard` listens with + # `:gen_tcp.listen` and has no TLS path, so the transport it actually serves is plain HTTP, and a browser + # refuses to store a Secure cookie from a non-trustworthy origin. Turn it on when a TLS-terminating proxy + # sits in front, which is the only shape where the dashboard is reached over HTTPS. This deliberately does + # not read `enable_tls`: that switch describes the broker listener on another port. + dashboard_secure_cookie: System.get_env("MALACHI_DASHBOARD_SECURE_COOKIE") == "true", # CORS configuration dashboard_cors_enabled: System.get_env("MALACHI_DASHBOARD_CORS_ENABLED") == "true", dashboard_cors_origins: diff --git a/docs/DOCKER_README.md b/docs/DOCKER_README.md index cc986ff..99caeae 100644 --- a/docs/DOCKER_README.md +++ b/docs/DOCKER_README.md @@ -84,7 +84,8 @@ Open [http://localhost:4041](http://localhost:4041) in your browser. | `MALACHI_TCP_PORT` | `4040` | TCP server port for clients | | `MALACHI_DASHBOARD_PORT` | `4041` | HTTP dashboard port | | `MALACHI_LOCALE` | `en_US` | Language (`en_US`, `pt_BR`) | -| `MALACHI_ENABLE_TLS` | `false` | Enable TLS encryption | +| `MALACHI_ENABLE_TLS` | `false` | Enable TLS encryption on the broker port. It says nothing about the dashboard, which has no TLS path. | +| `MALACHI_DASHBOARD_SECURE_COOKIE` | `false` | Mark the dashboard session cookie `Secure`. Set it only behind a TLS-terminating proxy; see the ports note below. | | `MALACHI_ADMIN_PASS` | *(generated)* | Admin password; if unset, a random one is generated and logged on first boot. Without a persistent ra volume, this happens again on every restart. See Users and credentials. | | `MALACHI_LOG_DATA_DIR` | *(tmp)* | Directory for the durable log segments. Must be an absolute path on a volume; see Data Persistence. | | `MALACHI_RA_DATA_DIR` | *(tmp)* | Directory for the ra log (users, ACLs, lockouts). Must be an absolute path on a volume; see Data Persistence. | @@ -140,6 +141,15 @@ the login form and the session cookie. Publishing it on every interface would ha cleartext next to a broker you had just taken the trouble to encrypt. Reach a remote dashboard through a reverse proxy that terminates TLS in front of it, not by widening this binding. +When you do put a proxy in front, set `MALACHI_DASHBOARD_SECURE_COOKIE=true`. That is the one thing +the server cannot work out for itself: it only ever sees a plain-HTTP connection from the proxy, so +whether the browser reached it over HTTPS is a fact about your deployment. Leave it off for a +dashboard reached directly, because a browser refuses to store a `Secure` cookie served over plain +HTTP, and the resulting login answers 200 and simply does nothing. The boot log states which of the +two is in effect. Note that this used to follow `MALACHI_ENABLE_TLS`, which describes the broker port +and made every production build mark the cookie `Secure` over plain HTTP; if you run behind a proxy +and login worked before without setting anything, that is the setting you now need. + The mounted files are read at boot and validated then, so a certificate that is expired or unreadable stops the container rather than being discovered by a client later. diff --git a/lib/malachi/dashboard.ex b/lib/malachi/dashboard.ex index e855146..7ebddd4 100644 --- a/lib/malachi/dashboard.ex +++ b/lib/malachi/dashboard.ex @@ -32,6 +32,16 @@ defmodule Malachi.Dashboard do case :gen_tcp.listen(port, opts) do {:ok, socket} -> Logger.info(I18n.t(:dashboard_started, port: port)) + # Stating the cookie policy at boot is part of the fix rather than decoration: the failure it + # guards against is a login that answers 200 and does nothing, which is invisible from the outside. + # The two states need different advice, not one sentence with a boolean in it, so they are two + # messages: one names the variable to set, the other names the assumption being made. + Logger.info( + if secure_cookie?(), + do: I18n.t(:dashboard_cookie_secure), + else: I18n.t(:dashboard_cookie_plain) + ) + send(self(), :accept) {:ok, %{socket: socket, port: port}} @@ -341,14 +351,37 @@ defmodule Malachi.Dashboard do "HTTP/1.1 302 Found\r\nLocation: /login\r\nSet-Cookie: malachi_token=; HttpOnly; Path=/; SameSite=Strict; Max-Age=0#{secure_cookie_flag()}\r\nCache-Control: no-store\r\nContent-Length: 0\r\n\r\n" end - # "; Secure" only under TLS, so the cookie is not marked Secure on a plain-HTTP dev server (where the - # browser would then silently drop it). Shared by the login cookie and the cookie-clearing redirects, so a - # change to the Secure policy cannot make them disagree. + # "; Secure" comes from the dashboard's own setting, and from nothing else. This module listens with + # `:gen_tcp.listen` and has no TLS path, so the transport it serves is always plain HTTP; the only way it + # is reached over HTTPS is behind a proxy that terminates TLS, and that is a fact about the deployment + # which only the operator can state. It used to read `:enable_tls`, the broker's switch for another port, + # which marked the cookie Secure over plain HTTP: browsers refuse to store that, so login failed with a + # 200 and no error for anyone not on localhost. Shared by the login cookie and the cookie-clearing + # redirects, so a change to the policy cannot make them disagree. defp secure_cookie_flag do - if Application.get_env(:malachi, :enable_tls), do: "; Secure", else: "" + if secure_cookie?(), do: "; Secure", else: "" + end + + defp secure_cookie?, do: Application.get_env(:malachi, :dashboard_secure_cookie, false) + + # `X-Forwarded-Proto` is read here to REPORT, never to decide. The cookie policy comes from configuration + # and nothing a client sends can change it, which is why this needs no trusted-proxy gate. What it buys is + # the one diagnosis the server can otherwise not make: whether the policy matches how the browser actually + # reached the dashboard. Only a header that is present and disagrees is worth a line, in either direction. + # A proxy that forwards nothing is ordinary, so its absence has to stay silent or the warning becomes + # noise and stops being read. + defp warn_on_proto_mismatch(headers) do + case {secure_cookie?(), Map.get(headers, "x-forwarded-proto")} do + {_same, nil} -> :ok + {true, "https"} -> :ok + {false, proto} when proto != "https" -> :ok + {true, proto} -> Logger.warning(I18n.t(:dashboard_cookie_secure_over_plain, proto: proto)) + {false, _https} -> Logger.warning(I18n.t(:dashboard_cookie_plain_over_https)) + end end - defp send_login_success(socket, token) do + defp send_login_success(socket, token, headers) do + warn_on_proto_mismatch(headers) response_body = Jason.encode!(%{"s" => "ok", "token" => token}) cookie_header = @@ -468,7 +501,7 @@ defmodule Malachi.Dashboard do %{} ) - send_login_success(socket, token) + send_login_success(socket, token, headers) {:error, _reason} -> Metrics.increment_dashboard_auth_failed() diff --git a/lib/malachi/i18n.ex b/lib/malachi/i18n.ex index a051540..6edd74d 100644 --- a/lib/malachi/i18n.ex +++ b/lib/malachi/i18n.ex @@ -129,6 +129,38 @@ defmodule Malachi.I18n do "pt_BR" => "🌐 Malachi Dashboard rodando em http://localhost:%{port}", "en_US" => "🌐 Malachi Dashboard running at http://localhost:%{port}" }, + dashboard_cookie_plain: %{ + "pt_BR" => + "Cookie de sessão do dashboard sem Secure. O listener serve HTTP puro; defina " <> + "MALACHI_DASHBOARD_SECURE_COOKIE=true se um proxy terminar TLS na frente dele", + "en_US" => + "Dashboard session cookie is not marked Secure. This listener serves plain HTTP; set " <> + "MALACHI_DASHBOARD_SECURE_COOKIE=true when a TLS-terminating proxy sits in front of it" + }, + dashboard_cookie_secure: %{ + "pt_BR" => + "Cookie de sessão do dashboard marcado como Secure. Isso pressupõe um proxy terminando TLS na " <> + "frente: alcançado direto por HTTP, o navegador descarta o cookie e o login falha sem erro", + "en_US" => + "Dashboard session cookie is marked Secure. That assumes a TLS-terminating proxy in front: " <> + "reached directly over HTTP, the browser drops the cookie and login fails with no error" + }, + dashboard_cookie_secure_over_plain: %{ + "pt_BR" => + "Cookie Secure emitido, mas a requisição chegou com X-Forwarded-Proto: %{proto}. Se o navegador " <> + "alcança o dashboard por HTTP puro, ele descarta o cookie e o login falha sem erro", + "en_US" => + "Issued a Secure cookie, but the request arrived with X-Forwarded-Proto: %{proto}. If the browser " <> + "reaches the dashboard over plain HTTP it drops the cookie and login fails with no error" + }, + dashboard_cookie_plain_over_https: %{ + "pt_BR" => + "Requisição chegou com X-Forwarded-Proto: https, mas o cookie de sessão não está marcado como " <> + "Secure. Defina MALACHI_DASHBOARD_SECURE_COOKIE=true para o navegador não o enviar por HTTP", + "en_US" => + "Request arrived with X-Forwarded-Proto: https, but the session cookie is not marked Secure. Set " <> + "MALACHI_DASHBOARD_SECURE_COOKIE=true so the browser will not send it over plain HTTP" + }, rate_limiter_started: %{ "pt_BR" => "✅ RateLimiter iniciado", "en_US" => "✅ RateLimiter started" diff --git a/test/dashboard_security_test.exs b/test/dashboard_security_test.exs index 90a3b58..73c5615 100644 --- a/test/dashboard_security_test.exs +++ b/test/dashboard_security_test.exs @@ -1,6 +1,8 @@ defmodule Malachi.DashboardSecurityTest do use ExUnit.Case, async: false + import ExUnit.CaptureLog + alias Malachi.Auth.UserStore alias Malachi.Dashboard.SecurityHeaders alias Malachi.Test.DashboardHelper @@ -424,6 +426,73 @@ defmodule Malachi.DashboardSecurityTest do end end + # Every test here sets `:enable_tls` to the opposite of the cookie policy, and that opposition is the + # assertion: the flag used to be read from `:enable_tls`, which describes the broker listener on 4040, + # while the cookie is issued on 4041, which has no TLS path at all. Pinning them against each other is + # what proves the broker's setting no longer participates. + describe "session cookie Secure flag" do + test "is set when the dashboard is configured for it, with the broker's TLS off" do + response = with_cookie_policy(true, false, &login_response/0) + + assert String.contains?(response, "Set-Cookie: malachi_token=") + assert String.contains?(response, "; Secure") + end + + test "is absent when the dashboard is not configured for it, with the broker's TLS on" do + # The regression. Marking the cookie Secure over plain HTTP makes the browser refuse to store it, + # so the login form posts, the server answers 200, and nothing happens. Only localhost escapes it, + # because browsers treat that origin as trustworthy. + response = with_cookie_policy(false, true, &login_response/0) + + assert String.contains?(response, "Set-Cookie: malachi_token=") + refute String.contains?(response, "; Secure") + end + + test "the cookie-clearing redirect follows the same policy as the login cookie" do + secure = with_cookie_policy(true, false, &clearing_response/0) + plain = with_cookie_policy(false, true, &clearing_response/0) + + # They share secure_cookie_flag/0 precisely so a policy change cannot make them disagree: a clear + # whose attributes do not match the cookie that was set is a clear the browser can ignore. + assert String.downcase(secure) =~ "set-cookie: malachi_token=;" + assert String.contains?(secure, "; Secure") + assert String.downcase(plain) =~ "set-cookie: malachi_token=;" + refute String.contains?(plain, "; Secure") + end + + test "warns when a forwarded protocol says the cookie will be dropped" do + log = + capture_log(fn -> + with_cookie_policy(true, false, fn -> login_response(%{"X-Forwarded-Proto" => "http"}) end) + end) + + assert log =~ "X-Forwarded-Proto" + end + + test "warns when the request arrived over HTTPS but the cookie is not Secure" do + log = + capture_log(fn -> + with_cookie_policy(false, true, fn -> login_response(%{"X-Forwarded-Proto" => "https"}) end) + end) + + assert log =~ "X-Forwarded-Proto" + end + + test "stays quiet when the forwarded protocol agrees, and when there is none" do + # A proxy that does not forward the header is ordinary, so its absence cannot be a warning without + # becoming noise. Silence here is what keeps the two warnings above worth reading. + agreeing = + capture_log(fn -> + with_cookie_policy(true, false, fn -> login_response(%{"X-Forwarded-Proto" => "https"}) end) + end) + + absent = capture_log(fn -> with_cookie_policy(true, false, &login_response/0) end) + + refute agreeing =~ "X-Forwarded-Proto" + refute absent =~ "X-Forwarded-Proto" + end + end + # CORS is off by default, so each test sets exactly the configuration it exercises. The preflight must # agree with what a real request would get: it answers from the same builder. describe "CORS preflight" do @@ -1125,6 +1194,51 @@ defmodule Malachi.DashboardSecurityTest do response end + # Applies a cookie policy for the duration of `fun` and restores both settings afterwards, including on + # a failing assertion: leaking either of these would silently change what the rest of the suite tests. + # `enable_tls` is passed explicitly rather than left alone because these tests exist to show it has no + # say, which only means something when it is set against the expected outcome. + defp with_cookie_policy(secure_cookie?, enable_tls?, fun) do + previous = { + Application.get_env(:malachi, :dashboard_secure_cookie), + Application.get_env(:malachi, :enable_tls) + } + + Application.put_env(:malachi, :dashboard_secure_cookie, secure_cookie?) + Application.put_env(:malachi, :enable_tls, enable_tls?) + + try do + fun.() + after + {previous_cookie, previous_tls} = previous + Application.put_env(:malachi, :dashboard_secure_cookie, previous_cookie) + Application.put_env(:malachi, :enable_tls, previous_tls) + end + end + + # Both of these bind the socket with a match rather than a case: a connection that fails has to fail the + # test, not pass it quietly. + defp login_response(extra_headers \\ %{}) do + {:ok, socket} = DashboardHelper.connect() + body = Jason.encode!(%{"username" => "dashboard_admin", "password" => "admin_pass_123"}) + + {:ok, response} = + DashboardHelper.request(socket, :POST, "/login", body: body, headers: extra_headers) + + :gen_tcp.close(socket) + response + end + + defp clearing_response do + {:ok, socket} = DashboardHelper.connect() + + {:ok, response} = + DashboardHelper.request(socket, :GET, "/", headers: %{"Cookie" => "malachi_token=not_a_real_token"}) + + :gen_tcp.close(socket) + response + end + # Extracts the numeric status from an HTTP response, and its JSON body. defp status_code(response) do case Regex.run(~r"HTTP/1\.1 (\d{3})", response) do