[tor-bugs] #16389 [Tor]: Redesign the HS client descriptor cache

[Tor Bug Tracker & Wiki]

#16389: Redesign the HS client descriptor cache
-------------------------+--------------------------------
     Reporter:  dgoulet  |      Owner:
         Type:  defect   |     Status:  needs_revision
     Priority:  major    |  Milestone:  Tor: 0.2.7.x-final
    Component:  Tor      |    Version:
   Resolution:           |   Keywords:  SponsorR, tor-hs
Actual Points:           |  Parent ID:  #16381
       Points:           |
-------------------------+--------------------------------

Comment (by nickm):

 Replying to [comment:6 dgoulet]:
 > Replying to [comment:5 nickm]:
 > > Suggestions:
 > >
 >
 > All suggestions are great! I will work on those!
 >
 > >   * should "unsigned int failure" be an enum?
 >
 > It should! but currently the failures are `#define` hence the current
 unsigned int. I had a plan (volatile) to make a patch to change those to
 an enum eventually.

 Okay.  Let's try to get that in with this?

 > >   * Does the new "goto_err" in rend_cache_store_v2_desc_as_client()
 correctly make us close all the pending connections?
 >
 > There shouldn't be any pending connections to those intro points because
 we are parsing the fetched descriptor and making sure the IP entries in
 there are in the failure cache or not. After validation, if the intro node
 list is empty it means we can't use any thus we `goto err`. At that point,
 no new connections have been established to those intro points nor are
 already established.

 Hm. I think I meant connection_edge connections from the client?

 > > Bigger issues:
 > >   * Should "signal NEWNYM" affect this cache?  Guessing yes.
 >
 > I thought about clearing this cache on a `NEWNYM` but I'm unsure...
 Failures are quite linked to the consensus and current "state" of the
 network that is if IPs aren't usuable, it goes beyond the scope of the
 cache and circuit state. A `NEWNYM` will clean the descriptor cache but I
 think we need to keep the IP failure so if we download again that same old
 descriptor, we won't initiate anything. I can be convinced that clearing
 this cache would be desirable like for example if we decide to link the IP
 expiry time to the descriptor expiry time. (see next question)

 I think that clearing the cache is probably a good idea; otherwise there's
 a way to correlate access across a NEWNYM, right?

 > >   * Should the entries in this cache ever expire?  I think probably
 yes.
 >
 > That was also a concern of mine. So this patch has a "natural rotation"
 for the cache entries meaning that when we download a new descriptor, it
 removes every IP that are not listed in the descriptor (for a service ID).
 But if an IP is re-used we can end up not using it even if it's suppose to
 work with that new descriptor.
 >
 > So, what would be a wise expiry time? Here are some I can think of:
 >
 >  1. Remove the service ID entry (meaning all the IP associated with an
 onion address) when the descriptor expires (`rend_cache_clean`). Currently
 a valid period of 24 hours.

 I think that seems right.  When we remove the descriptor entirely, we
 shouldn't remember the IP statuses in it.

 >  2. When we get a new consensus, we could consider that it indicates a
 "new era" of connectivity :)
 >  3. Expire them at each `RendPostPeriod` default value (what most HS
 will use?). (1 hour)
 >
 > We could also expire entries at a different rate based on their failure
 type. A NACK is much more different than a UNREACHABLE for instance but
 that's maybe a future enhancement.
 >
 > In terms of simplicity for now, I'm incline to use `1.` especially since
 the branch in #4862 makes it that the HS only uploads its descriptor when
 all IPs have been established. We avoid the race of fetching the
 descriptor before an IP is ready and thus not using it for 24 hours.
 >
 > >   * Removing the timestamp check in
 rend_cache_store_v2_desc_as_client() makes me worry about replacing a
 newer descriptor with an older one.  Should I be worried about that?
 > >
 >
 > Hrm, I thought it was harmless to remove it because an HSDir doesn't
 accept an older descriptor but then that does fix the fact that an evil
 HSDir could simply serve older descriptor non stop and break clients for a
 long time making them add lots of IPs to their failure cache. Good catch,
 I'll put it back!
 >
 > New branch coming soon.

 Yaaaay.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/16389#comment:7>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online