fix(passthrough): enforce the borrowed model's client-IP allowlist (#557 parity) - #704
Merged
Merged
Conversation
parity) Every typed handler gates on dispatch::check_ip_access, but the raw /passthrough/:provider/* tunnel resolved a model entry (which can carry allowed_cidrs) without ever checking it — and never resolved a client IP at all. An operator's per-model IP restriction was bypassable by lending the same credentials through passthrough. Extract ClientContext in the handler and gate on the borrowed model's allowed_cidrs right after ACL resolution — the same borrowed-model basis as the #911 [6] guardrail chain. Oneshot/no-peer requests fail closed against a configured allowlist, matching the typed handlers. Fixes #697
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe passthrough handler now captures client IP context via ChangesPassthrough IP Allowlist Enforcement
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PassthroughHandler
participant Dispatch
participant Upstream
Client->>PassthroughHandler: request with ConnectInfo
PassthroughHandler->>PassthroughHandler: extract ClientContext.source_ip
PassthroughHandler->>Dispatch: dispatch(..., source_ip)
Dispatch->>Dispatch: check_ip_access(model, source_ip)
alt IP not allowed
Dispatch-->>PassthroughHandler: 403 Forbidden
else IP allowed
Dispatch->>Upstream: forward request
Upstream-->>Dispatch: response
Dispatch-->>PassthroughHandler: response
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Audit finding #697 (parent: api7/AISIX-Cloud#950). Every typed handler gates on
dispatch::check_ip_access(#557), but the raw/passthrough/:provider/*tunnel resolved a model entry — which can carryallowed_cidrs— without checking it, and never resolved a client IP at all. A per-model IP restriction was bypassable by lending the same credentials through passthrough.Fix
Extract
ClientContextin the passthrough handler and callcheck_ip_accesson the borrowed model right after the #449 ACL resolution — the same borrowed-model basis the #911 [6] guardrail resolution uses. Requests with no resolvable peer fail closed against a configured allowlist, matching the typed handlers.LiteLLM has no per-model client-IP allowlist equivalent on its passthrough routes — no baseline to compare against; this restores internal parity with our own #557 feature.
Tests
Router-level: a model with
allowed_cidrs: [10.0.0.0/8]→ oneshot (no peer) gets 403permission_deniedand the upstream is never contacted; injectingConnectInfo(10.1.2.3)passes through 200. Verified fail-before/pass-after.Fixes #697
Summary by CodeRabbit
403 Forbiddenbefore reaching the upstream service.