Opened 13 months ago

Closed 4 months ago

Last modified 4 months ago

#27609 closed defect (fixed)

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, tbb-8.5-must, TorBrowserTeam201905
Cc: igt0, sisbell, sysrqb, hans@…, n8fr8 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
#28802closedtbb-teamIntegrate PTs and bridge support into Tor Browser for AndroidApplications/Tor Browser
#29080closedtbb-teamMerge OrbotService and TOPLApplications/Tor Browser
#29312closedtbb-teamCreate TBB Project for TOPLApplications/Tor Browser
#29313closedtbb-teamCreate TBB Project for tor-android-serviceApplications/Tor Browser
#29574closedtbb-teamConfigure Orbot to Use TOPL and tor-android-service LibrariesApplications/Tor Browser
#29575closedtbb-teamConfigure Firefox Project to Use New TOPL DependenciesApplications/Tor Browser
#30166closedtbb-teamIf custom bridges are specified, only use those bridges for connectingApplications/Tor Browser

Change History (67)

comment:1 Changed 13 months ago by gk

Cc: sisbell added

Shane: could you take a look at that?

comment:2 Changed 13 months ago by gk

Parent ID: #24856

comment:3 Changed 13 months ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201809 removed

Moving tickets to October

comment:4 Changed 11 months ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201810 removed

Moving our tickets to November.

comment:5 Changed 11 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 11 months ago by gk

Cc: sysrqb added

comment:7 Changed 11 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 11 months ago by pili

Sponsor: Sponsor8

comment:9 Changed 11 months ago by gk

Priority: MediumHigh

comment:10 Changed 10 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 10 months ago by gk

Keywords: TorBrowserTeam201812 added; TorBrowserTeam201811 removed

Moving our tickets to December.

comment:12 Changed 10 months ago by gk

Keywords: TBA-a3 added

Setting tag for third Tor Browser for Android alpha milestone.

comment:13 Changed 10 months ago by gk

Keywords: TBA-a2 removed

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

comment:14 Changed 10 months ago by gk

Priority: HighVery High

comment:15 Changed 10 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 10 months ago by sisbell

Status: newneeds_review

comment:17 Changed 10 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 10 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 10 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 10 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 9 months ago by gk

Keywords: TorBrowserTeam201901 added; TorBrowserTeam201812 removed

Moving tickets to Jan 2019.

comment:22 Changed 9 months 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 9 months 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 9 months 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 9 months 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 8 months ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201901 removed

Moving tickets to February.

comment:27 Changed 8 months 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 8 months 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 8 months 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.

comment:30 in reply to:  28 Changed 8 months ago by sisbell

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

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

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

comment:31 Changed 8 months ago by sisbell

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.

comment:32 Changed 8 months ago by eighthave

Cc: hans@… added

comment:33 Changed 7 months ago by gk

Keywords: TorBrowserTeam201903 added; TorBrowserTeam201902 removed

Moving remaining tickets to March.

comment:34 Changed 7 months ago by gk

Keywords: tbb-8.5 added

Tickets on our radar for 8.5

comment:35 Changed 7 months ago by gk

Okay, I looked over all the patches for #29312, #29313, #29574, and #29575. 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)

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)

++        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)

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.patch is 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'

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-shell bits from the gradle-dependencies-file. Thus, we can remove android-shell as a dependency in the build.gradle file as well?

ca269f08af2e17d07a0ec9d9612252348d6656f8 (#29575)

Do we still need android-shell in the gradle-dependencies list given that you removed orbotservice from build.gradle?

The dependencies.patch should be done in the tor-browser repo. I guess a fixup to Matt's original patch (fbad5b5dabb3c45a9da853a6b784a28e78b9583c) would be fine.

Leaving this ticket at needs_revision to address changes/questions.

comment:36 in reply to:  35 ; Changed 7 months ago by sysrqb

Replying to gk:

orbot-uber.patch is 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.

comment:37 Changed 7 months ago by gk

sisbell: ping.

comment:38 in reply to:  35 Changed 7 months ago by sisbell

Replying to gk:

Okay, I looked over all the patches for #29312, #29313, #29574, and #29575. 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)

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)

++        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)

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.patch is 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-shell bits from the gradle-dependencies-file. Thus, we can remove android-shell as a dependency in the build.gradle file as well?

I'll remove this. It is no longer needed.

