Opened 8 years ago

Last modified 4 weeks ago

#5915 needs_revision enhancement

Write patch to make socks handshakes succeed instantly

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: needs-proposal, tor-client, intro, performance, application, experiment, tbb-wants?, performance?, ux, 042-deferred-20190918
Cc: gk, tom, brade, mcs Actual Points:
Parent ID: Points: 3
Reviewer: nickm Sponsor:

Description

In #3875 Mike suggests we try out the usability of having the Tor in TBB just immediately succeed at all socks handshakes, and hang up if it gets an end cell rather than a connected cell.

We should whip up a Tor patch that does this behavior, so somebody can try it out.

(We may or may not want to merge it into mainline Tor branches. I guess it depends how complex the patch is and how successful the tests are.)

Child Tickets

Attachments (2)

tor-optimistic-data.patch (3.0 KB) - added by cypherpunks 8 months ago.
Optimistic Data patch for Tor.0.3.5.x
tor-optimistic-data.2.patch (3.2 KB) - added by cypherpunks 3 months ago.
Optimistic Data patch refreshed for Tor.0.4.1.6

Download all attachments as: .zip

Change History (29)

comment:1 Changed 8 years ago by nickm

This could be a socksport option, so that TBB could configure Tor with one "succeeds-immediately" socksport, and one regular socksport. That might be a good idea if (as I suspect) this breaks some but not all applications.

comment:2 Changed 7 years ago by nickm

Keywords: tor-client added

comment:3 Changed 7 years ago by nickm

Component: Tor ClientTor

comment:4 Changed 3 years ago by nickm

Keywords: intro performance application experiment added
Points: 3
Severity: Normal

comment:5 Changed 23 months ago by arma

Milestone: Tor: unspecifiedTor: 0.3.4.x-final

I independently re-came up with the idea of using a socksport option, when thinking about the issues on #19910. I think it is a good idea.

I'm moving the milestone up for this one, since it is looking like Tor Browser is going to want to remove their hack, and have Tor pick up the hack burden.

comment:6 Changed 23 months ago by gk

Cc: gk added

comment:7 Changed 22 months ago by gk

Keywords: tbb-wants added

comment:8 Changed 21 months ago by gk

FWIW: There were some reasonable arguments to not try this at the tor level for Tor Browser. Instead we think we should fix the Firefox code to do the right thing for us. I'll leave this ticket open for the original idea.

comment:9 Changed 21 months ago by nickm

Keywords: tbb-wants removed
Milestone: Tor: 0.3.4.x-finalTor: unspecified

comment:10 Changed 8 months ago by cypherpunks

this feature as a socksport option would be good choice

Changed 8 months ago by cypherpunks

Attachment: tor-optimistic-data.patch added

Optimistic Data patch for Tor.0.3.5.x

comment:11 Changed 8 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.1.x-final
Status: newneeds_review

comment:12 Changed 8 months ago by cypherpunks

The added "Optimistic Data patch for Tor.0.3.5.x" is only a quick workaround test, needs review. you may will notice more often something like in logs:

Apr 08 21:12:04.000 [info] {EDGE} connection_edge_process_inbuf(): data from edge while in 'waiting for connect response' state. Sending it anyway. package_partial=0, buflen=181
Apr 08 21:12:04.000 [info] {EDGE} connection_edge_process_inbuf(): data from edge while in 'waiting for connect response' state. Sending it anyway. package_partial=1, buflen=181
Apr 08 21:12:04.000 [info] {EDGE} connection_edge_process_inbuf(): data from edge while in 'waiting for connect response' state. Sending it anyway. package_partial=0, buflen=517
Apr 08 21:12:04.000 [info] {EDGE} connection_edge_process_inbuf(): data from edge while in 'waiting for connect response' state. Sending it anyway. package_partial=1, buflen=19

comment:14 Changed 8 months ago by teor

Keywords: needs-proposal added
Milestone: Tor: 0.4.1.x-finalTor: unspecified
Status: needs_reviewneeds_information

Thanks for this patch!

But I think we need a proposal and spec for this feature, because we need to tell applications how to use it:
https://trac.torproject.org/projects/tor/wiki/org/meetings/2018Rome/Notes/OptimisticData

Here's a bit more information about our proposals process:
https://gitweb.torproject.org/torspec.git/tree/proposals/001-process.txt

comment:15 Changed 6 months ago by tom

We talked about this at All Hands. At the browser level we are not really concerned about the error handling. It's not bad as is without modification and we think we can make it better with relatively little effort.

So the path forward we like for this is to implement it in tor with an option on SocksPort that Tor Browser can set.

comment:16 Changed 6 months ago by tom

Cc: tom added

comment:17 Changed 6 months ago by arma

In theory, as a further step, Tor Browser could watch the stream events on the controller, and learn what the socks error would actually have been, and could tailor its error page by how the stream actually failed. If we wanted to recover the ability for Tor Browser to give an accurate error.

comment:18 Changed 6 months ago by cypherpunks

the provided example patch in daily usage, gave me better latency feeling in interactive sessions, it's not bad as it is without modification of Tor Browser actually. works for me.

comment:19 Changed 6 months ago by nickm

Keywords: tbb-wants? performance? ux added
Milestone: Tor: unspecifiedTor: 0.4.2.x-final

comment:20 Changed 6 months ago by arma

Speaking of needs-proposal, Tom wrote a proposal here:
https://lists.torproject.org/pipermail/tor-dev/2019-June/013905.html

comment:21 Changed 6 months ago by mcs

Cc: brade mcs added

comment:22 Changed 3 months ago by nickm

Keywords: 042-deferred-20190918 added
Milestone: Tor: 0.4.2.x-finalTor: unspecified

Deferring various tickets from 0.4.2 to Unspecified.

Changed 3 months ago by cypherpunks

Attachment: tor-optimistic-data.2.patch added

Optimistic Data patch refreshed for Tor.0.4.1.6

comment:23 Changed 3 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.3.x-final
Status: needs_informationneeds_review

comment:24 Changed 3 months ago by tom

While I'm very appreciative of this patch; as detailed in https://lists.torproject.org/pipermail/tor-dev/2019-September/014045.html I do not think this is the best approach.

We do not need to use the display_error_message function (especially since it won't work for HTTPS) and we do need to avoid optimistic data for onion sites. Additionally, we should put this behind a SocksPort flag.

comment:25 Changed 2 months ago by asn

Reviewer: nickm

comment:26 Changed 2 months ago by nickm

Status: needs_reviewneeds_revision

The patch itself is a decent proof-of-concept, but it is not mergeable as-is: its indentation style does not match the surrounding code, and there is no option to control the behavior here.

Also, it looks like the people requesting this behavior here do not yet agree about what the best behavior is to suit their needs; I think that we should defer future implementation work on this until there is a rough consensus on what Tor should do.

comment:27 Changed 4 weeks ago by cypherpunks

the patch works since patched against tor stable for testing this poc.
only negative thing noticed so far, is the following line:

[warn] {BUG} connection_ap_handshake_socks_reply(): Bug: (Harmless.) duplicate calls to connection_ap_handshake_socks_reply. (on Tor 0.4.1.6 )

i believe its a race condition from SOCKS_COMMAND_IS_CONNECT
but without log lines wouldn't noticed this either

Note: See TracTickets for help on using tickets.