[tor-bugs] #18620 [Core Tor/Tor]: HSFORGET command to clear cached client state for a HS

[Tor Bug Tracker & Wiki]

#18620: HSFORGET command to clear cached client state for a HS
-------------------------------------------------+-------------------------
 Reporter:  str4d                                |          Owner:  str4d
     Type:  enhancement                          |         Status:
 Priority:  Medium                               |  needs_revision
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  tor-hs, 029-accepted, review-        |        Version:  Tor:
  group-1                                        |  0.2.7.6
Parent ID:                                       |     Resolution:
 Reviewer:  asn                                  |  Actual Points:
                                                 |         Points:  small
                                                 |        Sponsor:
                                                 |  SponsorR-can
-------------------------------------------------+-------------------------
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Hello and sorry for the slow review! This seems like a great feature for
 mobile devices, and the patch seems to be almost there. The main issue is
 the unittest not passing.

 Here are some comments:

 - Your tests don't work because your descriptor string was prepared for
 the control port tests. If you remove the carriage returns "\r" characters
 from the end of each line then the parsing should proceed.

  However, that's not enough for it to go through
 `rend_cache_store_v2_desc_as_client()`. You also need to fix up the desc
 id which currently is not correct, and you also need a dynamic descriptor
 timestamp to bypass the age check. I had to uncomment the following checks
 to complete the test (which passes fine btw):
 {{{
   /* if (parsed->timestamp < now - REND_CACHE_MAX_AGE-REND_CACHE_MAX_SKEW)
 { */
   /*   printf( "Service descriptor with service ID %s is too old.", */
   /*            safe_str_client(service_id)); */
   /*   goto err; */
   /* } */
 }}}
 {{{
 +  /* if (tor_memneq(desc_id, want_desc_id, DIGEST_LEN)) { */
 +  /*   printf( "Received service descriptor for %s with incorrect " */
 +  /*           "descriptor ID (%s).", service_id, want_desc_id); */
 +  /*   goto err; */
 +  /* } */
 }}}

  To debug this, I turned all the `log_warn()`s in the parsing functions to
 `printf`s, and then I could see the offending error messages when I ran
 the tests.

 - I think there is no need to add the base64 padding yourself in
 `descriptor_cookie_base64ext()`. Instead you can use
 `base64_decode_nopad()`. I think this should work, but there were no tests
 about descriptor cookies so I'm not 100% sure.

 - Also, in `rend_cache_remove_entry()` there is no need to handle v0
 descriptors. These have been deprecated, feel free to kill that code.

 - The patch does not compile with `./configure --enable-gcc-warnings`
 because you have a few unused function arguments in the tests. You can
 solve this by doing `(void) arg;`.

 - Also, it would be great if you could provide a git branch based on the
 current master, instead of a standalone patch because I had trouble
 merging it. Feel free to upload it ot any repo (github, gitlab) or even
 just post a detached git-bundle. If that's too hard, no worries.

 - Finally, I noticed that in `rend_client_note_connection_attempt_ended()`
 we do:
 {{{
   if (cache_entry != NULL) {
     SMARTLIST_FOREACH(cache_entry->parsed->intro_nodes,
                       rend_intro_point_t *, ip,
                       ip->timed_out = 0; );
   }
 }}}
  Do you think that would be useful for this patch?

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