Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#4567 closed project (implemented)

Extend Tor's UPnP support for pluggable transport ports

Reported by: karsten Owned by: asn
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: SponsorG20120930 tor-bridge
Cc: nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We need to teach Tor to set up port forwarding for a bridge's pluggable transports. Tor knows these ports, because Tor also tells obfsproxy where it should listen for incoming connections.

This ticket is part of the June 30, 2012 milestone for sponsor G.

Child Tickets

TicketStatusOwnerSummaryComponent
#6059closedExtend tor-fw-helper spec to support arbitrary ports.Core Tor/Tor

Change History (15)

comment:1 Changed 7 years ago by asn

I haven't started coding on this deliverable yet, but I've thought of a rough implementation plan:

  • I will need to refactor tor_check_port_forwarding() to accept a smartlist of ports, instead of just two.
  • I will need to refactor tor-fw-helper.c to accept multiple ports. Currently, it only accepts DirPort and ORPort, and it uses different functions for each port type (tor_fw_add_dir_port() and tor_fw_add_or_port()), but I'm not sure why. It might make sense to generalize those functions so that tor-fw-helper doesn't care what kind of port it forwards (except if there is a reason to care).
  • I should then find how to call tor-fw-helper from run_scheduled_events() correctly. That is, should I wait till all the transport proxies are configured and then call it? Or should I call tor-fw-helper just for the ORPort and the DirPort and then call it again when the transport proxies are configured?

I think that tor-fw-helper is supposed to be called repeatedly, so I think the second way is better, but I haven't looked into this too deeply to be sure.

comment:2 in reply to:  1 Changed 7 years ago by ioerror

Replying to asn:

I haven't started coding on this deliverable yet, but I've thought of a rough implementation plan:

  • I will need to refactor tor_check_port_forwarding() to accept a smartlist of ports, instead of just two.

Sounds fine. Please ensure that an ORPort is always present - that is - we need to know when the core port doesn't work - having only a DirPort isn't important, right?

  • I will need to refactor tor-fw-helper.c to accept multiple ports. Currently, it only accepts DirPort and ORPort, and it uses different functions for each port type (tor_fw_add_dir_port() and tor_fw_add_or_port()), but I'm not sure why. It might make sense to generalize those functions so that tor-fw-helper doesn't care what kind of port it forwards (except if there is a reason to care).

An ORPort is not optional - every Tor must have at least a single ORPort. It's a fatal error otherwise.

  • I should then find how to call tor-fw-helper from run_scheduled_events() correctly. That is, should I wait till all the transport proxies are configured and then call it? Or should I call tor-fw-helper just for the ORPort and the DirPort and then call it again when the transport proxies are configured?

Tor should do this, I think.

I think that tor-fw-helper is supposed to be called repeatedly, so I think the second way is better, but I haven't looked into this too deeply to be sure.

Yes, that's correct. The idea was to make tor-fw-helper a thing that exits after the job is finished - Tor needs to keep track of when it is called, etc.

comment:3 Changed 7 years ago by sjmurdoch

Maybe the responsibility for sanity checking should be with Tor, rather than tor-fw-helper, as Tor has a better idea of what is going on. tor-fw-helper could be given a list of ports which it then does its best to open them, before returning a status for each port which was opened. Tor could then decide whether that was good enough (e.g. did the ORPort forward succeed), and report back to the user.

comment:4 in reply to:  3 Changed 7 years ago by asn

Replying to sjmurdoch:

Maybe the responsibility for sanity checking should be with Tor, rather than tor-fw-helper, as Tor has a better idea of what is going on. tor-fw-helper could be given a list of ports which it then does its best to open them, before returning a status for each port which was opened. Tor could then decide whether that was good enough (e.g. did the ORPort forward succeed), and report back to the user.

Yes, that's what I think too.

Unfortunately, this way, the tor-fw-helper-spec has to change too, so that it reports back which ports were forwarded and which failed to be forwarded.
IIUC, tor is not using the SUCCESS/FAILED stdout strings of tor-fw-helper at the moment, so changing the spec. is not such a big deal.

If for some reason changing the spec _is_ a huge deal, we can keep the SUCCESS/FAILED signals of tor-fw-helper, by passing it two lists of addrports. One list which MUST be forwarded, and another list which SHOULD be forwarded. If any addrports in the first list failed to get forwarded, tor-fw-helper returns FAILED, otherwise it returns SUCCESS.

comment:5 Changed 7 years ago by asn

For the record, I've refactored tor_check_port_forwarding() and tor-fw-helper as described in the first comment of this ticket. I'm currently doing some tests on Windows to make sure it works.

Next step is to start giving transport ports to tor-fw-helper along with ORPort and DirPort.

I'll post a branch soon for review.

comment:6 Changed 7 years ago by asn

Status: newneeds_review

Pushed my branch! See branch bug4567 in https://git.gitorious.org/mytor/mytor.git.

I took care while squashing my 45-commits local branch, and I hope it makes sense when you review it. If not, tell me how it doesn't make sense and I'll try to improve it.

I hope to add some unit tests and stuff before the deadline too.

It also adds two XXXs:
a) We assume that external proxy ports are forwarded manually by the user.

b) We know that tor-fw-helper won't work when ports are bound in a different IP address (like, with ORPort 192.168.2.1234:5555) and we don't do anything about it. The old tor-fw-helper subsystem didn't do anything about it either, and I'm not sure if it should.

