Opened 4 months ago

Closed 3 months ago

#21641 closed defect (implemented)

Prop274: Rotate onion keys less frequently

Reported by: nickm Owned by: ahf
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201703
Cc: Actual Points: 2.1
Parent ID: Points: 2
Reviewer: nickm Sponsor: Sponsor4

Description

Let's implement proposal 274: This proposal will save a fair amount of bandwidth now, and even more once we have consensus diffs.

Child Tickets

Change History (7)

comment:1 Changed 4 months ago by nickm

  • Owner set to ahf
  • Status changed from new to assigned

comment:2 Changed 4 months ago by nickm

  • Keywords TorCoreTeam201703 added

comment:3 Changed 4 months ago by ahf

  • Status changed from assigned to accepted

comment:4 Changed 4 months ago by ahf

  • Actual Points set to 2.1
  • Status changed from accepted to needs_review

I've added a patch set that is ready for review in my Gitlab branch 'bugs/21641': https://gitlab.com/ahf/tor/commits/bugs/21641

comment:5 Changed 4 months ago by nickm

  • Reviewer set to nickm
  • Status changed from needs_review to needs_revision

Looks good!

Suggestions:

  • When a time interval (like MIN/MAX/DEFAULT_ONION_KEY_LIFETIME) has a unit other than seconds, put the unit in the name. (eg, "MIN_ONION_KEY_LIFETIME_DAYS"). Otherwise people tend to assume seconds. Same for DEFAULT/MIN onion_key_grace_period.
  • Maybe we should check whether the onion key is expired more frequently than the get_onion_key_lifetime() interval? -- otherwise, if the interval changes in the consensus, we won't reach the interior of rotate_onion_key_callback(). Same for the other callback.
  • The documentation for expire_old_onion_keys should make it clear that the function doesn't perform a grace period check.
  • This branch needs a changes file.

(Also please remember when you make these changes, fixup commits are easier to review than a completely new or rebased branch.)

comment:6 Changed 3 months ago by ahf

  • Status changed from needs_revision to needs_review

I've updated my branch with a set of fixup commits and one additional "real" commit.

comment:7 Changed 3 months ago by nickm

  • Resolution set to implemented
  • Status changed from needs_review to closed

LGTM; Squashed and merged. This should begin having a modest effect on directory bandwidth immediately, which will grow as more relays upgrade. We should keep an eye on upgraded relays, of course, to make sure that they don't have any weird side effects from the increased interval.

Note: See TracTickets for help on using tickets.