From c5a8f170da4638e7a9e322c4edc7c06d7c9ef9e0 Mon Sep 17 00:00:00 2001 From: stroblme Date: Tue, 6 Oct 2026 12:15:58 +0200 Subject: [PATCH] Count per-address limits on the hop a trusted proxy added; limit signup The limits read the leftmost X-Forwarded-For entry, which the client writes, and the LAN nginx appended to it. The address now comes from uvicorn's proxy-headers middleware (rightmost hop not in FORWARDED_ALLOW_IPS), applied in the app too so tunnel-replayed calls resolve the same way; nginx sets the header to $remote_addr and the compose backend trusts the Docker networks. /users/signup gets a per-address limit of 10 an hour. --- backend/fluksio/api/routes/oauth.py | 13 +++++------ backend/fluksio/api/routes/panels.py | 9 ++++---- backend/fluksio/api/routes/users.py | 16 ++++++++++++-- backend/fluksio/core/config.py | 4 ++++ backend/fluksio/main.py | 9 ++++++++ backend/tests/api/routes/test_panels.py | 10 ++++++--- backend/tests/api/routes/test_users.py | 29 +++++++++++++++++++++++++ docker/compose.yml | 5 +++++ docker/nginx-api-proxy.conf | 5 ++++- docs/reference/configuration.md | 1 + 10 files changed, 82 insertions(+), 19 deletions(-) diff --git a/backend/fluksio/api/routes/oauth.py b/backend/fluksio/api/routes/oauth.py index f69f512..fe4b797 100644 --- a/backend/fluksio/api/routes/oauth.py +++ b/backend/fluksio/api/routes/oauth.py @@ -107,16 +107,13 @@ def _too_many(bucket: str, limit: int, window: float, record: bool = True) -> bo def _client_ip(request: Request) -> str: - """The caller's address, or the proxy's if it did not forward one. + """The caller's address, as the proxy-headers middleware resolved it. - Behind Traefik `request.client.host` is the proxy for every request, which - made every per-address limit here one global bucket — so one caller could - lock out everyone. The leftmost forwarded entry is the client, and is what - the panel pairing screen already reads. + That is the rightmost forwarded hop not added by a trusted proxy + (`FORWARDED_ALLOW_IPS`). The leftmost one, which this used to read, is + whatever the client wrote, so every limit keyed on it could be dodged by + sending a new one each time. """ - forwarded = request.headers.get("x-forwarded-for", "").split(",")[0].strip() - if forwarded: - return forwarded return request.client.host if request.client else "unknown" diff --git a/backend/fluksio/api/routes/panels.py b/backend/fluksio/api/routes/panels.py index 5ec9e25..56ef9c7 100644 --- a/backend/fluksio/api/routes/panels.py +++ b/backend/fluksio/api/routes/panels.py @@ -213,11 +213,10 @@ def _describe(request: Request) -> str: # it reads badly on the approval screen. """ agent = request.headers.get("user-agent", "").strip()[:120] - forwarded = request.headers.get("x-forwarded-for", "").split(",")[0].strip() - # Proxied requests are replayed into this process over an ASGI transport, - # which reports every caller as localhost; the forwarded address is the - # only true one there, and behind the local reverse proxy it is too. - address = forwarded or (request.client.host if request.client else "") + # Resolved from X-Forwarded-For by the proxy-headers middleware, which is + # what makes a call replayed off the tunnel show the device's address + # rather than localhost — and stops a device naming any address it likes. + address = request.client.host if request.client else "" return " · ".join(part for part in (agent or "Unknown device", address) if part) diff --git a/backend/fluksio/api/routes/users.py b/backend/fluksio/api/routes/users.py index 6f47d7f..f26f4f0 100644 --- a/backend/fluksio/api/routes/users.py +++ b/backend/fluksio/api/routes/users.py @@ -1,7 +1,7 @@ import uuid from typing import Annotated, Any -from fastapi import APIRouter, Depends, HTTPException, Query +from fastapi import APIRouter, Depends, HTTPException, Query, Request from sqlmodel import col, func, select from fluksio import crud @@ -11,6 +11,7 @@ from fluksio.api.deps import ( get_current_active_superuser, ) from fluksio.api.routes.cloud import forget_remote_user +from fluksio.api.routes.oauth import too_many from fluksio.core.config import settings from fluksio.core.security import get_password_hash, verify_password from fluksio.models import ( @@ -28,6 +29,11 @@ from fluksio.utils import generate_new_account_email, send_email router = APIRouter(prefix="/users", tags=["users"]) +#: Sign-ups one address may attempt per window, once `SIGNUP_ENABLED` opens +#: the route to anyone who reaches it. +SIGNUP_ATTEMPTS = 10 +SIGNUP_WINDOW_S = 3600.0 + @router.get( "/", @@ -150,12 +156,18 @@ def delete_user_me(session: SessionDep, current_user: CurrentUser) -> Any: @router.post("/signup", response_model=UserPublic) -def register_user(session: SessionDep, user_in: UserRegister) -> Any: +def register_user(request: Request, session: SessionDep, user_in: UserRegister) -> Any: """ Create new user without the need to be logged in. """ if not settings.SIGNUP_ENABLED: raise HTTPException(status_code=403, detail="Signup is disabled") + # Every attempt counts, not only the refused ones: a successful one is + # an account, and an Argon2 hash on the shared threadpool. + if too_many(request, "signup", SIGNUP_ATTEMPTS, SIGNUP_WINDOW_S): + raise HTTPException( + status_code=429, detail="Too many sign-ups from here. Try again later." + ) user = crud.get_user_by_email(session=session, email=user_in.email) if user: raise HTTPException( diff --git a/backend/fluksio/core/config.py b/backend/fluksio/core/config.py index efe77c4..be60e8c 100644 --- a/backend/fluksio/core/config.py +++ b/backend/fluksio/core/config.py @@ -55,6 +55,10 @@ class Settings(BaseSettings): ACCESS_TOKEN_EXPIRE_MINUTES: int = 60 * 24 * 8 FRONTEND_HOST: str = "http://localhost:5173" ENVIRONMENT: Literal["local", "staging", "production"] = "local" + #: The proxies whose X-Forwarded-For is believed: uvicorn's own setting, + #: read here too so a call replayed off the portal tunnel resolves its + #: caller the same way. IPs or networks, comma-separated; `*` trusts all. + FORWARDED_ALLOW_IPS: str = "127.0.0.1" #: Everything this instance keeps: the database, the flow repository, #: secrets, artifacts and the user venv. The paths below derive from it diff --git a/backend/fluksio/main.py b/backend/fluksio/main.py index 6c32e34..695d546 100644 --- a/backend/fluksio/main.py +++ b/backend/fluksio/main.py @@ -16,6 +16,7 @@ from fastapi.responses import JSONResponse from fastapi.routing import APIRoute from fluksio_worker.worker_main import ARTIFACT_DIR_ENV, ARTIFACT_VOLATILE_DIR_ENV from starlette.middleware.cors import CORSMiddleware +from uvicorn.middleware.proxy_headers import ProxyHeadersMiddleware from fluksio import __version__ from fluksio.api.main import api_router @@ -433,6 +434,14 @@ if settings.all_cors_origins: allow_headers=["*"], ) +# The caller's address, for every per-address limit: the rightmost +# X-Forwarded-For hop that is not a trusted proxy. uvicorn does this for what +# it serves, but a call the portal tunnel replays arrives over an in-process +# transport as 127.0.0.1 and never passes through it. Doing it again here +# covers that, with uvicorn's own semantics, and is a no-op on a request +# uvicorn already resolved. Added last, so it is the outermost layer. +app.add_middleware(ProxyHeadersMiddleware, trusted_hosts=settings.FORWARDED_ALLOW_IPS) + app.include_router(api_router, prefix=settings.API_V1_STR) diff --git a/backend/tests/api/routes/test_panels.py b/backend/tests/api/routes/test_panels.py index e4f29cd..c4d2785 100644 --- a/backend/tests/api/routes/test_panels.py +++ b/backend/tests/api/routes/test_panels.py @@ -4,6 +4,7 @@ from fastapi.testclient import TestClient from fluksio.cloud import config as cloud_config from fluksio.core.config import settings +from fluksio.main import app PREFIX = f"{settings.API_V1_STR}/panels" DASHBOARDS = f"{settings.API_V1_STR}/dashboards" @@ -680,11 +681,13 @@ def test_the_device_asking_is_named_before_anyone_approves( client: TestClient, superuser_token_headers: dict[str, str] ) -> None: """Approving a code adopts whatever holds it, so it is worth a look.""" - started = client.post( + # Behind a trusted proxy (127.0.0.1 by default), as the tunnel replays it. + proxied = TestClient(app, client=("127.0.0.1", 50000)) + started = proxied.post( f"{PREFIX}/pair", headers={ "user-agent": "Mozilla/5.0 (X11; CrOS aarch64)", - "x-forwarded-for": "203.0.113.7, 10.0.0.1", + "x-forwarded-for": "198.51.100.1, 203.0.113.7", }, ).json() @@ -693,8 +696,9 @@ def test_the_device_asking_is_named_before_anyone_approves( ) assert looked.status_code == 200 assert "CrOS" in looked.json()["device"] - # The first hop, not the proxy that relayed it. + # The hop the proxy added, not the one the device wrote in front of it. assert "203.0.113.7" in looked.json()["device"] + assert "198.51.100.1" not in looked.json()["device"] assert looked.json()["remote"] is False # Nobody without an account gets to enumerate what is waiting. diff --git a/backend/tests/api/routes/test_users.py b/backend/tests/api/routes/test_users.py index 162ea35..0d38b20 100644 --- a/backend/tests/api/routes/test_users.py +++ b/backend/tests/api/routes/test_users.py @@ -5,8 +5,11 @@ from fastapi.testclient import TestClient from sqlmodel import Session, select from fluksio import crud +from fluksio.api.routes import oauth +from fluksio.api.routes.users import SIGNUP_ATTEMPTS from fluksio.core.config import settings from fluksio.core.security import verify_password +from fluksio.main import app from fluksio.models import User, UserCreate from tests.utils.user import create_random_user from tests.utils.utils import random_email, random_lower_string @@ -373,6 +376,32 @@ def test_register_user_already_exists_error(client: TestClient) -> None: assert r.json()["detail"] == "The user with this email already exists in the system" +def test_signup_is_limited_per_address_behind_a_proxy() -> None: + """The limit counts the hop the trusted proxy added, not one the client wrote. + + The leftmost hop is the client's own to fill in, so keying on it let one + caller look like a new one on every attempt. + """ + proxied = TestClient(app, client=("127.0.0.1", 50000)) # a trusted proxy + url = f"{settings.API_V1_STR}/users/signup" + taken = {"email": settings.FIRST_SUPERUSER, "password": random_lower_string()} + + def attempt(forwarded: str) -> int: + return proxied.post( + url, json=taken, headers={"x-forwarded-for": forwarded} + ).status_code + + try: + with patch("fluksio.core.config.settings.SIGNUP_ENABLED", True): + for n in range(SIGNUP_ATTEMPTS): + assert attempt(f"198.51.100.{n}, 203.0.113.7") == 400 + assert attempt("198.51.100.99, 203.0.113.7") == 429 + assert attempt("203.0.113.8") == 400 + finally: + for address in ("203.0.113.7", "203.0.113.8"): + oauth._hits.pop(f"signup:{address}", None) + + def test_update_user( client: TestClient, superuser_token_headers: dict[str, str], db: Session ) -> None: diff --git a/docker/compose.yml b/docker/compose.yml index 160e594..f78ed61 100644 --- a/docker/compose.yml +++ b/docker/compose.yml @@ -98,6 +98,11 @@ services: - FRONTEND_HOST=${FRONTEND_HOST?Variable not set} - ENVIRONMENT=${ENVIRONMENT} - BACKEND_CORS_ORIGINS=${BACKEND_CORS_ORIGINS} + # Whose X-Forwarded-For is believed: Traefik's and the frontend's nginx, + # which reach this from the compose networks (Docker's default pools sit + # in 172.16.0.0/12). Both write the header themselves, so the hop they + # add is the caller. No port is published here, so nothing else can. + - FORWARDED_ALLOW_IPS=${FORWARDED_ALLOW_IPS:-127.0.0.1,172.16.0.0/12} - SECRET_KEY=${SECRET_KEY?Variable not set} - FIRST_SUPERUSER=${FIRST_SUPERUSER?Variable not set} - FIRST_SUPERUSER_PASSWORD=${FIRST_SUPERUSER_PASSWORD?Variable not set} diff --git a/docker/nginx-api-proxy.conf b/docker/nginx-api-proxy.conf index cf93bdc..1e0cd10 100644 --- a/docker/nginx-api-proxy.conf +++ b/docker/nginx-api-proxy.conf @@ -13,7 +13,10 @@ location /api { proxy_http_version 1.1; proxy_set_header Host $host; - proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; + # Replaced, not appended to: whatever the client sent would otherwise sit + # in front of the one hop the backend believes, and anything reading the + # header naively would take it for the caller. + proxy_set_header X-Forwarded-For $remote_addr; proxy_set_header X-Forwarded-Proto $scheme; # The flow websocket (/api/v1/flows/ws) upgrades through here; passing the # client's own Connection header covers both it and ordinary requests. diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index d072ad4..8748945 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -73,6 +73,7 @@ stands. | `DOMAIN` | `localhost` | what the API and OAuth issuer are built from | | `FRONTEND_HOST` | `http://localhost:5173` | used in mails, OAuth metadata and panel pairing links | | `BACKEND_CORS_ORIGINS` | `[]` | comma-separated; `FRONTEND_HOST` is always allowed | +| `FORWARDED_ALLOW_IPS` | `127.0.0.1` (`127.0.0.1,172.16.0.0/12` in the compose stack) | proxies whose `X-Forwarded-For` is believed, as uvicorn reads it; the sign-in, sign-up and OAuth limits count the rightmost hop none of them added | ## Where the interface looks for the API