11/02/14 19:14, winterfairy@??? 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!
