[tbb-bugs] #27609 [Applications/Tor Browser]: TBA: Evaluate Tor Onion Proxy Library
Tor Bug Tracker & Wiki
blackhole at torproject.org
Mon Feb 25 22:41:35 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
-------------------------------------------------+-------------------------
Comment (by sisbell):
Replying to [comment:28 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).
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/27609#comment:30>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
More information about the tbb-bugs
mailing list