28/11/12 16:22, intrigeri wrote:
> Hi,
>
> anonym wrote (27 Nov 2012 13:16:53 GMT) :
>> I've now pushed a fix into the branch bugfix/tordate_vs_bridge_mode
>> (merged into experimental). Please review and merge this branch into
>> testing and devel so it ends up in the next Tails release.
>
> Great! Here's an initial (static) review, then.
Awesome!
> [WAN: please update todo/bridge_mode:_handle_way_off_clocks -- without
> proper tagging, it may end up being forgotten for the next release...]
Done.
> Once we have a branch that looks mergeable, before merging, I'd like
> someone to do the dozen of time sync' tests I had done for the last
> time sync bugfix branch (and that discovered the bug we're fixing).
I did this (in the branch's current (rebased) state).
>> d1e3258 Use shell library for tor_is_working() in the Unsafe Browser.
>> 98b48a9 Add logging for is_clock_way_off().
>> 0d9232b Extract Tor's ControlPort from torrc.
>
>> +TOR_CONTROL_PORT=$()
>
> What does this mean? Is this variable used at all?
It's just some useless residue I committed by mistake.
>> 4793282 White-list root to use Tor's ControlPort.
>
> Good. But I'm concerned this addition to the white-list is documented
> nowhere, but in this commit message. I think I would have a hard time,
> without "git blame" and "log -p", to understand why root is supposed
> to have access to the ControlPort if I look at it in a year. Also,
> I fear we forget to remove undocumented exceptions once they are not
> needed anymore.
>
> Our design doc [1], on these matters, pretends that "For specifics,
> see the firewall configuration where this is well commented:
> config/chroot local-includes/etc/ferm.conf". So, either we really do
> so (like adding comments to ferm.conf when we add an exception), or we
> should document such new exceptions in the design doc.
>
> [1] https://tails.boum.org/contribute/design/Tor_enforcement/Network_filter/
I'll add a comment in ferm.conf.
>> e9c2de1 Kill Vidalia when restarting Tor.
>
>> 44489d0 Make tordate work in bridge mode with an incorrect clock
>
> > As a bonus, split out some Tor-related functionality and put that in a
> > shell library.
>
> I think this commit should be split into a first one dedicated to
> refactoring, and a second one that, building on top of the new
> library, implements the bugfix.
Ok. I'll rebase the branch then. See below for list of list of affected
commits to re-review.
>> +if [ -e "${TOR_LOG}" ]; then
>> + rm -f "${TOR_LOG}"
>> +fi
>
> Why testing existence before rm -f?
If we had `set -e` that would otherwise kill the script. Or one could've
used "|| :" or "|| true" to handle all errors. Whatever. Removed during
rebase.
>> + # depends on grepping these error messages, so we termporarily
>> + # increase Tor's logging severity.
>
> s/termporarily/temporarily/
Fixed.
>> - grep -q "\[warn\] Certificate \(not yet valid\|already expired\)." \
>> + grep -q "Certificate \(not yet valid\|already expired\)\." \
>
> Wouldn't something like (warn|info) work, and avoid losing strictness?
True. Fixed.
>> -# 1. Tor completes a handshake with an authority.
>> -# 2. Tor fails the handshake with all authorities.
>> +# 1. Tor completes a handshake with an authority (or bridge).
>> +# 2. Tor fails the handshake with all authorities (or bridge).
>
> Does the second "(or bridge)" mean "(or all bridges)"?
The latter. Fixed.
Thanks a lot! Here's a list of the modified/new commits which may make
it easier to re-review this since a rebase occurred:
42628eb Remove line committed by mistake.
8db31a6 Document white-listing of Tor's ControlPort for root.
6a1a218 Stop Tor before messing with its log or data dir.
cbe71f5 Remove Tor's log before time syncing.
13d99c8 Make tordate work in bridge mode with an incorrect clock.
01a92fb Use shell library in tordate.
05f1d09 Use shell library in the Unsafe Browser.
0041c70 Add a shell library for Tor functions.
The branch has been merged into experimental.
Cheers!
