Stop serving version 2 directory information by default

changed milestone to %Tor: 0.2.5.x-final

added component::core tor/tor milestone::Tor: 0.2.5.x-final priority::medium resolution::implemented status::closed type::enhancement labels

Please find my branch bug10758 with the trivial fix.

Trac:
Milestone: N/A to Tor: 0.2.5.x-final
Status: new to needs_review

That patch will trigger this code for all non-authorities:

  if (options->DisableV2DirectoryInfo_ && ! authdir_mode(options)) {
    REJECT("DisableV2DirectoryInfo_ set, but we aren't an authority.");
  }

so, please do not merge :)

Nick, what would you think about a more comprehensive patch that rips out the v2 directory stuff? Is that an 0.2.5 thing or an 0.2.6 thing?

(Since there aren't that many authorities, I don't think it's so critical to have an easily configurable "oops we'd better turn that on" option. If for some reason we end up changing our mind down the road, we can get patches to the directory authorities operators quickly enough.)

Trac:
Status: needs_review to needs_revision

Replying to arma:

That patch will trigger this code for all non-authorities: {{{ if (options->DisableV2DirectoryInfo_ && ! authdir_mode(options)) { REJECT("DisableV2DirectoryInfo_ set, but we aren't an authority."); } }}} so, please do not merge :)

Argh! Sorry, I totally missed that.

Nick, what would you think about a more comprehensive patch that rips out the v2 directory stuff? Is that an 0.2.5 thing or an 0.2.6 thing?

I thought about that, too. Want me to write such a patch, or fix the patch above? (I even promise to test either patch, which I thought wasn't necessary for the trivial patch I wrote earlier today.)

Ripping out more v2 code is fine by me, on a merge-whenever-it's-done basis. That's assuming that the patch in question is pretty clean wrt the parts of the code that it isn't just ripping out.

I started poking at this, and it's a nontrivial job. I'll have a patch soon

See branch "bug10758" in my public repo. Please review soon; this code will rot.

In particular, I want to know whether the removed code in nodelist.c and in routerlist.c describe any functionality that we should really be reimplementing with consensus directories.

Trac:
Status: needs_revision to needs_review

Ha -- put V1_DIRINFO and related on the chopping block after this one is resolved, yes?

Yeah, assuming we can do so safely.

Your branch bug10758 looks good. I tweaked it a bit in my force-updated bug10758 branch.

I also ran my bug10758 branch in a Shadow network with 20 relays which worked without obvious issues.

With respect to nodelist.c and routerlist.c, I'm not really sure which functionality you'd want to keep. Can you be more specific, e.g., which functions would you want to reimplement for v3?

Replying to karsten:

Your branch bug10758 looks good. I tweaked it a bit in my force-updated bug10758 branch.

Looks okay to me.

I also ran my bug10758 branch in a Shadow network with 20 relays which worked without obvious issues.

Okay. That's not likely to turn up all possible errors, since any weird stuff that breaks is going to break because of:

Older versions of Tor
Nodes what not all authorities find out about
Directories going up and down
Other stuff

But it's better than no testing at all.

With respect to nodelist.c and routerlist.c, I'm not really sure which functionality you'd want to keep. Can you be more specific, e.g., which functions would you want to reimplement for v3?

Well, I'm mostly hoping for a review of all the complex pieces and subtle little lines there to make sure they didn't do something crucial that wasn't related to the v2 networkstatus system.

Here are some examples:

v2_ns_dl_status didn't have some extra unexpected meaning, did it?
Did anything bad just happen to routerlist_remove_old_routers?

I think the answers are "no", but that was some complex logic I just chopped out.

Replying to nickm:

Well, I'm mostly hoping for a review of all the complex pieces and subtle little lines there to make sure they didn't do something crucial that wasn't related to the v2 networkstatus system.

Here are some examples:

v2_ns_dl_status didn't have some extra unexpected meaning, did it?

Did anything bad just happen to routerlist_remove_old_routers?

I think the answers are "no", but that was some complex logic I just chopped out.

I just took another look at the two examples and once more at the entire patch. I didn't spot anything in the removed v2 code or changed functions that would break v3. But obviously, that doesn't necessarily mean there is no such code. A fresh pair of eyes would probably be much more reliable than mine.

I've looked through again and can't find anything either. Merging this to master.

Trac:
Status: needs_review to closed
Resolution: N/A to implemented

closed

mentioned in issue #10881 (moved)

mentioned in issue #11070 (moved)

moved to tpo/core/tor#10758 (closed)

Stop serving version 2 directory information by default

Child items 0

Activity