Opened 7 months ago

Closed 3 months ago

#27100 closed enhancement (implemented)

report connection to PT SOCKS proxy separately from OR connection

Reported by: catalyst Owned by: catalyst
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: 035-roadmap-subtask, 035-triaged-in-20180711, socks, pt, s8-bootstrap
Cc: catalyst, mcs, brade, intrigeri, rl1987 Actual Points:
Parent ID: #28018 Points:
Reviewer: Sponsor: Sponsor8

Description

Right now when acting as a PT client, we don't distinguish between connecting to the SOCKS port of a PT proxy and connecting to the OR that's behind the proxy. This means that we lose some intermediate progress reporting that can help users understand what might be going wrong.

Child Tickets

Change History (21)

comment:1 Changed 7 months ago by intrigeri

Cc: intrigeri added

comment:2 Changed 7 months ago by catalyst

It looks like we might be able to hook in near connection_or_finished_connecting() or connection_proxy_connect(). Right now we call control_event_bootstrap(BOOTSTRAP_STATUS_HANDSHAKE, 0) near the top of connection_or_finished_connecting() before we have begun to talk to the proxy, if any.

comment:3 Changed 7 months ago by rl1987

Cc: rl1987 added

comment:4 Changed 7 months ago by rl1987

Keywords: socks pt added

comment:5 Changed 7 months ago by catalyst

Keywords: s8-bootstrap added
Sponsor: Sponsor8

comment:6 Changed 7 months ago by catalyst

Type: defectenhancement

comment:7 Changed 6 months ago by catalyst

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

Move my post-freeze 0.3.5 items to 0.3.6.

comment:8 Changed 5 months ago by dgoulet

Parent ID: #25502#28018

This doesn't seem to have anything to do with #25502 but rather a change to help with bootstrap status of *tor* which from the roadmap looks like #28018.

comment:9 Changed 5 months ago by catalyst

This depends on some work in #27167. Otherwise, we would have to add more special cases in control_event_bootstrap() to disambiguate first connection vs application circuit connection.

comment:10 in reply to:  description ; Changed 5 months ago by dcf

Replying to catalyst:

Right now when acting as a PT client, we don't distinguish between connecting to the SOCKS port of a PT proxy and connecting to the OR that's behind the proxy.

Not only PT proxies, but also plain Socks4Proxy/Socks5Proxy/HTTPProxy, if I'm not mistaken.

comment:11 in reply to:  10 Changed 5 months ago by catalyst

Replying to dcf:

Replying to catalyst:

Right now when acting as a PT client, we don't distinguish between connecting to the SOCKS port of a PT proxy and connecting to the OR that's behind the proxy.

Not only PT proxies, but also plain Socks4Proxy/Socks5Proxy/HTTPProxy, if I'm not mistaken.

Yeah, that code doesn't distinguish PT proxies from firewall bypass proxies. I think it's still an improvement to start tracking proxy connection status even if we don't disambiguate for now.

comment:12 Changed 4 months ago by ahf

Owner: changed from catalyst to ahf

This is slightly related to some of the stuff I'm already working on for S8. Taking over this one.

comment:13 Changed 4 months ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

comment:14 Changed 4 months ago by mcs

In the context of #27239, I have been reviewing tickets related to the bootstrap status reporting improvements and thinking about how Tor Launcher / Tor Browser will be affected. For this ticket, is the resulting change likely to be the addition of a new bootstrap phase? Or something else?

comment:15 Changed 4 months ago by catalyst

I'm still looking at how best to do this.

It looks like connection_or_finished_connecting() is called when the TCP connect() succeeds, whether it's directly to a relay or to the TCP listener of a proxy (which could be a PT proxy or a firewall-bypass proxy). It looks like the call control_event_bootstrap(BOOTSTRAP_STATUS_HANDSHAKE, 0) is like many other parts of the bootstrap reporting in that it reports an anticipated next step, even if there might be multiple intermediate unreported steps yet to happen.

It's probably better to report the (TLS?) handshaking when it actually starts to happen, which is probably down in connection_tls_start_handshake().

There already seems to be somewhat of a state machine in connection_or.h already, making connection_or_change_state() a potentially useful place to hook in.

comment:16 Changed 4 months ago by catalyst

Proxy connections have their own small state machine, with the state variable in the proxy_state field of a connection_t. Currently, connection.c changes that field directly. If we wrap those state changes in a function, that function could publish notifications that we could then use in bootstrap reporting. There aren't many states a proxy connection goes through during connection setup, but maybe it's still enough to improve the bootstrap UX.

In terms of being comprehensible to a non-expert user, we might want to distinguish between firewall-bypass proxies and PT proxies. (I'm still not completely sure how those compose together.)

comment:17 Changed 3 months ago by catalyst

#28884 is a related problem.

comment:18 in reply to:  17 ; Changed 3 months ago by catalyst

Replying to catalyst:

#28884 is a related problem.

Also #27167 will partially solve this, because we notice the state change to OR_CONN_STATE_PROXY_HANDSHAKING. Some intermediate proxy states might not be captured in those patches though.

comment:19 in reply to:  18 Changed 3 months ago by catalyst

Owner: changed from ahf to catalyst

Replying to catalyst:

Replying to catalyst:

#28884 is a related problem.

Also #27167 will partially solve this, because we notice the state change to OR_CONN_STATE_PROXY_HANDSHAKING. Some intermediate proxy states might not be captured in those patches though.

Also also, having looked at how I worded this ticket originally, I'm pretty sure #27167 will completely solve this, so I'm taking this ticket back. Additional work on reporting intermediate progress from within a PT proxy are ahf's work in #25502.

comment:20 Changed 3 months ago by catalyst

Status: assignedneeds_review

Patch is with #27167.

comment:21 Changed 3 months ago by catalyst

Resolution: implemented
Status: needs_reviewclosed

Fixed by #27167. (But see #28925 for follow up work.)

Note: See TracTickets for help on using tickets.