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

Integrate shadowsocks-rust #2454

Merged
merged 58 commits into from
Apr 20, 2020
Merged

Integrate shadowsocks-rust #2454

merged 58 commits into from
Apr 20, 2020

Conversation

madeye
Copy link
Contributor

@madeye madeye commented Feb 23, 2020

This PR aims to replace shadowsocks-libev with shadowsocks-rust.

Fix issue #2452.

  • Gradle and Cargo integration
  • Build pass
  • VPNService callback
  • TCP relay
  • DNS realy
  • UDP relay
  • ACL (IP rules)
  • ACL (domain name rules)
  • Traffic statistics

Nice to have:

  • Parallel lookup to local and remote
  • Unix domain socket to local DNS resolver.

Future development plan (maybe not covered by this PR)

  • A local DNS cache
  • Local DNS resolver in Rust
  • Fine-grained policy routing
  • Multiple upstreams

@madeye

This comment has been minimized.

@Mygod

This comment has been minimized.

@madeye

This comment has been minimized.

@Mygod

This comment has been minimized.

@madeye
Copy link
Contributor Author

madeye commented Feb 24, 2020

The naming issue should be fixed via a51ad37.

@madeye
Copy link
Contributor Author

madeye commented Feb 27, 2020

I've finished the protect() callback and get the TCP relay, UDP relay, ACL (IP rules) works now.

Further development:

  1. Traffic stat requires changes in shadowsocks-rust's local client. I have already added the stat_path IPC interface. @zonyitoo can help to implement the traffic stat logic in local client.
  2. ACL (domain name rules) requires reverse local DNS lookup. @Mygod can help with this part. It requires our local DNS resolver supports PTR queries for in-addr.arpa addresses. We may need a IP/domain map in our local DNS resolver for this feature.

@zonyitoo
Copy link

zonyitoo commented Feb 27, 2020

Traffic stat requires changes in shadowsocks-rust's local client. I have already added the stat_path IPC interface. @zonyitoo can help to implement the traffic stat logic in local client.

Easy peasy. I will try to do that in this weekend.

@Mygod
Copy link
Contributor

Mygod commented Feb 27, 2020

@madeye In my proposal, I was saying that there is no need for a reverse DNS lookup -- this can be easily done using a reverse lookup map in sslocal. I think this is the better way as it also saves a RPC.

@madeye
Copy link
Contributor Author

madeye commented Feb 27, 2020

@Mygod Okay, if so, we need to implement the local resolver in sslocal. Can you help on this?

I think it can be implemented based on trust-dns. The good news is shadowsocks-rust has already used it for safe DNS resolving. What we need is to extend it to a ChinaDNS like local resolver.

@Mygod
Copy link
Contributor

Mygod commented Feb 27, 2020

I could if y'all can wait for me to pick up rust first. :)

@Mygod
Copy link
Contributor

Mygod commented Feb 28, 2020

Oh I was thinking of putting the code at core/src/main/rust/shadowsocks-rust and we can put other ancillary code at the rust directory... Let me see if I will have time to work on this over the weekends.

@zonyitoo
Copy link

zonyitoo commented Feb 29, 2020

It seems that adding a flow stat for sslocal is far more complicated than I was expected. :(

I am going to merge implementation of manager stat report in ssserver to make a unified manager report module both works for clients (TCP, UDP, HTTP, ...) and servers.

@madeye
Copy link
Contributor Author

madeye commented Feb 29, 2020

No hurry... In shadowsocks-libev, I have a counter in the context and a timer to push the counter value to shadowsocks-android.

@zonyitoo
Copy link

Because shadowsocks-rust could be run as a library, so I have to manage these stat variables by using reference counter.

@Mygod
Copy link
Contributor

Mygod commented Feb 29, 2020

Well it looks like this repo is going to take more than 4GB of RAM to build. Let me try again tomorrow. :)

@zonyitoo
Copy link

zonyitoo commented Feb 29, 2020

Well it looks like this repo is going to take more than 4GB of RAM to build. Let me try again tomorrow. :)

Consider disable LTO. It takes quite a lot of memory while linking. But output binary would be larger.

