If custom bridges are specified, only use those bridges for connecting
Currently it seems custom bridges are added to the preconfigured bridges list and then 3 are chosen randomly. What should happen, though, is that only those custom bridges should get used that are specified.
(That's based on a discussion by sisbell and sysrqb on #tor-dev. I am just the messenger in this case. :) )
Activity
Trac:
Parent Ticket: #27609 (moved)
Trac:
Description: Currently it seems custom bridges are added to the preconfigured bridges list and then 3 are chosen randomly. What should happen, though, is that only those custom bridges should get used that are specified.to
Currently it seems custom bridges are added to the preconfigured bridges list and then 3 are chosen randomly. What should happen, though, is that only those custom bridges should get used that are specified.
(That's based on a discussion by sisbell and sysrqb on #tor-dev. I am just the messenger in this case. :) )
-
I created bug_30166 (https://gitweb.torproject.org/user/gk/tor-browser-build.git/commit/?h=bug_30166&id=5ec4223148f43b2782bf88dc5b1b738eac75a5dd) in my
tor-browser-buildrepo but still have the issue that I can add a random IP address:port combination and Tor Browser is happily ignoring the custom bridge settings and connecting directly. So it's hard for me to test that changes actually. sisbell: are you seeing the same with your build or do you get any error when doing as I did?Trac:
Status: needs_review to needs_information -
Okay, I double-checked that I did not make a mistake here. The
tor-android-servicerepo I used is indeed on commit de13672fe1ef6eb180edd4ec80f3f416f5d5c6b1 and should thus have your fix. However, testing the resulting nightly as follows still shows the behavior in comment:5- Install the resulting nightly
- Before you start bootstrapping click on the gear icon and enter a random custom bridge. I just used 1.2.3.4:12345
- Go back to the bootstrap screen
- Start bootstrapping
- Expected: error about not working bridge or something
- Actual: Happy bootstrapping making a direct connection
Trac:
Status: needs_information to needs_revision
Keywords: TorBrowserTeam201904R deleted, TorBrowserTeam201904 added Replying to gk:
Okay, I double-checked that I did not make a mistake here. The
tor-android-servicerepo I used is indeed on commit de13672fe1ef6eb180edd4ec80f3f416f5d5c6b1 and should thus have your fix. However, testing the resulting nightly as follows still shows the behavior in comment:5- Install the resulting nightly
- Before you start bootstrapping click on the gear icon and enter a random custom bridge. I just used 1.2.3.4:12345
- Go back to the bootstrap screen
- Start bootstrapping
- Expected: error about not working bridge or something
- Actual: Happy bootstrapping making a direct connection
Yes, there is a bug in the code. The app is only processing bridges if the custom entry has obfs4 or meek at a first field. So the custom fields are being ignored and then the app proceeds to just make a direct connection. I'll put in a fix.
I updated the TOPL and android-tor-service code to fix this issue:
https://github.com/sisbell/tor-android-service/commit/30a7a83c5e715eb7ef739870bc0ff615ad52daeb https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/061b7e52310279cb70a125819d7abed8da66bfbe
I verified with Orbot app that it is picking up user-defined bridges (using one I pulled from torproject bridges site). When the user does not define a bridge but enables bridges, I verified that it is using one of the random bridges during startup. The PT entries are also in the torrc file.
The one case I'm not certain of (and the commits above don't handle), is if a user specifies a bridge type (like obfs4) as the first field in the 'Bridge Addresses' TextField. Is this allowed?
I also still need to test this on within the tor-browser-build.
-
Replying to sisbell:
I updated the TOPL and android-tor-service code to fix this issue:
https://github.com/sisbell/tor-android-service/commit/30a7a83c5e715eb7ef739870bc0ff615ad52daeb https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/061b7e52310279cb70a125819d7abed8da66bfbe
I verified with Orbot app that it is picking up user-defined bridges (using one I pulled from torproject bridges site). When the user does not define a bridge but enables bridges, I verified that it is using one of the random bridges during startup. The PT entries are also in the torrc file.
The one case I'm not certain of (and the commits above don't handle), is if a user specifies a bridge type (like obfs4) as the first field in the 'Bridge Addresses' TextField. Is this allowed?
Yes. If you e.g. test on https://bridges.torproject.org and obtain obfs4 bridges that way the idea is to copy those lines into your Tor Browser and the correct bridges are picked up. (see the howto for instructions, which is for now only for desktop, though: https://bridges.torproject.org/howto and the bridge options at https://bridges.torproject.org/options).
Fixed in branch 505
- I tested with a bad bridge. The console says that tor unable to start. This is not descriptive to the user but failing to start is what we expect
- Tested with working custom bridge and saw the bridge used during startup
- Tested with any obfs4 bridge (one from pre-configured list in apk) and saw one of the bridges in the list used during startup
This fix is in TOPL and tor-android-service
https://github.com/thaliproject/Tor_Onion_Proxy_Library/issues/115
https://github.com/sisbell/tor-android-service/issues/26
The commit in tor-browser-build that picks these up is here.
!https://github.com/sisbell/tor-browser-build/commit/c1cf5b6d77637b5095b3063d4f5d5fedfab6e9e7
Trac:
Status: needs_revision to needs_review-
Okay, we have now an own
tor-android-servicerepo and we are using that one for our builds. Thus, just bumping the git version in ourtor-browser-buildproject is not working anymore. We should start going the usual review process: changes totor-android-serviceneed to get reviewed and if there are no issues then the commit will be pushed tomasteron TPO infra. Then we need to bump the commit intor-browser-build. So, let me start by reviewing the proposedtor-android-servicecommits. -
Alright here come some questions:
-
It seems de13672fe1ef6eb180edd4ec80f3f416f5d5c6b1 and 30a7a83c5e715eb7ef739870bc0ff615ad52daeb fix the same bug (this one). Could you prepare a commit with both of those changes as the first of those commits does not fully fix the problem?
-
What's the need for commit 7e77a5cd6c9ec609963e3b60b7838116cb8b37e2? (I guess we should at least mention the reason for that in the commit message)
-
What's the deal with the TOPL libs in the repo given that we build those ourselves during the build?
Trac:
Keywords: TorBrowserTeam201905R deleted, TorBrowserTeam201905 added
Status: needs_review to needs_revision -
Replying to gk:
Alright here come some questions:
- It seems de13672fe1ef6eb180edd4ec80f3f416f5d5c6b1 and 30a7a83c5e715eb7ef739870bc0ff615ad52daeb fix the same bug (this one). Could you prepare a commit with both of those changes as the first of those commits does not fully fix the problem? Sure I'll get that fixed. I have been working off master, so I didn't want to rebase in case anyone else was using the library but now that we have brought in the code, I'll work off the branches with rebases.
- What's the need for commit 7e77a5cd6c9ec609963e3b60b7838116cb8b37e2? (I guess we should at least mention the reason for that in the commit message) This may be an issue for further discussion. My idea has been to create a library for general use in the community so I wanted to keep the Android versions updated. Otherwise, we lock developers into older versions of Android Studio and dependencies, which makes adoption more difficult.
The versions are isolated to the properties files so changes are easy. As long our patches for the versions still work, this seems a good compromise.
- What's the deal with the TOPL libs in the repo given that we build those ourselves during the build? This is also related to general community use. I want this to work outside of tor-browser-build. Ideally, when things are stable (which looks very close), we will get the TOPL libraries pushed to a maven repo, rather than including them in the libs directory.
-
Replying to sisbell:
Replying to gk:
Alright here come some questions:
- It seems de13672fe1ef6eb180edd4ec80f3f416f5d5c6b1 and 30a7a83c5e715eb7ef739870bc0ff615ad52daeb fix the same bug (this one). Could you prepare a commit with both of those changes as the first of those commits does not fully fix the problem? Sure I'll get that fixed. I have been working off master, so I didn't want to rebase in case anyone else was using the library but now that we have brought in the code, I'll work off the branches with rebases.
Thanks.
- What's the need for commit 7e77a5cd6c9ec609963e3b60b7838116cb8b37e2? (I guess we should at least mention the reason for that in the commit message) This may be an issue for further discussion. My idea has been to create a library for general use in the community so I wanted to keep the Android versions updated. Otherwise, we lock developers into older versions of Android Studio and dependencies, which makes adoption more difficult.
The versions are isolated to the properties files so changes are easy. As long our patches for the versions still work, this seems a good compromise.
Works for me. I'd like to have in the commit message a hint, though, what makes the update necessary or why we are doing it (maybe it's a new SDK we want to support, maybe a new Android version, maybe...), just so that one has something to reason about whether the actual change is good/necessary.
- What's the deal with the TOPL libs in the repo given that we build those ourselves during the build? This is also related to general community use. I want this to work outside of tor-browser-build. Ideally, when things are stable (which looks very close), we will get the TOPL libraries pushed to a maven repo, rather than including them in the libs directory.
Okay, yes, please. Meanwhile I am fine having them in the repo, although I'd like to avoid that as a permanent solution.
Fixed in branch 0512: !https://github.com/sisbell/tor-android-service/commits/0512
Trac:
Status: needs_revision to needs_review-
I tested a resulting build using
sisbell/0506+ pointingtor-android-servicetor your0512and this solves this bug, great! The changes in commit 5df4fd213ceda8320fc9e46b12d3bec3024e5c64 seem reasonable to me. A small nit: s/confusion../confusion./It seems to me we should open a follow-up ticket cleaning up the overloading of the bridges list. That's pretty confusing and makes it hard to follow the code. We should come up with a simpler way of setting/getting bridges in that regard.
Setting into
needs_revisionfor the nit and for redoing thetor-browser-buildpatches taking other review comments into account (http:// -> https://).Trac:
Status: needs_review to needs_revision -
Oh, and it just came to my mind that we'll probably have a snowflake transport in Tor Browser for Android, soon (at least in the alpha). Thus,
+ byte bridgeType = (byte) (userDefinedBridgeList.length() > 5 ? 1 : 0);might not work as expected anymore then. I guess we take the length of "snowflake" into account until we've redone the bridges list part.
Replying to gk:
I tested a resulting build using
sisbell/0506+ pointingtor-android-servicetor your0512and this solves this bug, great! The changes in commit 5df4fd213ceda8320fc9e46b12d3bec3024e5c64 seem reasonable to me. A small nit: s/confusion../confusion./It seems to me we should open a follow-up ticket cleaning up the overloading of the bridges list. That's pretty confusing and makes it hard to follow the code. We should come up with a simpler way of setting/getting bridges in that regard. Not overloading the preference is preferable. I think this change may have UI impact, since the UI field we are reading from is also overloaded. This will require some looking into to see if this is the case.
Setting into
needs_revisionfor the nit and for redoing thetor-browser-buildpatches taking other review comments into account (http:// -> https://).-
Replying to sisbell:
Replying to gk:
I tested a resulting build using
sisbell/0506+ pointingtor-android-servicetor your0512and this solves this bug, great! The changes in commit 5df4fd213ceda8320fc9e46b12d3bec3024e5c64 seem reasonable to me. A small nit: s/confusion../confusion./It seems to me we should open a follow-up ticket cleaning up the overloading of the bridges list. That's pretty confusing and makes it hard to follow the code. We should come up with a simpler way of setting/getting bridges in that regard. Not overloading the preference is preferable. I think this change may have UI impact, since the UI field we are reading from is also overloaded. This will require some looking into to see if this is the case.
Yes, which we should do it in a new ticket (could you open that one?) and not for 8.5.
I updated the nit and added https to the maven repo.
https://github.com/sisbell/tor-android-service/commits/0513
I also updated tor-browser-build to use the latest commits
https://github.com/sisbell/tor-browser-build/commits/0513
Note that the git_hash in tor-android-service/config file doesn't exist yet in the torproject repo. The commit is only in the github branch. So we will need to merge the tor-android-service changes for the ttb build to work.
git_hash: d5c9b1f569e1f04ccadeb75184cec0766c4c2f4d git_url: https://git.torproject.org/tor-android-service.gitFor testing, you can locally modify the git_url to
https://github.com/sisbell/tor-android-service.git-
Okay, for the
tor-browser-buildpatch just a nit:- ++in the
servive/build.gradlepatch you are adding a bunch of superfluous whitespace, please remove that. Otherwise this looks good.Trac:
Keywords: TorBrowserTeam201905R deleted, TorBrowserTeam201905 added
Status: needs_review to needs_revision