Opened 3 months ago

Last modified 34 hours ago

#28925 merge_ready defect

distinguish PT vs proxy for real in bootstrap tracker

Reported by: catalyst Owned by: catalyst
Priority: High Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: s8-bootstrap, usability, ux, tor-pt, tbb-needs, 040-must, network-team-roadmap-2019-Q1Q2
Cc: brade, mcs, gk Actual Points: 2
Parent ID: #28018 Points:
Reviewer: nickm Sponsor: Sponsor19

Description

The bootstrap tracker work in #27167 adds distinctions (in the form of additional bootstrap phases) between connecting directly to a relay vs connecting through a proxy. It also tries to distinguish proxies from PTs.

The changes to do the distinguishing between PT vs proxy don't work, because conn->proxy_type gets set to the protocol type of the underlying protocol that tor uses to talk to the PT locally.

Child Tickets

Change History (17)

comment:1 Changed 3 months ago by gaba

Keywords: tor-pt added

comment:2 Changed 3 months ago by catalyst

Parent ID: #28018

comment:3 Changed 3 months ago by mcs

Cc: brade mcs added

comment:4 Changed 2 months ago by catalyst

Sponsor: Sponsor8Sponsor19

comment:5 Changed 6 weeks ago by catalyst

#29341 is a duplicate.

comment:6 Changed 4 weeks ago by dgoulet

Keywords: 040-can added
Priority: MediumLow

Bug triage of 0.4.0 tickets. These are now in the "CAN" section. Lower priority than "040-must".

comment:7 Changed 4 weeks ago by mcs

Keywords: tbb-needs added

I added tbb-needs to this ticket (which I included on #29341 when I filed it).

comment:8 Changed 4 weeks ago by nickm

Keywords: 040-must added; 040-can removed
Priority: LowHigh

comment:9 Changed 2 weeks ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:10 in reply to:  7 ; Changed 8 days ago by catalyst

Replying to mcs:

I added tbb-needs to this ticket (which I included on #29341 when I filed it).

Sorry for dropping the tag. Do you need this to be in 0.4.0? It might be easier to put in 0.4.1 because we can use the pubsub framework. It might be possible to put it in 0.4.0 but it might take longer, and might be too big a change at this stage of the release.

comment:11 in reply to:  10 ; Changed 5 days ago by mcs

Cc: gk added

Replying to catalyst:

Replying to mcs:

I added tbb-needs to this ticket (which I included on #29341 when I filed it).

Sorry for dropping the tag. Do you need this to be in 0.4.0? It might be easier to put in 0.4.1 because we can use the pubsub framework. It might be possible to put it in 0.4.0 but it might take longer, and might be too big a change at this stage of the release.

I added Georg to the Cc list on this ticket.

I looked at the Core Tor release dates on the following page:
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/CoreTorReleases

The misleading bootstrap messages are confusing, but I think that is acceptable in a Tor Browser alpha. However, this seems like a bug we would not want to have in a stable release (at least not for a long period of time).

Georg can comment more accurately than I can, but we might ship tor 0.4.0 with Tor Browser 8.5 since the browser release date will be very close to the 0.4.0 stable date. That means that our users will have to live with this bug for 4 months or longer. Maybe that is okay because experts who are called upon to help troubleshoot bootstrap problems can be told about this bug, and most users probably do not use a local proxy (in which case one can assume that "proxy" must mean "pluggable transport"). As far as I know, we don't have any hard data about local proxy use though.

comment:12 in reply to:  11 Changed 5 days ago by gk

Replying to mcs:

Replying to catalyst:

Replying to mcs:

I added tbb-needs to this ticket (which I included on #29341 when I filed it).

Sorry for dropping the tag. Do you need this to be in 0.4.0? It might be easier to put in 0.4.1 because we can use the pubsub framework. It might be possible to put it in 0.4.0 but it might take longer, and might be too big a change at this stage of the release.

I added Georg to the Cc list on this ticket.

I looked at the Core Tor release dates on the following page:
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/CoreTorReleases

The misleading bootstrap messages are confusing, but I think that is acceptable in a Tor Browser alpha. However, this seems like a bug we would not want to have in a stable release (at least not for a long period of time).

I agree.

Georg can comment more accurately than I can, but we might ship tor 0.4.0 with Tor Browser 8.5 since the browser release date will be very close to the 0.4.0 stable date. That means that our users will have to live with this bug for 4 months or longer. Maybe that is okay because experts who are called upon to help troubleshoot bootstrap problems can be told about this bug, and most users probably do not use a local proxy (in which case one can assume that "proxy" must mean "pluggable transport"). As far as I know, we don't have any hard data about local proxy use though.

While 0.4.x probably won't make it into Tor Browser 8.5 it's very likely that one of the point releases could pick up 0.4.x. So, yes, this is unfortunate. I am inclined to agree that this bug is not a hard blocker for you for 0.4.0, though. If we feel strongly at some point we can just stay on 0.3.5.x at least until the bug is fixed in a later stable release.

comment:13 Changed 3 days ago by catalyst

WIP branch at https://github.com/tlyu/tor/tree/bug28925 , needs a bit of cleanup and some documentation.

Not the greatest strategy (adds more state into or_connection_t), but not too horrible either.

Manually tested with built-in PT bridges on TB 8.5a8; haven't yet tested non-PT proxies.

comment:14 Changed 2 days ago by catalyst

Status: assignedneeds_review

Pull request at https://github.com/torproject/tor/pull/827

I believe this merges cleanly to master, but it pushes a couple of files over their line count exceptions in practracker. I'm not sure how we want to handle that.

comment:15 Changed 2 days ago by catalyst

Actual Points: 2

comment:16 Changed 34 hours ago by nickm

Reviewer: nickm
Status: needs_reviewneeds_revision

posted a quick review.

comment:17 Changed 34 hours ago by nickm

Status: needs_revisionmerge_ready

changes look good; calling this merge_ready

Note: See TracTickets for help on using tickets.