Re: [Tails-dev] Please review and merge bugfix/tordate_vs_br…

Delete this message

Reply to this message
Author: intrigeri
Date:  
To: The Tails public development discussion list
Subject: Re: [Tails-dev] Please review and merge bugfix/tordate_vs_bridge_mode
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