Add Android VPN socket protection for SIP003 plugins#46
Add Android VPN socket protection for SIP003 plugins#46dalisyron wants to merge 1 commit intoMygod:mainfrom
Conversation
When running as a Shadowsocks plugin in Android VPN mode, the plugin's UDP sockets must be "protected" to bypass VPN routing. Otherwise, the plugin's traffic would be routed back through the VPN tunnel, creating an infinite loop. The protection mechanism uses a Unix domain socket at `./protect_path` to send the socket file descriptor to the Android VPN service, which then calls `VpnService.protect()` on it. Changes: - Add android.rs module with protect_socket() using SCM_RIGHTS - Parse __android_vpn flag from SS_PLUGIN_OPTIONS - Add android_vpn field to ClientConfig - Call protect_socket() after binding UDP socket when in VPN mode
|
This change is not necessary if you use shadowsocks/shadowsocks-android#3227. 😉 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2786a850c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Ok(false) => { | ||
| tracing::debug!("VPN protection not available (protect_path not found)"); | ||
| } |
There was a problem hiding this comment.
In Android VPN mode, android_vpn is only set when the __android_vpn flag is present, but this branch treats protect_socket returning Ok(false) as a soft failure and continues. That happens when protect_path is missing or the VPN service refuses the connection; in that case the UDP socket remains unprotected and traffic will loop back through the VPN tunnel, breaking connectivity (the exact scenario the change is meant to prevent). Consider treating Ok(false) as an error (or retry) when android_vpn is true so the client doesn’t run in a guaranteed-failure state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR adds Android VPN socket protection for Shadowsocks SIP003 plugins to prevent infinite routing loops when the plugin runs in Android VPN mode. When a plugin's UDP socket would normally be routed back through the VPN tunnel, this protection mechanism uses a Unix domain socket to send the socket file descriptor to the Android VPN service, which then marks it as protected from VPN routing.
Changes:
- New
android.rsmodule implementing socket protection via SCM_RIGHTS file descriptor passing - Parse
__android_vpnflag from SS_PLUGIN_OPTIONS environment variable - Thread
android_vpnconfiguration through ClientConfig to socket binding
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/slipstream-client/src/android.rs | New module implementing socket protection via Unix domain socket and SCM_RIGHTS, with timeout handling and error cases |
| crates/slipstream-core/src/sip003.rs | Extend allows_empty_value_key to accept __android_vpn flag without value, consistent with authoritative option |
| crates/slipstream-client/src/runtime/setup.rs | Call socket protection after UDP socket binding when android_vpn is enabled, with appropriate error handling |
| crates/slipstream-client/src/runtime.rs | Pass android_vpn flag from config to bind_udp_socket function |
| crates/slipstream-client/src/main.rs | Parse __android_vpn from plugin options and add to ClientConfig, with new has_option helper function |
| crates/slipstream-ffi/src/lib.rs | Add android_vpn boolean field to ClientConfig struct |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn allows_empty_value_key(key: &str) -> bool { | ||
| key.trim().eq_ignore_ascii_case("authoritative") | ||
| let key = key.trim(); | ||
| key.eq_ignore_ascii_case("authoritative") || key.eq_ignore_ascii_case("__android_vpn") | ||
| } |
There was a problem hiding this comment.
A test case should be added for the __android_vpn option parsing, similar to the existing test for the authoritative option (test allows_authoritative_without_value). This would ensure the parsing logic works correctly for this new option and follows the established testing pattern in the codebase.
|
Just to be clear, there is still some small marginal benefit for having this (mentioned in that PR). |
|
Thanks for your clarification. It's up to you, if you think having explicit network binding is worth the added complexity and maintenance cost of the code, let me know and I can re-open and finish the PR. Though, I personally, can't think of any scenarios where relying on the default routing would cause any issues even in multi-network scenarios. Especially considering the fact that Shadowsock's DefaultNetworkListener already only selects unrestricted networks: |
|
The shadowsocks-android PR mentioned earlier is now available on the latest GitHub release. |
When running as a Shadowsocks plugin in Android VPN mode, the plugin's UDP sockets must be "protected" to bypass VPN routing. Otherwise, the traffic is routed back through the VPN tunnel, creating an infinite loop.
The protection mechanism uses a Unix domain socket at
./protect_pathto send the socket file descriptor to the Android VPN service, which then callsVpnService.protect()on it.Changes:
android.rsmodule withprotect_socket()using SCM_RIGHTS__android_vpnflag from SS_PLUGIN_OPTIONSandroid_vpnfield toClientConfigprotect_socket()after binding UDP socket when in VPN modeTesting: