Opened 6 years ago

Last modified 5 months ago

#11211 assigned enhancement

Multiple ServerTransportListenAddr entries should be allowed per transport.

Reported by: yawning Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: tor-bridge, pt, needs-spec, tor-pt, bridgedb-parsers, ipv6, triaged-out-20170124
Cc: isis, kaie@…, phw Actual Points:
Parent ID: #10629 Points:
Reviewer: Sponsor: T/U

Description

Looking through or/config.c, it is apparent that the ServerTransportListenAddr line only allows one address/port to be specified per transport. This is problematic because there are cases where it is beneficial/required to list more than one.

A simple example of where this would be useful is:

ServerTransportListenAddr obfs3 0.0.0.0:443
ServerTransportListenAddr obfs3 [::]:443

The Pluggable Transport spec doesn't explicitly disallow having multiple bind addresses for TOR_PT_SERVER_BIND_ADDR, but I'm not sure what would happen if more than one is passed with each of the pt config protocol libraries in use.

The keys holding transport names must appear on the same order
as they appear on TOR_PT_SERVER_TRANSPORTS.

Currently the particular example I used is probably a moot point because of #7961, but in general I don't see a good reason why each transport should be limited to one bind address.

Child Tickets

Attachments (3)

tor-ticket-11211-v2.patch (16.8 KB) - added by kaie 3 years ago.
tor-ticket-11211-v3.patch (17.5 KB) - added by kaie 3 years ago.
tor-ticket-11211-v4.patch (17.5 KB) - added by kaie 3 years ago.

Download all attachments as: .zip

Change History (41)

comment:1 Changed 6 years ago by yawning

Parent ID: #10629

I'm not sure what would happen if more than one is passed.

pyptlib will trample all but the last bind address specified for a given transport.

liballium will raise a ENV-ERROR.

Another change that is probably better targeted for VERSION 2 perhaps? It's unfortunate that dual stack bridge setups appear to require kludges.

comment:2 Changed 6 years ago by asn

Good point. Probably worth doing.
I guess dual-stack PTs is something we will want to support in the long-term too, right?

BTW, we will also need to support multiple TransportProxy lines in the statefile too. Since that's where Tor keeps the IP:PORT that the PT listened last time.

comment:3 Changed 6 years ago by nickm

Keywords: needs-proposal tor-pt added
Milestone: Tor: 0.2.???

Putting in 0.2.??? until we know what the right thing to do is and have a design for it

comment:4 Changed 5 years ago by isis

Cc: isis added
Keywords: bridgedb-parsers added

comment:5 Changed 5 years ago by isis

Possible duplicate: #7884

comment:6 Changed 4 years ago by isis

Keywords: 028-triage added
Sponsor: T/U

comment:7 Changed 4 years ago by isis

Keywords: ipv6 added
Severity: Normal

comment:8 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:9 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:10 Changed 3 years ago by kaie

I'm trying to contribute a fix for this issue.

Would it be acceptable to use a different configuration syntax, that uses only a single line for each transport type, and allows multiple address:port combinations to be listed on the line, separated by space, as in the following example?

ServerTransportListenAddr obfs3 0.0.0.0:443 [::]:443

I discovered that the obfs4proxy tool already supports multiple addresses for the same transport type, as it already parses a comma separated list of type-address:port entries, which are passed to it by Tor in an environment variable.

Looking at the Tor C code, it seems that many places assume that only a single configuration line (and state line) is used for any given transport type. My impression is, implementing the syntax I'm suggesting requires a much smaller amount of code changes.

I'll attach an initial attempt to implement the above. It parses the above syntax, it passes the extended list to the external transport tool. It also saves extended TransportProxy lines into the state file, using the same approach that keeps a single line and allows multiple addresses.

So far I've only tested with a test network, and only tested that the listeners are created. I haven't done real world testing yet.

The code that loads and validates the state file only checks the first stored address:port, and will use the additional saved entries without validation. I've ran out of time today, this is a TODO.

Looking forward to your feedback.

Changed 3 years ago by kaie

Attachment: tor-ticket-11211-v2.patch added

comment:11 Changed 3 years ago by kaie

Cc: kaie@… added

Changed 3 years ago by kaie

Attachment: tor-ticket-11211-v3.patch added

comment:12 Changed 3 years ago by kaie

I've attached an updated version of the patch after a self-review, that contains code cleanup, a free fix, adds a few comments, and implements the TODO I had mentioned.

Last edited 3 years ago by kaie (previous) (diff)

Changed 3 years ago by kaie

Attachment: tor-ticket-11211-v4.patch added

comment:13 Changed 3 years ago by kaie

Patch v4 passes "make check", including the check for wide lines.

comment:14 Changed 3 years ago by kaie

Owner: set to kaie
Status: newassigned

comment:15 Changed 3 years ago by kaie

Status: assignedneeds_review

comment:16 Changed 3 years ago by kaie

Some more test results:

The patch works, when using specific IP addresses.

Localhost ipv4 + ipv6 works:

ServerTransportListenAddr obfs3 127.0.0.1:1234 [::1]:1234

Specific global ipv4 + ipv6 addresses works, too.

However, when trying to use unspecific addresses

ServerTransportListenAddr obfs3 0.0.0.0:1234 [::]:1234

I get an error message from obfs4proxy saying "port 1234 is already in use"

comment:17 Changed 3 years ago by kaie

I've filed https://trac.torproject.org/projects/tor/ticket/21136 for the separate issue I mentioned in comment 16.

comment:18 Changed 3 years ago by nickm

Milestone: Tor: unspecifiedTor: 0.3.0.x-final

comment:19 Changed 3 years ago by yawning

NACK. The spec currently explicitly forbids this behavior, and code was written as such, and will/is misbehaving under the hood in bad ways (See https://trac.torproject.org/projects/tor/ticket/21136#comment:6).

comment:20 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

comment:21 Changed 3 years ago by nickm

Type: defectenhancement

batch modify: I think these are "enhancement", though I could be wrong about some.

comment:22 in reply to:  10 ; Changed 3 years ago by dcf

Replying to kaie:

I'm trying to contribute a fix for this issue.

Would it be acceptable to use a different configuration syntax, that uses only a single line for each transport type, and allows multiple address:port combinations to be listed on the line, separated by space, as in the following example?

ServerTransportListenAddr obfs3 0.0.0.0:443 [::]:443

This is a good idea, but I think it's more complicated than just giving a list to ServerTransportListenAddr. You would need to also make ServerTransportOptions be similarly split, which would probably require new syntax in torrc. It would also require a change to pt-spec, because there would need to be a rule or something that states which options pertain to which listening address when there are multiple ones.

I've been frustrated by this in the past, too. For example, in comment:10:ticket:20348, I wanted to run three obfs4 bridges with slightly different configuration on the same IP address, and there's just no way to do it other than to run three instances of tor. It was probably a mistake for torrc to use the transport name as a key that links ServerTransportListenAddr and ServerTransportOptions, because that makes a built-in assumption that there's only one thing identified by that transport.

Incidentally, it might be the the case that using only the IPv6 syntax already does what you want. On some systems [::] will listen on both IPv6 and IPv4, so you don't need to separately list 0.0.0.0.

comment:23 in reply to:  22 Changed 3 years ago by yawning

Replying to dcf:

This is a good idea, but I think it's more complicated than just giving a list to ServerTransportListenAddr. You would need to also make ServerTransportOptions be similarly split, which would probably require new syntax in torrc. It would also require a change to pt-spec, because there would need to be a rule or something that states which options pertain to which listening address when there are multiple ones.

obfs4proxy would require fairly substantial changes to handle this correctly, as would core tor and probably the bridge distribution stuff. So while it's a good idea, getting it to bind to addresses is just the start, and even that needs to be preceded by a spec change so that code that supports this use case will work, and that code that doesn't will error out per the existing spec.

I've been frustrated by this in the past, too. For example, in comment:10:ticket:20348, I wanted to run three obfs4 bridges with slightly different configuration on the same IP address, and there's just no way to do it other than to run three instances of tor. It was probably a mistake for torrc to use the transport name as a key that links ServerTransportListenAddr and ServerTransportOptions, because that makes a built-in assumption that there's only one thing identified by that transport.

Lots of aspects of the design aren't great.

Incidentally, it might be the the case that using only the IPv6 syntax already does what you want. On some systems [::] will listen on both IPv6 and IPv4, so you don't need to separately list 0.0.0.0.

That's a general Linux-ism. Only one address will be distributed via BridgeDB though (fairly sure it's the IPv4 one).

