feat: add ENFORCE_OPERATIONAL_ROUTE_AUTH rollout toggle (PER-15243)#323
feat: add ENFORCE_OPERATIONAL_ROUTE_AUTH rollout toggle (PER-15243)#323dshoen619 wants to merge 6 commits into
Conversation
The auth-hardening PRs (#317/#320/#321) made the update-trigger routes (/policy-updater/trigger, /data-updater/trigger, /update_policy, /update_policy_data) and /kong unconditionally require the PDP token, with no way to relax it during a coordinated fleet upgrade. Add a single flag, ENFORCE_OPERATIONAL_ROUTE_AUTH (PDP_ prefixed env var, also overridable from the cloud control plane), defaulting to FALSE for a safe rollout: those five routes accept unauthenticated requests and only LOG the ones that would be rejected, so callers that don't yet send the token (e.g. the Kong OPA plugin, older SDK automations) keep working while the logs surface them. Flip to true per-fleet once every caller sends the token. Every other PDP route stays strictly enforced regardless. - enforce_pdp_token_operational: conditional gate reading the flag per request; delegates to enforce_pdp_token when on, warn-and-allows when off. Distinct named callable so the fail-closed route audit still recognises it as a gate. - /kong split onto its own router in the same closure so it can be mounted under the conditional gate while the rest of the enforcer router keeps the strict gate. - Startup warning while the flag is off (the rollout default) so a fleet in the permissive phase stays visible. - The route audit stays green but its guarantee is now structural (a gate is attached), not that the gate rejects at runtime. - Enforce-by-default tests from #317/#320/#321 now opt into enforcement; new tests assert the permissive default. 140 pass, ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🔍 Vulnerabilities of
|
| digest | sha256:aad8cfba0f5dbd21d44e306e4527cba4222926039e4f082cd3b09da150e053b4 |
| vulnerabilities | |
| platform | linux/amd64 |
| size | 225 MB |
| packages | 264 |
📦 Base Image python:3.10-alpine3.22
| also known as |
|
| digest | sha256:c8f94b3bb77e6ea9015ccd091b7f8aec1b1fcbca95159675235d9a93788797cd |
| vulnerabilities |
Description
Description
Description
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Description
|
…pt (PER-15243) Read ENFORCE_OPERATIONAL_ROUTE_AUTH once and call enforce_pdp_token a single time: honour a rejection by re-raising when the flag is on, downgrade it to warn-and-allow when off. Behaviour is unchanged from the two-branch version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a rollout toggle (ENFORCE_OPERATIONAL_ROUTE_AUTH) to conditionally enforce PDP-token auth on a small set of operational endpoints (OPAL trigger routes, legacy update aliases, and /kong) to support coordinated fleet upgrades while still surfacing non-compliant callers via warnings.
Changes:
- Add
ENFORCE_OPERATIONAL_ROUTE_AUTHconfig flag (defaultFalse) and anenforce_pdp_token_operationaldependency that enforces when on, and warn-allows when off. - Gate OPAL-mounted trigger routes and legacy
/update_policy*aliases with the operational wrapper; split/kongonto its own router so only it uses the operational gate. - Update/extend tests and route-auth audit to recognize the operational wrapper as a structural auth gate, plus new tests for permissive-default behavior and startup/per-request warnings.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| horizon/authentication.py | Adds enforce_pdp_token_operational dependency implementing the rollout-aware gating behavior. |
| horizon/config.py | Adds ENFORCE_OPERATIONAL_ROUTE_AUTH configuration flag with detailed description. |
| horizon/enforcer/api.py | Splits /kong onto a dedicated router and returns (enforcer_router, kong_router) for separate mounting/gating. |
| horizon/pdp.py | Mounts /kong under the operational gate, injects the operational gate into OPAL trigger routes, gates legacy update aliases, and adds a startup warning when permissive. |
| horizon/tests/test_route_auth_audit.py | Updates the route-auth audit to treat enforce_pdp_token_operational as a recognized gate and asserts OPAL trigger routes carry it. |
| horizon/tests/test_opal_trigger_auth.py | Adds enforcement-on fixtures for strict-mode assertions; adds permissive-default coverage and log-warning assertions. |
| horizon/tests/test_legacy_update_routes.py | Updates unauth rejection tests to enable enforcement explicitly (since default is now permissive for these routes). |
| horizon/tests/test_enforcer_api.py | Enables enforcement during protected-endpoint sweeps (to cover /kong) and adds a permissive-default /kong test. |
| horizon/tests/test_authentication.py | Adds unit tests for enforce_pdp_token_operational behavior and warning logging in permissive mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… flood (PER-15243) While ENFORCE_OPERATIONAL_ROUTE_AUTH is off (the rollout default), enforce_pdp_token_operational logged one WARNING per would-be-rejected request. /kong is a per-decision endpoint (potentially high QPS), so during the shadow-mode window that floods the synchronous stdout sink and adds a blocking write to the request path. Rewrite the warn-and-allow log to a per-route coalesced counter: at most one line per route per 60s, carrying the number of tokenless requests allowed in that window. Every request is still counted; only the logging is rate-limited. The counter is guarded by a threading.Lock because the gate runs in FastAPI's threadpool. That per-route count also doubles as the rollout signal - watch it fall to zero per route, then flip the flag on. - authentication.py: add _operational_warn_should_emit throttle + reset_operational_warn_throttle. - conftest.py: autouse fixture resets throttle state per test so asserted warnings aren't suppressed. - test_authentication.py: add anti-flood (burst -> one line) and coalesced-count tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… wording (PER-15243) - authentication.py: import loguru's logger directly instead of opal_client.logger, matching the rest of horizon (e.g. pdp.py) and avoiding a dependency on opal_client internals for the rollout warnings and test log capture. Same singleton, so behaviour is unchanged. - authentication.py + pdp.py: the warn-and-allow branch also fires for an *invalid* token, not only a missing one, so reword "unauthenticated" -> "without a valid PDP token (missing or invalid)" in both the per-request and the startup warning for accurate log search/alerting. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The autouse throttle-reset fixture sits directly above a module-level `if`; ruff-format (the pre-commit hook, distinct from ruff check) requires two blank lines there. Whitespace only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t-upgrade-to-the-patched-image
zeevmoney
left a comment
There was a problem hiding this comment.
Reviewed with python-pro and fastapi-pro. Blast radius verified: confined to exactly the five operational routes (/kong, /policy-updater/trigger, /data-updater/trigger, /update_policy, /update_policy_data) and nothing else — an empirical route sweep of the built app shows 5 routes on the conditional gate, 48 still on strict enforce_pdp_token/OPAL, 9 public, 0 ungated; a runtime toggle sweep confirmed control routes (/allowed, /user-permissions, /nginx_allowed, /local/*) return 401 in both flag states. With the flag ON the five routes are byte-identical to the previous strict gate; the /kong router split leaves the 8 sibling enforcer routes, their gating, tags, and OpenAPI schema untouched. Auth suite: 126 passed.
Approving. All findings are MEDIUM or lower (test-coverage gaps + an observability nuance in the warn-and-allow log throttle); none block the rollout mechanism. Inline comments below.
| if now - last_emit >= _OPERATIONAL_WARN_INTERVAL_SECONDS: | ||
| _operational_warn_state[path] = (0, now) | ||
| return True, pending | ||
| _operational_warn_state[path] = (pending, last_emit) |
There was a problem hiding this comment.
[MEDIUM] Coalesced would-be-rejection count is silently dropped when a route falls idle
Problem: Pending counts are flushed only by a later request that arrives after the interval elapses — there is no periodic or shutdown flush. If a burst of unauthenticated calls hits a low-traffic route (e.g. /update_policy) and then traffic stops, the accumulated pending (every hit but the first) is never logged. Since this count is the documented rollout signal ("watch it fall to zero, then flip the flag on"), an operator can see a route go quiet and enable enforcement while the tail of lagging callers was never surfaced — the signal under-reports exactly when traffic is sparse, which is when it matters most. /kong self-flushes under load; the quiet update routes are where this bites.
Suggestion: Flush the residual on a low-frequency timer or at shutdown, or document that the count is best-effort and a nonzero residual can go unreported when a route falls idle — so "logs went quiet" is not read as "all callers migrated."
| if emit: | ||
| logger.warning( | ||
| "ENFORCE_OPERATIONAL_ROUTE_AUTH is off: allowed {count} request(s) without a valid PDP token " | ||
| "(missing or invalid) to {method} {path} in the last ~{interval}s that would otherwise be " |
There was a problem hiding this comment.
[LOW] "in the last ~{interval}s" can misrepresent the actual window
Problem: last_emit only advances on an actual emit, so count spans the entire gap since the previous emitted line — which is >= the interval and unbounded when traffic is sparse. A 100-request burst at t0 followed by a single request at t0+180s emits allowed 100 request(s) ... in the last ~60s, though 99 occurred 180s earlier. An operator reading the count as a per-minute rate is misled. (The count itself is accurate; only the fixed-window phrasing is wrong. Flagged independently by both review passes.)
Suggestion: Drop the "~{interval}s" claim, or report the real elapsed span (now - last_emit) instead of the constant.
| assert not any(self.WARN_SUBSTRING in record for record in capture_loguru) | ||
|
|
||
| @pytest.mark.usefixtures("enforce_off") | ||
| def test_enforce_off_coalesces_repeated_warnings(self, capture_loguru): |
There was a problem hiding this comment.
[MEDIUM] Per-route (vs. global) coalescing is never asserted
Problem: Every coalescing/count test drives a single path (/kong or /policy-updater/trigger). Nothing asserts that two different routes each get their own immediate first warning. A regression that collapsed the throttle to one global key — dropping the per-path dict keying that the design comment (authentication.py:74-75, "one line per route per interval") promises — would pass the entire suite. This is precisely the property the throttle exists to provide, and it is unguarded.
Suggestion: Add a test (flag off) that calls the gate for path A (asserts one warning), then path B, and asserts B also emits its own first warning — i.e. sum(...) == 2 across two distinct paths — proving the coalescing is keyed per route, not globally.
| # Simulate the interval having elapsed without waiting on the wall clock. | ||
| pending, _last = auth._operational_warn_state["/kong"] | ||
| auth._operational_warn_state["/kong"] = (pending, time.monotonic() - (auth._OPERATIONAL_WARN_INTERVAL_SECONDS + 1)) | ||
| assert auth._operational_warn_should_emit("/kong") == (True, 5) |
There was a problem hiding this comment.
[LOW] Count-accuracy test couples to private internals rather than observable behavior
Problem: test_operational_warn_throttle_coalesces_count reaches into auth._operational_warn_state["/kong"], hand-mutates the raw (pending, last_emit) tuple to fake elapsed time, and asserts the exact tuple return (True, 5) of the private _operational_warn_should_emit. A valid refactor of the throttle internals (different structure, token bucket, injected clock) would break this test even with identical observable behavior — the "test behavior, not implementation" concern. There is real tension here (verifying count accuracy without a 60s sleep is hard), so this is a judgment call, not a defect.
Suggestion: Prefer driving the public enforce_pdp_token_operational with monkeypatch.setattr(auth.time, "monotonic", ...) (or an injected time source) and asserting on the captured log lines/counts, so the test exercises the coalescing contract rather than the tuple shape.
|
|
||
|
|
||
| def reset_operational_warn_throttle() -> None: | ||
| """Clear the throttle state. For tests, whose asserted warnings must not be suppressed by a prior test.""" |
There was a problem hiding this comment.
[LOW] Docstring is a sentence fragment / not Google-style
Problem: "Clear the throttle state. For tests, whose asserted warnings must not be suppressed by a prior test." — the second sentence is an ungrammatical fragment ("For tests, whose ..."). Minor, but this is a public module-level function.
Suggestion: e.g. "Clear the process-global throttle state. Used by tests so an asserted warning is not suppressed by a warning already logged for the same path in a prior test."
Summary
The auth-hardening PRs (#317/#320/#321) made five operational routes — the update-trigger routes (
/policy-updater/trigger,/data-updater/trigger,/update_policy,/update_policy_data) and the/kongdecision endpoint — unconditionally require the PDP token. That's correct as an end state, but there's no way to relax it during a coordinated fleet upgrade, and some callers (notably the Kong OPA plugin and older SDK automations) may not send the token yet. Turning it on fleet-wide today would 401 live traffic with no warning.This PR adds one flag,
ENFORCE_OPERATIONAL_ROUTE_AUTH, that gates those five routes. It defaults to FALSE for a safe rollout: the routes accept unauthenticated requests and only log the ones that would have been rejected, so lagging callers keep working while the logs surface exactly who they are. Once the logs go quiet, flip the flag to true per-fleet and enforcement kicks in. Every other PDP route stays strictly enforced regardless of this flag.How the flag behaves per request
enforce_pdp_tokenWhy
/kongneeded a router split (and the other four routes didn't)This is the one non-obvious part of the diff, so it's worth spelling out.
A gate (auth dependency) can be attached at two levels:
The four update-trigger routes already carried their gate at the route level (the legacy routes via their
@app.post(..., dependencies=[...])decorator; the OPAL trigger routes via per-route injection in_gate_opal_trigger_routes). Swapping their strict gate for the new conditional one is a one-line change in place./kongwas different. Its gate came from the enforcer group:include_router(enforcer_router, dependencies=[Depends(enforce_pdp_token)])puts the strict gate on every enforcer route at once. In FastAPI a route can't opt out of a gate inherited from its group — you can add a gate to a single route but not remove the group-level one. So/kongcouldn't be made conditional while it lived in that group; the strict gate would always fire first and 401 the request before the conditional gate could be lenient.The fix is to pull
/kongonto its ownkong_routerso it can be mounted under the conditional gate while the rest of the enforcer router keeps the strict gate. It's a split, not a move —/kong's handler closes overkong_routes_tableand the nested_is_allowedhelper, so relocating it entirely would be a much larger refactor; a router split in the same closure avoids that.What changed, by file
horizon/config.py— newENFORCE_OPERATIONAL_ROUTE_AUTHbool (default False),PDP_-prefixed env var, also overridable from the cloud control plane.horizon/authentication.py— newenforce_pdp_token_operationalgate. Reads the flag per request (never captured at import, so a control-plane override takes effect immediately). Flag on → delegates toenforce_pdp_token. Flag off → downgrades a rejection to warn-and-allow. Kept as a distinct named callable so the fail-closed route audit still recognises it as a gate.horizon/enforcer/api.py—/kongmoved onto its ownkong_router;init_enforcer_api_routernow returns(router, kong_router).horizon/pdp.py— mountskong_routerunderenforce_pdp_token_operational; the update-trigger routes now use the same operational wrapper; adds a startup warning (_warn_if_operational_route_auth_disabled) so a fleet in permissive mode stays visible.Log throttling (specific to
/kong)/kongis a high-QPS endpoint, so one warning per unauthenticated request would flood the synchronous stdout sink and add a blocking write to the request hot path. The warn-and-allow path therefore coalesces to one line per route per ~60s, carrying a count of how many would-be rejections it allowed. That count is also the rollout signal — watch it fall to zero on/kong, then flip the flag on.Route audit note
The route audit stays green, but for
/kongand the trigger routes its guarantee is now structural — it asserts a gate is attached, not that the gate rejects at runtime (the gate warns-and-allows while enforcement is off). That's why the startup warning exists.Setting the flag at startup
Read per request, so it takes effect immediately with no rebuild. Defaults to FALSE (permissive rollout). Set via:
PDP_prefix):PDP_ENFORCE_OPERATIONAL_ROUTE_AUTH=truedocker run -e PDP_ENFORCE_OPERATIONAL_ROUTE_AUTH=true permitio/pdp-v2:latest{ name: PDP_ENFORCE_OPERATIONAL_ROUTE_AUTH, value: "true" }to the containerenv.Leave it unset/false during the coordinated upgrade; flip to true only after the per-request warnings go quiet.
Where to find the "would be rejected" logs
While the flag is off, would-be rejections are emitted through loguru at WARNING level, filterable by the string
ENFORCE_OPERATIONAL_ROUTE_AUTH:Locations:
docker logs <container> 2>&1 | grep ENFORCE_OPERATIONAL_ROUTE_AUTH/kubectl logs <pdp-pod> | grep ENFORCE_OPERATIONAL_ROUTE_AUTHCENTRAL_LOG_ENABLED=truewith a valid token.service:permit-pdp "ENFORCE_OPERATIONAL_ROUTE_AUTH is off".