Extract client IP resolution and allowlist matching into ai.backend.common.client_ip
- Dominant language
- Python
- Stars
- 670
- Forks
- 183
- Avg merge
- 15h 13m
- Merged PRs (30d)
- 368
Description
### Goal
Move client IP handling into a single shared module, ai.backend.common.client_ip, holding two types with distinct responsibilities.
|Type|Responsibility|Built from|
|---|---|---|
|TrustedProxies (new)|Decide whether an address is a trusted proxy, and resolve the real client IP by walking the X-Forwarded-For chain inwards from the connection peer|Server configuration (manager.trusted-proxies, proxy_worker.trusted_proxies)|
|ClientIPValidator (moved from ai.backend.appproxy.common.client_ip)|Match a resolved client IP against an allowlist|Entity data (keypair allowed_client_ip, circuit allowed_client_ips)|
resolve_client_ip() takes plain arguments (peer address, raw header value) rather than an aiohttp Request, so the coordinator — which has no request object — and unit tests can use it directly and the module stays free of an aiohttp dependency.
### Semantics: empty means deny, absent means disabled
- An empty allowlist currently means 'no restriction', which makes an empty value and a disabled feature indistinguishable. Change it: ClientIPValidator([]) allows nothing.
- The feature is turned off by passing None instead of an empty validator. A caller constructs the validator only when there is something to enforce.
- Same rule for TrustedProxies: when the configuration is empty, no instance is constructed and the holder is None, so forwarding headers are not honored.
- Migration hazard: a stored empty allowlist (keypair allowed_client_ip = [], blank circuit allowed_client_ips) means 'unrestricted' today. The layer that constructs the validator must map an empty stored value to None so that existing rows keep their meaning; only an explicitly constructed empty validator denies everything.
### Dialects to reconcile
| |manager allowed_client_ip|appproxy allowed_client_ips|
|---|---|---|
|Input|list of CIDR strings (JSON array)|comma-separated string|
|Wildcard 10.1.**.**|supported via ReadableCIDR|not supported|
|Invalid entry|raises InvalidIpAddressValue|logged and skipped|
|Unparsable client IP|raises InvalidAuthParameters|rejected (fail-closed)|
- Absorb the input shapes with separate constructors (from_csv / from_networks).
- The error policy is the decision to make, not a detail: silently skipping a malformed entry loosens manager access control, while raising breaks every match for one bad circuit entry. Keep it selectable or document the divergence deliberately.
- ReadableCIDR already lives in ai.backend.common.types, so the wildcard dialect is available to both sides.
### Migration
1. Add ai.backend.common.client_ip with both types and their tests.
1. Manager: replace the inline chain walk added by BA-7252 (api/rest/middleware/auth.py) with TrustedProxies, and validate_ip() with the shared validator. Pure refactor — behavior must not change.
1. AppProxy: re-point ai.backend.appproxy.common.types and coordinator/models/circuit.py at the shared validator; ip_validator becomes ClientIPValidator | None and is_restricted goes away.
### Out of scope
Removing XForwardedStrict from the AppProxy worker is tracked separately — it is a live defect, not a refactor.
### Acceptance criteria
- ai.backend.common.client_ip exposes TrustedProxies and ClientIPValidator, with no aiohttp import.
- ClientIPValidator([]) allows nothing; callers express 'no restriction' with None.
- Existing rows with an empty stored allowlist stay unrestricted.
- Manager behavior is unchanged: the BA-7252 unit tests pass against the shared implementation without modification of their expectations.
- AppProxy coordinator and worker use the shared validator; ai.backend.appproxy.common.client_ip is gone.
- Unit tests for the resolver: zero hops, one hop, N trusted hops, an untrusted hop between trusted ones, a forged header from an untrusted peer.
### Related
- BA-7252 — added the inline chain walk in the manager that this issue generalizes.
- Traefik's ipStrategy.excludedIPs, configured by the coordinator, already implements the same walk; the shared type makes that equivalence explicit.
JIRA Issue: BA-7319
Contributor guide
Research direction
Start with the inline chain walk in api/rest/middleware/auth.py, the existing AppProxy client-IP modules, and the BA-7252 manager unit tests. Trace how manager and AppProxy construct their allowlists and trusted-proxy settings, then run the resolver and existing manager tests. Done means the shared module has both types without an aiohttp import, existing empty stored allowlists remain unrestricted, and the listed manager and AppProxy migrations are complete.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- api, backend, security
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100