Hi,
anonym wrote (22 Dec 2013 15:08:56 GMT) :
> 29/11/13 12:31, intrigeri wrote:
>> * remaining coding blockers
> I don't think there are any now. I recently fixed the following (IMHO)
> code blockers:
I've pushed a few tiny improvements (4ccf51cf, ff9a1dd) on top, and
merged devel in feature/spoof-mac.
The "use ip(8) instead of ifconfig(8)" thing that I raised in my
initial review on September 5 seems to have been forgotten somewhere.
I could find no ticket for it (which may explain why it was
forgotten), so I created #6540, assigned to you since (back in
October) you agreed to do it. Do we consider this issue is a blocker
(if so, please mark it for the 0.23 milestone), or something for
"later"?
Here's a code review.
> /usr/local/sbin/tails-unblock-network
> +if [ "$?" -ne 0 ]; then
> + log_error "tails-unblock-network exited with non-zero status"
> +fi
I'm unsure this log message will help a great deal in debugging.
Perhaps capture tails-unblock-network's stdout + stderr, and add this
and the exit code to the log message?
> log_error() {
> echo "`date "+day %j of %Y [%T]"` $1" >> /var/log/gdm3/tails-greeter.errors
> }
Should be added to
config/chroot_local-includes/usr/local/sbin/tails-debugging-info,
maybe? (We already have /var/log/gdm3/:0-{slave,greeter}.log
in there.)
> + install -m 0640 -o root -g "$LIVE_USERNAME" \
> + "${PHYSICAL_SECURITY_SETTINGS}" /var/lib/live/config/tails.physical_security
Why does the desktop user need to be allowed to read this? I haven't
seen any code that makes use of it, so perhaps it unnecessarily leaks
information to an attacker who can run code as `amnesia'?
> +nic_is_up() {
> + /sbin/ifconfig | grep -qe "^${1}\>"
> +}
Why use -e?
> +get_permanent_mac_of_nic() {
> + macchanger ${1} | sed -n "s/^Permanent\s*MAC:\s*\([0-9a-f:]\+\)\s.*$/\1/p"
> +}
> [...]
> +nic_has_spoofed_mac() {
> + [ "$(get_permanent_mac_of_nic ${1})" != "$(get_current_mac_of_nic ${1})" ]
> +}
I'm afraid this won't work very well for drivers that macchanger can't
retrieve the permanent MAC address from, e.g.:
$ sudo macchanger eth0
ERROR: Can't read permanent MAC: No such device
Permanent MAC: 00:00:00:00:00:00 (Xerox Corporation)
Current MAC: f0:de:f1:11:11:11 (Wistron Infocomm (kunshan)co)
I'm unsure what special casing can be done about it, but it would be
great to avoid *always* concluding such a NIC has a spoofed MAC.
Maybe we should simply save the original MAC before attempting to
spoof it, and compare later?
> +# Auxillary function for mod_rev_dep(). It recurses over the graph of
> +# kernel module depencies of $@. To deal with circular dependencies a
> +# global variable MOD_REV_DEP_VISITED keeps track of already visited
> +# nodes, and it should be unset before the first call of this
> +# function.
> +mod_rev_dep_aux() {
> [...]
> +# Prints a list of all modules depending on $1, including $1. It's
> +# ordered by descending "maximum dependency distance" from $1, so the
> +# output is ideal if we want to unload $1 and (by necessity) all
> +# modules that uses $1.
> +mod_rev_dep() {
I suggest we should make it clear that these functions only work for
*loaded* modules?
> +sub notify_maybe_blocked {
> + my $summary = gettext('Network connection blocked?');
Lacks decoding/encoding logic.
> + # We can't use Desktop::Notify since this script is supposed to be run
> + # as root (for access to syslog), started in an env without DESKTOP etc,
> + # which also causes issues with opening links in the text body.
... then we should *not* use links that don't work in the text body.
See how we've decided (mostly thanks to sajolida) to do for
incremental upgrades, when facing the same problem:
https://tails.boum.org/blueprint/incremental_upgrades/
> + system("/usr/local/sbin/tails-notify-user '$summary' '$body' 30000");
`system' is quite unsafe to use by default (not easy to retrieve the
exit code from its return value, goes through the shell in many
cases). Better use IPC::System::Simple's runx here.
> +my %state = ();
The "= ()" part is useless and non-idiomatic.
> + log "failed to up ${nic}"
Would be great to report the stdout + stderr here too.
> + macchanger_helper -e $1
> [...]
> +NIC=${1}
Please quote $1 and ${1}, and ${NIC} a few times below.
config/chroot_local-includes/usr/local/sbin/tails-unblock-network
could use "set -u" and from a bit more quoting too.
> +rm -f "${BLACKLIST}" || true
I don't guess what's the use of "|| true" here.
I'm flagging #6111 as needs more dev until these issues are resolved,
but I'm not going as far as creating tickets for each comment of mine.
Please don't rewrite the Git history, I don't plan to review it
entirely for the next iteration.
Great job!
I'm now going to test this branch a bit. I'll report back.
>> * remaining design documentation blockers (the blueprint didn't
>> make it into a design doc yet, right?)
> Done. Most of the blueprint was moved into our old
> `wiki/src/contribute/design/MAC_address.mdwn`, replacing its old
> contents. To me it reads like a decent design document and description
> of the current implementation, although it possibly could use another
> (external) read by now.
Yeah, that's a good one.
I've pushed a few minor improvements on top (oh crap, I realize I've
done this in feature/spoof-mac, which is the right place IMO, but not
the way you've been doing it — I'm sorry).
More comments:
* Would it be worth documenting the new fail-safe mechanism too?
* "The first is clearly less bad than the second, but the ideal, which
avoids these problems altogether." --> looks like
incomplete sentence.
* "The real solution is therefore to eliminate the problematic this
time frame completely by preventing any network devices from being
enabled at all until the decision has been made, and have the MAC
spoofing setting applied immediately when the device is added." -->
doesn't say clearly that this is, indeed, what we do.
* "Therefore it seems that the label of the option isn't what we
should focus on, so we might just as well call it" --> feels more
like blueprint-speak open for discussion (which it actually was)
than a design doc for a completed feature. Again, it doesn't make it
clear what we decided to do in the end.
* In the "Network blocking", it would be good to explicitly note that
this depends on all network device drivers to be compiled as
modules, if that's actually the case.
* In the "Connection failure detection" section, it would be good to
point to the place where tails-blocked-network-detector is
started from.
* There should be a link to the relevant pieces of code that live in
the Greeter.
=> I've split this into its own ticket (#6541), marked as "Dev
Needed" and assigned to anonym. Please mark as "Ready for QA" and
reassign to me when you're done.
>> * remaining user documentation blockers (are there placeholders doc
>> pages with indications for doc writers already?)
> There now is. The old user doc page was completely re-written in commit
> ed50222.
Now that this feature is enabled by default, and has a GUI, I don't
think its end-user documentation should live in doc/advanced_topics/.
Also, it's a Greeter option, so it should at least be mentioned
somewhere in doc/first_steps/startup_options. Anyway, that's now on
sajolida's plate.
#5668 is now dedicated to the end-user doc. Reassigned to sajolida,
marked as "Dev Needed" and adjusted "% Done" as he'll no doubt want to
deeply revamp this ;)
>> * once all coding blockers are solved, I think a bolder call for
>> testing (pointing to a nightly built ISO from experimental) would
>> be good: some of us told that they are lost in this huge thread,
>> and it's unclear to them how close we are to the "please widely
>> test this code" state
> I think we're here now. As we discussed privately, it's probably better
> to build the test ISO from the feature branch and keep it up-to-date
> with devel, which it currently is. Could any one with the know-how set
> up a nightly build of feature/spoof-mac?
> I'll start preparing a public call for testing post.
Good. ETA?
(I would personally have done this in two steps, first issuing a clear
call for testing on tails-dev, and second a broader public call for
testing, but well, I guess it's too late now, so go, go, go! :)
> The only blocker is, from how I understand our previous discussions, the
> following task you created:
> https://labs.riseup.net/code/issues/6454
> I've assigned myself to it (with a due date!). Hopefully it'll lead to a
> quick solution for its parent (#6453) so we can even have it in the
> first non-test release (0.23 I suppose) as well.
Great. Marked as blocking #5421.
Cheers!
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
