[tor-bugs] #14846 [Tor]: Controller: retrieve an HS descriptor of a service run by a user

[Tor Bug Tracker & Wiki]

#14846: Controller: retrieve an HS descriptor of a service run by a user
-------------------------+-------------------------------------------------
     Reporter:  dgoulet  |      Owner:  donncha
         Type:           |     Status:  needs_revision
  enhancement            |  Milestone:  Tor: 0.2.8.x-final
     Priority:  trivial  |    Version:  Tor: 0.2.7
    Component:  Tor      |   Keywords:  SponsorR, tor-hs, controller,
   Resolution:           |  027-triaged-1-in, SponsorS, TorCoreTeam201509
Actual Points:           |  Parent ID:  #3521
       Points:  small    |
-------------------------+-------------------------------------------------

Comment (by donncha):

 Replying to [comment:27 dgoulet]:
 > Replying to [comment:26 special]:
 > > I think the GETINFO should be consistent with the existing
 `hs/client/desc/id/<ADDR>` option. How about `hs/service/desc/id/<ADDR>`?
 >
 > Agree with special here.

 Changed.

 >
 > >
 > > Trivial typo in the changelog (for -> from).
 > >

 Fixed.

 > > Which replica of the descriptor you get is an implementation detail of
 the code, and the controller can't get both. Does that matter? I'm
 guessing no.
 >
 > Code suggest that you'll get both actually. See
 `control_event_hs_descriptor_created()`, the replica is outputed in the
 control message.

 Your correct special. Tor will attempt to generate and store the two
 replica descriptors. The local service descriptor cache is indexed by
 service ID so the second replica descriptor will replace the first
 descriptor. The controller will only be able to retrieve the second
 replica which was added to the cache last.

 The `HS_DESC CREATED` event alerts the controller that each descriptor
 (for each replica) has been created, the code doesn't currently allow both
 replicas to be retrieved.

 >
 > >
 > > From a read through of the code, this looks good to me.
 >
 > Any chance we can modify control-spec.txt also as we merge this feature?
 I see a branch for this ticket in your github but pointing it here
 explicitely with the latest branch ready for merge would be good once we
 make the change above.
 >
 > In `rend_cache_clean()`, I see this being added with static for which I
 don't think it's desired here, you just want a temporary "cache pointer"
 for the lifetime of the function.
 >
 > {{{
 > +  static strmap_t *cache = NULL;
 > }}}
 >
 > The side effect here would be that it's initialize to NULL *once* and
 then the if/else after sets it to whatever cache we need. The next call to
 that function, `cache` will point to the latest assignement and *NOT* be
 initialized to NULL thus always passing the assert() test after even if
 the cache type is wrong.

 Thank you for the explanation, I have fixed this now.

 I've pushed updated branches for the
 [https://github.com/DonnchaC/tor/tree/feature14846_4 tor] and
 [https://github.com/DonnchaC/torspec/tree/feature14846_2 torspec] patches.
 Thank you special and dgoulet for your feedback on this patch.

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