Opened 5 months ago

Closed 8 weeks ago

#28925 closed defect (fixed)

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: Tor: 0.4.0.1-alpha
Severity: Normal Keywords: asn-merge, teor-merge, 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: 2
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 (23)

comment:1 Changed 5 months ago by gaba

Keywords: tor-pt added

comment:2 Changed 5 months ago by catalyst

Parent ID: #28018

comment:3 Changed 5 months ago by mcs

Cc: brade mcs added

comment:4 Changed 4 months ago by catalyst

Sponsor: Sponsor8Sponsor19

comment:5 Changed 3 months ago by catalyst

#29341 is a duplicate.

comment:6 Changed 3 months 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 3 months 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 3 months ago by nickm

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

comment:9 Changed 2 months ago by gaba

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

comment:10 in reply to:  7 ; Changed 2 months 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 2 months 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 2 months 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 2 months 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 months 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 months ago by catalyst

Actual Points: 2

comment:16 Changed 2 months ago by nickm

Reviewer: nickm
Status: needs_reviewneeds_revision

posted a quick review.

comment:17 Changed 2 months ago by nickm

Status: needs_revisionmerge_ready

changes look good; calling this merge_ready

comment:18 Changed 8 weeks ago by teor

Keywords: asn-merge teor-merge added

comment:19 Changed 8 weeks ago by catalyst

Opened #29876 for a possible related issue that I discovered while working on this ticket.

comment:20 Changed 8 weeks ago by teor

Squashed as bug28925_040_squashed at https://github.com/torproject/tor/pull/836
Master merge is bug28925_master_squashed at https://github.com/torproject/tor/pull/837

I've read the code comments and warnings from practracker. I think I need to update the practracker exceptions file in a separate commit.

I've updated the merge policy to include a "wait for master and practracker" step:
https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/MergePolicy?action=diff&version=9

Longer-term, we should think about how to improve the ergonomics of practracker:

  1. If we push and build master first, then we'll catch practracker errors early, see #29879.
  2. If we write a script to update exceptions and merge forward, then we'll be able to handle exceptions in backport branches faster and more reliably, see #29880. We don't need to make this change until we split off an 0.4.2 branch.

But I'm not sure how to discover practracker errors in backport branches. We could require master merges for every backport branch. I wonder if there is a way to automate that? I opened #29881 to see if we could make a bot.

comment:21 Changed 8 weeks ago by teor

I tried rebuilding the file, but the diff was a mess:
https://github.com/torproject/tor/pull/837/commits/973451edd1a8f0fe7c07ae5ba7fe8256ef279f95

I think practracker is using OS file order (or maybe an order based on python minor versions?), but we really want a stable file and function order. See #29882.

I also wonder if we should replace the practracker list with every new release, so that we don't keep old exceptions. See #29883.

So I guess that I'm meant to manually edit the file by finding any modified exceptions and updating them? There has to be an easier way. (Maybe #29880?) And if there isn't, it should be documented in the warning message, and at the start of the script. See #29884.

comment:22 Changed 8 weeks ago by teor

Status: merge_readyneeds_revision

I need a squashed master branch for this issue, with the practracker fixes done the right way.

comment:23 Changed 8 weeks ago by teor

Points: 2
Resolution: fixed
Status: needs_revisionclosed
Version: Tor: 0.4.0.1-alpha

Merged to 0.4.0 and later, practracker updated in master for 2 large files which now have 16 extra lines.

Note: See TracTickets for help on using tickets.