Re: [tor-bugs] #5262 [Stem]: Implement Safe Cookie in Stem

["Tor Bug Tracker & Wiki" <torproject-admin@xxxxxxxxxxxxxx>]

#5262: Implement Safe Cookie in Stem
-------------------------+--------------------------------------------------
 Reporter:  atagar       |          Owner:  gsathya     
     Type:  enhancement  |         Status:  needs_review
 Priority:  normal       |      Milestone:              
Component:  Stem         |        Version:              
 Keywords:               |         Parent:              
   Points:               |   Actualpoints:              
-------------------------+--------------------------------------------------

Comment(by atagar):

 Review isn't complete, these are just some thoughts I've scribbled down
 while waiting for a movie to start. Seems better to send this along now
 rather than waiting until I'm done. I'll try to finish the code review
 tomorrow morning.

 As always please push back if you disagree on anything!

 > +  |  |- InvalidClientNonce - The client nonce is invalid.

 Are you sure that we want to reuse CookieAuthFailed exceptions for
 safecookie authentication? If it works then that's certainly preferable
 (since it simplifies the exception hierarchy), but I wonder if it'll
 confuse authenticate() callers since they then won't know if an exception
 is from a cookie or safecookie authentication attempt.

 One option would be for CookieAuthFailed to have an attribute that says if
 it was from a cookie or safecookie failure, but I'm not sure if that's
 cleaner or more confusing than having a separate branch of exceptions. At
 present I'm favoring it a bit since normal cookie authentication is slated
 for deprecation...

 > +import re

 Trivial detail but imports in stem are ordered by length of the module
 name. Yea, it's arbitrary - I just did it that way cuz it looks nice. I
 could be persuaded that alphabetical or something else is better.

 > +class AuthChallengeFailed(AuthenticationFailure):
 > +  "AUTHCHALLENGE command has failed."

 Looks like these fell out of sync with the header pydocs. Is there a
 reason that these are a AuthenticationFailure subclass rather than
 CookieAuthFailed? They only arise during cookie auth, no?

 > -  MissingAuthInfo,

 Why are you dropping this exception type? Is it somehow impossible for
 authenticate() to raise these?

 > +  AuthChallengeFailed,
 > +  AuthSecurityFailure,
 > +  InvalidClientNonce,
 > +  NoAuthCookie

 The AuthChallengeFailed entry negates the need for AuthSecurityFailure and
 InvalidClientNonce since it's the parent class. Also, the ordering doesn't
 match the authenticate() function's pydocs.

 Admittedly this might be a little confusing - here's how
 AUTHENTICATE_EXCEPTIONS is supposed to work:
 The authenticate() function might try multiple methods of authenticating
 to tor, and hence have multiple exceptions that it could raise to the
 caller. The AUTHENTICATE_EXCEPTIONS listing is used to pick which
 exception has the highest priority. This listing of priorities are stated
 in the authenticate() function's pydocs.

 Feel free to beef up the commenting to help future devs in understanding
 the code - a fresh pair of eyes are good for that. :)

 > +  if AuthMethod.SAFECOOKIE in auth_methods and
 protocolinfo_response.cookie_path is None:
 > +    auth_methods.remove(AuthMethod.SAFECOOKIE)
 > +    auth_exceptions.append(NoAuthCookie("our PROTOCOLINFO response did
 not have the location of our

 Might be worthwhile joining this with the below AuthMethod.COOKIE check
 since they're checking for the same thing (missing a path during some form
 of cookie auth).

 > +      elif auth_type == AuthMethod.SAFECOOKIE:
 > +        cookie_path = protocolinfo_response.cookie_path

 Again, we can share with AuthMethod.COOKIE.

 > +      log.debug("The authenticate_cookie/authenticate_safecookie method
 raised...

 If we had a flag in exceptions for if it's from cookie or safe cookie auth
 then we could customize this.

 > +      log.debug("The authenticate_safecookie method raised an \
 > +AuthSecurityFailure. This could be an attack. (%s)" % exc)

 Please don't try overly hard to meet a certain line length limit. It's
 good if we can write our code so the length of lines is short, but a line
 like this is more readable as a single line.

 > +def authenticate_safecookie(controller, cookie_path,
 suppress_ctl_errors = True):

 Hmm, I keep going back and forth about if it would be better as
 'authenticate_safecookie' or 'authenticate_safe_cookie' - thoughts?

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