Skip to content
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

Get rid of -V flag #1742

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Get rid of -V flag #1742

wants to merge 4 commits into from

Conversation

Mygod
Copy link
Contributor

@Mygod Mygod commented Apr 9, 2018

🚧 ONLY INTENDED FOR DEBUGGING PURPOSES DO NOT MERGE 🚧

By disallowing this app itself, every socket created by this app will go through default network automatically. However, since it's called disallowing, Android system won't let us bind to VPN tunnel. Therefore we will have to resort to using proxies to connect with Shadowsocks.

Pros:

  1. No more ancillary fd passing;
  2. No more go lib patching;
  3. Easier plugin development.

Cons:

  1. Ads might not work if Google is blocked; (unless it goes through Google Service Framework)
  2. Connection test will have to test through proxy instead of the better testing method through VPN tunnel;
  3. All sockets will use default network and therefore we are no longer using underlying network;
  4. Only works on Android 5.0+.

@Mygod Mygod requested a review from madeye April 9, 2018 04:11
@madeye
Copy link
Contributor

madeye commented Apr 9, 2018

I planed to do this years ago, but it wouldn't work if we need to support Android 4.x at that time.

One concern is about the plugin mode. Would it also work with SIP003 plugin?

@Mygod Mygod mentioned this pull request Apr 9, 2018
11 tasks
@Mygod
Copy link
Contributor Author

Mygod commented Apr 9, 2018

@madeye Yes since we are running plugins under our UID.

@Mygod
Copy link
Contributor Author

Mygod commented Apr 9, 2018

Disclaimer: I actually haven't tested it with plugins. But theoretically it should. But I've had enough debugging for a day.

@madeye
Copy link
Contributor

madeye commented Apr 9, 2018

Never mind, I submitted a fix for it.

@@ -159,6 +159,8 @@ class VpnService : BaseVpnService(), LocalDnsService.Interface {
}
}
if (profile.bypass) builder.addDisallowedApplication(me)
} else {
builder.addDisallowedApplication(packageName)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After adding these two lines, I can no longer reproduce #1740. Good catch though!

@Mygod
Copy link
Contributor Author

Mygod commented Apr 10, 2018

After thinking more about this, I think this is not the way Android VpnService is intended to be used, so even though this might work, it's still more of a hack. I think we could keep this open for plugin developers as a debugger tool?

EDIT: Considering that go 1.11 has been used, and there's no way to "unprotect" the socket if the app adds itself to disallowed applications, it became even less reasonable to merge this.

@madeye
Copy link
Contributor

madeye commented Apr 10, 2018

Sure. Let's keep it open in case someone else may find it useful.

@ayanamist
Copy link
Contributor

Since golang/go#9661 is fixed, "go lib patching" is no more necessary in upcoming Go 1.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants