diff --git a/.env.app.example b/.env.app.example index b939476a..dd8b9b91 100644 --- a/.env.app.example +++ b/.env.app.example @@ -57,6 +57,9 @@ KPUBDATA_BUILDER_CREDENTIAL_MASTER_KEY=replace-with-urlsafe-base64-32-bytes # OIDC_ALLOWED_SUBJECTS= # KPUBDATA_BUILDER_ADMIN_SUBJECTS=https://id.example.com/realms/kpubdata| # KPUBDATA_BUILDER_AUTH_FAILURE_LIMIT= +# 리버스 프록시(Caddy) 뒤에서 인증 실패 스로틀이 사용자별로 세게 하려면 Builder 가 보는 +# 프록시의 주소·대역을 적는다. 비워 두면 X-Forwarded-For 를 읽지 않는다(#1031, deploy.md). +# KPUBDATA_BUILDER_TRUSTED_PROXIES= # --- Caddy 공개 진입 (--profile caddy 사용 시) ------------------------------ # 공개 도메인. Cloudflare proxied DNS가 이 호스트의 80/443으로 향한다. diff --git a/CHANGELOG.md b/CHANGELOG.md index 4338f156..33f15862 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- The authentication failure throttle no longer locks everyone behind a proxy out together (#1031). It keyed on the TCP peer, which behind a reverse proxy is the proxy for every user, and it counted an expired token as a failure, so ordinary token expiry across users reached the limit and all of them got 429 `auth_throttled`. Two changes: an expired token (`code: token_expired`) is no longer counted — it verified and is only old; and `KPUBDATA_BUILDER_TRUSTED_PROXIES` (addresses or CIDR blocks, unset by default) names the proxies whose `X-Forwarded-For` is read, from the right, so each client behind them has its own allowance. Without the setting the header is still never read. The production compose passes the variable through and sets no value. - A dataset whose terms say `redistribution: allowed` without a confirmed attribution is not published publicly (#1030). The publish gate read only `license.redistribution`. kpubdata's rule is that a term nobody confirmed must not read as permission (kpubdata#785), and the `attribution` text is what shows the terms were read from the provider's page; kpubdata `main` still carries 16 specs that say `allowed` without it (`scripts/unconfirmed_terms_baseline.txt`). The gate now reads the attribution too: `allowed` without it is `unknown`, so a public publish is refused with `redistribution_unknown` and the reason says why. Private publishing and reading are unaffected, as for every other `unknown`. With the pinned kpubdata 0.8.0 nothing changes yet — none of those specs declares its terms in that release. - A credential in a `url` source's endpoint is not written to the spec snapshot (#1029). The endpoint was recorded as given, so a data.go.kr-style URL pasted whole left `?serviceKey=…` in `buildspec.yaml` and in `GET /builds/{run_id}/spec`. The value of a query parameter whose name is a credential name — the same set `params` uses, `key` included — is now ``, and userinfo (`https://user:password@host/`) is redacted whole. **Digest:** a `url` spec whose endpoint carried such a parameter or userinfo hashes differently from now on, and two specs that differ only in that credential share a digest; an endpoint with nothing to redact is recorded byte for byte and keeps its digest. Snapshots already written are not rewritten. - A run id whose only record is an interrupted submission is not free to another user (#1025). `check_existing_run_access` looked at the manifest and the job registry; a run a restart interrupted has neither, so another user could build under its id, read the first submitter's events while their job ran, and leave their own failure to the first submitter if interrupted in turn. The guard now reads the submission record #996 added: someone else gets 403 `forbidden: not run owner`, and the submitter may use the id again. Today this guards the synchronous `POST /build`; `POST /builds` gets it when #1008 routes that path through the same guard. diff --git a/docker-compose.prod.app.yml b/docker-compose.prod.app.yml index bd08b286..9ca3a60c 100644 --- a/docker-compose.prod.app.yml +++ b/docker-compose.prod.app.yml @@ -66,6 +66,7 @@ services: OIDC_ALLOWED_EMAILS: ${OIDC_ALLOWED_EMAILS:-} KPUBDATA_BUILDER_ADMIN_SUBJECTS: ${KPUBDATA_BUILDER_ADMIN_SUBJECTS:-} KPUBDATA_BUILDER_AUTH_FAILURE_LIMIT: ${KPUBDATA_BUILDER_AUTH_FAILURE_LIMIT:-} + KPUBDATA_BUILDER_TRUSTED_PROXIES: ${KPUBDATA_BUILDER_TRUSTED_PROXIES:-} ports: # 기본은 루프백이다. 예전 기본값("8000")은 모든 인터페이스에 공개돼서, # Caddy 없이 올리면 TLS 없는 8000 으로 API 키가 평문으로 오갔다. 헬스체크는 diff --git a/docs/deploy.md b/docs/deploy.md index b2de4e7b..b6e6b01a 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -313,21 +313,49 @@ docker compose -f docker-compose.prod.app.yml --profile caddy up -d # 공개 TL ## 인증 실패 스로틀 -인증 게이트는 클라이언트별(TCP peer 주소) 인증 실패를 슬라이딩 윈도로 세고, 한도를 +인증 게이트는 클라이언트별(TCP peer 주소, 또는 신뢰하는 프록시가 알려 준 주소) 인증 실패를 슬라이딩 윈도로 세고, 한도를 넘으면 인증을 시도하기 전에 `429`(`code: "auth_throttled"`, `retry_after_seconds`)로 끊는다. 정적 API 키 추측과 무효 토큰 서명 검증 CPU 소모를 공짜로 반복하지 못하게 하는 -것이 목적이다. 인증에 성공하면 그 클라이언트의 실패 기록은 즉시 비워지므로, 토큰 만료로 -몇 번 401을 받는 정상 사용자는 누적되지 않는다. +것이 목적이다. 인증에 성공하면 그 클라이언트의 실패 기록은 즉시 비워진다. - `KPUBDATA_BUILDER_AUTH_FAILURE_LIMIT` (기본 `60`, `0` 이하면 비활성) - `KPUBDATA_BUILDER_AUTH_FAILURE_WINDOW_SECONDS` (기본 `60`) +- `KPUBDATA_BUILDER_TRUSTED_PROXIES` (기본 미설정) — 아래 "리버스 프록시 뒤에서". - 401만 센다 — 403(유효 토큰의 인가 실패)과 503(JWKS 일시 장애)은 카운트하지 않는다. +- **만료된 토큰(`code: "token_expired"`)은 401 이지만 세지 않는다**(#1031). 서명은 + 검증됐고 오래됐을 뿐이며, 클라이언트가 할 일은 토큰 갱신이다. 세면 한 프록시 뒤 여러 + 사용자의 평범한 만료만으로 한도에 닿는다. - `/healthz`는 인증 게이트 밖이라 스로틀과 무관하게 항상 응답한다. -> **리버스 프록시 주의**: 식별자는 TCP peer 주소이며 `X-Forwarded-For`는 위조 가능하므로 -> 읽지 않는다. 클라이언트 IP를 보존하지 않는 프록시/로드밸런서 뒤에 있으면 모든 요청이 -> 한 버킷을 공유해 한 클라이언트의 실패가 다른 사용자에게 영향을 준다 — 그런 배포에서는 -> 한도를 `0`으로 두어 비활성화하고 프록시 계층에서 스로틀을 거는 편이 낫다. +### 리버스 프록시 뒤에서 + +기본 식별자는 TCP peer 주소다. `X-Forwarded-For`는 누구나 써 보낼 수 있으므로 기본으로는 +읽지 않는다. 그래서 프록시 뒤에서는 모든 사용자가 프록시의 주소로 도착해 **한 버킷을 +공유**한다 — 몇 사람의 실패가 모두를 429 로 막는다(#1031). + +`KPUBDATA_BUILDER_TRUSTED_PROXIES` 에 프록시의 주소나 CIDR 블록을 콤마로 적으면, TCP +peer 가 그중 하나일 때에만 `X-Forwarded-For` 를 읽는다. 오른쪽부터 읽어 신뢰하는 프록시가 +아닌 첫 주소를 클라이언트로 본다 — 프록시가 덧붙인 부분만 쓰고, 클라이언트가 직접 써 +보낸 왼쪽 부분에는 닿지 않는다. 주소로 읽히지 않는 항목은 경고 로그(값이 아니라 몇 번째 항목인지)와 함께 버린다(호스트 +이름은 쓸 수 없다). + +```bash +# compose 의 Caddy 가 Builder 와 같은 Docker 네트워크에 있을 때: 그 네트워크의 대역 +KPUBDATA_BUILDER_TRUSTED_PROXIES=172.18.0.0/16 +``` + +- 적을 값은 **Builder 가 보는 프록시의 주소**다. compose 에서는 Caddy 컨테이너가 붙은 + 네트워크의 대역이다(`docker network inspect kpubdata-builder-app-net` 의 `Subnet`). +- **프록시가 여러 겹이면 각 겹이 앞 겹을 신뢰해야 한다.** Cloudflare → Caddy → Builder + 에서 Caddy 는 기본적으로 앞단을 신뢰하지 않고 `X-Forwarded-For` 를 자기가 본 peer + (Cloudflare 의 주소)로 바꿔 쓴다. 그 상태로 이 변수만 켜면 Builder 는 사용자 대신 + Cloudflare 엣지 주소로 묶는다. 실제 클라이언트 주소까지 내려오게 하려면 Caddy 의 + `trusted_proxies` 에 Cloudflare 대역을 적어야 한다 — 저장소의 `ops/caddy/Caddyfile` 은 + 그렇게 설정돼 있지 않다. +- Builder 포트가 프록시를 거치지 않고도 닿는 배포에서는 이 변수에 그 경로의 주소가 + 들어가지 않게 한다. 신뢰하는 주소에서 온 요청은 헤더로 자기 식별자를 정할 수 있다. +- 설정하지 않으면 동작은 전과 같다. 프록시가 클라이언트 주소를 넘기지 않는 배포에서는 + 여전히 한도를 `0`으로 두고 프록시 계층에서 스로틀을 거는 편이 낫다. > > 카운터는 프로세스 로컬이다. 인스턴스를 여러 개 띄우면 인스턴스별로 센다(정확한 전역 > 한도가 아니라 남용 완화가 목적). diff --git a/docs/deployment.md b/docs/deployment.md index 06f4ed8d..657e7aef 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -13,6 +13,7 @@ | `KPUBDATA_BUILDER_ALLOWED_ORIGINS` | CORS 허용 오리진 (콤마 구분, default-deny). 응답에는 항상 `Vary: Origin`이 붙는다 | 미설정 | 선택 | | `KPUBDATA_BUILDER_AUTH_FAILURE_LIMIT` | 윈도당 허용할 인증 실패 횟수(클라이언트 IP별). 초과분은 `429 auth_throttled`. `0` 이하면 비활성 | `60` | 선택 | | `KPUBDATA_BUILDER_AUTH_FAILURE_WINDOW_SECONDS` | 인증 실패 카운트 윈도(초) | `60` | 선택 | +| `KPUBDATA_BUILDER_TRUSTED_PROXIES` | 리버스 프록시의 주소나 CIDR 블록(콤마 구분). TCP peer 가 그중 하나일 때에만 `X-Forwarded-For` 로 인증 실패 스로틀의 클라이언트를 가린다([deploy.md](deploy.md) "리버스 프록시 뒤에서") | 미설정 (헤더를 읽지 않음) | 선택 | | `KPUBDATA_BUILDER_CREDENTIAL_MASTER_KEY` | 사용자별 Provider credential AES-GCM master key (URL-safe base64 32 bytes) | 미설정 | credential CRUD 사용 시 필수 | | `KPUBDATA_BUILDER_ADMIN_SUBJECTS` | 관리자로 대우할 `\|` 목록(쉼표 구분, #679). 관리 엔드포인트(`GET /admin/runs`, `GET /admin/config`)를 열지만 남의 run 산출물은 열지 않는다. **issuer 를 반드시 함께 적는다** — `sub` 는 issuer 안에서만 유일하고 `OIDC_ISSUER` 는 복수를 허용한다. issuer 없는 항목은 경고와 함께 무시된다. OIDC 배포에서는 이 변수를 컨테이너까지 전달해야 한다 | 미설정 | 다중 사용자 배포 시 선택 | | `KPUBDATA_BUILDER_PROVIDER_TEST_TIMEOUT` | Provider connection test 전송 timeout(초) | `10` | 선택 | diff --git a/src/kpubdata_builder/service/app.py b/src/kpubdata_builder/service/app.py index a9086326..b5bdaaee 100644 --- a/src/kpubdata_builder/service/app.py +++ b/src/kpubdata_builder/service/app.py @@ -1459,8 +1459,9 @@ def _dispatch_impl( if isinstance(principal, AuthError): # Count only 401 (invalid credentials) — 403 is valid token with authz # failure (not worth throttling), 503 is JWKS transient outage (not client - # fault). - if principal.status_code == 401: + # fault). An expired token is a 401 too, but its signature verified: it is + # not a guess, and counting it lets ordinary expiry reach the limit (#1031). + if principal.status_code == 401 and not principal.expired: service._auth_throttle.record_failure(client_id) return ServiceResponse( principal.status_code, {"error": principal.reason, "code": principal.code} diff --git a/src/kpubdata_builder/service/auth_throttle.py b/src/kpubdata_builder/service/auth_throttle.py index 7f2b3cc0..a04376cc 100644 --- a/src/kpubdata_builder/service/auth_throttle.py +++ b/src/kpubdata_builder/service/auth_throttle.py @@ -15,23 +15,38 @@ Default is generous, unreachable by normal clients (60 failures within 60 seconds). To adjust or disable, use ``KPUBDATA_BUILDER_AUTH_FAILURE_LIMIT`` (≤0 disables). -**Client identification is TCP peer address.** ``X-Forwarded-For`` is forgeable, so we -don't read it. So if Builder is behind a reverse proxy that doesn't preserve client IP, -all requests share one bucket — in such deployments, better to gate throttle at proxy -layer and disable it here (see deploy.md). +**Client identification is TCP peer address**, unless the deployment names its reverse +proxy. ``X-Forwarded-For`` is forgeable, so it is not read by default: behind a proxy +every user then arrives from the proxy's address and shares one bucket (#1031). +``KPUBDATA_BUILDER_TRUSTED_PROXIES`` (comma-separated addresses or CIDR blocks) says +which peers are proxies. Only when the TCP peer is one of them is the header read, and +then from the right: the rightmost address that is not itself a trusted proxy is the +client — the part of the header the trusted proxy wrote, not the part the client sent. + +**An expired token is not a failure.** Its signature verified; it is only old, and the +client's next step is to refresh it. Counting it let ordinary expiry across the users +behind one proxy lock everyone out (#1031). The caller leaves it out. """ from __future__ import annotations +import ipaddress +import logging import math import os import threading import time from collections import deque -from collections.abc import Callable +from collections.abc import Callable, Sequence + +_logger = logging.getLogger(__name__) _LIMIT_ENV = "KPUBDATA_BUILDER_AUTH_FAILURE_LIMIT" _WINDOW_ENV = "KPUBDATA_BUILDER_AUTH_FAILURE_WINDOW_SECONDS" +TRUSTED_PROXIES_ENV = "KPUBDATA_BUILDER_TRUSTED_PROXIES" + +_Network = ipaddress.IPv4Network | ipaddress.IPv6Network +_Address = ipaddress.IPv4Address | ipaddress.IPv6Address _DEFAULT_LIMIT = 60 _DEFAULT_WINDOW_SECONDS = 60.0 @@ -63,6 +78,65 @@ def _positive_float_env(name: str, default: float) -> float: return value if value > 0 else default +def _address(text: str | None) -> _Address | None: + """``text`` as an address, an IPv4-mapped IPv6 one as its IPv4; None if it is not one.""" + if not text: + return None + try: + address = ipaddress.ip_address(text.strip()) + except ValueError: + return None + if isinstance(address, ipaddress.IPv6Address) and address.ipv4_mapped is not None: + return address.ipv4_mapped + return address + + +def parse_trusted_proxies(raw: str) -> tuple[_Network, ...]: + """The networks ``raw`` names. An entry that is not an address or CIDR block is + dropped with a warning: startup does not fail, and what is left trusts less, not + more.""" + networks: list[_Network] = [] + for position, entry in enumerate(raw.split(","), start=1): + entry = entry.strip() + if not entry: + continue + try: + networks.append(ipaddress.ip_network(entry, strict=False)) + except ValueError: + # The position, not the text: a variable set by mistake to something else + # must not end up in the log. + _logger.warning( + "KPUBDATA_BUILDER_TRUSTED_PROXIES: ignoring entry %d — not an address " + "or CIDR block", + position, + ) + return tuple(networks) + + +def client_identity( + peer: str | None, forwarded_for: Sequence[str], trusted: Sequence[_Network] +) -> str | None: + """Who to count a request against: the TCP peer, or the client a trusted proxy names. + + ``forwarded_for`` is every ``X-Forwarded-For`` header value, in order. It is read only + when ``peer`` is in ``trusted``, and from the right, skipping trusted proxies: each + proxy appends the address it saw, so what a client put in the header itself sits to + the left of what the trusted proxy wrote and is never reached. If the proxy wrote + nothing usable, the peer is the identity — as without the setting. + """ + peer_address = _address(peer) + if peer_address is None or not any(peer_address in network for network in trusted): + return peer + hops = [hop for value in forwarded_for for hop in value.split(",")] + for hop in reversed(hops): + address = _address(hop) + if address is None: + return peer + if not any(address in network for network in trusted): + return str(address) + return peer + + class AuthFailureThrottle: """In-process throttle counting authentication failures per-client in sliding window. @@ -78,6 +152,7 @@ def __init__( window_seconds: float | None = None, max_clients: int = _DEFAULT_MAX_CLIENTS, time_source: Callable[[], float] = time.monotonic, + trusted_proxies: Sequence[_Network] | None = None, ) -> None: self._limit = limit if limit is not None else _positive_int_env(_LIMIT_ENV, _DEFAULT_LIMIT) self._window = ( @@ -87,6 +162,11 @@ def __init__( ) self._max_clients = max_clients self._now = time_source + self._trusted_proxies = ( + tuple(trusted_proxies) + if trusted_proxies is not None + else parse_trusted_proxies(os.environ.get(TRUSTED_PROXIES_ENV, "")) + ) self._failures: dict[str, deque[float]] = {} self._lock = threading.Lock() @@ -95,6 +175,10 @@ def enabled(self) -> bool: """If limit ≤0, disabled — all calls become no-op.""" return self._limit > 0 + def client_id(self, peer: str | None, forwarded_for: Sequence[str] = ()) -> str | None: + """The identity to count this request against (see :func:`client_identity`).""" + return client_identity(peer, forwarded_for, self._trusted_proxies) + def retry_after(self, client_id: str | None) -> int | None: """If client should be blocked now, return remaining wait time (seconds, ceiling). @@ -160,4 +244,9 @@ def _evict_if_needed(self, now: float) -> None: del self._failures[oldest] -__all__ = ["AuthFailureThrottle"] +__all__ = [ + "TRUSTED_PROXIES_ENV", + "AuthFailureThrottle", + "client_identity", + "parse_trusted_proxies", +] diff --git a/src/kpubdata_builder/service/http.py b/src/kpubdata_builder/service/http.py index fe28168a..d02e931a 100644 --- a/src/kpubdata_builder/service/http.py +++ b/src/kpubdata_builder/service/http.py @@ -302,9 +302,14 @@ def _dispatch(self, method: str) -> None: publish_credential_headers=( self.headers.get_all(PUBLISH_CREDENTIAL_HEADER) or [] ), - # Client ID for auth failure throttling. Use TCP peer address only; - # don't read X-Forwarded-For — headers can be forged. - client_id=self.client_address[0] if self.client_address else None, + # Client ID for auth failure throttling. The TCP peer address, + # unless that peer is a proxy the deployment names: only then is + # X-Forwarded-For read, and only the part that proxy wrote — a + # header from anyone else can be forged (#1031). + client_id=service._auth_throttle.client_id( + self.client_address[0] if self.client_address else None, + self.headers.get_all("X-Forwarded-For") or [], + ), ) except Exception: _logger.error( diff --git a/tests/unit/test_auth_throttle_proxy.py b/tests/unit/test_auth_throttle_proxy.py new file mode 100644 index 00000000..6ca5e65f --- /dev/null +++ b/tests/unit/test_auth_throttle_proxy.py @@ -0,0 +1,214 @@ +"""The failure throttle behind a reverse proxy, and expired tokens (#1031). + +The throttle keyed every request on the TCP peer. Behind the proxy the compose ships, +that is the proxy's address for every user, so they shared one bucket; and an expired +token — verified, only old — counted as a failure. A few users whose tokens had expired +could lock everyone out. +""" + +from __future__ import annotations + +import json +import threading +import urllib.error +import urllib.request +from collections.abc import Iterable +from http.server import HTTPServer +from pathlib import Path + +import pytest + +from kpubdata_builder.service import app as app_module +from kpubdata_builder.service.app import BuilderService, ServiceResponse, dispatch +from kpubdata_builder.service.auth import AuthError +from kpubdata_builder.service.auth_throttle import ( + TRUSTED_PROXIES_ENV, + AuthFailureThrottle, + client_identity, + parse_trusted_proxies, +) +from kpubdata_builder.service.http import make_handler + +from .test_service import _FakeClient + +_PROXY = "172.18.0.2" +_TRUSTED = parse_trusted_proxies("172.18.0.0/16") + + +class TestClientIdentity: + def test_without_a_trusted_proxy_the_header_is_never_read(self) -> None: + assert client_identity(_PROXY, ["203.0.113.1"], ()) == _PROXY + + def test_a_peer_that_is_not_the_proxy_cannot_name_itself(self) -> None: + # Someone reaching Builder directly sends whatever header they like. + assert client_identity("198.51.100.9", ["203.0.113.1"], _TRUSTED) == "198.51.100.9" + + def test_the_proxy_names_the_client(self) -> None: + assert client_identity(_PROXY, ["203.0.113.1"], _TRUSTED) == "203.0.113.1" + + def test_what_the_client_put_in_the_header_is_not_reached(self) -> None: + # The client sent "10.9.9.9"; the proxy appended the address it saw. + assert client_identity(_PROXY, ["10.9.9.9, 203.0.113.1"], _TRUSTED) == "203.0.113.1" + assert client_identity(_PROXY, ["10.9.9.9", "203.0.113.1"], _TRUSTED) == "203.0.113.1" + + def test_a_chain_of_trusted_proxies_is_skipped_from_the_right(self) -> None: + assert client_identity(_PROXY, ["203.0.113.1, 172.18.0.7"], _TRUSTED) == "203.0.113.1" + + @pytest.mark.parametrize("header", [[], [""], ["172.18.0.7"], ["203.0.113.1, not-an-address"]]) + def test_a_header_that_names_no_client_leaves_the_peer(self, header: list[str]) -> None: + assert client_identity(_PROXY, header, _TRUSTED) == _PROXY + + def test_an_ipv4_mapped_peer_is_matched_as_ipv4(self) -> None: + assert client_identity(f"::ffff:{_PROXY}", ["203.0.113.1"], _TRUSTED) == "203.0.113.1" + + def test_no_peer_stays_unidentified(self) -> None: + assert client_identity(None, ["203.0.113.1"], _TRUSTED) is None + + +class TestTrustedProxySetting: + def test_addresses_and_blocks_are_read(self) -> None: + networks = parse_trusted_proxies(" 172.18.0.2 , 10.0.0.0/8,::1 ") + assert [str(network) for network in networks] == ["172.18.0.2/32", "10.0.0.0/8", "::1/128"] + + def test_an_entry_that_is_not_an_address_is_dropped_and_trusts_nothing(self) -> None: + assert parse_trusted_proxies("caddy, 172.18.0.2") == parse_trusted_proxies("172.18.0.2") + assert parse_trusted_proxies("*") == () + + def test_a_dropped_entry_is_named_by_position_not_by_its_text( + self, caplog: pytest.LogCaptureFixture + ) -> None: + with caplog.at_level("WARNING"): + parse_trusted_proxies("172.18.0.2, not-an-address-CANARY") + + assert "entry 2" in caplog.text + assert "CANARY" not in caplog.text + + def test_the_throttle_reads_it_from_the_environment( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + assert AuthFailureThrottle().client_id(_PROXY, ["203.0.113.1"]) == _PROXY + monkeypatch.setenv(TRUSTED_PROXIES_ENV, "172.18.0.0/16") + assert AuthFailureThrottle().client_id(_PROXY, ["203.0.113.1"]) == "203.0.113.1" + + +def _status(server: str, *, forwarded_for: str | None) -> int: + headers = {"X-API-Key": "wrong"} + if forwarded_for is not None: + headers["X-Forwarded-For"] = forwarded_for + request = urllib.request.Request(f"{server}/version", headers=headers) + try: + with urllib.request.urlopen(request, timeout=2.0) as response: + return int(response.status) + except urllib.error.HTTPError as exc: + json.loads(exc.read()) + return exc.code + + +class TestBehindAProxy: + """A real socket: the test client is the "proxy" (peer 127.0.0.1).""" + + @pytest.fixture + def serve(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterable[None]: + monkeypatch.delenv("KPUBDATA_BUILDER_DEV_MODE", raising=False) + monkeypatch.setenv("KPUBDATA_BUILDER_API_KEY", "secret") + monkeypatch.setenv("KPUBDATA_BUILDER_AUTH_FAILURE_LIMIT", "2") + self._servers: list[tuple[HTTPServer, threading.Thread]] = [] + self._tmp_path = tmp_path + yield + for httpd, thread in self._servers: + httpd.shutdown() + httpd.server_close() + thread.join(timeout=1.0) + + def _start(self) -> str: + client = _FakeClient({"datago.air_quality": [{"id": "1", "v": 10}]}) + service = BuilderService( + output_root=self._tmp_path, client_factory=lambda **_kwargs: client + ) + httpd = HTTPServer(("127.0.0.1", 0), make_handler(service)) + thread = threading.Thread(target=httpd.serve_forever, daemon=True) + thread.start() + self._servers.append((httpd, thread)) + return f"http://{httpd.server_address[0]}:{httpd.server_address[1]}" + + def test_two_clients_behind_one_configured_proxy_are_throttled_separately( + self, serve: None, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv(TRUSTED_PROXIES_ENV, "127.0.0.1") + server = self._start() + + assert _status(server, forwarded_for="203.0.113.1") == 401 + assert _status(server, forwarded_for="203.0.113.1") == 401 + assert _status(server, forwarded_for="203.0.113.1") == 429 + # The other user, through the same proxy, has their own allowance. + assert _status(server, forwarded_for="203.0.113.2") == 401 + + def test_a_forged_prefix_does_not_buy_a_new_bucket( + self, serve: None, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv(TRUSTED_PROXIES_ENV, "127.0.0.1") + server = self._start() + + assert _status(server, forwarded_for="10.0.0.1, 203.0.113.1") == 401 + assert _status(server, forwarded_for="10.0.0.2, 203.0.113.1") == 401 + assert _status(server, forwarded_for="10.0.0.3, 203.0.113.1") == 429 + + def test_without_the_setting_the_header_changes_nothing(self, serve: None) -> None: + server = self._start() + + assert _status(server, forwarded_for="203.0.113.1") == 401 + assert _status(server, forwarded_for="203.0.113.2") == 401 + # Every request is the peer's, whatever the header says — as before. + assert _status(server, forwarded_for="203.0.113.3") == 429 + assert _status(server, forwarded_for=None) == 429 + + +class TestExpiredTokensAreNotFailures: + @pytest.fixture + def service(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> BuilderService: + monkeypatch.delenv("KPUBDATA_BUILDER_DEV_MODE", raising=False) + client = _FakeClient({"datago.air_quality": [{"id": "1", "v": 10}]}) + service = BuilderService(output_root=tmp_path, client_factory=lambda **_kwargs: client) + service._auth_throttle = AuthFailureThrottle(limit=3, window_seconds=60.0) + return service + + def _version(self, service: BuilderService) -> ServiceResponse: + response = dispatch(service, "GET", "/version", None, client_id="203.0.113.7") + assert isinstance(response, ServiceResponse) + return response + + def test_an_expired_token_does_not_move_the_failure_count( + self, service: BuilderService, monkeypatch: pytest.MonkeyPatch + ) -> None: + expired = AuthError(reason="invalid token: ExpiredSignatureError", expired=True) + monkeypatch.setattr(app_module, "authenticate", lambda **_: expired) + + for _ in range(10): + response = self._version(service) + assert response.status_code == 401 + assert response.body["code"] == "token_expired" + assert service._auth_throttle.retry_after("203.0.113.7") is None + + def test_a_token_that_does_not_verify_still_counts( + self, service: BuilderService, monkeypatch: pytest.MonkeyPatch + ) -> None: + forged = AuthError(reason="invalid token: InvalidSignatureError") + monkeypatch.setattr(app_module, "authenticate", lambda **_: forged) + + assert [self._version(service).status_code for _ in range(4)] == [401, 401, 401, 429] + + def test_expired_tokens_do_not_hide_guesses_made_alongside_them( + self, service: BuilderService, monkeypatch: pytest.MonkeyPatch + ) -> None: + answers = [ + AuthError(reason="invalid api key"), + AuthError(reason="invalid token: ExpiredSignatureError", expired=True), + AuthError(reason="invalid api key"), + AuthError(reason="invalid token: ExpiredSignatureError", expired=True), + AuthError(reason="invalid api key"), + ] + monkeypatch.setattr(app_module, "authenticate", lambda **_: answers.pop(0)) + + assert [self._version(service).status_code for _ in range(5)] == [401] * 5 + # Three real failures reached the limit; the expired ones neither added nor cleared. + assert self._version(service).status_code == 429 diff --git a/tests/unit/test_prod_oidc_plumbing.py b/tests/unit/test_prod_oidc_plumbing.py index 0cc1bc85..262bdab9 100644 --- a/tests/unit/test_prod_oidc_plumbing.py +++ b/tests/unit/test_prod_oidc_plumbing.py @@ -22,6 +22,7 @@ "OIDC_ALLOWED_EMAILS", "KPUBDATA_BUILDER_ADMIN_SUBJECTS", "KPUBDATA_BUILDER_AUTH_FAILURE_LIMIT", + "KPUBDATA_BUILDER_TRUSTED_PROXIES", )