[tor-bugs] #27609 [Applications/Tor Browser]: TBA: Evaluate Tor Onion Proxy Library

[Tor Bug Tracker & Wiki]

#27609: TBA: Evaluate Tor Onion Proxy Library
-------------------------------------------------+-------------------------
 Reporter:  sysrqb                               |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Very High                            |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  tbb-mobile, TBA-a3,                  |  Actual Points:
  TorBrowserTeam201903, tbb-8.5                  |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor8
-------------------------------------------------+-------------------------

Comment (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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/27609#comment:35>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online