diff --git a/.env.example b/.env.example index 1a2ba62..2662ac8 100644 --- a/.env.example +++ b/.env.example @@ -39,6 +39,24 @@ POSTGRES_PASSWORD= # 127.0.0.1 so only the proxy can talk to it. #THOUGHTSYNC_BIND=0.0.0.0 +# How many proxies sit in front of this app. This is a SECURITY setting, not a +# preference: it decides which entry of X-Forwarded-For is believed, and therefore +# whether a caller can forge their own address and slip the per-address rate limit. +# +# 0 nothing in front — the app is directly exposed +# 1 one reverse proxy terminating TLS (the default, and the usual case) +# 2 a CDN in front of that proxy, e.g. Cloudflare +# +# Set it to the number you actually run. Too HIGH is the dangerous direction — it +# starts trusting entries no proxy of yours wrote. Too low just means callers share +# a rate-limit bucket. +#THOUGHTSYNC_TRUSTED_PROXY_HOPS=1 + +# How much the app says. Credential events (sign-ins, failures, throttles, new +# accounts, device tokens issued) are logged at INFO and read with +# `docker compose logs app`. +#THOUGHTSYNC_LOG_LEVEL=INFO + # Database identity. Changing these AFTER the first start does not rename anything # that already exists — the volume keeps whatever the first run created. #POSTGRES_USER=thoughtsync diff --git a/docs/public-hosting.md b/docs/public-hosting.md index dda5973..68c18aa 100644 --- a/docs/public-hosting.md +++ b/docs/public-hosting.md @@ -7,7 +7,7 @@ one guessed password away from someone's whole note history. This is what the app does about that on its own, and the four things it cannot do for you. -## Do these four things first +## Do these five things first **1. Check registration is closed.** On a fresh instance this now takes care of itself: the first account created becomes the admin *and* closes registration behind @@ -41,7 +41,19 @@ Once a browser has seen HSTS from your hostname it will refuse plain HTTP there year, even if the header stops. That is the point of it, but it is worth knowing before you put a hostname behind TLS temporarily. -**3. Stop publishing the app port.** The default compose binds `0.0.0.0:5000` so LAN +**3. Tell it how many proxies are in front of it.** `THOUGHTSYNC_TRUSTED_PROXY_HOPS` +defaults to `1` — one reverse proxy terminating TLS. Behind a CDN as well (Cloudflare +in front of your proxy) set it to `2`. + +This decides which entry of `X-Forwarded-For` is believed, and it is a security +setting rather than a preference. The header grows left to right as a request +traverses, so the rightmost entries are the ones your own infrastructure wrote and +anything a caller forged sits to the left of them. Counting in from the right by the +number of proxies you actually run means a forged prefix can never be selected. Set it +too HIGH and it starts trusting entries no proxy of yours wrote; too low and several +callers share one rate-limit bucket, which is merely inconvenient. + +**4. Stop publishing the app port.** The default compose binds `0.0.0.0:5000` so LAN clients can reach it directly. Behind a proxy that is a second, unprotected front door. In `.env`: @@ -49,7 +61,7 @@ door. In `.env`: THOUGHTSYNC_BIND=127.0.0.1 ``` -**4. Have a backup that includes the files.** Attachments are files on the +**5. Have a backup that includes the files.** Attachments are files on the `thoughtsync-data` volume, not rows — a `pg_dump` restores notes whose images are all gone. Back up both: @@ -92,7 +104,11 @@ Know these before you decide who gets an account. - **No second factor.** A password is the whole of it. - **No per-user storage quota.** Any account can upload attachments until the volume is full. `max_attachment_mb` caps a single file, not a total. -- **No audit log.** Device tokens record `last_used_at`; sign-ins are not recorded. +- **No audit TABLE.** Credential events — sign-ins, failures, throttle trips, new + accounts, device tokens issued — are written to the application log and readable + with `docker compose logs app`, which is enough to see whether anyone is knocking. + They are not queryable, not retained beyond the container's log rotation, and not + attributable after the fact. - **No invites.** Adding a second person means re-opening registration while they sign up, then closing it again. There is no per-person token, no expiry, and no record of who invited whom. diff --git a/src/thoughtsync/app.py b/src/thoughtsync/app.py index a1970ce..a47cd51 100644 --- a/src/thoughtsync/app.py +++ b/src/thoughtsync/app.py @@ -1,13 +1,14 @@ from __future__ import annotations import asyncio +import logging import mimetypes import os import secrets from contextlib import suppress from datetime import timedelta -from quart import Quart, has_request_context, jsonify, request, send_from_directory +from quart import Quart, jsonify, send_from_directory from quart.sessions import SecureCookieSessionInterface from . import __version__ @@ -17,12 +18,25 @@ from .config import Config from .db import session_scope from .labels import bp as labels_bp from .notes import bp as notes_bp +from .proxy import is_https from .retention import run_sweeper from .saved_filters import bp as saved_filters_bp from .settings import get_public_config, get_setting, load_or_create_secret_key from .settings_api import bp as settings_bp from .sync import bp as sync_bp, protocol_advertisement +# Without this, `logger.info` from this package goes nowhere: hypercorn configures its +# own access/error loggers and leaves the root logger at WARNING, so the credential +# events in auth.py would be invisible in `docker compose logs` — which is exactly +# where they are meant to be read until an audit table exists (task 2939). +# +# `force=False` (the default) so a host that has already configured logging keeps its +# own setup; LOG_LEVEL lets an operator turn it up without a code change. +logging.basicConfig( + level=os.environ.get("THOUGHTSYNC_LOG_LEVEL", "INFO").upper(), + format="%(asctime)s %(levelname)s %(name)s %(message)s", +) + STATIC_DIR = os.path.join(os.path.dirname(__file__), "static") # `.webmanifest` isn't in every base image's mime map; register it so the PWA @@ -30,19 +44,6 @@ STATIC_DIR = os.path.join(os.path.dirname(__file__), "static") mimetypes.add_type("application/manifest+json", ".webmanifest") -def _is_https() -> bool: - """Whether this request reached us over TLS — directly, or through a proxy that - terminated it and said so in X-Forwarded-Proto. - - Shared by the session cookie's Secure flag and by HSTS, because they are the same - question and answering it twice is how the two drift apart. - """ - if not has_request_context(): - return False - forwarded = request.headers.get("X-Forwarded-Proto", "").split(",")[0].strip().lower() - return forwarded == "https" or request.is_secure - - class _AutoSecureSessionInterface(SecureCookieSessionInterface): """Mark the session cookie `Secure` whenever the request arrived over HTTPS — directly, or via a TLS-terminating reverse proxy that sets X-Forwarded-Proto. @@ -54,7 +55,7 @@ class _AutoSecureSessionInterface(SecureCookieSessionInterface): """ def get_cookie_secure(self, app: Quart) -> bool: - return _is_https() + return is_https() def create_app() -> Quart: @@ -168,7 +169,7 @@ def create_app() -> Quart: # commit domains this app does not own. A browser still remembers the policy # for up to a year after the header stops being sent, which is the point of # it — worth knowing before putting a hostname behind TLS temporarily. - if _is_https(): + if is_https(): response.headers.setdefault("Strict-Transport-Security", "max-age=31536000") return response diff --git a/src/thoughtsync/auth.py b/src/thoughtsync/auth.py index 4018950..c8990a9 100644 --- a/src/thoughtsync/auth.py +++ b/src/thoughtsync/auth.py @@ -1,6 +1,7 @@ from __future__ import annotations import functools +import logging import uuid from datetime import datetime, timezone @@ -11,8 +12,8 @@ from .common import iso from .db import session_scope from .models.device_token import DeviceToken from .models.user import User +from .proxy import client_address from .ratelimit import ( - client_address, register_by_address, sign_in_by_account, sign_in_by_address, @@ -22,6 +23,16 @@ from .settings import get_setting, set_settings bp = Blueprint("auth", __name__, url_prefix="/api/auth") +# Every credential event goes to the app log — there is no audit TABLE yet (see task +# 2939), and until there is, `docker compose logs` is the only way to know whether +# anyone is knocking. That matters most in exactly the window this was written for: a +# freshly-exposed instance. +# +# The attempted email is included deliberately. It is the operator's own server, and +# "somebody failed a login" without saying against WHICH account tells you nothing you +# can act on. Passwords, obviously, never appear. +logger = logging.getLogger(__name__) + SESSION_KEY = "user_id" MIN_PASSWORD_LEN = 8 DEVICE_NAME_CAP = 100 @@ -120,6 +131,7 @@ def _throttled(retry_after: int): `Retry-After` is standard and is the one thing a legitimate client (or person) genuinely needs. """ + logger.warning("throttled credential attempt from=%s retry_after=%ss", client_address(), retry_after) return ( jsonify({"error": "too many attempts — try again shortly"}), 429, @@ -186,6 +198,7 @@ async def register(): # The first account bootstraps the admin and is always allowed, even when # registration is otherwise closed. if not is_first and not await get_setting(db, "allow_registration"): + logger.warning("registration refused (closed) email=%s from=%s", email, client_address()) return jsonify({"error": "registration is closed"}), 403 existing = await db.scalar(select(User).where(User.email == email)) if existing is not None: @@ -215,6 +228,9 @@ async def register(): await db.refresh(user) session[SESSION_KEY] = str(user.id) session.permanent = True + logger.info( + "account created email=%s admin=%s from=%s", email, is_first, client_address() + ) return jsonify(_serialize_user(user)), 201 @@ -236,13 +252,16 @@ async def login(): # difference is a reliable oracle for which emails have accounts here. dummy_verify(password) _sign_in_failed(email) + logger.warning("sign-in failed (no such account) email=%s from=%s", email, client_address()) return jsonify({"error": "invalid email or password"}), 401 if not verify_password(password, user.password_hash): _sign_in_failed(email) + logger.warning("sign-in failed (bad password) email=%s from=%s", email, client_address()) return jsonify({"error": "invalid email or password"}), 401 _sign_in_succeeded(email) session[SESSION_KEY] = str(user.id) session.permanent = True + logger.info("sign-in ok email=%s from=%s", email, client_address()) return jsonify(_serialize_user(user)) @@ -310,12 +329,19 @@ async def device_login(): if user is None or not user.password_hash: dummy_verify(password) _sign_in_failed(email) + logger.warning("device-login failed (no such account) email=%s from=%s", email, client_address()) return jsonify({"error": "invalid email or password"}), 401 if not verify_password(password, user.password_hash): _sign_in_failed(email) + logger.warning("device-login failed (bad password) email=%s from=%s", email, client_address()) return jsonify({"error": "invalid email or password"}), 401 _sign_in_succeeded(email) row, token = await _issue_device_token(db, user.id, data.get("name") or "") + # A device token outlives the session that made it, so its creation is the + # most consequential thing on this blueprint. + logger.info( + "device token issued email=%s device=%s from=%s", email, row.name, client_address() + ) await db.commit() return jsonify({"token": token, "device": _serialize_device(row), "user": _serialize_user(user)}), 201 diff --git a/src/thoughtsync/config.py b/src/thoughtsync/config.py index c2001fe..0ecd7e5 100644 --- a/src/thoughtsync/config.py +++ b/src/thoughtsync/config.py @@ -15,6 +15,9 @@ class Config: If unset, a key is generated and persisted in the DB (see ``thoughtsync.settings.load_or_create_secret_key``), so sessions survive restarts with no volume required. + - ``THOUGHTSYNC_TRUSTED_PROXY_HOPS`` — how many proxies in front of this app may + be believed when reading ``X-Forwarded-For`` / ``X-Forwarded-Proto``. Defaults + to 1 (one reverse proxy terminating TLS). See ``trusted_proxy_hops``. Uploaded media lives under ``DATA_DIR`` — a fixed, authoritative path (``/var/thoughtsync``), intentionally NOT configurable (a mutable data path only @@ -48,3 +51,36 @@ class Config: def secret_key_env(cls) -> str | None: """Optional break-glass override for the cookie-signing secret.""" return os.environ.get("THOUGHTSYNC_SECRET_KEY") or None + + @classmethod + def trusted_proxy_hops(cls) -> int: + """How many proxies in front of this app may be believed. + + `X-Forwarded-For` grows LEFT to RIGHT: each hop appends the address it saw. + So the RIGHTMOST entry was written by our own proxy and is the address that + actually connected to it, while anything a client sent arrives to the LEFT of + that — which is why the leftmost entry, the "original client", is precisely + the one a caller can forge. + + With `n` trusted hops the real client is the nth entry from the right: + + 0 no proxy — ignore the header entirely, use the socket address + 1 one reverse proxy terminating TLS (the default, and this deployment) + 2 a CDN in front of that proxy — Cloudflare appended the client, our + proxy appended Cloudflare + + Set it to the number of proxies you actually run. Too HIGH and a caller can + forge an address by padding the header; too low and everyone behind the CDN + shares one bucket. Too low is the safe direction, so it is the fallback when + the header is shorter than configured. + + Env rather than the Settings UI (rule 25's "absolute bootstrap only" carve- + out): it is a property of the deployment topology, not a preference, and the + rate limiter consults it before opening a database connection — which is the + whole point of checking a throttle before doing expensive work. + """ + raw = os.environ.get("THOUGHTSYNC_TRUSTED_PROXY_HOPS", "1") + try: + return max(0, int(raw)) + except (TypeError, ValueError): + return 1 diff --git a/src/thoughtsync/proxy.py b/src/thoughtsync/proxy.py new file mode 100644 index 0000000..90fa73e --- /dev/null +++ b/src/thoughtsync/proxy.py @@ -0,0 +1,74 @@ +"""Reading what the proxies in front of this app say about a request. + +Two headers carry information the app cannot see for itself — who the client is +(`X-Forwarded-For`) and whether they arrived over TLS (`X-Forwarded-Proto`) — and both +are trusted by the same rule, so the rule lives in one place. Writing it twice is +precisely how issue 2183 happened: two places holding one decision, and only one of +them updated. + +## The rule + +A forwarding header grows LEFT to RIGHT. Each hop appends what IT saw, so the +rightmost entries are the ones our own infrastructure wrote, and anything a caller +sent arrives to the LEFT of those. + +That inverts the intuitive reading. The leftmost entry is nominally "the original +client" — and is exactly the one a caller can forge, by sending the header themselves. +So we count in from the right by the number of proxies we actually run +(`THOUGHTSYNC_TRUSTED_PROXY_HOPS`, default 1), and a forged prefix can never be +selected no matter how much of it there is. + +Too HIGH a hop count is the dangerous direction: it starts believing entries no proxy +of ours wrote. Too low just means several callers share a bucket. So when the header +is shorter than configured — fewer proxies than expected — we fall back to the socket +address rather than reaching further left. +""" +from __future__ import annotations + +from quart import has_request_context, request + +from .config import Config + + +def trusted_entry(header: str, hops: int) -> str | None: + """The nth-from-the-right entry of a forwarding header, or None if there isn't one. + + Pure, so the trust boundary is testable without a request context. + """ + if hops <= 0: + return None + entries = [part.strip() for part in header.split(",") if part.strip()] + if len(entries) < hops: + return None + return entries[-hops] + + +def forwarded_for(header: str, remote_addr: str | None, hops: int) -> str: + """The client address a proxy chain vouches for, else this connection's peer.""" + entry = trusted_entry(header, hops) + return (entry or remote_addr or "unknown")[:64] # bounded: becomes a dict key + + +def client_address() -> str: + """The caller's address, as far as the deployment's own proxies vouch for it.""" + return forwarded_for( + request.headers.get("X-Forwarded-For", ""), + request.remote_addr, + Config.trusted_proxy_hops(), + ) + + +def is_https() -> bool: + """Whether this request reached us over TLS — directly, or via a trusted proxy. + + Shared by the session cookie's `Secure` flag and by HSTS, because they are the same + question. Read with the same hop count as the address: a caller who sets + `X-Forwarded-Proto: https` on a plain-HTTP request puts it to the left of whatever + our proxy appended, so it is not what gets read. + """ + if not has_request_context(): + return False + if request.is_secure: + return True + entry = trusted_entry(request.headers.get("X-Forwarded-Proto", ""), Config.trusted_proxy_hops()) + return (entry or "").lower() == "https" diff --git a/src/thoughtsync/ratelimit.py b/src/thoughtsync/ratelimit.py index 30f59d0..afdeb98 100644 --- a/src/thoughtsync/ratelimit.py +++ b/src/thoughtsync/ratelimit.py @@ -22,9 +22,11 @@ came from, and either one can refuse it: is what stops credential stuffing against one known email, no matter how many addresses the attempts arrive from. - **The address** bounds the damage from one source spraying many accounts. It is - best-effort by nature — behind a reverse proxy the client address is read from - ``X-Forwarded-For``, which a caller can set to anything if the app is exposed - directly. That is precisely why it is not the only key. + read from ``X-Forwarded-For``, counting in from the RIGHT by + ``THOUGHTSYNC_TRUSTED_PROXY_HOPS`` so that only entries our own proxies wrote are + believed — a forged header lands to the left of those and is never selected. It is + still the weaker of the two keys, because it depends on that setting matching the + deployment; the account key depends on nothing. Counting is by failure for the sign-in routes and by attempt for registration: a correct password should never move someone closer to being locked out, but every @@ -35,7 +37,6 @@ from __future__ import annotations import time from collections import deque -from quart import request # Failed sign-ins tolerated per account before it stops answering, and for how long. # Ten is comfortably above a person mistyping a password and far below anything that @@ -116,24 +117,6 @@ sign_in_by_address = SlidingWindow(ADDRESS_LIMIT, ADDRESS_WINDOW_S) register_by_address = SlidingWindow(REGISTER_LIMIT, REGISTER_WINDOW_S) -def client_address() -> str: - """The caller's address, as well as it can be known. - - ``X-Forwarded-For`` is a list appended to by each hop, so the leftmost entry is - the original client — and also the only entry a client can choose for itself. - It is trusted here anyway, because the alternative behind a reverse proxy is to - see the proxy's address for every request on earth and rate-limit the entire - internet as one caller. The account-keyed limit is the one that holds when this - one is lied to. - """ - forwarded = request.headers.get("X-Forwarded-For", "") - if forwarded: - first = forwarded.split(",")[0].strip() - if first: - return first[:64] # bounded: this becomes a dict key - return (request.remote_addr or "unknown")[:64] - - def reset_all() -> None: """Drop every counter. For tests — nothing in the app calls this.""" for window in (sign_in_by_account, sign_in_by_address, register_by_address): diff --git a/tests/test_proxy.py b/tests/test_proxy.py new file mode 100644 index 0000000..6e25bc5 --- /dev/null +++ b/tests/test_proxy.py @@ -0,0 +1,68 @@ +"""The proxy trust boundary. + +The whole security property is "a caller cannot forge their own address", and it rests +on counting in from the RIGHT of the header rather than the left. These are the cases +that tell the two apart — pure functions, no request context, no database. +""" +from thoughtsync.config import Config +from thoughtsync.proxy import forwarded_for, trusted_entry + +PEER = "10.0.0.1" # the socket address: our own proxy, or the caller when unproxied + + +def test_default_is_one_hop(): + # One reverse proxy terminating TLS — this deployment, and the only shape that is + # safe to assume. A wrong default here is a silent security bug, not a preference. + assert Config.trusted_proxy_hops() == 1 + + +def test_no_proxy_ignores_the_header_entirely(): + # hops=0 says nothing in front of us appends anything, so the header can only be + # something a caller invented. + assert forwarded_for("1.2.3.4", PEER, 0) == PEER + + +def test_one_hop_reads_what_our_proxy_wrote(): + assert forwarded_for("203.0.113.7", PEER, 1) == "203.0.113.7" + + +def test_a_forged_prefix_is_never_selected(): + # THE test. A caller sends `X-Forwarded-For: 1.2.3.4`; our proxy appends the + # address it actually saw. Reading from the left would hand the caller a fresh + # rate-limit bucket for every value they invent. + assert forwarded_for("1.2.3.4, 203.0.113.7", PEER, 1) == "203.0.113.7" + # …and padding it doesn't help either. + assert forwarded_for("a, b, c, d, 203.0.113.7", PEER, 1) == "203.0.113.7" + + +def test_two_hops_sees_past_a_cdn(): + # Cloudflare appended the real client; our proxy appended Cloudflare. + assert forwarded_for("203.0.113.7, 172.16.0.5", PEER, 2) == "203.0.113.7" + assert forwarded_for("1.2.3.4, 203.0.113.7, 172.16.0.5", PEER, 2) == "203.0.113.7" + + +def test_a_short_header_falls_back_rather_than_reaching_left(): + # Fewer proxies than configured. Reaching further left would start believing + # entries no proxy of ours wrote, so the safe direction is the socket address — + # at worst several callers share one bucket. + assert forwarded_for("203.0.113.7", PEER, 2) == PEER + assert forwarded_for("", PEER, 1) == PEER + + +def test_malformed_headers_do_not_crash_or_leak_empties(): + assert forwarded_for(",,,", PEER, 1) == PEER + assert forwarded_for(" , 203.0.113.7 , ", PEER, 1) == "203.0.113.7" + + +def test_the_key_is_length_bounded(): + # It becomes a dict key in the limiter; an unbounded header must not become an + # unbounded allocation. + assert len(forwarded_for("x" * 5000, PEER, 1)) <= 64 + + +def test_trusted_entry_reports_absence_rather_than_guessing(): + # `is_https` needs to tell "no trusted entry" apart from "an entry saying http", + # which is why this returns None rather than a default. + assert trusted_entry("", 1) is None + assert trusted_entry("https", 0) is None + assert trusted_entry("http, https", 1) == "https" diff --git a/tests/test_ratelimit.py b/tests/test_ratelimit.py index 912439c..224d64a 100644 --- a/tests/test_ratelimit.py +++ b/tests/test_ratelimit.py @@ -128,21 +128,10 @@ async def test_register_is_throttled_by_address(app): resp = await client.post( "/api/auth/register", json={"email": "new@example.com", "password": "a-long-enough-password"}, + # One entry, so with the default single trusted hop this IS the address the + # limiter keys on. The forged-prefix cases live in test_proxy.py. headers={"X-Forwarded-For": "203.0.113.9"}, ) assert resp.status_code == 429 -async def test_client_address_prefers_the_forwarded_client(app): - # Behind a reverse proxy, remote_addr is the PROXY for every request on earth — - # keying on it would rate-limit the entire internet as one caller. The leftmost - # X-Forwarded-For entry is the original client. - async with app.test_request_context("/", headers={"X-Forwarded-For": "198.51.100.4, 10.0.0.1"}): - assert ratelimit.client_address() == "198.51.100.4" - - -async def test_client_address_falls_back_to_the_peer(app): - async with app.test_request_context("/"): - # No proxy header: whatever the peer address is, it must be a usable key - # rather than an empty string sharing one bucket with everyone. - assert ratelimit.client_address()