-
Notifications
You must be signed in to change notification settings - Fork 7.6k
feat(network-proxy): add a SOCKS5 proxy with policy enforcement #9803
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
Conversation
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e8470a866
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let socks_addr = resolve_addr(&cfg.network_proxy.socks_url, 8081).with_context(|| { | ||
| format!( | ||
| "invalid network_proxy.socks_url: {}", | ||
| cfg.network_proxy.socks_url | ||
| ) |
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.
Gate SOCKS URL validation on SOCKS5 enablement
resolve_runtime now parses network_proxy.socks_url unconditionally, but NetworkProxyBuilder::build calls resolve_runtime even when enable_socks5 is false. This means an invalid or empty socks_url (e.g., a placeholder in existing configs) will now prevent the entire proxy (HTTP + admin) from starting, despite SOCKS5 being disabled. Consider only resolving/clamping the SOCKS address when enable_socks5 is enabled, or defaulting it without validation in the disabled case.
Useful? React with 👍 / 👎.
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5fb56e60b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let host = normalize_host(&server_address.ip_addr.to_string()); | ||
| let port = server_address.port; |
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.
Enforce SOCKS5 UDP policy against original hostnames
The UDP inspector normalizes server_address.ip_addr, which is the resolved IP, and uses that for NetworkPolicyRequest. Because host_blocked matches allowed_domains/denied_domains against the host string, domain-based rules will not apply to UDP packets: a client that sends UDP to evil.example will be evaluated against its IP and can slip past a domain denylist (e.g., allowed_domains=["*"], denied_domains=["evil.example"]), while allowlisted domains can be blocked unless their IPs are explicitly listed. This only happens when enable_socks5_udp = true and the UDP target is a hostname, but it undermines the host policy for UDP traffic. If RelayRequest exposes the original hostname from the SOCKS5 UDP header, use that for policy evaluation; otherwise consider rejecting UDP when domain-based policies are configured.
Useful? React with 👍 / 👎.
Summary
rama-socks5Config
New/used fields under
network_proxy:enable_socks5socks_urlenable_socks5_udpScope
codex-rs/network-proxy(+codex-rs/Cargo.lock)Testing