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

 Replying to [comment:31 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?

 Add to the branch a squash! commit for those commits, containing better
 messages.  (You might need to set some command-line options to be able to
 make a commit that doesn't change any code.)

 > >   * 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 *`?

 Yes.  Eventually we'll do a big uint8_t propagation and make all of those
 take uint8_t instead.

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

 Then add a flag to control the prefix of the cookie file format, and a
 pointer to which variable to set.  Or something.

 Duplicate code is very very bad indeed.

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

 Ug. I didn't remember that.  Okay, in that case what it needs is some
 comments. And a tor-spec patch.

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