Re: [tor-bugs] #4773 [Tor]: Implement Extended OR port (part of proposal 180)

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#4773: Implement Extended OR port (part of proposal 180)
------------------------------------------+---------------------------------
 Reporter:  asn                           |          Owner:                    
     Type:  defect                        |         Status:  needs_revision    
 Priority:  normal                        |      Milestone:  Tor: 0.2.5.x-final
Component:  Tor                           |        Version:                    
 Keywords:  pt needs-proposal tor-bridge  |         Parent:  #5408             
   Points:                                |   Actualpoints:                    
------------------------------------------+---------------------------------

Comment(by nickm):

 562419bca0de212e1c1ffc3479dbf21da9591daf
   * The commit message is incomplete. It also replaces the 20-byte
 identifier with an 8-byte pointer.  I don't agree with that change.  And I
 almost missed it because the commit message didn't document it.

 f915dc616cf4f9ec9058c3f69cdf3608cd360a75
   * The commit message is incomplete.  It doesn't just create a cookie
 file. It also does some other stuff too.  Please take the time to make
 your commit messages accurate as a courtesy to the people who will be
 reading them.
   * The cookie should probably be a uint8_t, not a char*, since it's
 cryptographic stuff.  All new things that manipulate piles-of-bytes should
 take uint8_t.
   * get_ext_or_auth_cookie_file should be named ...filename().  It returns
 a filename, not a file.
   * The argument to init_ext_or_auth_cookie_authentication isn't
 documented, nor is its behavior. (Why does an "init" function disable
 something?)
   * I bet that init_ext_or_auth_cookie_authentication is substantially
 shared with its counterpart in control.c.  Can they be one function?  It
 would be great to have as  little duplicated code here as possible.

 6ee4b4e96769d7db96e6495ce15d47123755ad90
   * Client hash of the Extended ORPort authentication scheme -- what on
 earth does that documentation mean? When is this field set? Under what
 circumstance? What does it contain?  How do you hash a scheme?

 a01f8a02f39d87c8bf6dbcdab1ab68a4054211f7
   * Copy-and-paste is not an okay way to program.  Use functions for
 abstraction; don't copy and paste code.
   * There's no point reviewing copy-and-paste code.  I'll come back to
 this once it and control.c use a common set of functions for their common
 features.

 99f1cdb1adefc8dbef689ec94c9a1a539ae3aec7
   * This is why you don't duplicate functionality or copy and paste: You
 fixed a bug in initializing the cookie (by checking crypto_rand's return
 value), but the same bug exists in control.c.

 Random thought:
   * Do we have code somewhere to check that ExtORPort is on localhost? We
 really should.

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/4773#comment:30>
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