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

Wed Feb 20 21:55:52 UTC 2019

#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:
  TorBrowserTeam201902                           |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor8
-------------------------------------------------+-------------------------
Changes (by sysrqb):

 * status:  new => needs_revision

Comment:

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

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