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 asn):

 Replying to [comment:30 nickm]:

 Thanks for the review. I have some questions on a subset of your comments:

 > 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.

 You are right about the commit messages. Sorry for the extra pain.
 What should I do now? Do you prefer that I create a branch with better
 commit messages, or should I leave it like this?

 >   * 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.

 Right. I tried fixing this, but functions like `crypto_rand()` and
 `crypto_hmac_sha256()` expect `char *`. Should I just cast the `uint8_t *`
 to `char *`?

 >   * 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.
 >

 It's true. However, the cookie file format is different in each case, and
 each of them uses a different global variable
 (`authentication_cookie_is_set` and `ext_or_auth_cookie_is_set`). Making a
 function that will help both cases, might look weird.

 > 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.
 >

 Hm, the common part between those two codebases is the part where I use
 the client's nonce. Unfortunately, both the protocol format and the HMAC
 construct are quite different between the control-safecookie and the
 extended-or-port authentication protocols. There was a big discussion on
 whether these two protocols should be different in #7098, and we decided
 to go with different protocols. I'm not sure how to merge
 `handle_control_authchallenge()` and `
 connection_ext_or_auth_handle_client_nonce() in a nice way.

 > 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:31>
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