comment:24 Changed 3 years ago by kaie

What do you think about the following alternative approach, which would make it unnecessary to improve the internals of transport plugins?

  • allow tor to spawn multiple instances per transport type
  • introduce some sort of aliasing, which allows the use separate transport configuration lines, where each use a separate alias name as the key

For example:

# alias transport-to-execute
ServerTransportAlias second-obfs4 obfs4
ServerTransportAlias third-obfs4 obfs4

ServerTransportListenAddr obfs4 1.2.3.4:443
ServerTransportListenAddr second-obfs4 2.3.4.5:443
ServerTransportListenAddr third-obfs4 [2001::2:3:4]:443

ServerTransportOptions obfs4 "abc"
ServerTransportOptions second-obfs4 "def"
ServerTransportOptions third-obfs4 "ghi"

comment:25 Changed 3 years ago by yawning

That doesn't solve the bridge distribution side of the problem.

comment:26 Changed 3 years ago by kaie

Which part of the software performs the bridge distribution?
Is it tor, or is it the transport plugin?

comment:27 Changed 3 years ago by yawning

At a minimum tor will need more changes, and probably BridgeDB as well. Solving this problem correctly requires all of:

  • Figuring out what to do with Bridge Descriptors and modifying tor to do the right thing.
  • Modifying the pt-spec such that it has backwards compatible provisions for multiple args and multiple bind addresses, and modifying tor to do the right thing.
  • Modifying obfs4proxy to implement the new pt spec changes.

Personally I'd rather see this fixed correctly than a kludgy alias thing, but since I'm not going to be the one that works on it in any major capacity, my opinion doesn't mean much.

comment:28 Changed 3 years ago by kaie

Yawning, I think your opinion matters, because you are the expert in this area and are providing review feedback.

The question is, how important is it to have this enhancement, and how could it be implemented with available resources?

If the ideal solution that you describe is the only acceptable solution, then I most likely won't be able to find enough time to help, and the feature needs to be delayed, until enough developer time resources are available.

However, if the feature is sufficiently important to not wait for the ideal implementatiom, but accept a simpler solution, then it's easier to find the time to get it done, and maybe I'll be able to help.

So, it's interesting to get more feedback, if the suggested alias solution seems like an acceptable implementation, or if there are good arguments to reject that approach, too.

Regarding BridgeDB registration: If Tor is the software that handles the registration into the BridgeDB, then the Tor software knows for which aliases a ListenAddr has been registered, and which transport type is used for each ListenAddr, and should be able to register accordingly?

comment:29 in reply to:  27 Changed 3 years ago by isis

Replying to yawning:

At a minimum tor will need more changes, and probably BridgeDB as well. Solving this problem correctly requires all of:

  • Figuring out what to do with Bridge Descriptors and modifying tor to do the right thing.
  • Modifying the pt-spec such that it has backwards compatible provisions for multiple args and multiple bind addresses, and modifying tor to do the right thing.
  • Modifying obfs4proxy to implement the new pt spec changes.

Personally I'd rather see this fixed correctly than a kludgy alias thing, but since I'm not going to be the one that works on it in any major capacity, my opinion doesn't mean much.


Yawning is correct; please don't do the alias thing. BridgeDB (because it uses Stem) can already handle multiple lines of the same transport type, without needing any changes at all.

comment:30 Changed 3 years ago by kaie

Owner: kaie deleted
Status: needs_revisionassigned

comment:31 Changed 3 years ago by nickm

Keywords: triaged-out-20170124 added
Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

comment:32 Changed 3 years ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: unspecified
Owner: set to nickm

Move "assigned" items with no owner from 031 to unspecified

comment:33 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:34 Changed 3 years ago by nickm

Keywords: 028-triage removed

comment:35 Changed 3 years ago by nickm

Keywords: needs-spec added; needs-proposal removed

comment:36 Changed 6 months ago by nickm

Owner: nickm deleted

I am not actually working on these tickets, so they shouldn't be assigned to me.

comment:37 Changed 6 months ago by phw

Cc: phw added

comment:38 Changed 5 months ago by arma

phw closed #30953 as a duplicate of this ticket.

Note: See TracTickets for help on using tickets.