Opened 5 years ago

Closed 4 years ago

#17688 closed defect (implemented)

Our default Guard value is still 3 if no latest consensus or no params

Reported by: dgoulet Owned by:
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Major Keywords: guard, tor-guards-revamp
Cc: Actual Points: 0.1
Parent ID: Points: 1
Reviewer: Sponsor: SponsorU-can


NumEntryGuards is a consensus params currently set to 1 which means that we rely on the consensus for the number of entry guards we want. However if tor can't get the "latest consensus" or if that params is not found, the default value is currently 3 (in decide_num_guards()):

  return networkstatus_get_param(NULL, "NumEntryGuards", 3, 1, 10);

I wonder why we keep 3 if we decided that 1 was actually more secure?

Important things here I would like to note. We should _NOT_ hardcode default values like this directly in a function call (especially important ones), they should be global defines with a _clear-non-misleading_ name. All of those, the default value, minimum value and maximum value should all be defined in one single location else this is way too error prone in the long run.

If it's indeed a mistake, we should backport this up to 026 I think that is when we set it to one guard.

Child Tickets

Change History (9)

comment:1 Changed 5 years ago by asn

I think it was done like this because we assumed that NumEntryGuards will always have the correct value in recent consensuses.

I can see how it would be nice to switch the hardcoded default to 1 in the code as well, to make our point more clearly (and to avoid any edge cases which I might be missing).

comment:2 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.9.x-final

Throw most 0.2.8 "NEW" tickets into 0.2.9. I expect that many of them will subsequently get triaged out.

comment:3 Changed 4 years ago by isabela

Sponsor: SponsorU-can

comment:4 Changed 4 years ago by nickm

Points: small

comment:5 Changed 4 years ago by nickm

Keywords: tor-guards-revamp added

comment:6 Changed 4 years ago by isabela

Points: small1

comment:7 Changed 4 years ago by asn

Actual Points: 0.1
Status: newneeds_review


please see my branch bug17688 for a fix here.

comment:8 Changed 4 years ago by andrea

Status: needs_reviewmerge_ready

This is a straightforward change to the default to use when the relevant consensus parameter is missing. Recommend merge.

comment:9 Changed 4 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

ok; merged!

Note: See TracTickets for help on using tickets.