Re: [Tails-dev] Next low-hanging fruits session
sajol...@pimienta.org: > Hi, > > We'll gather on #tails @ irc.oftc.net to pick low-hanging fruits on: > > - Saturday January 4 at 9am UTC / 10am CET Remember we will have a low-hanging fruit session next Saturday. signature.asc Description: OpenPGP digital signature ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev
Re: [Tails-dev] Please review'n'merge test/fix-persistence-checks
intrigeri wrote (27 Dec 2013 19:52:15 GMT) : > (FTR that's #5465.) > I'll bring this branch to completion. This branch now has everything I've asked for in my initial review, and the feature fully passes for me. Please review'n'merge. Assigned to bertagaz. 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
Re: [Tails-dev] Please review'n'merge test/keep-volumes-tag
intrigeri wrote (27 Jul 2013 18:18:31 GMT) : > anonym wrote (26 Jul 2013 12:50:11 GMT) : >> branch: test/keep-volumes-tag > At first glance this looks good to me, thanks! > I'll test and merge it once I can merge test/reorg. >> ticket: none >> This branch depends on test/reorg. > Same as of the other branches, unless concerns with test/reorg are > addressed in the next few days, please create a ticket and mark it as > blocked by the test/reorg ticket. Filed as #6544 so that we don't lose track of it. I'll bring this branch to completion, probably by cherry-picking its single commit on top of test/rjb-migration, so that we don't forever block on test/reorg. 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
Re: [Tails-dev] Please review'n'merge test/fix-persistence-checks
(FTR that's #5465.)
intrigeri wrote (27 Jul 2013 16:59:39 GMT) :
> anonym wrote (26 Jul 2013 12:47:46 GMT) :
>> branch: test/fix-persistence-checks
> I've had a look. This changeset generally looks generally good to me
> (though I haven't tested it yet, mostly because I don't expect my test
> setup to be able to go as far as testing these steps...).
> Nitpicking follows.
>> + return @vm.execute("test -e #{persistence_state_file}").success? &&
>> + @vm.execute(". #{persistence_state_file}; " +
>> + "test $TAILS_PERSISTENCE_ENABLED = true").success?
> Due to the lack of quotes around $TAILS_PERSISTENCE_ENABLED, if the
> tested condition is not verified (e.g. persistence_state_file is
> missing empty or something), then I guess that the *shell* will error
> out due to a syntax error, which IMHO is poor reporting from a test
> suite: in error conditions, I think a test suite should tell us that
> what we're testing is not verified, e.g. something like
> "$TAILS_PERSISTENCE_ENABLED is not set to true as it is supposed to
> be", instead of just erroring out with "parse error: condition
> expected: =" because it could actually *not* perform a test.
> What do you think?
> In the same spirit, perhaps '&&' could be used instead of ';' to
> chain commands in there.
Done in that branch.
> To end with, I fail to see the rationale to split the "all persistent
> directories are mounted" step out of "persistence is enabled". I think
> it's the kind of low-level implementation detail that makes it harder
> to read and maintain features, and that should instead be hidden in
> step definitions. It's probably not in these terms that a Tails user
> would express their experience of the Tails persistence feature.
> What do you think?
I think I'll just revert this part unless someone objects.
> I'm marking #5465 as "dev needed", and reassigning to anonym.
I'll bring this branch to completion.
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
Re: [Tails-dev] Please review'n'merge test/reorg
Filed as #6543 so that we don't lose track of it. Needs a tiny bit more dev. 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
Re: [Tails-dev] Please review'n'merge test/rjb-migration
berta...@ptitcanardnoir.org wrote (27 Dec 2013 09:36:32 GMT) : > Just one word to sum up my feelings: hurray! :) :) The test suite is now entirely green for me, with the two known exceptions that are documented (git grep XXX -- features). Please review my changes and merge it into devel if it looks good! Tickets that will go away: #6314, #6399. The parent ticket (#5731) is still blocked by #6400, as nobody commited to either upstream our stuff into the ruby-sikuli Gem, or to maintain our own adapter on the long term. I'm unsure what conclusion we should draw of it. Admitedly, the new setup is way less scary than the previous one, and our adapter is 85 lines long, but still, I'm concerned we might happily be replacing it with another kind of technical debt. Shall we just forget about it and close #6400 as rejected? Thoughts? In particular, I'd like to hear what anonym thinks of it. 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
Re: [Tails-dev] MAC spoofing: current status?
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' 3");
`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 i
[Tails-dev] Please review'n'merge bugfix/6536-IE-icon-in-Windows-camouflage-mode
Hi, bugfix/6536-IE-icon-in-Windows-camouflage-mode fixes a regression (#6536) brought in Tails 0.22 when we migrated to FF24. Candidate for 0.22.1 => please merge into stable and devel. Assigned to bertagaz, but I guess he won't mind if another reviewer beats him to it. 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
Re: [Tails-dev] Please review'n'merge test/rjb-migration
On Fri, Dec 27, 2013 at 01:16:05AM +0100, intrigeri wrote: > intrigeri wrote (26 Dec 2013 18:17:21 GMT) : > > I don't want to merge this branch with so many failing tests, as one > > expected failure (e.g. due to a missing image update) may very well > > hide other issues that might be caused by the migration to RJB. > > > So, I've started fixing every test case I could. Doing this in the > > test/rjb-migration branch too, even if it doesn't 100% belong here, > > because IMO this goes with merging this branch. > > > In the state where I've brought test/rjb-migration today, all tests > > now pass but: > > > * the USB -related tests. Reported as #6537. I'm going to try and > > fix those now. > > I believe I've done what #6537 was about, but this feature still fails > for me a bit later (#6539). I'll try re-running it entirely, but well, > seeing the installer stuck at various stages of the process is no > news, and that's why we have never removed these steps from the manual > test suite yet. So IMO this is not a blocker. > > > * the "Windows should appear like those in Microsoft Windows XP" > > scenario: in 0.22, the browser's title bar displays the Iceweasel > > icon, instead of the IE one, so it looks like the Windows > > camouflage script misses an update for FF24. Reported as #6536, > > will try to fix in time for 0.22.1 as it's a regression. > > I'm testing a fix for this. Once I'm done with validating this part of > the test suite, I'll ask bertagaz to review all the commits I've added > on top of what he submitted, and to merge the branch. > > Stay tuned, we're getting close :) > > /me is very happy to be able to run this test suite in good > conditions, finally! I am too, and find it is running quicker than when using Jruby. I intended to start to fix the outdated scenarios in another branch, as when I asked for the merge, the rjb test suite was running as good (or bad, depends how we see) as the previous Jruby implementation, failing at the same places. So I thought there were no regressions, and the branch was ok for a merge. But you spend time doing it (and having fun hacking it seems), and it's great. I'll reserve some time to test it when you'll send your review'n'merge. Just one word to sum up my feelings: hurray! :) bert. ___ tails-dev mailing list tails-dev@boum.org https://mailman.boum.org/listinfo/tails-dev
