Re: [Tails-dev] Made New Identity work again, please review

Delete this message

Reply to this message
Autor: anonym
Data:  
A: The Tails public development discussion list
Assumpte: Re: [Tails-dev] Made New Identity work again, please review
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!