Opened 8 months ago

Last modified 2 weeks ago

#30552 needs_review defect

Android - Clean up torrc

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

Description

As mentioned in ticket:30284#comment:22 and shown in ticket:30518#comment:3, the current torrc file has many entries we don't need. Let's clean out that file and only include the pieces we need.

Child Tickets

Change History (24)

comment:1 Changed 3 months ago by sysrqb

Points: 0.25

comment:2 Changed 3 months ago by sisbell

I'm not clear from the referenced issues which config entries we should remove.

comment:3 Changed 3 months ago by sisbell

From what I can tell, we have a bunch of entries with 0 value (the defaults for those fields). I'll remove those. It requires switching from primitive int to class Integer so we can detect if the value is null and shouldn't be set.

comment:4 Changed 3 months ago by sisbell

Keywords: TorBrowserTeam201910 added
Points: 0.250.5

comment:5 Changed 3 months ago by sisbell

From investigating, we don't need the following since 0 is the default value

  • StrictNodes 0
  • UseBridges 0
  • SafeSocks 0
  • TestSocks 0

The following also don't need to be set as far as I can tell (they are their specifically for Orbot support)

  • AutomapHostsOnResolve 1
  • HTTPTunnelPort 8218
  • TransPort 9140
  • VirtualAddrNetwork 10.192.0.0/10

comment:6 Changed 3 months ago by sisbell

Cc: gk added
Status: newneeds_review

I made the following changes to tor-onion-proxy-library. If the changes look good, I will need to make it compatible with tor-android-service.

  • StrictNodes, UseBridges, SafeSocks, TestSocks will not be included in the torrc if they have a 0 value
  • DnsPort, HttpTunnelPort, TransparentProxyPort are now all Integers. I also added an associated host field for each one.
  • ProxyPort, ProxySocks5ServerPort are now an Integers rather than Strings.
  • HttpTunnelPort and TransPort values are set to null so it won't appear in torrc by default.
  • AutomapHostsOnResolve is set to false so won't appear in torrc by default

The remaining field VirtualAddrNetwork will need to be removed in tor-android-service

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/73aeed144259a66930e605c7804946c9f6041b59

comment:7 Changed 3 months ago by gk

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed

sisbell: please add the review keyword (TorBrowserTeam2019YYYMMR) if you put tickets into needs_review.

comment:8 in reply to:  6 ; Changed 3 months ago by sysrqb

Status: needs_reviewneeds_revision

Replying to sisbell:

I made the following changes to tor-onion-proxy-library. If the changes look good, I will need to make it compatible with tor-android-service.

I think it would make more sense leaving TOPL's defaults unchanged, as much as possible. I don't see a problem with explicitly setting default values in the torrc (like TestSocks 0), but I do like cleaner defaults and a cleaner/smaller generated torrc.

  • StrictNodes, UseBridges, SafeSocks, TestSocks will not be included in the torrc if they have a 0 value

I don't have a strong opinion on this. It doesn't really have any impact until tor's default values change for whatever reason.

  • DnsPort, HttpTunnelPort, TransparentProxyPort are now all Integers. I also added an associated host field for each one.
  • ProxyPort, ProxySocks5ServerPort are now an Integers rather than Strings.
  • HttpTunnelPort and TransPort values are set to null so it won't appear in torrc by default.
  • AutomapHostsOnResolve is set to false so won't appear in torrc by default

These are the more important changes.

The remaining field VirtualAddrNetwork will need to be removed in tor-android-service

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/73aeed144259a66930e605c7804946c9f6041b59

universal/src/main/java/com/msopentech/thali/toronionproxy/DefaultSettings.java

     @Override
