diff --git a/README.md b/README.md index d1044a2..331e6b4 100644 --- a/README.md +++ b/README.md @@ -167,7 +167,8 @@ does not take any request parameters. ### `/metrics` This endpoint exposes counters describing the traffic this process has served and the work it did to serve it. It takes -no request parameters other than the format selector below. +no request parameters other than the format selector below. It is open by default and can be put behind a bearer token +— see [Protecting the endpoint](#protecting-the-endpoint). #### Query Parameters @@ -187,8 +188,7 @@ curl http://localhost:5000/metrics?fmt=json | jq # the same numbers, for a hu Everything is counted **in this process, in memory**. There is no database and no external dependency, and the counters **reset to zero when the process restarts** — which on the hosted environments is often. `processStartTimeSeconds` is -exposed so a dashboard can tell a restart apart from a drop in traffic; in PromQL, `rate()` already handles counter -resets, and `pesu_auth_process_start_time_seconds` makes the restart itself visible. +exposed so a dashboard can tell a restart apart from a drop in traffic. Collection happens at three layers, and which layer records what is deliberate: @@ -546,6 +546,46 @@ which is `null` rather than absent when nothing has been recorded yet, so the sh +#### Protecting the endpoint + +`/metrics` is **open by default**, which is what a local run and the Docker instructions above +expect. Set the `METRICS_TOKEN` environment variable on the server to require a bearer token +instead: + +```bash +TOKEN=$(openssl rand -hex 32) # keep it: whatever scrapes the endpoint needs the same value +docker run --name pesu-auth -d -p 5000:5000 -e METRICS_TOKEN="$TOKEN" pesu-auth +``` + +With it set, a request must carry that token or the endpoint answers `401` with +`WWW-Authenticate: Bearer` and the same error body as every other failure. Both formats are +covered, so `?fmt=json` is not a way around it. + +```bash +curl http://localhost:5000/metrics # 401 +curl -H "Authorization: Bearer " http://localhost:5000/metrics # 200 +``` + +The variable is read once at startup, so changing it needs a restart. Leaving it blank counts as +unset. No other endpoint is affected — `/health` in particular stays open, since uptime monitors +and the hosting platform's own health check send no credentials. + +#### Scraping the endpoint + +The default format is the Prometheus text exposition format precisely so that a scraper pointed at +this path needs no configuration. Any Prometheus-compatible collector works: + +```yaml +scrape_configs: + - job_name: pesu-auth + metrics_path: /metrics + scheme: https + static_configs: + - targets: [ "pesu-auth.onrender.com" ] + authorization: + credentials: # omit when METRICS_TOKEN is unset +``` + ### `/readme` This endpoint redirects to the project's official GitHub repository. This endpoint does not take any request parameters. diff --git a/app/app.py b/app/app.py index 07f005f..86fc52a 100644 --- a/app/app.py +++ b/app/app.py @@ -12,7 +12,7 @@ from zoneinfo import ZoneInfo import uvicorn -from fastapi import FastAPI +from fastapi import Depends, FastAPI from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse, PlainTextResponse, RedirectResponse @@ -27,6 +27,7 @@ from app.docs import authenticate_docs, health_docs, metrics_docs, readme_docs from app.exceptions.base import PESUAcademyError +from app.metrics.auth import require_metrics_token from app.metrics.collector import ( AUTHENTICATION_REQUESTS, AUTHENTICATION_RESULTS, @@ -223,6 +224,7 @@ async def pesu_exception_handler(request: Request, exc: PESUAcademyError) -> JSO "message": exc.message, "timestamp": datetime.datetime.now(IST).isoformat(), }, + headers=exc.headers, ) @@ -267,6 +269,9 @@ async def health() -> JSONResponse: response_model=None, responses=metrics_docs.response_examples, tags=["Monitoring"], + # Enforced only when METRICS_TOKEN is set in the environment; open otherwise, which is what + # every existing caller and the local Docker instructions expect. + dependencies=[Depends(require_metrics_token)], ) async def metrics_endpoint(fmt: MetricsFormat = MetricsFormat.PROMETHEUS) -> Response: """Expose the collected metrics. diff --git a/app/docs/metrics.py b/app/docs/metrics.py index 2851a01..95f953d 100644 --- a/app/docs/metrics.py +++ b/app/docs/metrics.py @@ -175,6 +175,23 @@ } }, }, + 401: { + "description": ( + "The server has `METRICS_TOKEN` set and the request did not present it. The " + "response carries `WWW-Authenticate: Bearer`. While `METRICS_TOKEN` is unset this " + "cannot occur and the endpoint needs no credentials." + ), + "model": ResponseModel, + "content": { + "application/json": { + "example": { + "status": False, + "message": "Invalid or missing metrics token.", + "timestamp": "2024-07-28T22:30:10.103368+05:30", + } + } + }, + }, 500: _INTERNAL_SERVER_ERROR, }, ) diff --git a/app/exceptions/base.py b/app/exceptions/base.py index 6692ded..b447ae7 100644 --- a/app/exceptions/base.py +++ b/app/exceptions/base.py @@ -4,10 +4,13 @@ class PESUAcademyError(Exception): """Base class for all PESU Academy-related errors.""" - def __init__(self, message: str, status_code: int) -> None: - """Initialize the PESUAcademyError with a custom message and status code.""" + def __init__(self, message: str, status_code: int, headers: dict[str, str] | None = None) -> None: + """Initialize the PESUAcademyError with a custom message, status code and response headers.""" self.message = message self.status_code = status_code + # Only a 401 needs these today, to carry WWW-Authenticate. None for every other error, and + # JSONResponse accepts None, so the handler passes it through unconditionally. + self.headers = headers super().__init__(self.message) def __str__(self) -> str: diff --git a/app/exceptions/metrics.py b/app/exceptions/metrics.py new file mode 100644 index 0000000..5d1978d --- /dev/null +++ b/app/exceptions/metrics.py @@ -0,0 +1,13 @@ +"""Custom exception classes for the metrics endpoint. All errors inherit from PESUAcademyError.""" + +from app.exceptions.base import PESUAcademyError + + +class MetricsAuthorizationError(PESUAcademyError): + """Raised when the metrics endpoint is token-protected and the request did not present it.""" + + def __init__(self, message: str = "Invalid or missing metrics token.") -> None: + """Initialize the MetricsAuthorizationError with a custom message.""" + # A 401 is required to say how to authenticate, so a scraper can tell "your credentials + # are wrong" apart from "this endpoint wants no credentials at all". + super().__init__(message, status_code=401, headers={"WWW-Authenticate": "Bearer"}) diff --git a/app/metrics/__init__.py b/app/metrics/__init__.py index 7da790c..98917a8 100644 --- a/app/metrics/__init__.py +++ b/app/metrics/__init__.py @@ -4,7 +4,7 @@ PESUAcademy client, so importing the package has no side effects and a test can swap the collector out by patching one module attribute. -Import from the modules directly -- `app.metrics.collector` for the families and the collector, -`app.metrics.prometheus` for exposition -- following the same convention as `app.exceptions`. A -re-export list here would be one more place to remember when a metric family is added. +Import from the modules directly rather than from this package, following the same convention as +`app.exceptions`. A re-export list here would be one more place to remember when a metric family or +a module is added -- and it had already fallen out of date once. """ diff --git a/app/metrics/auth.py b/app/metrics/auth.py new file mode 100644 index 0000000..68c8365 --- /dev/null +++ b/app/metrics/auth.py @@ -0,0 +1,86 @@ +"""Optional bearer-token protection for the metrics endpoint.""" + +from __future__ import annotations + +import os +import secrets +from typing import Annotated + +from fastapi import Security + +# Imported at runtime on purpose, not under TYPE_CHECKING. FastAPI resolves a dependency's +# annotations with get_type_hints() when the route is built, and this module uses +# `from __future__ import annotations`, so a name that exists only for type checkers would be a +# NameError at import time rather than a typing nicety. +from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer + +from app.exceptions.metrics import MetricsAuthorizationError + + +def _configured_token() -> str | None: + """Read the configured metrics token from the environment. + + Returns: + str | None: The token, or None when it is unset or blank -- a variable declared and left + empty means "no token", not "the empty token". + """ + return os.environ.get("METRICS_TOKEN") or None + + +# Read once at import, so whether this process enforces a token is fixed for its lifetime and +# cannot start or stop halfway through. +METRICS_TOKEN: str | None = _configured_token() + +# auto_error=False so this never raises by itself: every failure path -- no header, no scheme, no +# credentials, or a scheme that is not Bearer -- returns None, and the 401 is raised below as a +# PESUAcademyError. That is what keeps the body in this API's `{status, message, timestamp}` shape +# and gets the failure counted in errors_total. HTTPBasic cannot be used the same way; it raises +# HTTPException on a malformed credential regardless of auto_error, which would answer in +# Starlette's `{"detail": ...}` shape and bypass the error metric entirely. +_bearer = HTTPBearer( + auto_error=False, + scheme_name="MetricsToken", + description=( + "Set the METRICS_TOKEN environment variable on the server to require this token. While it " + "is unset the endpoint is open and any credential here is ignored." + ), +) + + +async def require_metrics_token( + credentials: Annotated[HTTPAuthorizationCredentials | None, Security(_bearer)] = None, +) -> None: + """Reject the request unless it carries the configured metrics token. + + Args: + credentials (HTTPAuthorizationCredentials | None): Parsed bearer credentials, or None when + the request carried no usable `Authorization: Bearer` header. + + Raises: + MetricsAuthorizationError: If a token is configured and the request did not present it. + """ + # Looked up on the module at call time rather than captured, so a test can swap it with + # monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", ...) + expected = METRICS_TOKEN + if expected is None: + return + # compare_digest, not ==, so a wrong token cannot be recovered a character at a time from how + # long the comparison took. + # + # Compared as bytes, not str: compare_digest *raises TypeError* on a str holding any non-ASCII + # character, so `Authorization: Bearer ü` would turn this 401 into a 500 with a logged + # traceback -- something any caller could do at will, and it would land in + # failures_total{fault="server"}, which is the one metric worth alerting on. A non-ASCII + # METRICS_TOKEN was worse still: every request 500ed, including the correct one. + # + # The two codecs are not interchangeable. A header value reaches us already latin-1 decoded, + # per the HTTP spec and every ASGI server, so encoding it back through latin-1 recovers the + # exact bytes the client sent; the configured token comes from the environment as UTF-8, with + # surrogates standing in for any byte sequence that was not valid UTF-8. Encoding each back the + # way it arrived makes the comparison byte-exact, so a non-ASCII token works rather than + # silently never matching. + if credentials is None or not secrets.compare_digest( + credentials.credentials.encode("latin-1", "replace"), + expected.encode("utf-8", "surrogateescape"), + ): + raise MetricsAuthorizationError diff --git a/app/metrics/collector.py b/app/metrics/collector.py index ab66fa8..f8a7ef0 100644 --- a/app/metrics/collector.py +++ b/app/metrics/collector.py @@ -254,8 +254,8 @@ def __init__(self, *, clock: Callable[[], float] | None = None) -> None: self._start_time = self._clock() self._values: defaultdict[str, dict[LabelKey, float]] = defaultdict(dict) # Seed the unlabelled series so a freshly started process still exposes them. Without this - # a Grafana panel has no series at all until the first request, and rate() over a series - # that springs into existence mid-window reads as a spike. + # there is no series at all until the first request, and a rate over a series that springs + # into existence mid-window reads as a spike. for family in FAMILIES: if family.labels: continue diff --git a/pyproject.toml b/pyproject.toml index 279670f..b07d09b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "pesu-auth" -version = "4.2.0" +version = "4.3.0" description = "A simple API to authenticate PESU credentials using PESU Academy." readme = "README.md" requires-python = ">=3.14" diff --git a/scripts/benchmark/benchmark_requests.py b/scripts/benchmark/benchmark_requests.py index fc990ed..30a48a3 100644 --- a/scripts/benchmark/benchmark_requests.py +++ b/scripts/benchmark/benchmark_requests.py @@ -40,7 +40,7 @@ parser.add_argument( "--route", type=str, - choices=["authenticate", "health", "readme", "metrics"], + choices=["authenticate", "health", "readme"], default="authenticate", help="The route to make the request to (default: authenticate)", ) diff --git a/scripts/benchmark/util.py b/scripts/benchmark/util.py index 4c194bd..ab07204 100644 --- a/scripts/benchmark/util.py +++ b/scripts/benchmark/util.py @@ -89,9 +89,9 @@ def make_request( follow_redirects=True, ) elapsed_time = time.time() - start_time - # Not every route answers with JSON: /readme is a 308 to GitHub, and /metrics is Prometheus - # text. An unconditional .json() crashes the sequential runner outright and, in the parallel - # runner, is swallowed as a failed request -- which silently skews the numbers being measured. + # Not every route answers with JSON: /readme is a 308 to GitHub. An unconditional .json() + # crashes the sequential runner outright and, in the parallel runner, is swallowed as a failed + # request -- which silently skews the numbers being measured. try: body = response.json() except ValueError: diff --git a/tests/conftest.py b/tests/conftest.py index 08f428f..56758af 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,8 +1,23 @@ +import pytest from dotenv import load_dotenv load_dotenv() +@pytest.fixture(autouse=True) +def _metrics_token_unset(monkeypatch): + """Keep /metrics open unless a test asks for a token. + + METRICS_TOKEN is read from the environment when app.metrics.auth is imported, and the + load_dotenv() above runs before any test module imports the app. So a token left in a local + .env -- or merely exported in the shell -- would make roughly thirty /metrics tests across the + suite fail with a confusing 401 that has nothing to do with what they are testing. Pinning it + here makes the suite independent of the ambient environment; the tests that exercise the token + set it themselves. + """ + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", None) + + def pytest_collection_modifyitems(config, items): # Force directory-based test ordering: unit > functional > integration priority = { diff --git a/tests/unit/test_metrics_auth.py b/tests/unit/test_metrics_auth.py new file mode 100644 index 0000000..42dd6e2 --- /dev/null +++ b/tests/unit/test_metrics_auth.py @@ -0,0 +1,176 @@ +"""Tests for the optional bearer token on /metrics. + +Every test here pins `METRICS_TOKEN` explicitly rather than inheriting it from the environment. +tests/conftest.py calls load_dotenv() before this module imports app.app, so a METRICS_TOKEN in a +local .env would otherwise start returning 401 to every other /metrics test in the suite -- the +same class of silent, environment-dependent breakage as the ".env vanished" case. +""" + +from unittest.mock import AsyncMock, patch + +import pytest +from fastapi.security import HTTPAuthorizationCredentials +from fastapi.testclient import TestClient + +from app.app import app +from app.exceptions.authentication import AuthenticationError +from app.exceptions.metrics import MetricsAuthorizationError +from app.metrics.auth import _configured_token, require_metrics_token +from app.metrics.collector import MetricsCollector + +TOKEN = "test-metrics-token" + + +@pytest.fixture +def client(monkeypatch): + """A client with a fresh collector and no token configured.""" + monkeypatch.setattr("app.app.metrics", MetricsCollector()) + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", None) + with ( + patch("app.app.pesu_academy.prefetch_client_with_csrf_token", new_callable=AsyncMock), + patch("app.app.pesu_academy.close_client", new_callable=AsyncMock), + ): + with TestClient(app, raise_server_exceptions=False) as test_client: + yield test_client + + +@pytest.fixture +def protected(client, monkeypatch): + """The same client, with a token required.""" + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", TOKEN) + return client + + +@pytest.mark.parametrize("query", ["", "?fmt=json", "?fmt=prometheus"]) +def test_open_when_no_token_is_configured(client, query): + """The default deployment, and what every existing caller and the Docker instructions expect.""" + assert client.get(f"/metrics{query}").status_code == 200 + + +@pytest.mark.parametrize("query", ["", "?fmt=json", "?fmt=prometheus"]) +def test_the_right_token_is_accepted_in_either_format(protected, query): + response = protected.get(f"/metrics{query}", headers={"Authorization": f"Bearer {TOKEN}"}) + assert response.status_code == 200 + + +@pytest.mark.parametrize( + ("label", "headers"), + [ + ("no header at all", {}), + ("the wrong token", {"Authorization": "Bearer not-the-token"}), + ("a prefix of the token", {"Authorization": f"Bearer {TOKEN[:-1]}"}), + ("the token without its scheme", {"Authorization": TOKEN}), + ("basic instead of bearer", {"Authorization": "Basic dXNlcjpwYXNz"}), + # HTTPBasic would raise HTTPException on this one, answering in Starlette's shape and + # skipping errors_total; HTTPBearer rejects the scheme before that can happen. + ("malformed basic credentials", {"Authorization": "Basic !!!!"}), + ("an empty bearer value", {"Authorization": "Bearer "}), + ], +) +def test_rejected_without_the_token(protected, label, headers): + response = protected.get("/metrics", headers=headers) + assert response.status_code == 401, label + + +def test_the_rejection_is_shaped_like_every_other_error(protected): + """A 401 here must not be Starlette's {"detail": ...}, which is what HTTPBasic would produce.""" + response = protected.get("/metrics") + body = response.json() + assert set(body) == {"status", "message", "timestamp"} + assert body["status"] is False + assert body["message"] == "Invalid or missing metrics token." + + +def test_the_rejection_says_how_to_authenticate(protected): + """Required of a 401, and it tells a scraper "wrong credentials" from "none wanted".""" + assert protected.get("/metrics").headers["www-authenticate"] == "Bearer" + + +def test_the_json_format_is_protected_too(protected): + """The format selector must not be a way around the token.""" + assert protected.get("/metrics?fmt=json").status_code == 401 + + +def test_a_rejection_is_counted_as_an_error_and_a_client_fault(protected): + """The middleware/handler split from the metrics work still holds for this new error.""" + protected.get("/metrics") + body = protected.get("/metrics?fmt=json", headers={"Authorization": f"Bearer {TOKEN}"}).json() + assert body["errorsByType"] == {"MetricsAuthorizationError": 1} + assert body["responsesByStatus"]["401"] == 1 + assert body["failuresByFault"] == {"client": 1} + + +def test_health_is_not_protected(protected): + """Render's own health check and the cron-job.org monitors send no credentials.""" + assert protected.get("/health").status_code == 200 + + +@pytest.mark.parametrize( + ("value", "expected"), + [(None, None), ("", None), (" ", " "), ("a-token", "a-token")], +) +def test_reading_the_token_from_the_environment(monkeypatch, value, expected): + """A variable declared but left blank means "no token", not "the empty token". + + Tested through the reader rather than by reloading the module: a reload mutates the module + dict in place, which would leave the new value in force for every test that follows it. + """ + if value is None: + monkeypatch.delenv("METRICS_TOKEN", raising=False) + else: + monkeypatch.setenv("METRICS_TOKEN", value) + assert _configured_token() == expected + + +#: Header values are passed as **bytes** in the non-ASCII tests below. httpx refuses to encode a +#: non-ASCII str header value, so a str would fail in the client and never reach the app -- but +#: curl and any other raw client send those bytes happily, which is how this reached a real server. +@pytest.mark.parametrize( + "credential", + [b"\xc3\xbc", b"t\xc3\xb6k\xc3\xa9n", b"\xc3\xa9" * 200, b"\xff\xfe", b"\x80"], +) +def test_a_non_ascii_credential_is_rejected_not_a_server_error(protected, credential): + """`secrets.compare_digest` raises TypeError on a str holding any non-ASCII character. + + Comparing the presented credential as a str let any caller turn this 401 into a **500** with a + logged traceback, and put the result in `failures_total{fault="server"}` -- the one metric + worth alerting on. Against a str comparison every case here is a 500. + """ + response = protected.get("/metrics", headers={b"Authorization": b"Bearer " + credential}) + assert response.status_code == 401 + assert response.json()["message"] == "Invalid or missing metrics token." + + +def test_a_non_ascii_token_actually_works(client, monkeypatch): + """A token is compared byte for byte, so an operator is not silently locked out by an umlaut. + + Against a str comparison this was worse than a rejection: *every* request 500ed, the correct + one included, leaving the endpoint unreachable with only a traceback to explain why. + """ + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", "tökén-höchst") + correct = "tökén-höchst".encode() # what a client actually puts on the wire + assert client.get("/metrics", headers={b"Authorization": b"Bearer " + correct}).status_code == 200 + assert client.get("/metrics", headers={b"Authorization": b"Bearer t\xc3\xb6k\xc3\xa9n"}).status_code == 401 + + +@pytest.mark.parametrize("credential", ["ü", "tökén", "\udcff", "é" * 500]) +@pytest.mark.asyncio +async def test_the_dependency_itself_never_raises_typeerror(monkeypatch, credential): + """Pinned one level below the HTTP layer, where the TypeError actually happened. + + Includes a lone surrogate, which no HTTP client would send but which `.encode("utf-8")` would + choke on -- the reason the comparison encodes latin-1 rather than UTF-8 on this side. + """ + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", TOKEN) + credentials = HTTPAuthorizationCredentials(scheme="Bearer", credentials=credential) + with pytest.raises(MetricsAuthorizationError): + await require_metrics_token(credentials) + + +def test_other_errors_carry_no_stray_headers(client): + """Regression on adding `headers` to PESUAcademyError: it is None for everything else.""" + with patch("app.app.pesu_academy.authenticate", new_callable=AsyncMock) as authenticate: + authenticate.side_effect = AuthenticationError("Invalid username or password.") + response = client.post("/authenticate", json={"username": "PES1201800001", "password": "x"}) + assert response.status_code == 401 + assert "www-authenticate" not in response.headers diff --git a/tests/unit/test_openapi_docs.py b/tests/unit/test_openapi_docs.py index bf947a5..9a19eee 100644 --- a/tests/unit/test_openapi_docs.py +++ b/tests/unit/test_openapi_docs.py @@ -140,6 +140,31 @@ def test_the_metrics_format_enum_is_documented(schema): assert sorted(values) == ["json", "prometheus"] +def test_the_metrics_token_scheme_is_documented(schema): + """Swagger's Authorize button is how a reader discovers the endpoint can be protected.""" + scheme = schema["components"]["securitySchemes"]["MetricsToken"] + assert scheme["type"] == "http" + assert scheme["scheme"] == "bearer" + assert "METRICS_TOKEN" in scheme["description"] + + +def test_only_metrics_requires_the_token(schema): + """/health must stay open: Render's own health check and the uptime monitors send no token.""" + secured = {path for path, _, operation in _operations(schema) if operation.get("security")} + assert secured == {"/metrics"} + assert schema["paths"]["/metrics"]["get"]["security"] == [{"MetricsToken": []}] + + +def test_the_documented_metrics_401_matches_a_real_response(client, schema, monkeypatch): + """The 401 body a scraper gets must be the one Swagger shows.""" + documented = schema["paths"]["/metrics"]["get"]["responses"]["401"]["content"]["application/json"]["example"] + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", "some-token") + response = client.get("/metrics") + assert response.status_code == 401 + assert response.json()["message"] == documented["message"] + assert set(response.json()) == set(documented) + + def test_the_documented_400_matches_a_real_response(client, schema): """The example a reader copies must be the body they will actually receive.""" documented = schema["paths"]["/metrics"]["get"]["responses"]["400"]["content"]["application/json"]["example"] diff --git a/uv.lock b/uv.lock index 38e3654..7f9bc05 100644 --- a/uv.lock +++ b/uv.lock @@ -630,7 +630,7 @@ wheels = [ [[package]] name = "pesu-auth" -version = "4.2.0" +version = "4.3.0" source = { editable = "." } dependencies = [ { name = "fastapi" },