Opened 6 months ago

Last modified 3 weeks ago

#30199 needs_revision defect

tor-android-service: Review 2019/04/16

Reported by: sysrqb Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, tbb-8.5, TorBrowserTeam201910
Cc: sisbell Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Overall, nicely done. This review is on master (6a9314aff4418a4edac33ff39fae266b097cf000):

the initial import (36f9873ff075253f4c1c9e394c91031fd4ba9d4a), do you know the git hash of the orbot commit you used?

1d635a925ca1728542067ee7bf34ff532d623a3f - The renaming is Tor Browser specific, so we should probably carry this as a patch in tor-browser-build, instead of hard coding "Tor Browser" within a general purpose "Tor service" library, right?

45244c49fbe382e97655b8f8d8f482e54f95ed07 - I wonder if we should carry a patch for this too. If tor-android-services+TOPL is intended as a general Android Tor library, then we probably shouldn't make it Tor Browser specific.

f3b3df4e66630a68c9987d52d93580549a0acaf4 - this package doesn't exist yet, but it will exist in a few weeks, so leaving it is probably okay.

1a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304 - hasCookieAuthentication() and runAsDaemon() seem out of place in this commit

784919d8eb19083cf761b3e7314c49d8befc00cd - custom tor, the binaries should be compiled as dependencies by tor-browser-build and injected into the build, TorService.TOR_VERSION should be patched at build time

6a9314aff4418a4edac33ff39fae266b097cf000 - In the future, we should take the bridges used on desktop (currently specified in Firefox's pref format and injected at build time by tor-browser-build), and pre-process them for inclusion on Android, too.

Child Tickets

Change History (23)

comment:1 Changed 6 months ago by sysrqb

Keywords: tbb-mobile tbb-8.5 TorBrowserTeam201904 added

comment:2 Changed 6 months ago by gk

Keywords: tbb-8.5-must added; tbb-8.5 removed
Parent ID: #27609

comment:3 Changed 5 months ago by gk

Keywords: tbb-8.5 added; tbb-8.5-must removed

Nice to have but no blocker anymore.

comment:4 Changed 5 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201904 removed

Moving tickets to May

comment:5 Changed 4 months ago by gk

Parent ID: #27609

comment:6 Changed 4 months ago by gk

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201905 removed

Moving tickets to June

comment:7 Changed 4 months ago by sisbell

The original branch point was from commit: 8ad7668013a7291b98cf8689cd99961db69f8ed3

comment:8 in reply to:  description Changed 4 months ago by sisbell

I wasn't able to follow the exact commits in the descriptions (I can't seem to locate the hashes) but I have a general idea of what some of them are addressing:

Replying to sysrqb:

Overall, nicely done. This review is on master (6a9314aff4418a4edac33ff39fae266b097cf000):

the initial import (36f9873ff075253f4c1c9e394c91031fd4ba9d4a), do you know the git hash of the orbot commit you used?

The original branch point was from commit: 8ad7668013a7291b98cf8689cd99961db69f8ed3

1d635a925ca1728542067ee7bf34ff532d623a3f - The renaming is Tor Browser specific, so we should probably carry this as a patch in tor-browser-build, instead of hard coding "Tor Browser" within a general purpose "Tor service" library, right?

I did update the resources to be the latest ones in Orbot. We will need to provide a patch to add 'Tor Browser' text now.

45244c49fbe382e97655b8f8d8f482e54f95ed07 - I wonder if we should carry a patch for this too. If tor-android-services+TOPL is intended as a general Android Tor library, then we probably shouldn't make it Tor Browser specific.

f3b3df4e66630a68c9987d52d93580549a0acaf4 - this package doesn't exist yet, but it will exist in a few weeks, so leaving it is probably okay.

1a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304 - hasCookieAuthentication() and runAsDaemon() seem out of place in this commit

I was not able to locate the 3 commits above to review

784919d8eb19083cf761b3e7314c49d8befc00cd - custom tor, the binaries should be compiled as dependencies by tor-browser-build and injected into the build, TorService.TOR_VERSION should be patched at build time

I'll open an issue for this. I think a good approach would be to place the version in a property file rather than using static fields. Orbot handles things a little differently now as well and pulls a binary artifact that contains the correct version in the meta-data.

