Opened 3 weeks ago

Last modified 6 days ago

#32516 needs_review defect

Make Write Methods Clearer in TorConfigBuilder

Reported by: sisbell Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-mobile, TorBrowserTeam201912R
Cc: gk, sysrqb Actual Points:
Parent ID: Points: .25
Reviewer: sysrqb Sponsor:

Description

Make Write Methods Clearer in TorConfigBuilder. We have a lot of duplicate buffer appends that we can cleanup to make code more readable.

Child Tickets

Change History (8)

comment:1 Changed 3 weeks ago by sisbell

Status: newneeds_review

Ready for Review. No breaking API changes.

Changes

  • Added write line and write properties methods and used throughout property settings
  • Did a format on the code to clear up previously flag nits
  • I added a write address method. Its currently unused now but will be used in subsequent commits

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/1f95271ae222839797825020a41491ac8aab0d8f

comment:2 Changed 3 weeks ago by gk

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed

comment:3 Changed 12 days ago by sysrqb

Status: needs_reviewneeds_revision
     public TorConfigBuilder makeNonExitRelay(String dnsFile, int orPort, String nickname) {
-        buffer.append("ServerDNSResolvConfFile ").append(dnsFile).append('\n');
-        buffer.append("ORPort ").append(orPort).append('\n');
-        buffer.append("Nickname ").append(nickname).append('\n');
-        buffer.append("ExitPolicy reject *:*").append('\n');
-        return this;
+        writeLine("ServerDNSResolvConfFile", dnsFile);
+        writeLine("ORPort", String.valueOf(orPort));

nit: ORPort should take an address, as well, and writeAddress seems more explicit. But don't worry about that right now, we can come back to this later.

public TorConfigBuilder proxyOnAllInterfaces() {
-        buffer.append("SocksListenAddress 0.0.0.0").append('\n');
-        return this;
+        return writeLine("SocksListenAddress 0.0.0.0");
     }

nit: This could use writeAddress("SocksListenAddress", "0.0.0.0", null, null) instead

     public TorConfigBuilder transportPluginObfs(String clientPath) {
-        buffer.append("ClientTransportPlugin obfs3 exec ").append(clientPath).append('\n');
-        buffer.append("ClientTransportPlugin obfs4 exec ").append(clientPath).append('\n');
-        return this;
+        return writeLine("ClientTransportPlugin obfs3 exec", clientPath)
+                .writeLine("ClientTransportPlugin obfs4 exec", clientPath);
     }

This can be simplified to ClientTransportPlugin obfs3,obfs4 exec. Or, if we take this two steps further, combine this method and transportPluginMeek into a single transportPlugin and use ClientTransportPlugin meek_lite,obfs2,obfs3,obfs4,scramblesuit exec

writeAddress

+        if (port != null) {
+            buffer.append(port <= 0 ? "auto" : port);
+        } else {
+            buffer.append("auto");
+        }

I'm still not sure we should change port <= 0 into auto. *Port 0 is already defined as disabling that type of port. Do we need two ways of setting auto?

+    public TorConfigBuilder writeLine(String value, String value2) {
+        return writeLine(value + " " + value2);
+    }

This seems like a surprising overload. I'm not opposed to having it, but it doesn't seem helpful and it is more confusing (less readable) than simply passing the string concatenation directly into writeLine(String).

comment:4 Changed 12 days ago by gk

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201911R removed

comment:5 in reply to:  3 Changed 11 days ago by sisbell

Replying to sysrqb:

     public TorConfigBuilder makeNonExitRelay(String dnsFile, int orPort, String nickname) {
-        buffer.append("ServerDNSResolvConfFile ").append(dnsFile).append('\n');
-        buffer.append("ORPort ").append(orPort).append('\n');
-        buffer.append("Nickname ").append(nickname).append('\n');
-        buffer.append("ExitPolicy reject *:*").append('\n');
-        return this;
+        writeLine("ServerDNSResolvConfFile", dnsFile);
+        writeLine("ORPort", String.valueOf(orPort));

nit: ORPort should take an address, as well, and writeAddress seems more explicit. But don't worry about that right now, we can come back to this later.

Good catch, I missed that one. WriteAddress with all the host support will come in another commit. Its a minor breaking change as we will need to change the field params for each host/port pair.

public TorConfigBuilder proxyOnAllInterfaces() {
-        buffer.append("SocksListenAddress 0.0.0.0").append('\n');
-        return this;
+        return writeLine("SocksListenAddress 0.0.0.0");
     }

nit: This could use writeAddress("SocksListenAddress", "0.0.0.0", null, null) instead

     public TorConfigBuilder transportPluginObfs(String clientPath) {
-        buffer.append("ClientTransportPlugin obfs3 exec ").append(clientPath).append('\n');
-        buffer.append("ClientTransportPlugin obfs4 exec ").append(clientPath).append('\n');
-        return this;
+        return writeLine("ClientTransportPlugin obfs3 exec", clientPath)
+                .writeLine("ClientTransportPlugin obfs4 exec", clientPath);
     }

This can be simplified to ClientTransportPlugin obfs3,obfs4 exec. Or, if we take this two steps further, combine this method and transportPluginMeek into a single transportPlugin and use ClientTransportPlugin meek_lite,obfs2,obfs3,obfs4,scramblesuit exec

Yes that will look cleaner.

writeAddress

+        if (port != null) {
+            buffer.append(port <= 0 ? "auto" : port);
+        } else {
+            buffer.append("auto");
+        }

I'm still not sure we should change port <= 0 into auto. *Port 0 is already defined as disabling that type of port. Do we need two ways of setting auto?

I believe you made this comment before, so I put in the following fix. Its in the commit link at top, just under the issue description.

        if (port != null && port >= 0) {
            buffer.append(port);
        } else {
            buffer.append("auto");
        }
+    public TorConfigBuilder writeLine(String value, String value2) {
+        return writeLine(value + " " + value2);
+    }

This seems like a surprising overload. I'm not opposed to having it, but it doesn't seem helpful and it is more confusing (less readable) than simply passing the string concatenation directly into writeLine(String).

This was a bit of a shortcut on my part. I think the correct solution would be to take varargs on the writeLine and just loop the append. I'll get that fixed.

comment:6 Changed 6 days ago by sisbell

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: needs_revisionneeds_review

Changes:

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/75491165af6339f28503eae09b1984632df78b0c

  1. Use writeAddress for orPort. SocksListenAddress doesn't take a port (as far as I can tell) so I can't use the writeAddress method for it as this would add ':auto' constant to the field.
  2. ClientTransportPlugin loads meek_lite, obf3 and obfs4 on same line. The implementation will write out all of the supported bridges from settings onto a single line. This assumes that all bridges can be handled by one clientTransportPlugin. This is suitable for the Android implementation but we should revisit this issue later.
  3. Use varargs for writeLine method so we can add as many values as we like
  4. Changed writeTrueProperty and writeFalseProperty to use writeLine
  5. The auto port issue was fixed in previous commit (and is also in this commit). Port will be "auto" if null or if port value is less than 0.

Open Issues:

  1. SocksListenAddress is deprecated. So need to investigate removal/migration of the field. This needn't be addressed in this commit.

comment:7 Changed 6 days ago by sisbell

Keywords: TorBrowserTeam201912R added; TorBrowserTeam201911R removed

comment:8 Changed 6 days ago by pili

Reviewer: sysrqb

sysrqb to review

Note: See TracTickets for help on using tickets.