27/12/13 18:05, intrigeri wrote:
> Hi,
Thanks for an awesome code and documentation review! Sorry for the long
delay, but frankly I've been swamped until now.
> 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"?
See commit 6881869 for migration from ifconfig to ip. However, ip isn't
particularly well-suited for grep:ing the output of (not that ifconfig
is any better) so I opted to use the sysfs interface instead when
possible for less regex spaghetti and greater clarity. For that, see
commit 940690c.
> Here's a code review.
All done in commits ebba967..7b7ba02d in Tails Git, and e18223a..657ad42
in Tails Greeter's Git. Below are some comments when needed, or
explanations why I didn't fix it:
>> +nic_is_up() {
>> + /sbin/ifconfig | grep -qe "^${1}\>"
>> +}
>
> Why use -e?
Bad habit. This is irrelevant now after the move to sysfs.
>> +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.:
To be continued after the issue pointed out in the post "Serious issue:
fail-safe and hotplugging" (since it seems like the solution to it
determines how to deal with this).
>> + # 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.
Either you misunderstood that comment, or I misunderstand you. By using
the `tails-notify-user` wrapper, like the script currently does, links
work just fine. The comment is about Desktop::Notify not being an
alternative to `tails-notify-user` if we want links to work, which I
think is a good thing to do when we can, like in this case.
Do you agree? If so, feel free to clarify the code comment if you feel
it is unclear.
>> + 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.
I tried adding `set -u` this but apparently `/usr/bin/gettext.sh` uses
ZSH_VERSION without setting it, so the script fails.
> 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.
Reassigned to you now.
>>> * 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:
These are fixed in commits e67eb0d..1fd36b8.
> * Would it be worth documenting the new fail-safe mechanism too?
Definitely, now when it's settled how it should work.
> * "The first is clearly less bad than the second, but the ideal, which
> avoids these problems altogether." --> looks like
> incomplete sentence.
That paragraph should never have been committed so I removed it.
> * "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.
It does in the implementation part. See the first sentence in the
"Network blocking" section.
> * "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.
I removed that section completely. If people will find issues with this
feature's UX in the Greeter they will do it by using it, not by reading
the design document. Or what do you think?
> * 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.
Very good point! Added.
> * In the "Connection failure detection" section, it would be good to
> point to the place where tails-blocked-network-detector is
> started from.
Done. Actually I both renamed the script and moved its start from Tails
Greeter's `PostLogin` into `tails-unblock-network`.
> * There should be a link to the relevant pieces of code that live in
> the Greeter.
I linked to Tails Greeter's `PostLogin.default` script, stating that
`tails-unblock-network` is started from there.
> => 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.
Reassigned.
Thanks again for the review!
Cheers!
