Repository navigation
feat(api): add optional fastapi-guard security middleware - #5664
Conversation
Off by default (XINFERENCE_GUARD_ENABLED=1 to enable): IP block/allow lists, general rate limiting with auto-ban, user-agent blocking, penetration detection, passive mode, and optional Redis-backed shared state. Ships as a 'guard' extra; nothing is imported or attached when disabled. Complements the XINFERENCE_ALLOWED_IPS allowlist (which stays first in line) and the auth rate limiter (which stays auth-scoped).
qinxuye
left a comment
qinxuye
left a comment
There was a problem hiding this comment.
Reviewed current head 2961609d724840f8dbabb6d2a37a110da15163c4. The optional/lazy integration is useful, but the four inline issues need addressing before approval: client-IP spoofing bypasses the new controls; default body scanning rejects legitimate inference input; guard rejections bypass the existing audit/IP wrapper; and the default response policy disables Web UI microphone recording.
Validation: with fastapi-guard==8.1.0 and its allowed guard-core==4.3.2, the focused guard/audit/streaming/metrics tests produced 36 passed, 1 skipped. I additionally exercised the middleware assembled by RESTfulAPI.serve() through Uvicorn's loaded ASGI application, which reproduces the failures below; the existing bare-app guard tests do not cover that integration. Basic streaming still worked in this probe. Please add the regression cases described in each thread so these behaviors are covered together.
Also, the repository's pinned Black 25.1.0 reports that both new Python files would be reformatted; please run the configured formatting hooks. This is separate from the four behavioral findings.
Current GitHub checks show the two ReadTheDocs builds passing, but no Python CI results are visible for this head. The PR is mergeable; this review is a COMMENT, not an approval.
… with review Review-response pass (qinxuye's four findings): - Client identity: serve() launched Uvicorn with forwarded_allow_ips="*", so Uvicorn rewrote scope["client"] from any peer's X-Forwarded-For before middleware ran and the guard's proxy trust was moot. When the guard is enabled, only proxies listed in XINFERENCE_GUARD_TRUSTED_PROXIES may rewrite the client (loopback-only otherwise); Uvicorn and guard read the same list, and guard's own trusted_proxies no longer falls back to RFC1918 ranges. Guard off keeps the historical behavior. - Body scanning: penetration detection no longer reads or signature-scans prompt bodies by default (model input is data, not commands this API executes; SQL/HTML/shell chat text was rejected with 400). URL, query, and header screening stay on; XINFERENCE_GUARD_SCAN_BODY opts in. - Ordering: attach_guard runs before the audited IP middleware registers, so guard nests inside it and its rejections keep audit recording and X-Request-ID, with the XINFERENCE_ALLOWED_IPS allowlist still first. - Headers: the default Permissions-Policy now allows same-origin microphone/camera so the Web UI audio recorder keeps working (XINFERENCE_GUARD_PERMISSIONS_POLICY overrides the whole policy). Regression tests through a real Uvicorn server thread: untrusted peers cannot spoof or rotate X-Forwarded-For past blocklist/limiter, configured proxies are honored, guard rejections stay audited, ordinary code/security prompts pass with scanning off (and are blocked when opted in), and the Permissions-Policy preserves microphone access.
|
All four findings are addressed in 06b5268, each with a regression test (replies in-thread): Uvicorn/guard now share one explicitly trusted client-IP resolution (XINFERENCE_GUARD_TRUSTED_PROXIES, loopback-only by default when the guard is enabled, "*" behavior untouched when disabled), prompt bodies are no longer signature-scanned by default (XINFERENCE_GUARD_SCAN_BODY opts in), the guard nests inside the audited IP middleware (rejections keep audit and X-Request-ID), and the default Permissions-Policy preserves same-origin microphone/camera for the Web UI. Both new files are reformatted with the repo-pinned Black 25.1.0. Local run of the focused guard suite with fastapi-guard 8.1.0 / guard-core 4.3.2: 13 passed, 1 skipped. |
qinxuye
left a comment
qinxuye
left a comment
There was a problem hiding this comment.
Re-reviewed 06b5268. The client-IP trust, inference-body scanning, and microphone-policy fixes are verified, and those three threads are resolved. The audit/request-ID part of the middleware-order fix also works. One part of the existing ordering finding remains:
[P2] Keep CORS outside guard-generated rejection responses
In RESTfulAPI.serve(), attach_guard() still runs after CORS registration (current code). The effective order is audit/IP -> SecurityMiddleware -> CORSMiddleware. Therefore guard-generated early responses never reach CORS.
I reproduced this with the actual serve() middleware assembly, XINFERENCE_GUARD_ENABLED=true, and Origin: https://client.example: an allowed request returns 200 with Access-Control-Allow-Origin, but an IP-blocked request returns 403 without it. With XINFERENCE_GUARD_RATE_LIMIT=1, the second request returns 429 with Retry-After: 60 but likewise lacks Access-Control-Allow-Origin. Both rejection responses are audited and carry X-Request-ID, so the audit fix is confirmed; the missing CORS handling is the remaining issue. Cross-origin browser clients cannot read these error responses and receive a CORS/network failure instead of the intended denial or rate-limit status.
Please register guard before the CORS middleware while keeping the existing audit/IP wrapper outside both. Add regressions using the production middleware assembly and an Origin header for guard-generated 403 and 429 responses: check CORS headers, audit recording, and request IDs, and retain the existing allowlist-first behavior. This is the unfinished CORS portion of the original ordering thread, which remains open.
Focused local validation: 44 passed, 1 skipped.
qinxuye's re-review reproduced that guard still registered after CORSMiddleware, so guard-generated 403/429 responses left without Access-Control-Allow-Origin and cross-origin browser clients got an opaque network failure instead of the denial. attach_guard now runs first in serve(), making the effective chain audit/IP -> CORS -> guard -> app: guard rejections gain CORS headers, keep audit recording and X-Request-ID, and the XINFERENCE_ALLOWED_IPS allowlist still rejects before the guard sees the request. Regressions drive the production assembly with an Origin header: a guard 403 and a guard 429 both assert CORS headers plus X-Request-ID, and the allowlist-first case asserts the existing IP check still denies ahead of the guard.
|
Thanks for the two rounds of hands-on review - the Uvicorn-level probes caught real gaps that the unit suite couldn't see (proxy-header identity, CORS on guard rejections), and both rounds made the middleware better. The fixes shipped straight back into every other fastapi-guard integration we maintain. Pleasure working through it. |
3 participants
Closes #5663
Summary
Opt-in fastapi-guard security middleware wired into
RESTfulAPI.serve()right after theaudited_ip_middlewareblock, so the existingXINFERENCE_ALLOWED_IPSallowlist still runs first and the audit middleware still wraps everything. Default off:XINFERENCE_GUARD_ENABLEDunset means no imports, no middleware, zero overhead. Enabled-but-not-installed raises a clear error naming the extra install command (same shape as the OTEL block's error handling).What Changes
xinference/api/guard_integration.py: env-driven fastapi-guard wiring (attach_guard(app),_build_guard_config()). Config is read at call time for the same fork-safety reasonis_auth_advanced()reads at call timexinference/api/restful_api.py: oneattach_guard(self._app)call inserve()between the IP-allowlist middleware and the OTEL setupguardextra inpyproject.toml(fastapi-guard>=8.1.0,<9.0.0), following theotelextra's lazy-import pattern; the core dependency set doesn't growXINFERENCE_GUARD_IPINFO_TOKEN), Redis-backed shared counters and bans (XINFERENCE_GUARD_REDIS_URL- relevant for multi-instance deployments behindxinference-router; the single supervisor process matches the existing auth rate limiter's per-process design)enable_redis=Falseunless a URL is configured: an opt-in feature must not require a running RedisTest Plan
xinference/api/tests/test_guard_integration.py: disabled-by-default no-op, fail-loud without the package, blocked IP 403, rate-limit 200/200/429, passive mode never blocks, Redis-off default. Env state is snapshotted/restored per test; each scenario uses its own TEST-NET client IP because guard's rate-limit state is process-wide. Runs without services (--noconftestlocally since the root conftest spawns a cluster; in CI the standard job env covers it)