What I hope to do before the deadline is to issue a warning when a user has set PortForwarding while binding to a different IP address, and also stop the TCP forwarding request from actually happening (the current code actually issues the useless request).

comment:7 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

okay, so fwict all the commits before eff60fc5fc are refactoring, code moving, etc that don't actually have much to do with this ticket. Worth merging anyway, though, of course : I'll be especially glad to say goodbye to the old behavior of mp->transports (where sometimes it contains strings and sometimes it contains transports).

Making ports generic in eff60fc5fc is also a win IMO: "There are only three numbers in computer science: zero, one, and infinity." I think the tor_parse_long calls in that commit are wrong, though: they have a maximum of 6556, when the maximum port number is 65535.

d180c576da516f is the big Tor change. BTW, there is no such thing as "Refactor Tor to support the new tor-fw-helper protocol." Refactoring means to improve the code without changing its functionality or behavior. I try not to get pedantic about stuff like that, but when I see a commit which claims to be refactoring but isn't, I get all twitchy.

The right way to stick things together for a log message is esc_for_log and its kin.

The documentation for any function taking a smartlist_t really needs to explain what the smartlist contains.

In the loop that builds argv, I'd like more assurance that argv_index can never overflow.

The manpage doc/tor-fw-helper.1.txt needs to change.

When I tried to build this, I got an error because get_list_of_ports_to_forward doesn't have a prototype in config.h. Have you been building this with --enable-gcc-warnings? Please do, if not.

Needs a changes file.

Breaks make check-spaces.

How much have you been able to test this?

comment:8 Changed 7 years ago by karsten

What's the status here? asn, is the plan still to get this merged or into mergeable state by June 30?

comment:9 in reply to:  7 Changed 7 years ago by asn

Replying to nickm:

okay, so fwict all the commits before eff60fc5fc are refactoring, code moving, etc that don't actually have much to do with this ticket. Worth merging anyway, though, of course : I'll be especially glad to say goodbye to the old behavior of mp->transports (where sometimes it contains strings and sometimes it contains transports).

Correct. Commits before eff60fc5fc are cherry-picked from #3589. They were needed for the same reason they were needed in #3589, so that server managed proxies keep their transport_ts around even after registering them. This is necessary because we need to know the address:port of server transports to port forward them (in the past, when server managed proxies registered their transports, they replaced them with a string of their name ;)).

This means that we will need to fix the issues you found during your review of #3589, before we merge this feature.

Making ports generic in eff60fc5fc is also a win IMO: "There are only three numbers in computer science: zero, one, and infinity." I think the tor_parse_long calls in that commit are wrong, though: they have a maximum of 6556, when the maximum port number is 65535.

True. Fixed. (Also, #6218...)

d180c576da516f is the big Tor change. BTW, there is no such thing as "Refactor Tor to support the new tor-fw-helper protocol." Refactoring means to improve the code without changing its functionality or behavior. I try not to get pedantic about stuff like that, but when I see a commit which claims to be refactoring but isn't, I get all twitchy.

Gotcha.

The right way to stick things together for a log message is esc_for_log and its kin.

Hm, where is the actual place you want me to use this function in this case?

The documentation for any function taking a smartlist_t really needs to explain what the smartlist contains.

Updated the documentation of tor_check_port_forwarding().

In the loop that builds argv, I'd like more assurance that argv_index can never overflow.

Hm, I initially had some checks, but I removed them because I considered them outside of our threat model (and they made the code look ugly). To overflow argv_index or the allocation of argv, a HUGE ports_to_forward must be given; which means that the config file is humongous, or the transport proxy is trying to screw us.

In any case, I added the checks back.

The manpage doc/tor-fw-helper.1.txt needs to change.

Done.

When I tried to build this, I got an error because get_list_of_ports_to_forward doesn't have a prototype in config.h. Have you been building this with --enable-gcc-warnings? Please do, if not.

Done.

Needs a changes file.

Done.

Breaks make check-spaces.

Done.

How much have you been able to test this?

I've been testing this with both NAT-PMP and UPnP. I would surely like someone else to test this too.

BTW, another thing I don't like about this code is that get_list_of_ports_to_forward() will be called every PORT_FORWARDING_CHECK_INTERVAL, even if it's not needed inside tor_check_port_forwarding(). I didn't find a nice way to fix this without radically redesigning the tor_check_port_forwarding() approach

Finally, there is no nice way to say to tor_check_port_forwarding(): "OK, I understood that you failed to forward port X, stop sending log lines". That would also need refactoring of the tor_check_port_forwarding() logic. This issue also existed before my changes.

comment:10 Changed 7 years ago by karsten

Milestone: Sponsor G: June 30, 2012Sponsor G: September 30, 2012

Moving to the September milestone, because did not finish everything by end of June.

comment:11 Changed 7 years ago by asn

Status: needs_revisionneeds_review

Forgot to put this in needs_review.

comment:12 Changed 7 years ago by karsten

Keywords: SponsorG20120930 added
Milestone: Sponsor G: September 30, 2012

Switching from using milestones to keywords for sponsor deliverables. See #6365 for details.

comment:13 Changed 7 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Merged to master. Should appear in 0.2.4.2-alpha.

comment:14 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:15 Changed 7 years ago by nickm

Component: Tor BridgeTor
Note: See TracTickets for help on using tickets.