Tails

#3 Updated by anonym 2017-06-19 10:28:44

• Assignee set to lamby
• QA Check set to Info Needed

Sure. But why can it happen at all, and could it cause differences elsewhere? I’m curious if there some nondeterminism in the order things are processed. Without delving to deep into this one, do you have any ideas, lamby?

#4 Updated by intrigeri 2017-06-22 13:49:51

• Parent task set to Feature #5630

#6 Updated by lamby 2017-06-23 09:05:10

• Assignee changed from lamby to anonym

> I suggest we reorder the content of these files as a post-processing step.

I disagree; re-ordering a recursive XML structure seems a little fraught and more complicated than just fixing the issue.

I think this is http://sources.debian.net/src/gconf/3.2.6-4/backends/markup-tree.c/#L681 or http://sources.debian.net/src/gconf/3.2.6-4/backends/xml-dir.c/#L885

No patch yet. Should be reasonably straightforward - permission to proceed? :)

#7 Updated by anonym 2017-07-07 15:20:19

• Assignee changed from anonym to lamby

lamby wrote:
> > I suggest we reorder the content of these files as a post-processing step.
>
> I disagree; re-ordering a recursive XML structure seems a little fraught and more complicated than just fixing the issue.

Huh, I expected it to be easy, which seems supported by this SO answer: https://stackoverflow.com/questions/8385358/lxml-sorting-tag-order/8387132#8387132

Of course, things often look “easy” before you actually try. :)

> I think this is http://sources.debian.net/src/gconf/3.2.6-4/backends/markup-tree.c/#L681 or http://sources.debian.net/src/gconf/3.2.6-4/backends/xml-dir.c/#L885
>
> No patch yet. Should be reasonably straightforward - permission to proceed? :)

… but that could also apply to your hunch here. :) But I trust you if you think this is the way to go still. But given that we probably have a workaround for this reproducibility issue (but don’t on the others) I think you should spend at most four hours on this. If you think so, feel free to unset the “Info Needed” status and proceed!

#8 Updated by lamby 2017-07-07 16:01:42

• QA Check changed from Info Needed to Dev Needed

Well, it’s not only nasty hack to fiddle the XML… and we might be breaking it in subtle ways; I don’t trust my XPath to only select the parent elements we want to solve. Also, upstream fixes \o/

> I think you should spend at most four hours on this

ACK, going ahead… :)

#9 Updated by lamby 2017-07-09 20:57:47

• Assignee changed from lamby to anonym

Here you go… :)

diff --git a/backends/markup-tree.c b/backends/markup-tree.c
index 4857cae..fc508dd 100644
--- a/backends/markup-tree.c
+++ b/backends/markup-tree.c
@@ -255,6 +255,13 @@ struct _MarkupDir
   guint is_dir_empty : 1;
 };

