[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #3523 [Tor]: Allow controllers to post HS descriptors to the HSDir system

#3523: Allow controllers to post HS descriptors to the HSDir system
-----------------------------+-----------------------------------
     Reporter:  rransom      |      Owner:
         Type:  enhancement  |     Status:  needs_revision
     Priority:  minor        |  Milestone:  Tor: 0.2.7.x-final
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  maybe-proposal tor-hs
Actual Points:               |  Parent ID:  #8993
       Points:               |
-----------------------------+-----------------------------------

Comment (by dgoulet):

 > Hmm. Perhaps something like
 https://gist.github.com/Yawning/b41f29b6893dcdb83cc1 may resolve the
 remaining issues (NB: Untested because I don't have a HS descriptor etc.).
 >
 > Make sure I didn't mess anything up, and probably start seeking a 2nd
 reviewer (dgoulet maybe since he also has been doing control port stuff
 recently).

 Here is the actual gist ;) -->
 https://gist.github.com/Yawning/3feb8c26c3be3355f0e3

 I was reviewing but then you fixed the patch with the above so my review
 is a bit useless now ;). As for the `memchr() and *cp++ = '\n'` thingy,
 that's just plain bad. Assigning something to an originally const char
 pointer is not advise. Actually, if that memory was not in a writable
 section, you hit a big segfault. POSTDESCRIPTOR code should be fixed
 because this is very very bad practice.

 The rest of the patch is fine. I'll have another look at the next revision
 :).

 Here is my review of the tor-spec:

 * This will be problematic because parser and library expect `HSAddress`
 and `AuthType` even if none can be provided. This is why AuthType has a
 "NOAUTH" value and HSAddress, with #14847, will have a "UNKNOWN" value.
 Thus, I would suggest not putting them optional.
 {{{
 - "650" SP "HS_DESC" SP Action SP HSAddress SP AuthType SP HsDir [SP
 DescriptorID]
 - [SP "REASON=" Reason]
 + "650" SP "HS_DESC" SP Action [SP HSAddress] [SP AuthType] SP HsDir
 + [SP DescriptorID] [SP "REASON=" Reason]
 }}}
 * `Server = Fingerprint`, you should allow `LongName` instead.
 `node_get_by_hex_id()` in the code supports that format.
 * "are provide" --> "are provided" ?
 * `Descriptor` should be defined. Again, like in #14847:
 {{{
 Descriptor = The text of the descriptor formatted as specified
 in rend-spec.txt section 1.3 or empty string on failure.
 }}}

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/3523#comment:17>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs