[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #4773 [Tor]: Implement Extended OR port (part of proposal 180)
#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