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. [WAN: please update todo/bridge_mode:_handle_way_off_clocks -- without proper tagging, it may end up being forgotten for the next release...] 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). > 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? > 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/ > 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. > +if [ -e "${TOR_LOG}" ]; then > + rm -f "${TOR_LOG}" > +fi Why testing existence before rm -f? > + # depends on grepping these error messages, so we termporarily > + # increase Tor's logging severity. s/termporarily/temporarily/ > - 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? > -# 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)"? Cheers, -- intrigeri | GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc | OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc _______________________________________________ tails-dev mailing list [email protected] https://mailman.boum.org/listinfo/tails-dev
