intrigeri wrote:
> 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.
>
> Congrats! This usability regression was one of the top mentionned ones
> in my experience when doing user support in the last few months.
>
> I won't be doing the entire review'n'merge process, but I had a quick
> look, and here are a few quick notes:
>
> 1. I'm not entirely convinced by the idea of running
> tor-controlport-filter as the vidalia user. Granted, it's the
> shortest path, but it feels weird, and having a bit more privilege
> separation would be good.
Fixed. Same branch.
> 2. The use of string.find(expected text) seems a bit fragile.
> Couldn't this matching be done in a bit stricter way?
I am not sure what you mean by stricter.
I changed it to startswith() anyway, which is equivalent
to what I did, but clearer.
find() reports position of first occurence (-1 on error).
I was testing that the position was == 0.
> 3. It might be overkill, and surely adds some code, but I would pass
> the port to listen on, control port socket path and authentication
> cookie path as command-line arguments (preferably named, not
> positional). Just to make it easier for others to reuse this piece
> of code, and increase collaboration between similar projects.
A quick research showed up that this can probably be done
with only a few lines of code if using the modules
optparse or argparse.
Python 2.6 (which Tails uses) only has optparse.
In python 2.7 optparse is deprecated and replaced by argparse.
This would mean writing one version for squeeze and one for wheezy.
I don't think it is worth it currently.
> Other than this, I was (once again!) impressed by the quality of this
> submission. I've merged it into our "experimental" branch, and pushed
> to our master repository + to our Jenkins auto-builder.
Thanks.
>> (I did not use Whonix existing implementation, because I felt very uneasy
>> about running a potentially security critical application as a shell
>> script.
>
> Fully agreed. I suggest you point the Whonix developers to your own
> implementation, once it's merged.
Ok.
