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 _______________________________________________ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev