From 08ffe75bc88e6a4c9f848b8911a565f2fb036d7a Mon Sep 17 00:00:00 2001 From: Brandon Allen Date: Wed, 4 Nov 2026 05:57:29 -0500 Subject: [PATCH 1/3] fix(auth): stop serving the OpenAPI schema and docs anonymously `/openapi.json`, `/api` (redoc_url) and the MkDocs site at `/docs` were in WHITELISTED_ROUTES, so AgentexAuthMiddleware skipped the auth provider for them. On an internet-facing deployment that means an unauthenticated `GET /openapi.json` returns a complete description of every route, parameter, request body and response model the service exposes -- the whole attack surface, enumerated, with no credential. `/swagger` was already authenticated, which made the gap inconsistent as well as open. Nothing is unmounted: the schema and both UIs are still served, they just go through the auth provider like any other route, so an authenticated session reaches them exactly as before. Client and SDK codegen is unaffected. `scripts/generate_openapi_spec.py` builds the committed spec in-process from `fastapi_app.openapi()`; it has never fetched it over HTTP. Adds tests over `is_whitelisted_route` covering the three groups that matter -- the schema and docs surfaces, the routes that are anonymous by design (probes, the agent registration handshake, agent forwarding), and routes that merely share a prefix with an allowlisted one -- plus a pin on the allowlist contents, so adding anonymous internet-facing surface has to be argued in review. Co-Authored-By: Claude Opus 5 --- .../auth_provider_contract.md | 9 +- agentex/src/api/middleware_utils.py | 18 ++-- .../unit/api/test_auth_route_allowlist.py | 90 +++++++++++++++++++ 3 files changed, 110 insertions(+), 7 deletions(-) create mode 100644 agentex/tests/unit/api/test_auth_route_allowlist.py diff --git a/agentex/docs/docs/development_guides/auth_provider_contract.md b/agentex/docs/docs/development_guides/auth_provider_contract.md index 008ea85a..5adb930b 100644 --- a/agentex/docs/docs/development_guides/auth_provider_contract.md +++ b/agentex/docs/docs/development_guides/auth_provider_contract.md @@ -45,12 +45,17 @@ behind the same wire format. These routes bypass the provider entirely and are never sent to `/v1/authn`: ``` -/agents/register /agents/forward /docs /api /openapi.json /redoc -/favicon.ico /health /healthcheck /healthz /readyz /ping /echo +/agents/register /agents/forward /slack /linear /favicon.ico +/health /healthcheck /healthz /readyz /ping /echo ``` (plus any sub-path under them, and all `OPTIONS` preflight requests). +The API documentation surfaces — `/openapi.json`, `/swagger`, `/api` (ReDoc) +and the MkDocs site at `/docs` — are **not** allowlisted. They are still +served, but the provider authenticates them like any other route, because the +schema enumerates every route, parameter and model the service exposes. + --- ## Conventions diff --git a/agentex/src/api/middleware_utils.py b/agentex/src/api/middleware_utils.py index 8f82ac75..7c344ab6 100644 --- a/agentex/src/api/middleware_utils.py +++ b/agentex/src/api/middleware_utils.py @@ -13,16 +13,24 @@ logger = make_logger(__name__) -# Routes (and their prefixes) that bypass authentication +# Routes (and their prefixes) that bypass authentication. +# +# The OpenAPI schema, the UIs that render it (/swagger, and /api, which is +# redoc_url) and the MkDocs site at /docs are deliberately NOT here. The schema +# is a complete description of every route, parameter and model this service +# exposes; served anonymously on an internet-facing deployment it hands an +# unauthenticated caller the whole attack surface for free, and the docs site +# describes the same setup in prose. They all stay mounted -- an authenticated +# session still reaches them -- they just no longer bypass the auth provider. +# +# Client and SDK codegen is unaffected: scripts/generate_openapi_spec.py builds +# the committed spec in-process from `fastapi_app.openapi()` and never fetches +# it over HTTP. WHITELISTED_ROUTES: set[str] = { "/agents/register", "/agents/forward", "/slack", "/linear", - "/docs", - "/api", - "/openapi.json", - "/redoc", "/favicon.ico", "/health", "/healthcheck", diff --git a/agentex/tests/unit/api/test_auth_route_allowlist.py b/agentex/tests/unit/api/test_auth_route_allowlist.py new file mode 100644 index 00000000..0b526703 --- /dev/null +++ b/agentex/tests/unit/api/test_auth_route_allowlist.py @@ -0,0 +1,90 @@ +"""The unauthenticated route allowlist is a security boundary, not a config list. + +`is_whitelisted_route` decides which requests skip the auth provider entirely +(`AgentexAuthMiddleware.dispatch`). Anything it returns True for is served to +the internet with no principal, so each entry is asserted explicitly here +rather than inferred from the set. +""" + +import pytest +from src.api.middleware_utils import WHITELISTED_ROUTES, is_whitelisted_route + + +@pytest.mark.unit +@pytest.mark.parametrize( + "path", + [ + "/openapi.json", + "/swagger", + "/swagger/oauth2-redirect", + "/api", + "/api/", + "/redoc", + "/docs", + "/docs/index.html", + ], +) +def test_api_schema_and_docs_require_authentication(path): + """The schema enumerates every route, parameter and model this service + exposes; the docs describe its setup. Neither is served anonymously.""" + assert not is_whitelisted_route(path) + + +@pytest.mark.unit +@pytest.mark.parametrize( + "path", + [ + "/health", + "/healthz", + "/readyz", + "/ping", + "/echo", + "/favicon.ico", + "/agents/register", + "/agents/forward/name/some-agent/v1/messages", + ], +) +def test_anonymous_routes_stay_anonymous(path): + """Probes, the agent registration handshake and agent forwarding carry no + user principal by design and must not start demanding one.""" + assert is_whitelisted_route(path) + + +@pytest.mark.unit +@pytest.mark.parametrize( + "path", + [ + "/agents", + "/agents/some-agent-id", + "/agents/registered", + "/tasks", + "/messages", + "/spans", + "/health-metrics", + "/echoes/1", + ], +) +def test_resource_routes_are_authenticated(path): + """Entries match a whole path segment, so a route that merely shares a + prefix with an allowlisted one ("/agents/registered" vs + "/agents/register") is not born anonymous.""" + assert not is_whitelisted_route(path) + + +@pytest.mark.unit +def test_allowlist_contents_are_pinned(): + """A new entry is a new piece of internet-facing anonymous surface. Adding + one here is the prompt to say why in review.""" + assert WHITELISTED_ROUTES == { + "/agents/register", + "/agents/forward", + "/slack", + "/linear", + "/favicon.ico", + "/health", + "/healthcheck", + "/healthz", + "/readyz", + "/ping", + "/echo", + } From c8bfe228cbdfc6d12a4793f939f24e03c56a62fc Mon Sep 17 00:00:00 2001 From: Brandon Allen Date: Wed, 4 Nov 2026 06:36:18 -0500 Subject: [PATCH 2/3] fix(otel): stop reporting a random UUID as service.instance.id `_detected_resource()` used `get_aggregated_resources([OTELResourceDetector()])`, which merges the SDK's default resource. Since opentelemetry-sdk 1.3x that default generates a random UUID for `service.instance.id` when nothing else supplies one, and the dependency floor here is `opentelemetry-sdk>=1.28.0`, so an ordinary resolver bump turned it on. `_unique_instance_id` then always took its "existing value" branch, and both call sites -- the auto-instrumentation bootstrap and the standalone MeterProvider resource -- started publishing `.`. Still unique per worker, which is what the function guarantees, but it drops the service and pod the series belongs to and changes on every process start, so metric identity no longer survives a restart. `_detected_resource` exists to answer "what did the operator configure", so it now uses the detector alone and reports `service.instance.id` as absent when the operator did not set one. An operator-supplied value, via OTEL_RESOURCE_ATTRIBUTES, still wins and still gets the pid suffix. The two `_unique_instance_id` tests rebuilt the aggregated resource by hand instead of calling `_detected_resource()`, which is why they drifted from the code path they name; `test_unique_instance_id_builds_when_missing` had been failing on main. They now go through the real function, and a new test pins the absence of the SDK-generated id directly. Co-Authored-By: Claude Opus 5 --- agentex/src/utils/otel_metrics.py | 18 +++++++++++++--- agentex/tests/unit/utils/test_otel_metrics.py | 21 +++++++++++++++---- 2 files changed, 32 insertions(+), 7 deletions(-) diff --git a/agentex/src/utils/otel_metrics.py b/agentex/src/utils/otel_metrics.py index be6f0bc3..884d0b3e 100644 --- a/agentex/src/utils/otel_metrics.py +++ b/agentex/src/utils/otel_metrics.py @@ -63,7 +63,6 @@ SERVICE_VERSION, OTELResourceDetector, Resource, - get_aggregated_resources, ) from src.utils.logging import make_logger @@ -88,8 +87,21 @@ def _detected_resource() -> Resource: - """Resource attributes from OTEL_* env (operator-injected or local).""" - return get_aggregated_resources([OTELResourceDetector()]) + """Resource attributes from OTEL_* env (operator-injected or local). + + `OTELResourceDetector().detect()`, not `get_aggregated_resources([...])`. + The aggregating helper merges the SDK's default resource, and since + opentelemetry-sdk 1.3x that default supplies a freshly generated random + UUID for `service.instance.id` when nothing else sets one. Aggregating + here would mean `_unique_instance_id` always sees an "existing" value and + returns `.` -- a different identity on every process + start, with no trace of the service or pod it belongs to. + + This function exists only to answer "what did the operator actually + configure", so it must report `service.instance.id` as absent when the + operator did not set one. + """ + return OTELResourceDetector().detect() def _unique_instance_id(resource: Resource) -> str: diff --git a/agentex/tests/unit/utils/test_otel_metrics.py b/agentex/tests/unit/utils/test_otel_metrics.py index 7089b4c4..644866fd 100644 --- a/agentex/tests/unit/utils/test_otel_metrics.py +++ b/agentex/tests/unit/utils/test_otel_metrics.py @@ -21,9 +21,7 @@ from opentelemetry.sdk.metrics import MeterProvider from opentelemetry.sdk.metrics.export import InMemoryMetricReader from opentelemetry.sdk.resources import ( - OTELResourceDetector, Resource, - get_aggregated_resources, ) from src.utils import cache_metrics, otel_metrics @@ -176,7 +174,7 @@ def test_unique_instance_id_extends_operator_value(monkeypatch): "k8s.pod.name=my-pod,service.instance.id=agentex.my-pod.agentex", ) monkeypatch.setattr(otel_metrics.os, "getpid", lambda: 42) - base = get_aggregated_resources([OTELResourceDetector()]) + base = otel_metrics._detected_resource() assert otel_metrics._unique_instance_id(base) == "agentex.my-pod.agentex.42" @@ -185,10 +183,25 @@ def test_unique_instance_id_builds_when_missing(monkeypatch): monkeypatch.setenv("OTEL_SERVICE_NAME", "agentex") monkeypatch.setenv("OTEL_RESOURCE_ATTRIBUTES", "k8s.pod.name=my-pod") monkeypatch.setattr(otel_metrics.os, "getpid", lambda: 42) - base = get_aggregated_resources([OTELResourceDetector()]) + base = otel_metrics._detected_resource() assert otel_metrics._unique_instance_id(base) == "agentex.my-pod.42" +@pytest.mark.unit +def test_detected_resource_omits_the_sdk_generated_instance_id(monkeypatch): + """`_detected_resource` reports what the operator configured, nothing else. + + Since opentelemetry-sdk 1.3x the SDK's default resource invents a random + UUID for `service.instance.id`. If that leaks in here, `_unique_instance_id` + always takes its "existing value" branch and every process reports + `.` -- unique, but opaque and different on every restart. + """ + monkeypatch.setenv("OTEL_SERVICE_NAME", "agentex") + monkeypatch.setenv("OTEL_RESOURCE_ATTRIBUTES", "k8s.pod.name=my-pod") + + assert "service.instance.id" not in otel_metrics._detected_resource().attributes + + @pytest.mark.unit def test_sync_instance_id_to_env_updates_env(monkeypatch): monkeypatch.setenv("OTEL_SERVICE_NAME", "agentex") From f6c3e82ae4f320b4d803fc81369893e26bf9f0b7 Mon Sep 17 00:00:00 2001 From: Brandon Allen Date: Wed, 4 Nov 2026 06:43:24 -0500 Subject: [PATCH 3/3] fix(otel): keep instance ids unique when there is no pod name Review catch on the previous commit. Dropping the SDK's generated `service.instance.id` left `.unknown.` as the fallback whenever neither an operator value nor `k8s.pod.name` is present -- off Kubernetes, in local runs, in plain containers. Pids repeat freely across containers and hosts, so two replicas could report the same instance id and merge their counter streams into one wrong series at a shared collector. That is the exact uniqueness guarantee the function exists to provide. The SDK's random id is still the wrong thing to *prefer* -- opaque, and new on every restart -- but it is a sound tiebreaker, so it now stands in for the missing pod name instead of the literal "unknown". A configured identity, and a real pod name, both still win. Co-Authored-By: Claude Opus 5 --- agentex/src/utils/otel_metrics.py | 25 ++++++++++++-- agentex/tests/unit/utils/test_otel_metrics.py | 33 +++++++++++++++++++ 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/agentex/src/utils/otel_metrics.py b/agentex/src/utils/otel_metrics.py index 884d0b3e..f62e710b 100644 --- a/agentex/src/utils/otel_metrics.py +++ b/agentex/src/utils/otel_metrics.py @@ -63,6 +63,7 @@ SERVICE_VERSION, OTELResourceDetector, Resource, + ServiceInstanceIdResourceDetector, ) from src.utils.logging import make_logger @@ -104,6 +105,21 @@ def _detected_resource() -> Resource: return OTELResourceDetector().detect() +def _sdk_generated_instance_id() -> str | None: + """The random per-process id the SDK invents when nothing configures one. + + The detector memoizes it, so repeated calls in one process return the same + value. Useful only as a last-resort uniqueness source -- it is opaque and + changes on every restart, so a configured identity always beats it. + """ + generated = ( + ServiceInstanceIdResourceDetector() + .detect() + .attributes.get("service.instance.id") + ) + return str(generated) if generated else None + + def _unique_instance_id(resource: Resource) -> str: """Worker-unique service.instance.id (OTel #4390).""" pid = os.getpid() @@ -117,8 +133,13 @@ def _unique_instance_id(resource: Resource) -> str: or os.environ.get("OTEL_SERVICE_NAME") or "unknown" ) - pod = resource.attributes.get("k8s.pod.name") or "unknown" - return f"{service}.{pod}.{pid}" + # Off Kubernetes there is no pod name, and pids repeat freely across + # containers and hosts -- two replicas would otherwise both report + # `.unknown.` and silently merge their counter streams at a + # shared collector. The SDK's random id is a poor identity but a sound + # tiebreaker, so it stands in only when nothing better is configured. + pod = resource.attributes.get("k8s.pod.name") or _sdk_generated_instance_id() + return f"{service}.{pod or 'unknown'}.{pid}" def _sync_instance_id_to_env(instance_id: str) -> None: diff --git a/agentex/tests/unit/utils/test_otel_metrics.py b/agentex/tests/unit/utils/test_otel_metrics.py index 644866fd..18eefbc2 100644 --- a/agentex/tests/unit/utils/test_otel_metrics.py +++ b/agentex/tests/unit/utils/test_otel_metrics.py @@ -202,6 +202,39 @@ def test_detected_resource_omits_the_sdk_generated_instance_id(monkeypatch): assert "service.instance.id" not in otel_metrics._detected_resource().attributes +@pytest.mark.unit +def test_unique_instance_id_without_a_pod_name_stays_unique(monkeypatch): + """Off Kubernetes there is no pod name and pids repeat across containers. + + Two replicas must not both report `agentex.unknown.42` -- at a shared + collector that merges their counter streams into one wrong series. The + SDK's random id is a poor identity but a sound tiebreaker, so it stands in + for the missing pod rather than the literal "unknown". + """ + monkeypatch.setenv("OTEL_SERVICE_NAME", "agentex") + monkeypatch.delenv("OTEL_RESOURCE_ATTRIBUTES", raising=False) + monkeypatch.setattr(otel_metrics.os, "getpid", lambda: 42) + + instance_id = otel_metrics._unique_instance_id(otel_metrics._detected_resource()) + + assert instance_id == f"agentex.{otel_metrics._sdk_generated_instance_id()}.42" + assert instance_id != "agentex.unknown.42" + + +@pytest.mark.unit +def test_unique_instance_id_prefers_the_pod_over_the_sdk_id(monkeypatch): + """The SDK id is a last resort: a real pod name is readable and stable + across restarts, so it wins whenever one is present.""" + monkeypatch.setenv("OTEL_SERVICE_NAME", "agentex") + monkeypatch.setenv("OTEL_RESOURCE_ATTRIBUTES", "k8s.pod.name=my-pod") + monkeypatch.setattr(otel_metrics.os, "getpid", lambda: 42) + + assert ( + otel_metrics._unique_instance_id(otel_metrics._detected_resource()) + == "agentex.my-pod.42" + ) + + @pytest.mark.unit def test_sync_instance_id_to_env_updates_env(monkeypatch): monkeypatch.setenv("OTEL_SERVICE_NAME", "agentex")