-
Notifications
You must be signed in to change notification settings - Fork 13
feat: add ENFORCE_OPERATIONAL_ROUTE_AUTH rollout toggle (PER-15243) #323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
1aea110
6b9bc72
b958948
88679bc
baee764
0ac38f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,11 @@ | ||
| import hmac | ||
| import threading | ||
| import time | ||
| from typing import Annotated | ||
|
|
||
| from fastapi import Depends, HTTPException, status | ||
| from fastapi import Depends, HTTPException, Request, status | ||
| from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer | ||
| from loguru import logger | ||
|
|
||
| from horizon.config import MOCK_API_KEY, sidecar_config | ||
| from horizon.startup.api_keys import get_env_api_key | ||
|
|
@@ -65,6 +68,84 @@ def enforce_pdp_token(credentials: PdpCredentials = None): | |
| raise HTTPException(status.HTTP_401_UNAUTHORIZED, detail="Invalid PDP token") | ||
|
|
||
|
|
||
| # Rate-limit for the warn-and-allow path of enforce_pdp_token_operational. While | ||
| # ENFORCE_OPERATIONAL_ROUTE_AUTH is off, every unauthenticated request to a governed route would | ||
| # otherwise emit one WARNING - which on the high-QPS /kong decision endpoint floods the synchronous | ||
| # stdout log sink (and adds a blocking write to the request path). Instead we coalesce to one line | ||
| # per route per interval that reports how many would-be rejections were allowed; that count is also | ||
| # the rollout signal - watch it fall to zero, then flip the flag on. The gate runs in a threadpool | ||
| # (it is a sync dependency), so the shared counters are guarded by a plain threading.Lock. | ||
| _OPERATIONAL_WARN_INTERVAL_SECONDS = 60.0 | ||
| _operational_warn_lock = threading.Lock() | ||
| # path -> (would-be rejections accumulated since the last emitted line, monotonic time of that emit) | ||
| _operational_warn_state: dict[str, tuple[int, float]] = {} | ||
|
|
||
|
|
||
| def reset_operational_warn_throttle() -> None: | ||
| """Clear the throttle state. For tests, whose asserted warnings must not be suppressed by a prior test.""" | ||
| with _operational_warn_lock: | ||
| _operational_warn_state.clear() | ||
|
|
||
|
|
||
| def _operational_warn_should_emit(path: str) -> tuple[bool, int]: | ||
| """Decide whether to emit the warn-and-allow line for ``path`` now, coalescing bursts. | ||
|
|
||
| Returns ``(emit_now, count_since_last_emit)``. Every call counts as one would-be rejection; a | ||
| line is emitted on the first hit for a path and then at most once per | ||
| ``_OPERATIONAL_WARN_INTERVAL_SECONDS``, carrying the number of rejections coalesced into it. | ||
| """ | ||
| now = time.monotonic() | ||
| with _operational_warn_lock: | ||
| state = _operational_warn_state.get(path) | ||
| if state is None: | ||
| _operational_warn_state[path] = (0, now) | ||
| return True, 1 | ||
| pending, last_emit = state | ||
| pending += 1 | ||
| if now - last_emit >= _OPERATIONAL_WARN_INTERVAL_SECONDS: | ||
| _operational_warn_state[path] = (0, now) | ||
| return True, pending | ||
| _operational_warn_state[path] = (pending, last_emit) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [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. 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." |
||
| return False, pending | ||
|
|
||
|
|
||
| def enforce_pdp_token_operational(request: Request, credentials: PdpCredentials = None): | ||
| """PDP-token gate for the operational routes hardened by PER-15244/PER-15245, with a rollout default. | ||
|
|
||
| Governed by ``ENFORCE_OPERATIONAL_ROUTE_AUTH``. When it is true this is exactly ``enforce_pdp_token``. | ||
| When it is false - the default, for a safe fleet rollout - a request that would be rejected is allowed | ||
| through but logged, so callers that don't yet send the PDP token keep working while the logs surface | ||
| them before enforcement is switched on. | ||
|
|
||
| The flag is read per-request (never captured at import) so a cloud control-plane override takes | ||
| effect and tests can toggle it. Kept as a distinct, named module-level function because the | ||
| fail-closed route audit recognises auth gates by callable name - a bare ``enforce_pdp_token`` here | ||
| could not carry the conditional behaviour, and an inline lambda would be invisible to the audit. | ||
| """ | ||
| # Reuse enforce_pdp_token's exact reject logic in one place. When the flag is on, a rejection is | ||
| # honoured (re-raised). When it is off - the rollout default - the rejection is downgraded to | ||
| # warn-and-allow so no caller breaks; every would-be rejection is still counted, and surfaced in a | ||
| # per-route coalesced warning (see _operational_warn_should_emit) rather than one line per request. | ||
| try: | ||
| enforce_pdp_token(credentials) | ||
| except HTTPException as exc: | ||
| if sidecar_config.ENFORCE_OPERATIONAL_ROUTE_AUTH: | ||
| raise | ||
| emit, coalesced = _operational_warn_should_emit(request.url.path) | ||
| 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 " | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] "in the last ~{interval}s" can misrepresent the actual window Problem: Suggestion: Drop the "~{interval}s" claim, or report the real elapsed span ( |
||
| "rejected (e.g. {detail}). Set ENFORCE_OPERATIONAL_ROUTE_AUTH=true to enforce the PDP token " | ||
| "on this route.", | ||
| count=coalesced, | ||
| method=request.method, | ||
| path=request.url.path, | ||
| interval=int(_OPERATIONAL_WARN_INTERVAL_SECONDS), | ||
| detail=exc.detail, | ||
| ) | ||
|
|
||
|
|
||
| def enforce_pdp_control_key(credentials: PdpCredentials = None): | ||
| if sidecar_config.CONTAINER_CONTROL_KEY == MOCK_API_KEY: | ||
| raise HTTPException( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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."