[tor-bugs] #14847 [Tor]: Controller: add a command to fetch HS descriptor from HSdir(s)

[Tor Bug Tracker & Wiki]

#14847: Controller: add a command to fetch HS descriptor from HSdir(s)
-----------------------------+----------------------------------------
     Reporter:  dgoulet      |      Owner:  dgoulet
         Type:  enhancement  |     Status:  needs_revision
     Priority:  normal       |  Milestone:  Tor: 0.2.7.x-final
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  SponsorR tor-hs controller
Actual Points:               |  Parent ID:  #3521
       Points:               |
-----------------------------+----------------------------------------
Changes (by yawning):

 * status:  needs_review => needs_revision


Comment:

 As promised, here's the review.

 Spec:
  * 552 isn't the only failure code returned.
  * `The DescId MUST have at least once Server specified else a 552 error
 is returned.` feels awkward/confusing.  `If a DescId is specified, at
 least one Server MUST also be provided, otherwise a 552 error is
 returned.`.
  * `If no Server are speficied` should be `If no DescId and Server(s) are
 specified`.
  * `The caching behavior of a fetched descriptor using this command is
 unchanged that is it's the same as the Tor client.` should be something
 like `The caching behavior when fetching a descriptor using this command
 is identical to normal Tor client behavior.`, if that's what you meant,
 otherwise clarification needed.
  * `Detail on how to` should be `Details on how to`.
  * `If any values is unrecognized` should be `If any values are
 unrecognized`.
  * Is there a good reason to specify the length of a HSAddress?  This will
 require revision once Prop. 224 comes into play.
  * `"650" SP "HS_DESC_CONTENT" SP HSAddress SP DescId SP HsDir CRLF`
 should be `"650" "+" "HS_DESC_CONTENT" SP HSAddress SP DescId SP HsDir
 CRLF` per comment 29.
  * Behavioral question.  If I issue 2 identical "HSFETCH" commands in
 rapid succession, how many events do I get?  The spec implies that tor
 will send me duplicate events but that's not the case.

 Branch:
  * New code:
    * `src/common/util.c:string_is_hex()`:
      * `unsigned int i;` -> `size_t i;`
      * I suggest adding a `tor_assert(string);`.
      * Consider `return strlen(string) == strspn(string, HEX_CHARACTERS);`
 instead of the loop.
    * `src/or/control.c:handle_control_hsfetch()`:
      * Use `rend_valid_service_id()` to check HS address validity.
      * When checking `arg1` to see if it is a v2 descriptor ID, use
 `strcmpstart()` instead of `strstr()`.
      * Instead of removing out arg1, just so you can use
 `SMARTLIST_FOREACH`, `for (int i = 1; i < smartlist_len(args); ++i)`
 followed by `smartlist_get(args, i)` will suffice (and is consistent with
 the rest of the control code).
      * The standard status code for unrecognized/unexpected arguments is
 513, not 552.
      * The standard status code for missing arguments is 512, not 552.
      * `smartlist_free()` is fine with the list being NULL, there's no
 reason to check if `hsdirs` is valid.
      * (Style) I for one welcome our new C99 overlords, moving variable
 declarations closer to the code might make sense.
      * (Style) You can just `return 0;` if `getargs_helper()` returns NULL
 since there is 0 cleanup to be done.  See above regarding moving variable
 declarations around.
      * (Style) `if (hsaddress) { ... } else { tor_assert(desc_id); }`
 instead of a branch containing an assert.
    * `src/or/control.c:control_event_hs_descriptor_content()`:
      * (Behavior bikeshedding) If I did this, I would have allowed NULL
 content, since the dot encoded form of that is ".\r\n", and is trivial to
 special case.

  * Changes to old code:
    * `src/or/rendclient.c:lookup_last_hid_serv_request()`:
      * `rend_query` should be removed from the Doxygen comment.
    * `src/or/rendclient.c:directory_get_from_hs_dir()`:
      * `pick_hsdir()` could/should take desc_id_base32 as an argument.
 The code is encoding it twice in rapid succession.
    * `src/or/rendclient.c:rend_client_refetch_v2_renddesc()`:
      * No longer need to log a warning when you fail to compute the v2
 rend descriptor ID?

 The rest of the refactor looks ok, sorry if the bulk of the things are
 nitpicky/personal taste.

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