[tor-bugs] #15588 [Core Tor/Tor]: Allow client authorization on control port ADD_ONION services

[Tor Bug Tracker & Wiki]

#15588: Allow client authorization on control port ADD_ONION services
-------------------------------------------------+-------------------------
 Reporter:  special                              |          Owner:  special
     Type:  enhancement                          |         Status:
 Priority:  High                                 |  needs_review
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.9.x-final
 Keywords:  tor-hs, control, TorCoreTeam201605,  |        Version:
  TorCoreTeam-postponed-201604                   |     Resolution:
Parent ID:  #8993                                |  Actual Points:
 Reviewer:  nickm                                |         Points:  small
                                                 |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by special):

 * status:  needs_revision => needs_review
 * reviewer:  dgoulet => nickm


Comment:

 Replying to [comment:27 nickm]:
 > I'm reviewing the diff rather than the patch series, since the history
 looks long.
 >
 > (Special, do you know about --autosquash? That's how most folks use the
 fixup! convention. This FIXUP thing you've been doing is less automatable.
 No need to change this branch, but it might help for next time.)

 Sorry about that mess - I'll avoid that in the future. To make the merge
 easier, I cleaned this one up.

 My feature15588 branch is the same as what you reviewed, plus three new
 commits. The other 5 commits are an _unmodified_ squash of what you
 reviewed.

 The new commits are the two 'fixup!' on top of the branch, plus 896271d5
 ("Use uint8_t for rend descriptor_cookie fields"). Again, sorry about the
 confusion.

 >
 > * NM.1 -- the output case of handle_control_add_onion is now possibly
 inconsistent? It looks like it can output some 250- lines followed by a
 551 line.  That's not allowed, I think.

 Fixed by making rend_auth_encode_cookie never fail and asserting that it
 doesn't instead of having this error case.

 > * NM.2 -- If it's not possible for add_onion_helper_clientauth to be
 called with missing created or err_msg_out parameters, should we maybe
 assert that they are present?

 Fixed by requiring those parameters, and simplifying the code slightly.

 > * NM.3 -- I'm a little worried that for some functions, err_msg_out
 includes the status code, and for others it doesn't.  That doesn't seem to
 be documented.

 This is consistent when read in context. The control.c helper function
 returns a status code line suitable for the control port. Utility
 functions from other files do not return anything specific to the control
 protocol. The documentation of add_onion_helper_clientauth mentions that
 it returns "a control port error message on failure".

 > * NM.4 -- I was about to complain about how awful the
 rend_auth_decode_cookie code is, but apparently it isn't new code, so I
 won't complain.  (sigh)

 This is slightly better now, with less random addition sprinkled
 throughout. Let me know if there's anything else specific I should do.

 > * NM.5 -- rend_auth_encode_cookie should really be using uint8_t, not
 char, especially since you're looking at the numberic value of the bytes.
 Probably same with rend_auth_decode_cookie().

 896271d525b2b31950572293c512224ca57cee02 changes these fields to uint8_t,
 and the fixup commits fix the auth_(encode|decode) functions to use
 uint8_t.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/15588#comment:31>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online