Opened 5 months ago

Last modified 33 hours ago

#27609 needs_revision defect

TBA: Evaluate Tor Onion Proxy Library

Reported by: sysrqb Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, TBA-a3, TorBrowserTeam201902
Cc: igt0, sisbell, sysrqb Actual Points:
Parent ID: Points:
Reviewer: Sponsor: Sponsor8

Description

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"?

Child Tickets

TicketStatusOwnerSummaryComponent
#29080newtbb-teamMerge OrbotService and TOPLApplications/Tor Browser
#29312newtbb-teamCreate TBB Project for TOPLApplications/Tor Browser
#29313newtbb-teamCreate TBB Project for tor-android-serviceApplications/Tor Browser

Change History (29)

comment:1 Changed 5 months ago by gk

Cc: sisbell added

Shane: could you take a look at that?

comment:2 Changed 5 months ago by gk

Parent ID: #24856

comment:3 Changed 5 months ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201809 removed

Moving tickets to October

comment:4 Changed 4 months ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201810 removed

Moving our tickets to November.

comment:5 Changed 4 months ago by sisbell

The 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?

comment:6 Changed 4 months ago by gk

Cc: sysrqb added

comment:7 Changed 4 months ago by sisbell

We should cleanup the OnionProxyManager class. It loops/sleeps as it probes the port for bootstrap messages. This can be error prone and also slows down the bootstrap process significantly.

comment:8 Changed 3 months ago by pili

Sponsor: Sponsor8

comment:9 Changed 3 months ago by gk

Priority: MediumHigh

comment:10 Changed 3 months ago by sisbell

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 which creates the native libraries.

comment:11 Changed 3 months ago by gk

Keywords: TorBrowserTeam201812 added; TorBrowserTeam201811 removed

Moving our tickets to December.

comment:12 Changed 2 months ago by gk

Keywords: TBA-a3 added

Setting tag for third Tor Browser for Android alpha milestone.

comment:13 Changed 2 months ago by gk

Keywords: TBA-a2 removed

We are beyond TBA-a2, TBA-a3 is the new black.

comment:14 Changed 2 months ago by gk

Priority: HighVery High

comment:15 Changed 2 months ago by sisbell

Evaluation:

  1. 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.
  2. Settings Screen: there is no settings screen: configure ports, delete/add onion addresses [these features need to be defined]
  3. 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.
  4. 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.

comment:16 Changed 2 months ago by sisbell

Status: newneeds_review

comment:17 Changed 2 months ago by gk

Status: needs_reviewnew

Let's add some evaluation as well for what we need to do to get Pluggable Transports integrated and running with this library.

comment:18 in reply to:  15 ; Changed 2 months ago by sysrqb

Replying to sisbell:

Evaluation:

  1. 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.

  1. 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.

  1. 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.
  2. 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?

comment:19 in reply to:  18 ; Changed 2 months ago by sisbell

Replying to sysrqb:

Replying to sisbell:

Evaluation:

  1. 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.

  1. 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.

  1. 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.
  2. 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)

  1. Build our pluggable transports (this is currently handled as part of orbot dependency)
  2. Transport lifecycle (start/stop)
  3. Onboarding flow
  4. Cookie management
  5. Hidden services management
  6. Additional permissions handling
  7. UI for bootstrap
  8. Tor, transports and other binary installations (some of this is already included in TOPL)
  9. 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.

comment:20 in reply to:  19 Changed 2 months ago by sysrqb

Replying to sisbell:

Replying to sysrqb:

Replying to sisbell:

Evaluation:

[snip]

  1. 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.

  • Re-write of Orbot UI and providers - 3 weeks

The UI is mostly the goal of #28329. 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.

comment:21 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201901 added; TorBrowserTeam201812 removed

Moving tickets to Jan 2019.

comment:22 Changed 4 weeks ago by 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:

https://code.briarproject.org/briar/briar/blob/master/bramble-android/src/main/java/org/briarproject/bramble/plugin/tor/AndroidTorPlugin.java

Pluggable transport build - ??? needs more investigation

We can combine with this #28802.

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.

comment:23 in reply to:  22 Changed 3 weeks ago by sisbell

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:

Thank you. I'll take a look.

comment:24 Changed 3 weeks ago by sisbell

Submitted a pull request to make TOPL changes

https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/68

comment:25 in reply to:  22 Changed 3 weeks ago by sisbell

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:

1) 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.

2) 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.

3) 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.

comment:26 Changed 2 weeks ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201901 removed

Moving tickets to February.

comment:27 Changed 34 hours ago by sysrqb

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.

comment:28 Changed 33 hours ago by sysrqb

Status: newneeds_revision

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

https://github.com/thaliproject/Tor_Onion_Proxy_Library/pull/45/commits/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-a5155bb08733d67f397556a9ada17249R110

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/0ebdd33a287a5d7b10f40a0bea7bcec7d76d257f#diff-17352e51d9de729ccd4d10b27b8e2141R144
Nit: Two spaces

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.

https://github.com/thaliproject/Tor_Onion_Proxy_Library/commit/83436482fba7701c7ebd82ba34dbda15b216fad8#diff-9aaca4263fb73e2f8615d49825b318f2R313

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 call hasControlConnection() and handle the situation where it returns false (controlConnection == null).

comment:29 Changed 33 hours ago by sysrqb

One 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.

Note: See TracTickets for help on using tickets.