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/src/utils/otel_metrics.py b/agentex/src/utils/otel_metrics.py index be6f0bc3..f62e710b 100644 --- a/agentex/src/utils/otel_metrics.py +++ b/agentex/src/utils/otel_metrics.py @@ -63,7 +63,7 @@ SERVICE_VERSION, OTELResourceDetector, Resource, - get_aggregated_resources, + ServiceInstanceIdResourceDetector, ) from src.utils.logging import make_logger @@ -88,8 +88,36 @@ 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 _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: @@ -105,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/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", + } diff --git a/agentex/tests/unit/utils/test_otel_metrics.py b/agentex/tests/unit/utils/test_otel_metrics.py index 7089b4c4..18eefbc2 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,58 @@ 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_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")