Opened 7 months ago

Last modified 5 days ago

#30501 needs_review defect

BridgesList Preferences is an overloaded field

Reported by: sisbell Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, TorBrowserTeam201912R
Cc: igt0, sisbell, sysrqb, hans@…, n8fr8 Actual Points:
Parent ID: #31280 Points: 2
Reviewer: sysrqb Sponsor: Sponsor30-can

Description

BridgesList is an overloaded field, which can cause some confusion. The list can be:

  1. a filter like obfs4 or meek OR  
  2. can be a custom bridge

For (1), we just pass back all bridges, the filter will occur elsewhere in the library. For (2) we return the bridge list as a raw stream. If length is greater than 5, then we know this is a custom bridge

We should fix this so that we have separate preferences for 1 and 2. We also shouldn't be looking at string length to determine field type.

Child Tickets

Change History (19)

comment:1 Changed 7 months ago by sisbell

Keywords: tbb-8.5-must removed

comment:2 Changed 4 months ago by pili

Parent ID: #31280

comment:3 Changed 4 months ago by pili

Sponsor: Sponsor30-can

comment:4 Changed 4 months ago by pili

Parent ID: #31280#31349

comment:5 Changed 4 months ago by pili

Parent ID: #31349#31280

comment:6 Changed 7 weeks ago by sysrqb

Points: 0.5

comment:7 Changed 7 weeks ago by gk

Points: 0.51

comment:8 Changed 7 weeks ago by sisbell

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201905 removed
Points: 12
Status: newneeds_review

Involves fixes in both TOPL code and tor-android-service

https://github.com/sisbell/tor-android-service/commit/0835e623f802127898df89cfb13685220dbe24b2

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/ac9ad2295b3dcb312a9aff4f3f7b27343bc4b73d

This will create an incompatibility with Orbot, so I'll need to open another issue for that.

comment:9 Changed 6 weeks ago by sysrqb

Thanks! I'll start with TOPL in one comment and then post tor-android-services in another comment.

I think any incompatibility introduced between tor-android-service and Orbot should be resolved after a larger and more general conversation about where we're going with these libraries and whether it is worth our time maintaining compatibility (and if we should maintain these libraries in the long-term).

comment:10 Changed 6 weeks ago by sysrqb

universal/src/main/java/com/msopentech/thali/toronionproxy/TorConfigBuilder.java:

