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
