11/02/14 19:14, [email protected] wrote: > I believe I have fixed the regression described in ticket #6383. When > access to Tor's control port was restricted (to prevent GETINFO address), > Torbutton could no longer do "New Identity". I have created a filtering > proxy for the control port, that only allows SIGNAL NEWNYM. This is enough > to make "New Identity" work again as expected. > > Branch "winterfairy:bugfix/torbutton-new-identity" in my Tails repo (based > on devel).
Thanks for you contribution! [...] > Please review and test it, and merge it is it looks fine. If you believe > something could be improved, or should be done differently, please say. My test looked good. When it comes to reviewing I don't have much to add as intrigeri caught most blockers: > +TOR_CONTROL_PASSWD='passwd' Why do we set this? It actually bit me in my work on integrating Tor Launcher into Tails; if it's set, it overrides the `TOR_CONTROL_COOKIE_AUTH_FILE` env var I wanted Tor Launcher to use so I had to explicitly unset `TOR_CONTROL_PASSWD`. This is not a blocker, but I'd like to know why it was added. > + # Read in a newline terminated line (max 128 chars) > + line = readh.readline(128) Why do we read 128 characters specifically (same thing done for all other socket reads)? Tor's control language always terminate responses with CRLF so why can't we use `readline()` without argument, which reads until a LF? I suppose that'll leave a trailing CR, but that could be dropped if necessary, which I don't think we need to care about because of how we verify responses with `startswith`. While I'm pretty sure this will be ok in the subset of the control language we deal with (*currently*), I'd at least want to hear how you thought about this with a code comment. While you're at it, it'd be great if you made the reads use a nicely named constant (and put the comment where it's defined) instead of magic 128:s everywhere. After that's fixed I'm ready to merge this. As a side note, I suppose it'd be nice to use stem [1] to greatly simplify the script and in particular avoid the low-level socket stuff but it's not packaged in Debian. :/ [1] https://stem.torproject.org/ Any way, Cheers! _______________________________________________ tails-dev mailing list [email protected] https://mailman.boum.org/listinfo/tails-dev To unsubscribe from this list, send an empty email to [email protected].