@@ -92,32 +92,19 @@ public final class TorConfigBuilder {
[snip]
        if (!pluggableTransportClient.exists() || !pluggableTransportClient.canExecute()) {
             throw new IOException("Bridge binary does not exist: " + pluggableTransportClient
                     .getCanonicalPath());
         }

Please make this exception's message reflect the correct error - does it noe exist or is it not executable?

@@ -153,6 +140,27 @@ public final class TorConfigBuilder {
         return controlPortWriteToFile(context.config.getControlPortFile().getAbsolutePath());
     }

+    public TorConfigBuilder customBridges(List<String> bridges) {
+        if(bridges.size() > 1) {

nit: "if ("

+            Collections.shuffle(bridges, new Random(System.nanoTime()));
+        }

I still don't completely understand this (or above). What's the reasoning for shuffling? Tor chooses randomly from the list, in any case.

+        for (String bridge : bridges) {
+            if(!isNullOrEmpty(bridge)) {
+                line("Bridge " + bridge);
+            }

nit: Can you use bridge() instead of line() here? I think it'll be more readable if bridge() is used consistently when we are adding a bridge. (see below comment about bridge(), as well)

+    @SettingsConfig
+    public TorConfigBuilder customBridgesFromSettings() {
+        if (!settings.hasBridges() || !hasCustomBridges()) {
+            return this;
+        }
+        List<String> bridges = settings.getCustomBridges();
+        return customBridges(bridges);

Can you pass settings.getCustomBridges() directly into customBridges()?

+    /**
+     * Write up two bridges from packaged bridge list if bridges option is enabled and if no custom bridges

nit: "Write up to"?
I also don't understand why it only writes two bridges? Why not write all of them?

+    TorConfigBuilder addPredefinedBridgesFromResources(List<BridgeType> types, int maxBridges) {

Should this have package-level visibility? I realize it was before this change, too.

+        ArrayList<String> bridgeTypes = new ArrayList<>();
+        for(BridgeType bridgeType : types) {

nit: "for ("

+            bridgeTypes.add(bridgeType.name().toLowerCase());
+        }
+        if(settings.hasBridges() && hasPredefinedBridges() && !hasCustomBridges()) {

nit: "if ("

-            if (b.type.equals(bridgeType)) {
+            if(bridgeTypes.contains(b.type)) {

"if ("

public TorConfigBuilder bridge(String type, String config) {

Can you change the name of this method to something like "addBridgeLine"? "bridge()" is not descriptive.

+    private boolean hasPredefinedBridges() {
+        return !settings.getBridgeTypes().isEmpty();
     }

Isn't this only testing that we have pre-defined bridge types, but not actual bridges? We'd want to know if the stream for addPredefinedBridgesFromStream() returns any bridges, right? Maybe rename it as hasPredefinedBridgeTypes()? It seem like we also want to know if we actually have predefined bridges in some cases, too. In particular, useBridgesFromSettings() and addPredefinedBridgesFromResources() should use this, right?

comment:11 Changed 6 weeks ago by sysrqb

Status: needs_reviewneeds_revision

service/src/main/java/org/torproject/android/service/AndroidTorSettings.java:

-    @Override
-    public boolean hasDormantCanceledByStartup() {
-        return true;
-    }

Don't we need this?

service/src/main/java/org/torproject/android/service/util/Prefs.java

+    public static void setBridgeTypes(List<String> bridgeTypes) {
+        if(bridgeTypes == null || bridgeTypes.isEmpty()) {

nit: "if ("

+    public static List<String> getBridgeTypes() {
+        String bridgeTypes = prefs.getString(PREF_BRIDGE_TYPES, null);
+        if(bridgeTypes == null || bridgeTypes.isEmpty()) {

nit: "if (" (and the other instances in the patch).

+            BridgeType defaultBridgeType = (Locale.getDefault().getLanguage().equals("fa")) ? BridgeType.MEEK_LITE : BridgeType.OBFS4;

We should check with the anti-censorship team that this is still the correct default. I know we inherited this from Orbot.

+            return Collections.singletonList(defaultBridgeType.name().toLowerCase());

I'm not opposed to this, but did you choose a singletonList for a reason?

+        return Arrays.asList(bridgeTypes.split("[|]"));

Please create a wrapper-method around split(), like join(), in case we need to change the separate at a later point.

+    private static String join(List<String> list) {
+        StringBuilder sb = new StringBuilder();
+        for (int i = 0; i < list.size(); i++) {
+            sb.append(list.get(i));
+            if (i != list.size() - 1) {
+                sb.append("|");

I think | is a safe divider, but we should confirm with the anti-censorship team. In particular, I think | could be used in the PT arguments, but I don't think any of the current transports use an encoding that could include |.

comment:12 in reply to:  10 Changed 6 weeks ago by sisbell

Replying to sysrqb:

universal/src/main/java/com/msopentech/thali/toronionproxy/TorConfigBuilder.java:

@@ -92,32 +92,19 @@ public final class TorConfigBuilder {
[snip]
        if (!pluggableTransportClient.exists() || !pluggableTransportClient.canExecute()) {
             throw new IOException("Bridge binary does not exist: " + pluggableTransportClient
                     .getCanonicalPath());
         }

Please make this exception's message reflect the correct error - does it noe exist or is it not executable?

Sure, easy enough

@@ -153,6 +140,27 @@ public final class TorConfigBuilder {
         return controlPortWriteToFile(context.config.getControlPortFile().getAbsolutePath());
     }

+    public TorConfigBuilder customBridges(List<String> bridges) {
+        if(bridges.size() > 1) {

nit: "if ("

+1

+            Collections.shuffle(bridges, new Random(System.nanoTime()));
+        }

I still don't completely understand this (or above). What's the reasoning for shuffling? Tor chooses randomly from the list, in any case.

I wasn't aware that tor itself does the shuffling. I will remove this line.

+        for (String bridge : bridges) {
+            if(!isNullOrEmpty(bridge)) {
+                line("Bridge " + bridge);
+            }

nit: Can you use bridge() instead of line() here? I think it'll be more readable if bridge() is used consistently when we are adding a bridge. (see below comment about bridge(), as well)

line method is a general method for just writing out a line. Its also used in torrcCustomFromSettings method. I can change this is writeLine to make clearer.

+    @SettingsConfig
+    public TorConfigBuilder customBridgesFromSettings() {
+        if (!settings.hasBridges() || !hasCustomBridges()) {
+            return this;
+        }
+        List<String> bridges = settings.getCustomBridges();
+        return customBridges(bridges);

Can you pass settings.getCustomBridges() directly into customBridges()?

Sure, its a stylistic preference but easy enough.

+    /**
+     * Write up two bridges from packaged bridge list if bridges option is enabled and if no custom bridges

nit: "Write up to"?
I also don't understand why it only writes two bridges? Why not write all of them?

I thought this was a requirement but if not, I'll remove it.

+    TorConfigBuilder addPredefinedBridgesFromResources(List<BridgeType> types, int maxBridges) {

Should this have package-level visibility? I realize it was before this change, too.

I'll change it. I left some open for the unit tests but this method is not under direct test.

+        ArrayList<String> bridgeTypes = new ArrayList<>();
+        for(BridgeType bridgeType : types) {

nit: "for ("

+            bridgeTypes.add(bridgeType.name().toLowerCase());
+        }
+        if(settings.hasBridges() && hasPredefinedBridges() && !hasCustomBridges()) {

nit: "if ("

-            if (b.type.equals(bridgeType)) {
+            if(bridgeTypes.contains(b.type)) {

"if ("

public TorConfigBuilder bridge(String type, String config) {

Can you change the name of this method to something like "addBridgeLine"? "bridge()" is not descriptive.

The TorConfigBuilder just maps method name to fieldName. If I changed this one I would need to do such for all methods, which I think would junk up the readability of all the public methods, as they would all be add{FieldName}Line.

+    private boolean hasPredefinedBridges() {
+        return !settings.getBridgeTypes().isEmpty();
     }

Isn't this only testing that we have pre-defined bridge types, but not actual bridges? We'd want to know if the stream for addPredefinedBridgesFromStream() returns any bridges, right? Maybe rename it as hasPredefinedBridgeTypes()?

Makes sense, I'll rename method to be clearer

It seem like we also want to know if we actually have predefined bridges in some cases, too. In particular, useBridgesFromSettings() and addPredefinedBridgesFromResources() should use this, right?

This would occur if the bridges.txt is empty. This check occurs in readBridgesFromStream, which would return an empty list. addPredefinedBridgesFromStream invokes readBridgesFromStream so that is where the check is currently occurring (an empty list is effectively a noop). I can document this to be clearer.

I'll need to investigate further the case of useBridgesFromSettings if the list is empty, in that case it may require explicit check.

comment:13 Changed 6 weeks ago by sisbell

One other thing we will need as part of this issue. We need to have code to migrate from the old format to the new format on upgrade so that we don't wipe out existing user preferences.

comment:14 Changed 6 weeks ago by gk

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed

needs revision

comment:15 Changed 5 weeks ago by sisbell

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201910 removed
Status: needs_revisionneeds_review

I collapsed #30552 into the same commit since it involves same code area. I removed the shuffling, removed max bridge check, renamed methods to make clearer, more tests, more docs. For one case, I check for existence of bridges.txt before setting bridges flag. Various other changes, cleanups.

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/b72487be7b53d1ffcbd5f3942f2d0f7b1c79c859

comment:16 in reply to:  13 ; Changed 5 weeks ago by sysrqb

Replying to sisbell:

One other thing we will need as part of this issue. We need to have code to migrate from the old format to the new format on upgrade so that we don't wipe out existing user preferences.

I can see this being in either Tor Browser or tor-android-service. I haven't thought hard about which place is better. We'll need a patch for Tor Browser so it uses the new API, in either case.

comment:17 in reply to:  16 Changed 5 weeks ago by sisbell

Replying to sysrqb:

Replying to sisbell:

One other thing we will need as part of this issue. We need to have code to migrate from the old format to the new format on upgrade so that we don't wipe out existing user preferences.

I can see this being in either Tor Browser or tor-android-service. I haven't thought hard about which place is better. We'll need a patch for Tor Browser so it uses the new API, in either case.

We need to do this in tor-android-service. The tor settings interface in topl just has getter methods, so their is no way at a interface level to pull out the values and then to set them back to the new format. Its up to each implementation to do migration like this.

comment:18 Changed 5 days ago by gk

Keywords: TorBrowserTeam201912R added; TorBrowserTeam201911R removed

We are in December now.

comment:19 Changed 5 days ago by pili

Reviewer: sysrqb

sysrqb to review these tickets

Note: See TracTickets for help on using tickets.