TBA: Evaluate Tor Onion Proxy Library
https://github.com/thaliproject/Tor_Onion_Proxy_Library/
Does it have everything we need? If not, what is it missing? Can we simply drop it into the build system (with some additional tweaks), and it "just works"?
Activity
Trac:
Parent: #24856 (moved) to N/AThe Android part of this project is dependent on https://github.com/n8fr8/tor-android for the tor binaries. Will this be the official source of tor binaries or should we add android support to projects/tor?
One area we should discuss is breaking out the Android tor installer from tor-onion-proxy/orbot. An Android installer could be a separate Android project library and would be related to #28704 (moved) which creates the native libraries.
Evaluation:
- Tor Installation: Tor Onion Proxy library is using tor-android-binary - Who is going to install the tor binary? Orbot or Tor Onion Proxy Libray. If orbot installs, we will need to configure the onion proxy to use those locations for tor files/libraries.
- Settings Screen: there is no settings screen: configure ports, delete/add onion addresses [these features need to be defined]
- Starting Proxy: This is a simple operation to start but we need to display the startup logs to the user. We also need to define UI to handle things like restart or shutdown.
- Stability: Its workable for an alpha, but we should cleanup up the threading and socket connections
In summary: this is not a simply a drop-in. It will need a UI defined and built but core functionality is all there. Suggest cleanup of threading before a production release.
Replying to sisbell:
Evaluation:
- Tor Installation: Tor Onion Proxy library is using tor-android-binary - Who is going to install the tor binary? Orbot or Tor Onion Proxy Libray. If orbot installs, we will need to configure the onion proxy to use those locations for tor files/libraries.
We'd use this instead of Orbot. This library would completely replace our Orbot implementation.
- Settings Screen: there is no settings screen: configure ports, delete/add onion addresses [these features need to be defined]
Right. We'll need to create our own UI for Orbot and TOPL, so I don't consider this a blocker.
- Starting Proxy: This is a simple operation to start but we need to display the startup logs to the user. We also need to define UI to handle things like restart or shutdown.
- Stability: Its workable for an alpha, but we should cleanup up the threading and socket connections
This is what I'm primarily worried about. Is the code quality where we need it. Is it tested? Is the code safe (for some value of safe)? With Orbot, we weren't exposing our users to new code (because Orbot was previously a dependency). If we use TOPL then we should be confident this won't be worse.
In summary: this is not a simply a drop-in. It will need a UI defined and built but core functionality is all there. Suggest cleanup of threading before a production release.
Can you estimate how much time this will require? Would we need to re-write much of the code and redesign it?
Replying to sysrqb:
Replying to sisbell:
Evaluation:
- Tor Installation: Tor Onion Proxy library is using tor-android-binary - Who is going to install the tor binary? Orbot or Tor Onion Proxy Libray. If orbot installs, we will need to configure the onion proxy to use those locations for tor files/libraries.
We'd use this instead of Orbot. This library would completely replace our Orbot implementation.
- Settings Screen: there is no settings screen: configure ports, delete/add onion addresses [these features need to be defined]
Right. We'll need to create our own UI for Orbot and TOPL, so I don't consider this a blocker.
- Starting Proxy: This is a simple operation to start but we need to display the startup logs to the user. We also need to define UI to handle things like restart or shutdown.
- Stability: Its workable for an alpha, but we should cleanup up the threading and socket connections
This is what I'm primarily worried about. Is the code quality where we need it. Is it tested? Is the code safe (for some value of safe)? With Orbot, we weren't exposing our users to new code (because Orbot was previously a dependency). If we use TOPL then we should be confident this won't be worse. I tested it back in February of this year and it works. While working on the code, I didn't see anything jumping out as unsafe. It's straight-forward.
In summary: this is not a simply a drop-in. It will need a UI defined and built but core functionality is all there. Suggest cleanup of threading before a production release.
Can you estimate how much time this will require? Would we need to re-write much of the code and redesign it?
Its no more than one week to cleanup threading and the other async handling. So not much of the code would need to be re-written. I'm seeing TOPL library as more of a replacement for orbotservice.
So far I see we need to bring in Orbot app features (not complete list)
- Build our pluggable transports (this is currently handled as part of orbot dependency)
- Transport lifecycle (start/stop)
- Onboarding flow
- Cookie management
- Hidden services management
- Additional permissions handling
- UI for bootstrap
- Tor, transports and other binary installations (some of this is already included in TOPL)
- Tor lifecycle/bootstrap (already there but needs some modification)
We can break this into service components and front-end UI components.
its hard to give exact estimate without going through full list of features first. Here is an initial estimate
- Bring TOPL inline with orbotservice - 1 week
- Pluggable transport build - ??? needs more investigation
- Re-write of Orbot UI and providers - 3 weeks
So best case would be about one-man-month. If we wanted to be keep Orbot but just integrate TOPL and remove orbotservice, we would be looking at more like 2 weeks.
Replying to sisbell:
Replying to sysrqb:
Replying to sisbell:
Evaluation: [snip]
- Stability: Its workable for an alpha, but we should cleanup up the threading and socket connections
This is what I'm primarily worried about. Is the code quality where we need it. Is it tested? Is the code safe (for some value of safe)? With Orbot, we weren't exposing our users to new code (because Orbot was previously a dependency). If we use TOPL then we should be confident this won't be worse. I tested it back in February of this year and it works. While working on the code, I didn't see anything jumping out as unsafe. It's straight-forward.
Great, thanks.
In summary: this is not a simply a drop-in. It will need a UI defined and built but core functionality is all there. Suggest cleanup of threading before a production release.
Can you estimate how much time this will require? Would we need to re-write much of the code and redesign it?
Its no more than one week to cleanup threading and the other async handling. So not much of the code would need to be re-written. I'm seeing TOPL library as more of a replacement for orbotservice.
[snip]
its hard to give exact estimate without going through full list of features first. Here is an initial estimate
- Bring TOPL inline with orbotservice - 1 week
I believe we need a new ticket for this. Please open it when you know what work is needed here.
- Pluggable transport build - ??? needs more investigation
We can combine with this #28802 (moved).
- Re-write of Orbot UI and providers - 3 weeks
The UI is mostly the goal of #28329 (moved). Implementing the Hidden Services provider will require more work, but we don't need that for the first stable version of TBA, so we can ignore it for now.
So best case would be about one-man-month. If we wanted to be keep Orbot but just integrate TOPL and remove orbotservice, we would be looking at more like 2 weeks.
Hopefully the above provides a better idea of what we're trying to accomplish.
TOPL's OnionProxyManager was originally based on Briar's TorPlugin. Changes on the Briar side since the code was forked include support for bridges with pluggable transports, so if you're planning to make similar changes to TOPL it might make sense to check out the current version of TorPlugin:
Pluggable transport build - ??? needs more investigation
We can combine with this #28802 (moved).
In case it's useful, we went through a similar process recently for Briar and we've published our binaries for tor and obfs4 on JCenter:
https://bintray.com/briarproject/org.briarproject
These are reproducible builds from n8fr8's tor-android repo and Yawning's obfs4 repo respectively, using the following Docker images:
https://code.briarproject.org/briar/tor-reproducer
https://code.briarproject.org/briar/go-reproducer
If I can do anything to make any of this code useful for you, let me know.
Trac:
Username: akwizgranReplying to akwizgran:
TOPL's OnionProxyManager was originally based on Briar's TorPlugin. Changes on the Briar side since the code was forked include support for bridges with pluggable transports, so if you're planning to make similar changes to TOPL it might make sense to check out the current version of TorPlugin:
Thank you. I'll take a look.
Submitted a pull request to make TOPL changes
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/68
Replying to akwizgran:
TOPL's OnionProxyManager was originally based on Briar's TorPlugin. Changes on the Briar side since the code was forked include support for bridges with pluggable transports, so if you're planning to make similar changes to TOPL it might make sense to check out the current version of TorPlugin:
This would replace most of the code in OnionProxyManager, OnionProxyContext and TorInstaller. Getting connection status is much cleaner. The OnionProxyContext would then just be used for getting settings and tor config information.
I think there are 3 primary questions/issues:
-
We need to override the EventHandler methods so we can send Android broadcasts on events but the AndroidTorPlugin class is package-private. Does the framework expect/allow plugins from the org.briarproject.bramble.plugin.tor package? If this is allowed I could just have OnionProxyManager as a subclass of AndroidTorPlugin without a lot of modification to the rest of the code.
-
We need to broadcast event messages and states (like starting and stopping) to the UI component in Android. Currently I have an EventBroadcaster class that OnionProxyManager uses to broadcast events and states during the startup. I see that TorPlugin throws a PluginException on failures within the startup. I think this would be workable since I can just capture these exceptions external to TorPlugin and then broadcast. However, some of the thrown PluginExceptions do not have any message passed in. So to be usable, I'd at least need those messages.
-
EVENTS are not configurable. This is one of the issues I logged with TOPL. https://github.com/thaliproject/Tor_Onion_Proxy_Library/issues/51
Overall, it looks good. Thanks for pointing this out.
-
In general, I'd like to see the Android (and Java) ecosystem agree on a single Tor controller. I don't know if Briar want more functionality in the future than what TorPlugin currently provides. If we're going to put time/effort into TOPL, then we should be reasonably sure it won't be wasted.
Shane, nice patches. I reviewed them and have some comments. I guess some follow-up PRs on the repo can resolve most of them.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR96 https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR117 https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR135 https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR158 https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR169
These are all public methods, should we catch the case where config is null?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-81e0435044baa197ee1db0ad93a5a940R185 Same as above but for onionProxyContext
This methods assigns the resolved torrc to the instance variable, is this what we want? The comment doesn't say it overwrites the current value
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-187809677e9cf785e207eb5077425815R15 Nit: Is this import necessary?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-5c2594ca8e8f166cdf275dbd29be29e5R20 Is this import needed?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/2ba4a486c6849affbacb012d8d181e1d7b47527d#diff-81e0435044baa197ee1db0ad93a5a940R331 Nit: Indentation
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/44911f6b1f7791947652139ff8432090f3efe914#diff-9045687b92ef42f4b5a7c35b7479228aR1 It would've been nice if each of these prototypes had a descriptive comment Is there a reason getListOfSupportedBridges() returns a String (instead of a List or Vector)? Socks5 should be the default SocksPort, should DefaultSettings reflect this? Should the *Port() accessors return an int instead of a string? Starting with "DisableNetwork 1" is usually safer
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR161 Hmmmm. We shouldn't use Google DNS as the default. (as some background, see https://medium.com/@nusenu/who-controls-tors-dns-traffic-a74a7632e8ca). Related to the comment below, I wonder if we actually need this at all.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR201 Nit: White space
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R73 Nit: This comment isn't necessarily correct because it's based on
input, right?https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R138
getListOfSupportedBridges()may return null, yes?https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R201 Maybe TorSettings.disableNetwork() should be renamed as TorSettings.shouldDisableNetwork()?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R268 Non-exit relays shouldn't perform DNS queries - but we should confirm this.
Nit:
s/yo uare/you are/https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R432 This is saying configuring an explicit SOCKS port may result in Tor auto-selecting a different port if the configured port is already used? This seems like it'll result in a bad user experience, don't you think?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/3637342b742e9688265b0c41b052a5c43dcd6bf6#diff-81e0435044baa197ee1db0ad93a5a940R248 Do you prefer HALT over TERM for a particular reason?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/3637342b742e9688265b0c41b052a5c43dcd6bf6#diff-81e0435044baa197ee1db0ad93a5a940R397 Nit: These log lines have a leading space? Was that intentional?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/3637342b742e9688265b0c41b052a5c43dcd6bf6#diff-a8dd49b5a9ec49e701e335d9cb0cf398R20 Nit: Did you use Strings here instead an enum for a reason? Consistency with Orbot?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/bc26ca5168ba027fbd458adea3e33f09dce9d252#diff-81e0435044baa197ee1db0ad93a5a940R557 Nit: Is disabling and enabling network important here?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/d8071138faa63130d6eeacb088ae87b96fae385e Nit: Does wrapping this in try/finally provide something?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/issues/64 I worry this prevents handling a failure case gracefully. Maybe it should be documented somewhere that if
isRunning()returns false, then the developer should callhasControlConnection()and handle the situation where it returns false (controlConnection == null).Trac:
Status: new to needs_revisionOne other wishlist item I'd like from TOPL is support for using a ControlSocket and SocksSocket (Unix domain sockets) instead of TCP ports. In pariticular, this would be lovely on Android where there is a system library implementation. TOPL could ship its own impl for other *nix, too.
One further (and crazier idea), is providing a Binder interface (somehow streaming over binder). But that'll require a little more thought.
Replying to sysrqb:
Shane, nice patches. I reviewed them and have some comments. I guess some follow-up PRs on the repo can resolve most of them.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR96 https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR117 https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR135 https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR158 https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR169
These are all public methods, should we catch the case where config is null? The torConfig is checked for null in the OnionProxyContext constructor. So we don't need to check for null in each method.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-81e0435044baa197ee1db0ad93a5a940R185 Same as above but for onionProxyContext OnionProxyContext is checked in constructor of OnionProxyManager
This methods assigns the resolved torrc to the instance variable, is this what we want? The comment doesn't say it overwrites the current value We do want to override the instance field but I'll open an issue to clarify this is the javadoc.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/44911f6b1f7791947652139ff8432090f3efe914#diff-9045687b92ef42f4b5a7c35b7479228aR1 It would've been nice if each of these prototypes had a descriptive comment Is there a reason getListOfSupportedBridges() returns a String (instead of a List or Vector)? I'm maintaining the currently logic that Orbot was using with a string (now located in TorConfigBuilder.configurePluggableTransportsFromSettings(). Its worth opening an issue to track.
Socks5 should be the default SocksPort, should DefaultSettings reflect this? I wasn't aware sock5 should be the default. I'll change this.
Should the *Port() accessors return an int instead of a string? I know there is some inconsistency in whether something is int or String so all these will need to be cleaned. I'll ID these and open the issues
Starting with "DisableNetwork 1" is usually safer I'll open an issue for this
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-6701d53cd7d0cd826c18c5e8c5c7dbfeR161 Hmmmm. We shouldn't use Google DNS as the default. (as some background, see https://medium.com/@nusenu/who-controls-tors-dns-traffic-a74a7632e8ca). Related to the comment below, I wonder if we actually need this at all. I had some question about this myself. Yaron mentioned this as well. It was in the Orbot code. Definitely worth looking into.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R73 Nit: This comment isn't necessarily correct because it's based on
input, right?That's true. The comment is outdated. I added inputStream to make the logic independent of Android's file-system access. There are a number of nits on spacing, comments, typo. I should be able to do this as part of single commit.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R138
getListOfSupportedBridges()may return null, yes? Good point, DefaultSettings has a return value as null, which could crash the app when checking for transports. I'll fix this.https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R201 Maybe TorSettings.disableNetwork() should be renamed as TorSettings.shouldDisableNetwork()?
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R268 Non-exit relays shouldn't perform DNS queries - but we should confirm this.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R432 This is saying configuring an explicit SOCKS port may result in Tor auto-selecting a different port if the configured port is already used? This seems like it'll result in a bad user experience, don't you think? I'm following the logic currently in Orbot. Potential options include 1) fail tor when the port is being used; 2) choose a new port (current behavior); 3) Prompt the user to choose different port. The current behavior seems reasonable but we can open an issue to track it.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/3637342b742e9688265b0c41b052a5c43dcd6bf6#diff-81e0435044baa197ee1db0ad93a5a940R248 Do you prefer HALT over TERM for a particular reason? Checking the control spec, HALT was the standard name with TERM being the posix alias. So I just went with the standard name.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/3637342b742e9688265b0c41b052a5c43dcd6bf6#diff-a8dd49b5a9ec49e701e335d9cb0cf398R20 Nit: Did you use Strings here instead an enum for a reason? Consistency with Orbot? I think these fields should be private since they are only used within the Status class. So that can be changed. There is no real need for an enum here since it is fully encapsulated in a data class.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/bc26ca5168ba027fbd458adea3e33f09dce9d252#diff-81e0435044baa197ee1db0ad93a5a940R557 Nit: Is disabling and enabling network important here? This occurs when going into airplane mode. it will prevent any outbound connections from being attempted.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/d8071138faa63130d6eeacb088ae87b96fae385e Nit: Does wrapping this in try/finally provide something? Its possible that we could have a null controlconnection with an open controlsocket. The finally block will make sure we close the socket on stop.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/issues/64 I worry this prevents handling a failure case gracefully. Maybe it should be documented somewhere that if
isRunning()returns false, then the developer should callhasControlConnection()and handle the situation where it returns false (controlConnection == null).I opened issues 76 - 82 to address a number of key issues raised by sysrqb
https://github.com/thaliproject/Tor_Onion_Proxy_Library/issues
I'll add more in the coming week.
Trac:
Cc: igt0, sisbell, sysrqb to igt0, sisbell, sysrqb, hans@guardianproject.info-
Okay, I looked over all the patches for #29312 (moved), #29313 (moved), #29574 (moved), and #29575 (moved). Nice work! It feels we are pretty close here. We can use the parent ticket to address all comments/review requests in one place. Here are my 2cents:
e38175545f4de75030fcf48b9950b65118228688 (#29312 (moved))
Why are we using 0.3.5.6-rc (which is a release candidate and no stable tor)? Are we sure we are better off security- and/or stability- and/or performance-wise than with 0.3.4.9? We have been testing the latter for quite a while now but not the -rc yet and we plan to have a stable soon for Android users...
d79c8ef41190cc37c16c7b90cfe65f083d6a3b95 (#29313 (moved))
++ ndk { ++ abiFilters "armeabi-v7a" ++ }How does that work for x86 builds which we have now as well? Do we need a similar entry for them as well? If so, what is supposed to happen if we omitted both entries?
8e3d539f76e3bd45a809dc1b14277d2b1a2fa3c4 (#29574 (moved))
I assume "tor-0.3.4.9" is just part of the filename of the .aar file but there is no tor 0.3.4.9 or 0.3.4.9 related code in it? (Especially as you are using 0.3.5.6-rc) Otherwise I'd be worried about possible bad interactions with code belonging to different tor versions.
orbot-uber.patchis tricky to review, the diff to the older patchset isdiff --git a/app/build.gradle b/app/build.gradle index 3051dd5c..4e33472c 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -76,12 +76,16 @@ android { dependencies { // implementation 'com.github.delight-im:Android-Languages:v1.0.1' implementation 'com.android.support.constraint:constraint-layout:1.1.3' - implementation project(':orbotservice') // Match Fennec's ANDROID_SUPPORT_LIBRARY_VERSION implementation 'com.android.support:design:23.4.0' implementation 'pl.bclogic:pulsator4droid:1.0.3' // These require higher versions of ANDROID_SUPPORT_LIBRARY_VERSION //implementation 'com.github.apl-devs:appintro:v4.2.2' //implementation 'com.github.javiersantos:AppUpdater:2.6.4' - + implementation fileTree(dir: 'libs', include: ['*.jar', '*.aar']) + implementation 'com.android.support:appcompat-v7:26.1.0' + implementation 'net.freehaven.tor.control:jtorctl:0.2' + implementation 'org.torproject:tor-android-binary:0.3.5.6-rc' + implementation 'org.slf4j:slf4j-api:1.7.25' + implementation 'org.slf4j:slf4j-android:1.7.25' } diff --git a/build.gradle b/build.gradle index edcfc84f..ce06f082 100644 --- a/build.gradle +++ b/build.gradle @@ -3,7 +3,6 @@ buildscript { repositories { jcenter() google() - maven { url System.getenv("GRADLE_MAVEN_REPO") } } dependencies { // Match Fennec @@ -17,6 +16,5 @@ allprojects { maven { url "https://raw.githubusercontent.com/guardianproject/gpmaven/master" } google() maven { url 'https://jitpack.io' } - maven { url System.getenv("GRADLE_MAVEN_REPO") } } } diff --git a/settings.gradle b/settings.gradle index 9984a03e..e7b4def4 100644 --- a/settings.gradle +++ b/settings.gradle @@ -1,2 +1 @@ -include ':jsocksAndroid', ':orbotservice' include ':app'We have a
+- implementation 'com.android.support:appcompat-v7:27.1.1' ++ // Match Fennec's version ++ implementation 'com.android.support:appcompat-v7:23.4.0'in the orbot patch. So, we explicitly downgraded appcompat to 23.4.0 with the intent to match what Mozilla is using. However in the uber patch above you require 26.1.0. Is that necessary?
Additionally, we have
+ implementation 'com.jrummyapps:android-shell:1.0.1'there, yet you remove all the
android-shellbits from the gradle-dependencies-file. Thus, we can removeandroid-shellas a dependency in thebuild.gradlefile as well?ca269f08af2e17d07a0ec9d9612252348d6656f8 (#29575 (moved))
Do we still need
android-shellin the gradle-dependencies list given that you removedorbotservicefrombuild.gradle?The
dependencies.patchshould be done in thetor-browserrepo. I guess a fixup to Matt's original patch (fbad5b5dabb3c45a9da853a6b784a28e78b9583c) would be fine.Leaving this ticket at
needs_revisionto address changes/questions. Replying to gk:
Okay, I looked over all the patches for #29312 (moved), #29313 (moved), #29574 (moved), and #29575 (moved). Nice work! It feels we are pretty close here. We can use the parent ticket to address all comments/review requests in one place. Here are my 2cents:
e38175545f4de75030fcf48b9950b65118228688 (#29312 (moved))
Why are we using 0.3.5.6-rc (which is a release candidate and no stable tor)? Are we sure we are better off security- and/or stability- and/or performance-wise than with 0.3.4.9? We have been testing the latter for quite a while now but not the -rc yet and we plan to have a stable soon for Android users...
Good point. I'm moving this back to 0.3.4.9. I'll do this within tor-android-service project.
d79c8ef41190cc37c16c7b90cfe65f083d6a3b95 (#29313 (moved)) {{{ ++ ndk { ++ abiFilters "armeabi-v7a" ++ } }}} How does that work for x86 builds which we have now as well? Do we need a similar entry for them as well? If so, what is supposed to happen if we omitted both entries?
This is no longer needed. We don't use VPN so don't need any native builds. I'll remove this from the patch.
8e3d539f76e3bd45a809dc1b14277d2b1a2fa3c4 (#29574 (moved))
I assume "tor-0.3.4.9" is just part of the filename of the .aar file but there is no tor 0.3.4.9 or 0.3.4.9 related code in it? (Especially as you are using 0.3.5.6-rc) Otherwise I'd be worried about possible bad interactions with code belonging to different tor versions.
We are just pulling in the android-binary library for the project. I'll align the versions to 0.3.4.9
orbot-uber.patchis tricky to review, the diff to the older patchset is {{{ diff --git a/app/build.gradle b/app/build.gradle index 3051dd5c..4e33472c 100644 --- a/app/build.gradle +++ b/app/build.gradle @@ -76,12 +76,16 @@ android { dependencies { // implementation 'com.github.delight-im:Android-Languages:v1.0.1' implementation 'com.android.support.constraint:constraint-layout:1.1.3'- implementation project(':orbotservice') // Match Fennec's ANDROID_SUPPORT_LIBRARY_VERSION implementation 'com.android.support:design:23.4.0' implementation 'pl.bclogic:pulsator4droid:1.0.3' // These require higher versions of ANDROID_SUPPORT_LIBRARY_VERSION //implementation 'com.github.apl-devs:appintro:v4.2.2' //implementation 'com.github.javiersantos:AppUpdater:2.6.4'
- implementation fileTree(dir: 'libs', include: ['.jar', '.aar'])
- implementation 'com.android.support:appcompat-v7:26.1.0'
- implementation 'net.freehaven.tor.control:jtorctl:0.2'
- implementation 'org.torproject:tor-android-binary:0.3.5.6-rc'
- implementation 'org.slf4j:slf4j-api:1.7.25'
- implementation 'org.slf4j:slf4j-android:1.7.25' } diff --git a/build.gradle b/build.gradle index edcfc84f..ce06f082 100644 --- a/build.gradle +++ b/build.gradle @@ -3,7 +3,6 @@ buildscript { repositories { jcenter() google()
-
maven { url System.getenv("GRADLE_MAVEN_REPO") }
} dependencies { // Match Fennec @@ -17,6 +16,5 @@ allprojects { maven { url "https://raw.githubusercontent.com/guardianproject/gpmaven/master" } google() maven { url 'https://jitpack.io' }
-
maven { url System.getenv("GRADLE_MAVEN_REPO") }
} } diff --git a/settings.gradle b/settings.gradle index 9984a03e..e7b4def4 100644 --- a/settings.gradle +++ b/settings.gradle @@ -1,2 +1 @@ -include ':jsocksAndroid', ':orbotservice' include ':app' }}}
I just have the uber patch for convenience. My expectation is that when we merge with the new UI there will be a bunch of new patches we apply and then can remove the uber patch. What I'll do is move the tor-android-service parts into a new patch(es) so its easier to track and migrate. This patch involves
- Remove the building of orbotservice and jsocksAndroid
- Add in gradle dependencies so that the orbot build can use the tor-android-service aar (this includes local libs for libraries to compile against)
- Change gradle.build to use our local maven repo
I'm not concerned with anything else touching orbot app code since Matt is handling that.
We have a {{{ +- implementation 'com.android.support:appcompat-v7:27.1.1' ++ // Match Fennec's version ++ implementation 'com.android.support:appcompat-v7:23.4.0' }}} in the orbot patch. So, we explicitly downgraded appcompat to 23.4.0 with the intent to match what Mozilla is using. However in the uber patch above you require 26.1.0. Is that necessary? I'll change back to using 23.4.0
Additionally, we have {{{
-
implementation 'com.jrummyapps:android-shell:1.0.1'
}}} there, yet you remove all the
android-shellbits from the gradle-dependencies-file. Thus, we can removeandroid-shellas a dependency in thebuild.gradlefile as well? I'll remove this. It is no longer needed.ca269f08af2e17d07a0ec9d9612252348d6656f8 (#29575 (moved))
Do we still need
android-shellin the gradle-dependencies list given that you removedorbotservicefrombuild.gradle? No longer neededThe
dependencies.patchshould be done in thetor-browserrepo. I guess a fixup to Matt's original patch (fbad5b5dabb3c45a9da853a6b784a28e78b9583c) would be fine.Leaving this ticket at
needs_revisionto address changes/questions.Replying to sysrqb:
Replying to gk:
orbot-uber.patchis tricky to review On this note, did you combine all the patches for a reason? The separate patches make reviewing changes much easier and they provide logical separation. A single patch is a little overwhelming.I'm going to break apart between uber and tor-android-service/TOPL related changes. The uber patch should only contain changes to orbot app module. We dump it completely once this branch is merged with your new UI/app changes, keeping only the tor-android-service/TOPL related patches.
Replying to gk:
Why are we using 0.3.5.6-rc (which is a release candidate and no stable tor)? Because this is an alpha. Are we sure we are better off security- and/or stability- and/or performance-wise than with 0.3.4.9? All of them, but mostly performance. We have been testing the latter for quite a while now but not the -rc yet and we plan to have a stable soon for Android users... Then somebody should make a 0.3.5.8 one with https://www.openssl.org/news/secadv/20190226.txt
Replying to gk:
sisbell: i'll wait with further review as your
android-0314branch does not compile with the latest nightly code (it seems yourdependencies.patchin thefirefoxproject needs updating). Further, there are conflicts with the latesttor-browser-buildmasterbranch.The issue here is that the latest orbot is copying obfs4proxy.so from orbotservice. This addition is causing a conflict. I'll finish the 'Include Support for Pluggable Transports' issue in tor-android-service and then modify the build script to use that library.
-
Replying to sisbell:
Replying to gk:
sisbell: i'll wait with further review as your
android-0314branch does not compile with the latest nightly code (it seems yourdependencies.patchin thefirefoxproject needs updating). Further, there are conflicts with the latesttor-browser-buildmasterbranch.The issue here is that the latest orbot is copying obfs4proxy.so from orbotservice. This addition is causing a conflict. I'll finish the 'Include Support for Pluggable Transports' issue in tor-android-service and then modify the build script to use that library.
Sounds good. Where are we with that? I'd like to have at least a bunch of nightly builds with all the new code first now that we missed the alpha this week. To give it at least a bit more testing then "just" one alpha.
Replying to gk:
Replying to sisbell:
Replying to gk:
sisbell: i'll wait with further review as your
android-0314branch does not compile with the latest nightly code (it seems yourdependencies.patchin thefirefoxproject needs updating). Further, there are conflicts with the latesttor-browser-buildmasterbranch.The issue here is that the latest orbot is copying obfs4proxy.so from orbotservice. This addition is causing a conflict. I'll finish the 'Include Support for Pluggable Transports' issue in tor-android-service and then modify the build script to use that library.
Sounds good. Where are we with that? I'd like to have at least a bunch of nightly builds with all the new code first now that we missed the alpha this week. To give it at least a bit more testing then "just" one alpha.
I've added the PT support into TOPL Android. Since TOPL deals with configuration and settings, it seems to be a logical place for it. The issue below links to the commit with changes, if you are interested.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/issues/86
Since tor-android-service uses TOPL, it will pull in the PT libraries.
Next. I'm going cleanup some of the bridge settings methods that sysrqb identified in the code review. Also I'm going to configure the use the bridges.txt in tor-binary-android aar, rather than the one in tor-android-service.
https://github.com/thaliproject/Tor_Onion_Proxy_Library/issues/78
I've got TOPL bridge settings fixed and checked into my branch. So we will be able to pull PT + bridge settings from TOPL Android library. I've also fixed up the config/settings for Android Q support.
https://github.com/sisbell/Tor_Onion_Proxy_Library/tree/march
There are some minor breaking API changes that will affect tor-android-service, so I'm going to create a branch to fix tor-android-service/TOPL integration. Once that's done, I'll submit a TOPL PR for review.
The last step is to fix the tor-browser-build/orbot merge conflict (this part should be simple since we've done the fixes in other projects).