-    public int getHttpTunnelPort() {
-        return 8118;
+    public String getHttpTunnelHost() {
+        return null;
+    }
+
+    @Override
+    public Integer getHttpTunnelPort() {
+        return null;
     }

This is changing the default port, is that intentional?

     @Override
-    public String transPort() {
-        return "9040";
+    public String getTransparentProxyAddress() {
+        return null;
+    }
+
+    @Override
+    public Integer getTransparentProxyPort() {
+        return null;

This is changing the default port, is that intentional?

+    TorConfigBuilder addAddress(String fieldName, String address, Integer port, String flags) {
+        if(isNullOrEmpty(address) && port == null) {
+            return this;
+        }
+        buffer.append(fieldName).append(" ");
+        if(!isNullOrEmpty(address)) {
+            buffer.append(address).append(":");
+        }
+        if (port != null) {
+            buffer.append(port <= 0 ? "auto" : port);

Please pass this directly to tor, we shouldn't change the intended behavior if the app configures a 0 or negative port number. Tor will emit a warning which the app should handle itself.

universal/src/test/java/com/msopentech/thali/toronionproxy/TorConfigBuilderTest.java

+    /**
+     * SHould be empty
+     */

nit.

comment:9 Changed 3 months ago by gk

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed

needs revision

comment:10 Changed 3 months ago by sysrqb

Keywords: TorBrowserTeam201910R added; TorBrowserTeam201910 removed

Something else, but not for this ticket, is that TOPL should be consistent about how it handles integer config options.

    @Override
    public String getSocksPort() {
        return "9050";
    }

At this point, I don't think it matters if they are stored and handled as Strings or Integers, but they should all be the same.

comment:11 Changed 3 months ago by sysrqb

Keywords: TorBrowserTeam201910 added; TorBrowserTeam201910R removed

sigh.

comment:12 in reply to:  10 ; Changed 3 months ago by sisbell

Replying to sysrqb:

Something else, but not for this ticket, is that TOPL should be consistent about how it handles integer config options.

    @Override
    public String getSocksPort() {
        return "9050";
    }

I left this since we also need to handle "auto" and we have the auto detect on the port so its a bigger change for this field.

At this point, I don't think it matters if they are stored and handled as Strings or Integers, but they should all be the same.

comment:13 in reply to:  8 ; Changed 3 months ago by sisbell

Replying to sysrqb:

Replying to sisbell:

I made the following changes to tor-onion-proxy-library. If the changes look good, I will need to make it compatible with tor-android-service.

I think it would make more sense leaving TOPL's defaults unchanged, as much as possible. I don't see a problem with explicitly setting default values in the torrc (like TestSocks 0), but I do like cleaner defaults and a cleaner/smaller generated torrc.

  • StrictNodes, UseBridges, SafeSocks, TestSocks will not be included in the torrc if they have a 0 value

I don't have a strong opinion on this. It doesn't really have any impact until tor's default values change for whatever reason.

  • DnsPort, HttpTunnelPort, TransparentProxyPort are now all Integers. I also added an associated host field for each one.
  • ProxyPort, ProxySocks5ServerPort are now an Integers rather than Strings.
  • HttpTunnelPort and TransPort values are set to null so it won't appear in torrc by default.
  • AutomapHostsOnResolve is set to false so won't appear in torrc by default

These are the more important changes.

The remaining field VirtualAddrNetwork will need to be removed in tor-android-service

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/73aeed144259a66930e605c7804946c9f6041b59

universal/src/main/java/com/msopentech/thali/toronionproxy/DefaultSettings.java

     @Override
-    public int getHttpTunnelPort() {
-        return 8118;
+    public String getHttpTunnelHost() {
+        return null;
+    }
+
+    @Override
+    public Integer getHttpTunnelPort() {
+        return null;
     }

This is changing the default port, is that intentional?

Yes, I don't believe we use this by default. it can be set in tor-android-service if needed.

     @Override
-    public String transPort() {
-        return "9040";
+    public String getTransparentProxyAddress() {
+        return null;
+    }
+
+    @Override
+    public Integer getTransparentProxyPort() {
+        return null;

This is changing the default port, is that intentional?

Yes, I don't believe we use this by default. it can be set in tor-android-service if needed.

+    TorConfigBuilder addAddress(String fieldName, String address, Integer port, String flags) {
+        if(isNullOrEmpty(address) && port == null) {
+            return this;
+        }
+        buffer.append(fieldName).append(" ");
+        if(!isNullOrEmpty(address)) {
+            buffer.append(address).append(":");
+        }
+        if (port != null) {
+            buffer.append(port <= 0 ? "auto" : port);

Please pass this directly to tor, we shouldn't change the intended behavior if the app configures a 0 or negative port number. Tor will emit a warning which the app should handle itself.

The negative int or null value is treated a "magic" number for "auto". This is really because tor config accepts strings (auto) or int types for port so we have to be able to handle this somehow.

universal/src/test/java/com/msopentech/thali/toronionproxy/TorConfigBuilderTest.java

+    /**
+     * SHould be empty
+     */

nit.

comment:14 in reply to:  12 Changed 3 months ago by sysrqb

Replying to sisbell:

Replying to sysrqb:

Something else, but not for this ticket, is that TOPL should be consistent about how it handles integer config options.

    @Override
    public String getSocksPort() {
        return "9050";
    }

I left this since we also need to handle "auto" and we have the auto detect on the port so its a bigger change for this field.

Okay. That's unfortunate, but it's fine for now.

comment:15 in reply to:  13 Changed 3 months ago by sysrqb

Replying to sisbell:

Replying to sysrqb:

universal/src/main/java/com/msopentech/thali/toronionproxy/DefaultSettings.java

     @Override
-    public int getHttpTunnelPort() {
-        return 8118;
+    public String getHttpTunnelHost() {
+        return null;
+    }
+
+    @Override
+    public Integer getHttpTunnelPort() {
+        return null;
     }

This is changing the default port, is that intentional?

Yes, I don't believe we use this by default. it can be set in tor-android-service if needed.

     @Override
-    public String transPort() {
-        return "9040";
+    public String getTransparentProxyAddress() {
+        return null;
+    }
+
+    @Override
+    public Integer getTransparentProxyPort() {
+        return null;

This is changing the default port, is that intentional?

Yes, I don't believe we use this by default. it can be set in tor-android-service if needed.

Okay, it seems a little weird to change the default value in TOPL instead of change the values we want in AndroidTorSettings (in tor-android-service), but if this will be accepted by upstream then that's fine.

+    TorConfigBuilder addAddress(String fieldName, String address, Integer port, String flags) {
+        if(isNullOrEmpty(address) && port == null) {
+            return this;
+        }
+        buffer.append(fieldName).append(" ");
+        if(!isNullOrEmpty(address)) {
+            buffer.append(address).append(":");
+        }
+        if (port != null) {
+            buffer.append(port <= 0 ? "auto" : port);

Please pass this directly to tor, we shouldn't change the intended behavior if the app configures a 0 or negative port number. Tor will emit a warning which the app should handle itself.

The negative int or null value is treated a "magic" number for "auto". This is really because tor config accepts strings (auto) or int types for port so we have to be able to handle this somehow.

The null check is fine, but 0 has a special meaning, so that should be passed unmodified to tor. Do we need both null and negative integers representing auto?

comment:16 Changed 3 months ago by pili

Keywords: TorBrowserTeam201911 added; TorBrowserTeam201910 removed

Moving tickets to November 2019

comment:17 Changed 3 months ago by sisbell

I'll do the following: throw exception if port less than 1. accept any value >=0, and treat null as auto.

According to the specs transparentProxyPort (as well all tunnel port) are 0 default, which means it is disabled. So I'm thinking TOPL should not have any default values. If Orbot needs these values set, it can set them through the prefs backing the AndroidTorSettings impl.

comment:18 Changed 3 months ago by sisbell

I'm looking into how to handle SocksPort with an Integer (if possible):

  • negative, throw exception
  • >=0 pass directly
  • null is "auto"
  • unix file path(???) not quite sure how to handle this with an Int

comment:19 Changed 3 months ago by sisbell

I made the change to accept 0 value for a port and null will assign "auto" as the value. Anything less than 0 will throw an IllegalArgumentException. This is a runtime exception so we won't start tor at all if user passes in negative value. I added unit tests to verify behavior.

https://github.com/sisbell/Tor_Onion_Proxy_Library/commit/47b53aeb243639a3ffd46916d88111d71e8ea124

comment:20 Changed 3 months ago by sisbell

Keywords: TorBrowserTeam201911R added; TorBrowserTeam201911 removed
Status: needs_revisionneeds_review

comment:21 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201912R added; TorBrowserTeam201911R removed

We are in December now.

comment:22 Changed 7 weeks ago by pili

Reviewer: sysrqb

sysrqb to review these tickets

comment:23 Changed 6 weeks ago by sisbell

Actual Points: 1

comment:24 Changed 2 weeks ago by sysrqb

Keywords: TorBrowserTeam202001R added; TorBrowserTeam201912R removed
Note: See TracTickets for help on using tickets.