+static gint
+compare_markupdirs (MarkupDir *a,
+                    MarkupDir *b)
+{
+  return strcmp(a->name, b->name);
+}
+
 static MarkupDir*
 markup_dir_new (MarkupTree *tree,
                 MarkupDir  *parent,
@@ -271,7 +278,9 @@ markup_dir_new (MarkupTree *tree,
   if (parent)
     {
       dir->subtree_root = parent->subtree_root;
-      parent->subdirs = g_slist_prepend (parent->subdirs, dir);
+      parent->subdirs = g_slist_insert_sorted (parent->subdirs,
+                                               dir,
+                                               (GCompareFunc)compare_markupdirs);
     }
   else
     {

#10 Updated by intrigeri 2017-07-18 13:05:35

• Status changed from Confirmed to In Progress
• % Done changed from 0 to 30
• Type of work changed from Research to Code

It looks like next step is: anonym applies this patch and uploads a package to our custom APT repo.

#14 Updated by anonym 2017-07-23 13:10:10

• Feature Branch set to wip/feature/12738-remove-gconf

I completely agree that removing gconf2, gksu and friends is the proper solution. But it’s certainly not trivial — the feature branch is a WIP so far:

• The gksu package provides the Root Terminal .desktop file and the icon. In commit:235d9e0 I add a hook that downloads the gksu package and extracts these files only, instead of installing the package. It lacks an appropriate policy file to make pkexec work with gnome-terminal (i.e. allow_gui).
• Oh, and such a policy file is also missing for Tor Launcher: commit:d4b1667 should be fixed up with the addition of such a file.
• We explicitly install the following two packages that (transitively) depend on gconf2:
  • libgnomevfs2-extra (introduced in commit:5afa2a2) which we installed in for FTP/SSH handlers in Nautilus, but those protocols works fine for me without this package so it seems we can safely drop it (didn’t test other protocols like Samba though).
  • libgnome2-bin (Bug #11031, commit:36db19c) for /usr/bin/gnome-open. Since some applications hard-depend on that path we can’t just remove gnome-open, so I’m thinking that create a symlink /usr/bin/gnome-open -> /usr/bin/xdg-open.

Also, messing with all this for the Tails 3.1 point-release feels a bit daring, but we want 3.1 to be as reproducible as possible since we’re gonna ask the larger reproducible builds community to join us then. So I’m torn.

#18 Updated by anonym 2017-08-04 19:40:53

• Assignee changed from anonym to bertagaz
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch changed from wip/feature/12738-remove-gconf to 3.1:feature/12738-reproducible-gconf-xml 3.2:wip/feature/12738-remove-gconf

I applied lamby’s patch from Bug #12738#note-9, built new gconf packages and uploaded them to the feature/12738-reproducible-gconf-xml branch’s APT overlay. Jenkins successfully built and tested the branch with only a single unrelated failing scenario (Tor bootstrap timeout).

As for manual verification that this didn’t introduce any regression, I verified that gconftool-2 --dump / produces the exact same output vs Tails 3.0.1, and I looked at XML files, and it seemed all that differed was that they were sorted. And du -s reports the same amount of bytes for the whole directory. So it seems fine.

Please review’n’merge the feature/12738-reproducible-gconf-xml branch into stable for Tails 3.1! But let’s leave this ticket alive (so no Fix-committed!) since the real fix isn’t in yet — just reassign it to me as Dev Needed even if merged.

#20 Updated by bertagaz 2017-08-05 10:13:34

• Assignee changed from bertagaz to anonym
• QA Check changed from Ready for QA to Dev Needed

anonym wrote:
> Please review’n’merge the feature/12738-reproducible-gconf-xml branch into stable for Tails 3.1! But let’s leave this ticket alive (so no Fix-committed!) since the real fix isn’t in yet — just reassign it to me as Dev Needed even if merged.

Ok, done, hotfix will be in 3.1! :)

#21 Updated by anonym 2017-08-07 13:53:15

• Target version changed from Tails_3.1 to Tails_3.2
• Feature Branch changed from 3.1:feature/12738-reproducible-gconf-xml 3.2:wip/feature/12738-remove-gconf to wip/feature/12738-remove-gconf

#22 Updated by anonym 2017-08-14 16:19:04

• blocks Bug #13625: Root Terminal cannot start graphical applications added

#23 Updated by anonym 2017-08-22 20:27:57

• File diffoscope-muri.txt.bz2 added

lamby: FYI, your patch doesn’t seem to be enough. The attached diff exhibits (seemingly) the same sorting issue (+ Bug #12740, which is ok), and it’s between two 3.1 builds, which has the patched gconf installed.

I say: Whatever, we’re still on course to have this solved in Tails 3.2.

#24 Updated by lamby 2017-08-24 21:45:47

• Assignee changed from anonym to lamby

(Taking ticket back)

Ah yes, some “nested” non-determinism we didn’t spot before because the outer stuff was masking it. On it!

#25 Updated by anonym 2017-08-24 21:58:45

• Assignee changed from lamby to anonym

lamby wrote:
> (Taking ticket back)
>
> Ah yes, some “nested” non-determinism we didn’t spot before because the outer stuff was masking it. On it!

Wait! What I meant in Bug #12738#note-23 with:

> I say: Whatever, we’re still on course to have this solved in Tails 3.2.

was: “Whatever, we will have another solution in Tails 3.2 any way, so let’s not bother fixing deprecated software”. So please forget about looking into gconf any further!

Taking back the ticket for that reason.

#29 Updated by anonym 2017-09-04 16:52:34

• % Done changed from 30 to 50
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch changed from wip/feature/12738-remove-gconf to feature/12738-remove-gconf

I think this branch is ready for merging now! Let’s see what Jenkins thinks…

#33 Updated by intrigeri 2017-09-07 12:11:21

• Priority changed from Normal to Elevated

#34 Updated by anonym 2017-09-12 23:20:14

• Assignee changed from anonym to intrigeri

The test suite results do not look stellar, and I’m not sure why. Can you begin with a code review and manual test while I investigate?

#36 Updated by intrigeri 2017-09-13 08:42:37

• Assignee changed from intrigeri to anonym
• QA Check changed from Ready for QA to Dev Needed

UX regression: in greeter.git:gdm/PostLogin.default we delete gksu.desktop when no admin password was set. This was not adjusted on this branch, so “Root Terminal” is listed even when it’s not usable.

UX and security regression: previously, when starting Root Terminal, the authentication prompt would make it explicit (although not very user-friendly) what exactly the user was being asked to authenticate for, which I find useful because it’s not merely about authenticating the user, it’s also (incidentally) about asking the user permission to use their admin privileges for $operation — in some way, the polkit authentication agent (GNOME Shell) acts as a Portal to build mutual trust between the system and the user. On this branch, the authentication prompt instead says “Authentication Required” followed by “Debian Live User”, and I cannot be certain I’m actually typing my password to allow what I asked (starting a terminal as root) or to allow something else (e.g. my exploited Pidgin could monitor execution of gnome-terminal-pkexec, win the race, ask me permission to do unspecified stuff, I’ll blindly assume it’s the prompt I expect for opening the Root Terminal, and the attacker will persistently root my Tails).
My understanding is that the right way to fix this is to have the polkit authentication agent that mediates the permission request & privilege escalation tell the user what the permission is asked for, see e.g. /usr/share/polkit-1/actions/com.ubuntu.pkexec.synaptic.policy. Thankfully it looks like intltool supports this file format, e.g. https://sources.debian.net/src/synaptic/0.84.2/po/synaptic.pot/#L3447 and https://sources.debian.net/src/synaptic/0.84.2/data/com.ubuntu.pkexec.synaptic.policy.in/ :)
After looking at grep --extended-regexp '<message>' /usr/share/polkit-1/actions/org.gnome.*,
I suggest using “To start a Root Terminal, you need to authenticate.” as the message: most other prompts use passive voice, which is recommended against by all the tech writing guides I’ve read, so I’ve picked the message from org.gnome.controlcenter.datetime.policy, which uses the active voice.

In passing, here’s another issue I’ve noticed is a strange behavior when starting Root Terminal for the 2nd time from the Overview: sometimes I’m asked to authenticate again and a second window appears, sometimes the active window indicator (top bar, right of “Places”) says “Root Terminal” with a waiting cursor for a few seconds and then disappears without anything else happening. I don’t think it’s worth spending any of your time on this, but I thought I would mention it in case it rings a bell or makes you think something seriously wrong is happening.

And finally, all Root Terminal windows are labeled “Terminal”, which I find really bad for UX, but that’s the case on Tails 3.1 too so well.. I say let’s put energy towards removing the need for the Root Terminal instead of making it better.

Other than that, manual testing went fine, congrats!

#38 Updated by anonym 2017-09-13 14:52:41

• Assignee changed from anonym to intrigeri
• % Done changed from 50 to 60
• QA Check changed from Dev Needed to Ready for QA
• Feature Branch changed from feature/12738-remove-gconf to tails:feature/12738-remove-gconf greeter:feature/12738-remove-gconf

intrigeri wrote:
> Code review:
>
> * dpkg-deb --extract would nicely replace ar x + tar (let’s not rely on .deb current implementation details)

Thanks!

> * I’m surprised we need <allow_inactive>auth_admin</allow_inactive> in the Root Terminal and Tor Launcher polkit policies, but admitedly my polkit-fu is rusty.

Those were totally copy-paste mistakes. :S Thanks for noticing!

> * Wrt. commit:38cb09cc8c3a8c7eaaa61a20ef91de74768013b6, I’m surprised we still need gnome-open in the path: it’s not installed by default on Stretch anymore. We re-added it in Tails 2.0.1 to fix “URL handling at least in KeePassX, Electrum and Icedove”. But on a Debian Stretch GNOME live CD, Thunderbird, KeePassX and Electrum all open URLs just fine without gnome-open, so I’m wondering if we might be forward-porting cruft uselessly.

Ok, to me it seemed this area was still messy so I didn’t want to have to hope I tested enough of the applications we ship if I dropped it, only to have Help Desk drown in these sort of reports yet again. Any way, I tested the applications you mention, and they work fine without a gnome-open, so let’s try this!

> * Wrt. gconf settings, did you verify that everything we used to configure in there is still behaving as expected? I thought I had checked them all for obsolescence when porting to Stretch, but it might have been when porting to Jessie and I bet at least some of them are useless nowadays :)

Yes, I checked that nothing relevant is lost, or that we already are doing something we think is better via DConf. I should have included that in the commit message, sorry!

> UX regression: in greeter.git:gdm/PostLogin.default we delete gksu.desktop when no admin password was set. This was not adjusted on this branch, so “Root Terminal” is listed even when it’s not usable.

Good catch! I pushed a branch to the greeter repo! I didn’t bother building a package so I suggest you do like me, and boot with rootpw=something and test by editing in the same (few) changes to /etc/gdm3/PostLogin/Default. Ok? As long as you merge this into the greeter’s master branch all will be fine once I build a new package tomorrow.

> UX and security regression: previously, when starting Root Terminal, the authentication prompt would make it explicit (although not very user-friendly) what exactly the user was being asked to authenticate for […]

I agree, although I think the security benefits are marginal (replicating the PolicyKit password prompt with arbitrary text isn’t exactly hard).

> Thankfully it looks like intltool supports this file format, e.g. https://sources.debian.net/src/synaptic/0.84.2/po/synaptic.pot/#L3447 and https://sources.debian.net/src/synaptic/0.84.2/data/com.ubuntu.pkexec.synaptic.policy.in/ :)

I implemented translations. Seems to work!

> After looking at grep --extended-regexp '<message>' /usr/share/polkit-1/actions/org.gnome.*, I suggest using “To start a Root Terminal, you need to authenticate.” as the message: most other prompts use passive voice, which is recommended against by all the tech writing guides I’ve read, so I’ve picked the message from org.gnome.controlcenter.datetime.policy, which uses the active voice.

Ack, that wording sounds good and I went with it!

> In passing, here’s another issue I’ve noticed is a strange behavior when starting Root Terminal for the 2nd time from the Overview: sometimes I’m asked to authenticate again and a second window appears, sometimes the active window indicator (top bar, right of “Places”) says “Root Terminal” with a waiting cursor for a few seconds and then disappears without anything else happening. I don’t think it’s worth spending any of your time on this, but I thought I would mention it in case it rings a bell or makes you think something seriously wrong is happening.

This is just the way the GNOME Applications Overview works: if you type a string that matches an open application, the default will be to select the window of running instances. Possibly there’s a race between finding a match and noticing an instance is already running, so sometimes you get a new instance, sometimes an existing one, depending on your timing.

(Yup, I was also hit by this, and wondered what was going on. :))

> And finally, all Root Terminal windows are labeled “Terminal”, which I find really bad for UX, but that’s the case on Tails 3.1 too so well.. I say let’s put energy towards removing the need for the Root Terminal instead of making it better.

Yeah, I did spend a few minutes trying to fix this a week ago. I cannot recall what I tried, but I quickly gave up.

#39 Updated by anonym 2017-09-13 15:01:08

• Assignee changed from intrigeri to anonym

Wait, I need to fix a few things…

#40 Updated by anonym 2017-09-13 15:33:47

• Assignee changed from anonym to intrigeri

Done! Force-pushed!

#44 Updated by intrigeri 2017-09-13 20:05:13

• Assignee changed from intrigeri to anonym
• % Done changed from 60 to 80

Pushed fixes to your fixes.

#45 Updated by anonym 2017-09-13 22:53:01

• Status changed from In Progress to Fix committed
• Assignee deleted (anonym)
• % Done changed from 80 to 100

Thanks for fixing my failed translation attempt! It would have taken me ages to realize your solution! ♥

PS. I also did the merge in the greeter repo.

#46 Updated by intrigeri 2017-09-13 23:31:47

• Status changed from Fix committed to In Progress

Applied in changeset commit:2b770d45391588e13dc28b005c803e2a27be7c99.

#47 Updated by intrigeri 2017-09-14 05:32:47

• Assignee set to anonym
• % Done changed from 100 to 90

I see the merge in greeter.git but not in tails.git.

#49 Updated by lamby 2017-09-14 08:26:01

• Subject changed from %gconf-tree-*.xml not reproducible in some environments to Remove gconf

(editing title as it kept confusing me when seeing activity!)

#50 Updated by anonym 2017-09-14 10:15:31

• Status changed from In Progress to Fix committed
• Assignee deleted (anonym)
• % Done changed from 90 to 100
• QA Check changed from Ready for QA to Pass

intrigeri wrote:
> intrigeri wrote:
> > I see the merge in greeter.git but not in tails.git.
>
> … which might actually be good news as the corresponding automated tests fail on Jenkins. Perhaps they need to be updated?

I tried to add in some dogtailification, which apparently is more robust on my system compared to Jenkins. I reverted those parts (follow-up: Feature #14656) => merged!

#51 Updated by anonym 2017-09-15 20:50:11

• Status changed from Fix committed to In Progress

Applied in changeset commit:ce2a222041d84b0d6314d090a7b35a203bfeae82.

#52 Updated by anonym 2017-09-25 20:11:35

• Status changed from In Progress to Fix committed

#53 Updated by anonym 2017-09-28 18:49:04

• Status changed from Fix committed to Resolved