ca269f08af2e17d07a0ec9d9612252348d6656f8 (#29575)

Do we still need android-shell in the gradle-dependencies list given that you removed orbotservice from build.gradle?

No longer needed

The dependencies.patch should be done in the tor-browser repo. I guess a fixup to Matt's original patch (fbad5b5dabb3c45a9da853a6b784a28e78b9583c) would be fine.

Leaving this ticket at needs_revision to address changes/questions.

comment:39 in reply to:  36 Changed 7 months ago by sisbell

Replying to sysrqb:

Replying to gk:

orbot-uber.patch is 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.

comment:40 in reply to:  35 Changed 7 months ago by cypherpunks

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

comment:41 Changed 7 months ago by gk

sisbell: i'll wait with further review as your android-0314 branch does not compile with the latest nightly code (it seems your dependencies.patch in the firefox project needs updating). Further, there are conflicts with the latest tor-browser-build master branch.

comment:42 in reply to:  41 ; Changed 7 months ago by sisbell

Replying to gk:

sisbell: i'll wait with further review as your android-0314 branch does not compile with the latest nightly code (it seems your dependencies.patch in the firefox project needs updating). Further, there are conflicts with the latest tor-browser-build master branch.

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.

https://github.com/sisbell/tor-android-service/issues/16

comment:43 in reply to:  42 ; Changed 7 months ago by gk

Replying to sisbell:

Replying to gk:

sisbell: i'll wait with further review as your android-0314 branch does not compile with the latest nightly code (it seems your dependencies.patch in the firefox project needs updating). Further, there are conflicts with the latest tor-browser-build master branch.

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.

https://github.com/sisbell/tor-android-service/issues/16

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.

comment:44 in reply to:  43 Changed 7 months ago by sisbell

Replying to gk:

Replying to sisbell:

Replying to gk:

sisbell: i'll wait with further review as your android-0314 branch does not compile with the latest nightly code (it seems your dependencies.patch in the firefox project needs updating). Further, there are conflicts with the latest tor-browser-build master branch.

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.

https://github.com/sisbell/tor-android-service/issues/16

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

comment:45 Changed 7 months ago by sisbell

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

comment:46 Changed 7 months ago by gk

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

Marking blockers for Tor Browser 8.5.

comment:47 Changed 7 months ago by gk

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

Tickets that block the next 8.5 alpha.

comment:49 Changed 6 months ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201903 removed

Moving tickets to April.

comment:50 Changed 6 months ago by sisbell

I did a deep dive into the library loading for tor.

  1. After going through older builds and then our newer ones, the browser APK has never extracted the tor binaries. It does extract for the firefox/mozilla native libraries.
  2. Orbot extracts libraries: I tried using tor-android-service with a build of Orbot app and the tor library extracts correctly. So this looks like something specific to the firefox build. It is a little strange that the libraries in Orbot show up as arm rather than armv7 (as shown in Native Libs Monitor)
  3. Not a toolchain issue: I also built Orbot/tor-android-service with the same toolchain as tbb and the libraries still extract so it doesn't appear to be a toolchain related issue.

Comparing versions tor-android-binary library

  1. 0.3.4.9 - we can see that the tor.so is in the tor-browser apk and it registers as an arm library within the apk.
  2. 0.3.5.8 - tor.so isn't picked up at all as a native library, even within the tor-browser apk (I verified with Native Libs Monitor).
  3. When I try to launch the latest version of tor, I get "no such directory or file". When I check the app space on the device tor is correctly located on the device. So this error doesn't make much sense. I managed to reproduce this behavior with a sample app by having APK A dependent on aar B. B had /lib/armeabi-v7/tor.so in the aar. I then built APK A and also reincluded jniLibs/armeabli-v7/tor.so in APK A. So there are two versions in the build chain. With this build I was able to reproduce the "no such directory or file" error. However, I was unable to find any double inclusion within the firefox build.
  4. I took my local/latest version of the tor-browser APK that fails as defined in (3) and replaced version 0.3.5.8 of tor.so with version 0.3.4.9. I directly modified the APK so there is no other change. The app started up correctly.

Summary:
We will need to fall back to the older version of tor for our build. In regards to the latest version of tor-android-binary, it works correctly outside of firefox. The only thing that looks strange is that it registers as arm rather than armv7 so that would be worth a look in the future.

I've upgraded TOPL and tor-android-service to use the latest version of tor-android so I will create a patch to move to an older version in the firefox build. The APIs are slightly different between versions so this will touch some of the code.

comment:51 Changed 6 months ago by gk

Cc: n8fr8 added

Thanks for digging through all of that. I wonder whether Nathan has an idea about what's up with that.

Last edited 6 months ago by gk (previous) (diff)

comment:52 Changed 6 months ago by n8fr8

I don't have more time to respond right now, but please try the "0.3.5.8-rc-v2" tor-android-binary gradle dependency as it has some fixes.

In summary, we are trying to rely on the native library loading that Android provides, and executing the .so files in places, instead of unpacking them from the APK ourselves and/or copying them over or to the app's r/w file space. This is due to changes in Android Q that requires this.

comment:53 Changed 6 months ago by sisbell

When I readelf on libmozglue.so (a library which is recognized by android as armeabl-v7), I see

Attribute Section: aeabi
File Attributes

Tag_CPU_name: "ARM v7"
Tag_CPU_arch: v7
Tag_CPU_arch_profile: Application
Tag_ARM_ISA_use: Yes
Tag_THUMB_ISA_use: Thumb-2
Tag_FP_arch: VFPv3

On the latest version of tor.so, I see

Attribute Section: aeabi
File Attributes

Tag_CPU_name: "arm1022e"
Tag_CPU_arch: v5TE
Tag_ARM_ISA_use: Yes
Tag_THUMB_ISA_use: Thumb-1
Tag_FP_arch: VFPv2

This is why Android is detecting tor as arm and not armv7.  So this looks correct.

comment:54 Changed 6 months ago by n8fr8

Below are all the binaries I package in the tor-android-binary distribution... In truth, I think there are not any more plain "arm" aka "arm5" devices out there we need to support, so I can probably just build for armv7 as the low-end.

Also, I am not actually compiling tor for 64-bit yet, but I just package the 32-bit binaries as 64 since the shared library management system expects them to be there. These are still being executed in a separate process, so there is no issue with them being 32 bit.

├── arm64-v8a
│   └── tor.so
├── armeabi
│   └── tor.so
├── armeabi-v7a
│   └── tor.so
├── x86
│   └── tor.so
└── x86_64

└── tor.so

comment:55 Changed 6 months ago by eighthave

I totally agree that anything older than armeabi-v7a should be dropped.

comment:56 Changed 6 months ago by gk

Looking over the code of your 0411 branch and the essentially same changes in android-0318 I realized I forgot to mention two issues in addition to those raised in comment:35:

1) please check the respective gradle_dependencies_version declarations in the changed/added config files, they should either get incremented by 1 or start with 1 (the one for tor_android_service starts with 3 it seems)

