[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:               |
-----------------------------+----------------------------------------

Comment (by yawning):

 Replying to [comment:32 dgoulet]:
 > > * 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.
 >
 > Weird, one HSFETCH should trigger two events, HS_DESC and
 HS_DESC_CONTENT. Thus in your case 4 events. I just tested multiple times
 and I actually have the right amount. Maybe that's worth investigating
 more if you can reproduce that. Maybe if no more hsdirs are usable, we
 loose events?

 Hmm, my mistake here, I misread the code.

 > > * Use rend_valid_service_id() to check HS address validity.
 >
 > Fixed! Added a patch on top.
 >
 > > * 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).
 >
 > Not sure I understand this one. I don't see this being used in
 control.c. Can you clarify the loop you have in mind for me to skip the
 first arg?

 `handle_control_closecircuit()` has an example of what I have in mind:
 {{{
     for (i=1; i < smartlist_len(args); ++i) {
       if (!strcasecmp(smartlist_get(args, i), "IfUnused"))
         safe = 1;
       else
         log_info(LD_CONTROL, "Skipping unknown option %s",
                  (char*)smartlist_get(args,i));
     }
 }}}

 Going through the trouble to remove the first arg, just to use an
 iteration macro, when a simple loop works just as well seems a bit
 excessive.

 > > * (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
 >
 > Personally, I *try* to have only one return call site with goto labels
 for cleanup. In this case, I would prefer an `exit:` label instead of
 adding a return. I'm not strongly fighting against that tbh, I prefer
 consistent and maintainable code over stylish vanity thus here I would
 probably hand it over to the maintainer for a decision? :)

 Yeah, that makes sense.

 > > * (Style) if (hsaddress) { ... } else { tor_assert(desc_id); } instead
 of a branch containing an assert.
 >
 > I strongly advocate against `if/else if` statement that DOES NOT have a
 final `else`. Over time, that's only error prone but sometimes you don't
 have a choice thus a nice comment to explain why is important.
 >
 > In this case, I went for the "else tor_assert" because `desc_id` should
 be treated as a single value and not a "fallback". Assuming future code
 that might add an other variable, we'll end up splitting the `else`
 statement and breaking the git blame :(. It's so much more easier to read
 an if/else statement when the value being tested is actually the one being
 worked on (in the case of input parsing).

 This makes sense too.

 > > 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?
 >
 > I'm not seeing any log warnings in that function about that. Maybe you
 had an other function in mind?

 {{{
 @@ -745,44 +895,12 @@ rend_client_refetch_v2_renddesc(const rend_data_t
 *rend_query)

 fe452c39248e99165b8756f38446eda8493f7300
 [stuff snipped]

 -   if (rend_compute_v2_desc_id(descriptor_id, rend_query->onion_address,
 -                               rend_query->auth_type == REND_STEALTH_AUTH
 ?
 -                               rend_query->descriptor_cookie : NULL,
 -                               time(NULL), chosen_replica) < 0) {
 -     log_warn(LD_REND, "Internal error: Computing v2 rendezvous "
 -                       "descriptor ID did not succeed.");
 -     /*
 -      * Hmm, can this write anything to descriptor_id and still fail?
 -      * Let's clear it just to be safe.
 }}}

 That `log_warn` should be in `fetch_v2_desc_by_addr()` in the new code?

 Let me know when you want me to do another pass over the new branch.

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