build.gradle Outdated
@@ -14,6 +14,7 @@ buildscript {
junitVersion = '4.13'
androidTestVersion = '1.2.0'
androidEspressoVersion = '3.2.0'
ndkVersion = '21.0.6113669'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to specify ndkVersion btw?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see the warning message that ANDROID_NDK_HOME is deprecated now. Any suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we are using that env?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, I cannot get the project built with Android Studio 4.0...

noDefaultBut "sodium", "android", "rc4", "aes-cfb", "aes-ctr", "camellia-cfb", "openssl-vendored"
}
exec { spec, toolchain ->
spec.environment("RUSTFLAGS", "-C link-arg=-o -C link-arg=target/${toolchain.target}/${profile}/libsslocal.so")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would think that the plugin would take care of the output so name... Is this an upstream issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's actually a limitation of Cargo, which cannot customize the output binary name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Their sample app is far less involved than this. I would assume it's upstream bug (I wonder if anyone is actually using the plugin).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I got the workaround here: rust-lang/cargo#1706 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah as I figured nobody is actually maintaining this plugin.

@Mygod Mygod force-pushed the rust branch 3 times, most recently from deb5de4 to 1bd347a Compare March 1, 2020 01:38
@madeye
Copy link
Contributor Author

madeye commented Mar 1, 2020

I think the submodule is not correctly moved in this branch...

@madeye
Copy link
Contributor Author

madeye commented Mar 1, 2020

The latest commit should fix the issue.

@Mygod
Copy link
Contributor

Mygod commented Mar 1, 2020

Yeah sorry for force pushing lol. @madeye

@Mygod
Copy link
Contributor

Mygod commented Mar 1, 2020

Sadly it seems like I don't have time to understand the rust part for now, so instead I just added a JVM part for local_dns_path. You can use it just like a nonreusable TCP DNS (DNS request/response format <length:i16><payload>). The implementation is extremely drafty for now but it will serve its purpose for the time being. Thanks guys for the hard work! 👍

@Mygod
Copy link
Contributor

Mygod commented Mar 1, 2020

P.S. It is only enabled for VpnService for now because I am not sure what is going on in shadowsocks-rust for now.

There is a possibly more efficient implementation -- we can let local_dns_path instead simply return the desired Network.getNetworkHandle(): net_handle_t and then shadowsocks-rust can handle the DNS resolving using Android NDK APIs android_getaddrinfofornet (undocumented), android_getaddrinfofornetwork on API 23+ or android_res_nsend on API 29+. (if dynamic linking is not too hard)

@madeye
Copy link
Contributor Author

madeye commented Mar 1, 2020

Currently, there's no DNS queries from shadowsocks-rust, as the SOCKS5 requests from tun2socks are all IPv4 or IPv6 types, no domain name. That's also why ACL (domain name) doesn't work now.

With the local_dns_path, I will try to implement an internal reverse DNS resolver to enable ACL (domain name) first.

@Mygod
Copy link
Contributor

Mygod commented Mar 1, 2020

@madeye No... I am thinking that sslocal will listen on port 5450 (or local DNS port) directly to handle the DNS traffic, and use local_dns_path to do local DNS lookups. I want to minimize the packet handling in JNI and use Rust as much as possible. :)

@Mygod
Copy link
Contributor

Mygod commented Mar 1, 2020

I would like to propose the following to-do list (based on the one in OP), with priorities.

  • ACL function
  • Traffic statistics (assigned to @zonyitoo?)
  • Move LocalDnsServer implementation to rust using local_dns_path (but I guess we could start with using LocalDnsServer for all the work, and change local_dns_path to use LocalDnsServer instead? but then we need to write some code that we will later throw away)