2) On a branch for review, could you please only put the commits on top of master that are actually needed/intended to get reviewed? I am sure the patch for #28764 is not, but am not so sure about the one for #28803.

comment:57 Changed 6 months ago by gk

Keywords: tbb-8.5-must added; TBA-a3 tbb-8.5-must-alpha removed

comment:58 Changed 6 months ago by sysrqb

What's the reasoning behind this changing? It is for future-proofing so we don't accidentally include other binaries if we add support for another arch? or is this for something else?

-
-zip -d $apk lib/armeabi/tor.so
+unzip $apk lib/*
+zip -d $apk lib/\*
 
 [% IF c("var/android-x86") %]
-    zip -d $apk lib/armeabi-v7a/tor.so
+    zip $apk lib/x86/*
 [% END %]
 
 [% IF c("var/android-armv7") %]
-    zip -d $apk lib/x86/tor.so
+    zip $apk lib/armeabi-v7a/*
 [% END %]
 
+rm -fR lib
+

comment:59 in reply to:  58 ; Changed 6 months ago by sisbell

Replying to sysrqb:

What's the reasoning behind this changing? It is for future-proofing so we don't accidentally include other binaries if we add support for another arch? or is this for something else?

We are bringing in multiple tor libraries from 5 different architectures in the latest release. So this method is more manageable.

https://github.com/sisbell/tor-android-service/tree/784919d8eb19083cf761b3e7314c49d8befc00cd/service/src/main/jniLibs

-
-zip -d $apk lib/armeabi/tor.so
+unzip $apk lib/*
+zip -d $apk lib/\*
 
 [% IF c("var/android-x86") %]
-    zip -d $apk lib/armeabi-v7a/tor.so
+    zip $apk lib/x86/*
 [% END %]
 
 [% IF c("var/android-armv7") %]
-    zip -d $apk lib/x86/tor.so
+    zip $apk lib/armeabi-v7a/*
 [% END %]
 
+rm -fR lib
+

comment:60 Changed 6 months ago by eighthave

In my experience, it is generally a lot smoother to do these kinds of file management activities before everything gets packaged into the APK. There are lots of little hacks in the final packaging steps to support zipaligning, zeroing out timestamps, v1 signatures and v2/v3 signatures, etc. So by manipulating the APK, it'll make all those steps harder to deal with.

For example, this could be done by a gradle Task that is called after compile or maybe as the first step of packaging.

comment:61 in reply to:  59 Changed 6 months ago by sysrqb

Replying to sisbell:

Replying to sysrqb:

What's the reasoning behind this changing? It is for future-proofing so we don't accidentally include other binaries if we add support for another arch? or is this for something else?

We are bringing in multiple tor libraries from 5 different architectures in the latest release. So this method is more manageable.

I see, thanks.

comment:62 in reply to:  60 ; Changed 6 months ago by sysrqb

Replying to eighthave:

In my experience, it is generally a lot smoother to do these kinds of file management activities before everything gets packaged into the APK. There are lots of little hacks in the final packaging steps to support zipaligning, zeroing out timestamps, v1 signatures and v2/v3 signatures, etc. So by manipulating the APK, it'll make all those steps harder to deal with.

For example, this could be done by a gradle Task that is called after compile or maybe as the first step of packaging.

I wonder if we can build a single aar for this (where it includes all the libs), and then make copies of it and tweak each copy so it is arch-specific and only includes the needed binaries. Then we only inject the correct arch-specific aar into the firefox build stage. I'll open another ticket for this.

I opened #30199 for other review comments on tor-android-service.

comment:63 in reply to:  60 ; Changed 6 months ago by sisbell

Replying to eighthave:

In my experience, it is generally a lot smoother to do these kinds of file management activities before everything gets packaged into the APK. There are lots of little hacks in the final packaging steps to support zipaligning, zeroing out timestamps, v1 signatures and v2/v3 signatures, etc. So by manipulating the APK, it'll make all those steps harder to deal with.

For example, this could be done by a gradle Task that is called after compile or maybe as the first step of packaging.

I agree this would be better. I tried using ndk.abiFilters at the firefox stage but the apk wasn't including any of the dependent native libraries from the aars. This will require more investigation as to why it wasn't working.

comment:64 in reply to:  62 Changed 6 months ago by sisbell

Replying to sysrqb:

Replying to eighthave:

In my experience, it is generally a lot smoother to do these kinds of file management activities before everything gets packaged into the APK. There are lots of little hacks in the final packaging steps to support zipaligning, zeroing out timestamps, v1 signatures and v2/v3 signatures, etc. So by manipulating the APK, it'll make all those steps harder to deal with.

For example, this could be done by a gradle Task that is called after compile or maybe as the first step of packaging.

I wonder if we can build a single aar for this (where it includes all the libs), and then make copies of it and tweak each copy so it is arch-specific and only includes the needed binaries. Then we only inject the correct arch-specific aar into the firefox build stage. I'll open another ticket for this.

The dependent aars include all architectures. So we aren't filtering in tor-onion-proxy-library, tor-android-service. Filtering is only occurring in tor-browser on master (although it would be best if we could filter at the firefox stage).

I opened #30199 for other review comments on tor-android-service.

comment:65 in reply to:  63 ; Changed 6 months ago by gk

Replying to sisbell:

Replying to eighthave:

In my experience, it is generally a lot smoother to do these kinds of file management activities before everything gets packaged into the APK. There are lots of little hacks in the final packaging steps to support zipaligning, zeroing out timestamps, v1 signatures and v2/v3 signatures, etc. So by manipulating the APK, it'll make all those steps harder to deal with.

For example, this could be done by a gradle Task that is called after compile or maybe as the first step of packaging.

I agree this would be better. I tried using ndk.abiFilters at the firefox stage but the apk wasn't including any of the dependent native libraries from the aars. This will require more investigation as to why it wasn't working.

Could you open a ticket for that detailing what you tried so far (and what did not work), so this does not fall through the cracks?

Last edited 4 months ago by gk (previous) (diff)

comment:66 Changed 5 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201904 removed

Moving tickets to May

comment:67 in reply to:  65 Changed 4 months ago by gk

Resolution: fixed
Status: needs_revisionclosed

Replying to gk:

Replying to sisbell:

Replying to eighthave:

In my experience, it is generally a lot smoother to do these kinds of file management activities before everything gets packaged into the APK. There are lots of little hacks in the final packaging steps to support zipaligning, zeroing out timestamps, v1 signatures and v2/v3 signatures, etc. So by manipulating the APK, it'll make all those steps harder to deal with.

For example, this could be done by a gradle Task that is called after compile or maybe as the first step of packaging.

I agree this would be better. I tried using ndk.abiFilters at the firefox stage but the apk wasn't including any of the dependent native libraries from the aars. This will require more investigation as to why it wasn't working.

Could you open a ticket for that detailing what you tried so far (and what did not work), so this does not fall through the cracks?

I think that's #30270. We are done here, closing.

Note: See TracTickets for help on using tickets.