6a9314aff4418a4edac33ff39fae266b097cf000 - In the future, we should take the bridges used on desktop (currently specified in Firefox's pref format and injected at build time by tor-browser-build), and pre-process them for inclusion on Android, too.

I did see sysrqb added this in commit d9b049c8cee225b4e7bb6f0191093f543d0f9f65

comment:9 Changed 4 months ago by gk

sisbell: you get the hashes if you clone our tor-android-service repo. You might need to add sysrqb's user repo as well (https://github.com/sysrqb/tor-android-service/). 1a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304 is a typo and should be a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304.

comment:10 Changed 3 months ago by gk

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201906 removed

Moving tickets to July

comment:11 Changed 3 months ago by sisbell

45244c49fbe382e97655b8f8d8f482e54f95ed07

Yes, you are right. In tor-android-service/0628 branch I changed these back to ones that Orbot uses. We will now need to add a patch to change to our default ports.

comment:12 Changed 3 months ago by sisbell

f3b3df4e66630a68c9987d52d93580549a0acaf4

Orbot project is no longer using BROWSER_APP_USERNAME. So this will also be eliminated from our code. I'll open an issue for this.

comment:13 Changed 3 months ago by sisbell

a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304
Yes I think they are out of place. We can modify the commit log.

comment:14 Changed 3 months ago by sisbell

Status: newneeds_review

comment:15 Changed 3 months ago by gk

Keywords: TorBrowserTeam201907R added; TorBrowserTeam201907 removed

comment:16 Changed 3 months ago by gk

Keywords: TorBrowserTeam201908R added; TorBrowserTeam201907R removed

No July any longer.

comment:17 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201909R added; TorBrowserTeam201908R removed

No August anymore.

comment:18 Changed 4 weeks ago by sysrqb

is update_tor_version the branch we should review?

comment:19 Changed 3 weeks ago by sysrqb

Status: needs_reviewneeds_revision

Okay, I decided branch 0801a is probably the correct one.

a70452c592ed4b2586a020a234d5835cfb83d704: seems fine, looks like a cherry-pick from Orbot
39d7fc1c0e75a56bddfc1c0cc5902b100244fce8: seems fine, but I don't see any use of the new binary tor version pref
1513f8d2ccaf99fa9d7e8fe1ae00b1f535697030:

  1. we'll need to carry a patch for s/Orbot/Tor Browser/.
  2. Were those localizations directly copied from Orbot?

03dc6cd426eba5ce43d800356e92a5e89cb65161: TODO (need to come back for this one, very large)
4471410b742ddf771b401bfb8b1d2c10a4e884e3: Okay. (I'd prefer we build this ourselves, but really we can delete the entire module.)
ac0a066c9ee75c0e869f6c95401b8134a2248c25:

     public String getProxySocks5Host() {
-        return OrbotVpnManager.sSocksProxyLocalhost;
+        return "127.0.0.1";
     }
  1. We'll want this configurable in the future. See #29498

In vpn/src/main/java/org/torproject/android/service/vpn/OrbotVpnManager.java

-    public static int sSocksProxyServerPort = -1;
-    public static String sSocksProxyLocalhost = null;
+    private int socksProxyServerPort = -1;
  1. Can you use mSocksProxyServerPort instead of socksProxyServerPort?
-                                               
-                               sSocksProxyLocalhost = "127.0.0.1";// InetAddress.getLocalHost().getHostAddress();
-                               sSocksProxyServerPort = (int)((Math.random()*1000)+10000); 
-                               
+
+                               socksProxyServerPort = (int)((Math.random()*1000)+10000);
+                                               prefs.edit().putInt(SOCKS_PROXY_PORT, socksProxyServerPort).apply();
                                        } catch (Exception e) {
                                                Log.e(TAG,"Unable to access localhost",e);
                                                throw new RuntimeException("Unable to access localhost: " + e);
  1. The Log.e error message is incorrect now.
  2. Is the try-catch necessary now?

4c964a06bf1259fbbb0f6972834a8a47b90139ab: TODO (but it's only changing the vpn, so I'm less concerned about it right now)
8ece3cb1cc7dbf61ad867d65b7cc5b4fef4522a3
509ef7245d22f8dd5114aa0945632652a370a51e
c7b8d3c8ba1bd2ff10a6a39b32a2501f35c17bcb
4dfca8dfb48797450a52c1cba0a0be2833823b61
7afc999ee941ece9a72356dbb60c3c48bb36c780
5553d81c02720c92ace0982317c5b455425dee14
62e3ac83b7357c9c513b5fa5027817d9e9cf0909
e3f80d70fd6c8e2b8ea0152c861241121e210f05: Is there a reason you didn't cherry-pick these and keep their commit messages?


Regarding previous comments,
a2121e2a7ee8a6c0f3b5edd150b38eaae8bef304: We shouldn't modify the commit, so I think we can live with the current situation.

comment:20 Changed 3 weeks ago by cypherpunks

TorBrowserTeam201909R->TorBrowserTeam201909

comment:21 Changed 3 weeks ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201909R removed

comment:22 Changed 3 weeks ago by pili

Keywords: TorBrowserTeam201910 added

comment:23 Changed 3 weeks ago by pili

Keywords: TorBrowserTeam201909 removed
Note: See TracTickets for help on using tickets.