Opened 5 years ago

Closed 5 years ago

#12688 closed enhancement (implemented)

Make NumEntryGuards configurable by the consensus

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: #11480 Points:
Reviewer: Sponsor:

Description

I plan to move clients to using 1 guard rather than 3. But I'd like to do that in a way that we can move them over as a group, once enough people have upgraded. I think that calls for a consensus param.

I shall call it NumEntryGuards.

And the controversial point is that I want to put it into 0.2.4.x as a security fix.

The timeline will be for some folks to upgrade, then we stick it in the consensus as NumEntryGuards=1, then when nothing explodes, we change the default-if-it's-not-in-the-consensus to 1, and eventually we don't need to set the consensus param anymore.

Child Tickets

Change History (13)

comment:1 Changed 5 years ago by arma

Status: newneeds_review

See my branch ticket12688.

I believe it applies cleanly to maint-0.2.4 and maint-0.2.5 and master.

And it's nice and short too. :)

comment:2 Changed 5 years ago by arma

I picked 10 as an upper bound for what the consensus param can set. I could also see picking 3.

comment:3 Changed 5 years ago by arma

Hm. I see from #12206 that we might want to leave clients with three directory guards? Shall I add a consensus param for that too?

comment:4 Changed 5 years ago by arma

Ok, my branch now has a second commit on it, that adds a NumDirectoryGuards consensus param. I chose the magic value 0 to mean "ignore this consensus param and do what you used to do", and that's the default.

comment:5 Changed 5 years ago by arma

Parent ID: #11480

comment:6 Changed 5 years ago by asn

Looks good to me.

comment:7 Changed 5 years ago by arma

I pushed a third commit which updates the man page entries.

comment:8 Changed 5 years ago by nickm_mobile

What will this do during bootstrapping? I guess we don't pick guards before we have a consensus, so we don't need to worry about clients using the no-consensus default.

Assuming I'm right there, this seems okay to me for 025. I am kinda nervous about 024, but I can defer to your judgment there.

comment:9 Changed 5 years ago by andrea

Begin code review of arma's ticket12688 branch:

1e3af73898cc51a75298de08204fccaedf97da25:

  • I think nickm is right about not picking guards before having a consensus, but I thought we were trying to get away from 3 guards? I'd feel slightly better if the default value passed into networkstatus_get_param() weren't the case we were trying to avoid, but I believe this is okay.

7ecc86463ca42ee37f0140006ef077847439e97b:

  • I think this is okay.

5c7356468bc6e08fafbc3a84b06e90b471d63c3d:

  • This is fine.

End code review.

comment:10 in reply to:  9 Changed 5 years ago by arma

Replying to andrea:

I thought we were trying to get away from 3 guards? I'd feel slightly better if the default value passed into networkstatus_get_param() weren't the case we were trying to avoid, but I believe this is okay.

My hope here is to exactly duplicate the current situation, just making it configurable via the consensus.

Then later, once enough people have upgraded that there won't be some weird partitioning thing going on for users who just upgraded, we can flip the switch in the consensus and move people to 1 guard.

(And if things go horribly in some unforseen way, we can move it back to 3 and maybe they'll go unhorribly again.)

comment:11 Changed 5 years ago by arma

(And if we flip the switch to 1 guard and it works smoothly, we can change the default from 3 to 1 in a future code update, and eventually we won't need the consensus param anymore.)

comment:12 Changed 5 years ago by andrea

Okay. Fine by me, I think.

comment:13 Changed 5 years ago by arma

Resolution: implemented
Status: needs_reviewclosed

Merged.

Note: See TracTickets for help on using tickets.