Lower priority:

  • Investigate the easiest way to do DNS resolving in rust in proxy/transproxy mode (in these mode we always send our traffic to default network. should we use local_dns_path vs some native approach in rust?)
  • Investigate possibility of using android_res_nsend directly in sslocal? Integrate shadowsocks-rust #2454 (comment)
  • Implement async I/O for LocalSocket in JNI (assigned to @Mygod)
  • Investigate binary size and make rust binary smaller? (say https://github.com/johnthagen/min-sized-rust?)
  • Code review & cleanup after we get things working

Lowest priority: (these are usually new features, can implement these in the future)

  • Fine-grained policy routing
  • Rust tun2socks? (https://github.com/iCepa/tun2tor)
  • If we have multiple rust binary, maybe we can make a master binary so that stdlib is shared, etc
  • Make tun2socks pass source address so that we can look up connection by UID (useful for firewall, etc)
  • Multiple upstreams (I wonder what an intuitive user interface would look like for this)

Thanks everyone again for the hard work!

Copy link
Contributor

@Mygod Mygod left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the rust part yet (probably next thing to do for me) but Android part looks good to me, besides the following comments:

  • This is related to multiple upstreams but we should implement UDP fallback support?
  • We should remove hosts setting or add hosts support?
  • We should remove TFO setting or TFO support?
  • Build speed is unreasonably slow. (use debug profile for debugging and CI builds? enable one target when building debug build? if only the gradle plugin is still being maintained)

For future:

url = https://github.com/google/re2.git
[submodule "core/src/main/rust/shadowsocks-rust"]
path = core/src/main/rust/shadowsocks-rust
url = https://github.com/madeye/shadowsocks-rust
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be changed to shadowsocks/shadowsocks-rust?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shadowsocks/shadowsocks-rust#211 is not merged yet.

After that, we should change the submodule to upstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned I think that PR doesn't seem too important for this PR right now. 🤣

@madeye
Copy link
Contributor Author

madeye commented Apr 19, 2020

This is related to multiple upstreams but we should implement UDP fallback support?

A future plan maybe.

We should remove hosts setting or add hosts support?

Ditto. For now, I think we can remove this feature.

We should remove TFO setting or TFO support?

I suggest remove TFO as well, which is kind of broken in real usage.

Build speed is unreasonably slow. (use debug profile for debugging and CI builds? enable one target when building debug build? if only the gradle plugin is still being maintained)

Yes, I think we should do that. Previously, I tried to use rust debug target in debug build, but failed.

It's not a bad idea to do some cleanup now. IMO, many features like HOSTS and TFO are not quite useful.

@Mygod
Copy link
Contributor

Mygod commented Apr 19, 2020

I agree except for UDP fallback because this is the only way to get UDP support for plugins. (UDP is useful for VoIP, gaming, etc and plugin is useful for apparent reasons). Maybe it's best to resolve this before merging it?

@madeye
Copy link
Contributor Author

madeye commented Apr 19, 2020

Okay, let's assume no multi-upstream for now. So, the UDP fallback logic should be the same as before.

@Mygod
Copy link
Contributor

Mygod commented Apr 19, 2020

Addressed UDP fallback via a252f19 for now (not tested yet) but the real fix should probably be implemented via some kind of multi-upstream, as now we are putting DNS relay inside sslocal.

What's left to do is probably to address build speed and binary size... (22 minutes of CI build is really slow)

@madeye
Copy link
Contributor Author

madeye commented Apr 19, 2020

Now the CI duration dropped to 10m 8s. https://app.circleci.com/pipelines/github/shadowsocks/shadowsocks-android/8/workflows/8ad4e812-60c7-4aa5-8423-e2d944ac0b20/jobs/526

Copy link
Contributor

@Mygod Mygod left a comment

Choose a reason for hiding this comment

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

-u does not seem to be working in shadowsocks-rust, other than that, LGTM.

(I guess this would be the best binary size we could get, good enough for me)

P.S. Once we fix UDP fallback, we can merge this and remove TFO and hosts in separate PRs.

@Mygod
Copy link
Contributor

Mygod commented Apr 20, 2020

Oh should probably update license too due to gitmodules changes...

@madeye
Copy link
Contributor Author

madeye commented Apr 20, 2020

UDP relay should work now.

@madeye
Copy link
Contributor Author

madeye commented Apr 20, 2020

Tested locally, fallback UDP relay works now.

@madeye
Copy link
Contributor Author

madeye commented Apr 20, 2020

I think this PR is ready for merge.

Thanks for all the contributions!

@madeye madeye merged commit 66f01b6 into master Apr 20, 2020
This was referenced Apr 20, 2020
@tiancaicx

This comment has been minimized.

@Mygod Mygod deleted the rust branch April 15, 2021 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Considering alternative coroutine sslocal impls
4 participants