Opened 6 months ago

Closed 4 months ago

Last modified 4 months ago

#25409 closed enhancement (implemented)

rip out PortForwarding options

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt, 034-triage-20180328
Cc: yawning, neel@… Actual Points:
Parent ID: Points: 0.5
Reviewer: isis Sponsor:

Description

A relay operator on tor-relays@ just got snookered into setting PortForwarding to 1, probably because he thought it would help him get port forwarding working on his relay.

I think the reality is that almost nobody uses this option, and also we don't recommend it.

Yawning rewrote the crappy dangerous C upnp apps in go:
https://gitweb.torproject.org/tor.git/tree/src/tools/tor-fw-helper/README
https://gitweb.torproject.org/tor-fw-helper.git/tree/README.md

But I don't think we've taken any steps to get that go version into any user's hands.

Also, our past use case, where Vidalia would launch Tor and want to let ordinary users turn themselves into relays or bridges, is long deprecated.

Alternatives to "rip it out" would be "get yawning's go stuff packaged properly in Debian", or "add yawning's go stuff to the tor tarball and build it and ship it too".

See also #21765 and its great phrase "I wonder how long this has been broken for".

Child Tickets

TicketTypeStatusOwnerSummary
#21765defectclosedPortForwardingHelper stdout/stderr log forwarding seems fragile

Attachments (1)

b25409-001.patch (22.3 KB) - added by neel 5 months ago.
[PATCH] Remove PortForwarding options (Revision 1)

Download all attachments as: .zip

Change History (15)

comment:1 Changed 6 months ago by nickm

IIUC, one of yawning's findings was that a depressingly large number of consumer routers can't actually be used this way, since they tend to brick themselves when upnp'd too hard.

I would be okay with removing this option only if we first deprecate it for a full release cycle.

comment:2 Changed 6 months ago by teor

Keywords: technical-debt added
Milestone: Tor: 0.3.4.x-final
Points: 0.5

Let's deprecate it in 0.3.4 then?

comment:3 Changed 6 months ago by ahf

This is related to the PT windows code I'm looking at, but we can still rip this out if nobody is using it.

Also see: https://lists.torproject.org/pipermail/tor-relays/2017-April/012198.html - only one response here so I think it's safe to remove.

comment:4 in reply to:  1 Changed 6 months ago by yawning

Replying to nickm:

IIUC, one of yawning's findings was that a depressingly large number of consumer routers can't actually be used this way, since they tend to brick themselves when upnp'd too hard.

While I still stand behind the replacement implementation I wrote, and it tries to avoid certain behavior that has historically been troublesome, my view is that for users that this sort of tool would be useful for (someone that can't configure port forwarding on their own), the support burden for "your router doesn't implement UPnP-IGD or NAT-PMP/NAT-PCP correctly" would be rather large, because of the vast number of broken/buggy implementations of said protocols.

The general consensus around the time the rewrite was completed was "apart from flashproxy, there isn't much use for this sort of thing due to consumer grade NAT being horrific", so I support the removal.

It is somewhat of a shame because the utility will work fine for a sizeable fraction of router implementations out there, but when things go wrong, they go really wrong, which is a poor fit for a non-technical end user oriented piece of software.

Changed 5 months ago by neel

Attachment: b25409-001.patch added

[PATCH] Remove PortForwarding options (Revision 1)

comment:5 Changed 5 months ago by neel

I have a patch which removes PortForwarding options under the file b25409-001.patch.

comment:6 Changed 5 months ago by nickm

Status: newneeds_review

comment:7 Changed 5 months ago by neel

Cc: neel@… added

comment:8 Changed 5 months ago by nickm

Keywords: 034-triage-20180328 added

comment:9 Changed 5 months ago by asn

Reviewer: isis

comment:10 Changed 4 months ago by isis

Status: needs_reviewmerge_ready

Hi Neel! Thanks for the patch! This looks good to me (and Travis passes). I've applied your patch in my bug25409 branch.

comment:11 Changed 4 months ago by nickm

Status: merge_readyneeds_revision

Small changes needed: Instead of removing these lines in config.c, replace them to use OBSOLETE instead:

-  V(PortForwarding,              BOOL,     "0"),
-  V(PortForwardingHelper,        FILENAME, "tor-fw-helper"),

(That's how we remove options.)

comment:12 in reply to:  11 Changed 4 months ago by isis

Status: needs_revisionmerge_ready

Replying to nickm:

Small changes needed: Instead of removing these lines in config.c, replace them to use OBSOLETE instead:

-  V(PortForwarding,              BOOL,     "0"),
-  V(PortForwardingHelper,        FILENAME, "tor-fw-helper"),

(That's how we remove options.)


Okay, fixed! I've also added a changes file describing why the feature was removed.

comment:13 Changed 4 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Rockin'! Merged!

comment:14 Changed 4 months ago by nickm

There was a dangling log_from_handle() causing warnings on Windows. I removed it in d3b9b5a3dd6a6068557fc53176c41bc917100be2

Note: See TracTickets for help on using tickets.