03/03/14 14:23, intrigeri wrote:
> Hi,
>
> I've had a look at the rewritten branch, at commit 7d0ea0b.
>
>> root ALL = (tor-launcher) NOPASSWD: /usr/bin/tor-launcher
>
> What is this useful for? root can already run any command as any user
> without password, no?
Woops, indeed. Removed.
>> amnesia ALL = (root) NOPASSWD: /usr/local/sbin/tails-tor-launcher
>
> It could be worth restricting the arguments that amnesia can pass to
> this command. That would be none, or --force-net-config, right?
We only need to call it with --force-net-config. Actually, that sudo
rule was intended for the GNOME menu entry that we're now not so sure we
want to add after all. I left it there with the option forced.
> In /usr/local/sbin/tails-tor-launcher, I'd rather see the four
> instances of:
>
> VAR=value
> export VAR
>
> ... written "export VAR=value" instead, but that's purely a matter of
> personal taste, and I don't care much.
That's a bashism that just happens to work in dash too. It should be
noted that the very similar bashism `local VAR=value` does *not* work in
dash. As I've been bitten by the latter one quite severely at one point
(when working on persistence in live-boot) I've come to prefer the POSIX
way at all times.
>> touch /etc/authbind/byport/53
>> chgrp debian-tor /etc/authbind/byport/53
>> chmod g=x /etc/authbind/byport/53
>
> Nowadays, I would instead write:
>
> install --group=debian-tor --mode=0710 /dev/null /etc/authbind/byport/53
>
> ... but again, purely a matter of personal taste.
I agree. However, I've dropped the authbind stuff in favor of a
netfilter REDIRECT, as you suggested earlier, so this is now moot.
>> $NICE \
>> $AA_EXEC \
>> - --exec $DAEMON -- $AA_EXEC_ARGS $DEFAULT_ARGS $ARGS
>> + --exec /usr/bin/authbind -- $DAEMON $AA_EXEC_ARGS $DEFAULT_ARGS $ARGS
>
> Any reason why /usr/bin/authbind is not grouped with the other prefix
> commands ($NICE and $AA_EXEC), instead of being part of the arguments
> passed to --exec?
... and this is now moot as well for the same reason.
>> Don't ever run Vidalia with -bridgeconf.
>
> So we could update our Vidalia package:
>
> 1. to drop vidalia-bridgeconf.patch: not needed anymore
> 2. to hide bridge settings (either in
> tails-remove-useless-controls.patch, or with a new patch, whatever
> is more practical)
>
> I guess #1 is not a blocker, but I'm unsure about #2. What happens if
> a user changes bridges settings in Vidalia, after having set it in Tor
> Launcher? And after *not* having set it in Tor Launcher?
In both situations the bridges are changed, no more no less. Was there
any specific potential problem you thought of?
The one issue I can think of at the moment is that Tor Launcher (thanks
to my modifications to it) will add an appropriate ClientTransportPlugin
line to Tor when bridges are input through it, but vidalia won't.
> (And if we address #2, implementing #1 as well does not add more than
> a few minutes of work.)
Agreed, leaving it there should be of no consequence and saves us the
packaging overhead.
--
I've rebased the branch completely since I've imported a new Tor
Launcher. However, you can review again from after commit 7d0ea0b (which
corresponds to where your review ended last time). The only thing that's
happened before that except the Tor Launcher bump is that I dropped the
commit introducing authbind, per you suggestion. From now on I'll fix
the Tor Launcher bumps differently to avoid history rewrites.
Big thanks for the review!
Cheers!
