Project

General

Profile

Bug #9059

Feature #14568: Additional Software Packages

"Additional software" locks the opening of the desktop

Added by sajolida almost 3 years ago. Updated 27 days ago.

Status:
Fix committed
Priority:
Normal
Assignee:
-
Category:
Persistence
Target version:
Start date:
03/15/2015
Due date:
12/15/2017
% Done:

100%

QA Check:
Pass
Feature Branch:
bugfix/9059-additional-software-after-session greeter:bugfix/9059-additional-software-after-session
Type of work:
Code
Starter:
Affected tool:
Additional Software Packages

Description

From PostLogin.default in the Greeter, tails-additional-software install is run when the desktop is started. This can take several minutes during which the desktop is unresponsive. This should instead be done in the background.

See a patch in attachment that corrects this behavior, but I'm not knowledgeable about the GNOME internal to know if that's a good idea.

Team: alan (code), sajolida (ux), kurono (reviewer), segfault (reviewer), u (maybe)

0001-Start-tails-additional-software-install-in-the-backg.patch View (784 Bytes) sajolida, 03/15/2015 06:47 PM


Related issues

Related to Tails - Bug #11319: There should be user feedback when the session is loading Duplicate 04/04/2016
Related to Tails - Feature #9819: Check for updates of "additional software" only once Fix committed 07/29/2015
Related to Tails - Feature #14571: Code review for Implement Offline Mode Resolved 08/30/2017 01/15/2018
Blocked by Tails - Bug #15132: devel branch FTBFS since aufs-dkms 4.14 is in sid Fix committed 12/29/2017

Associated revisions

Revision 4e609909 (diff)
Added by alant 2 months ago

Install additional software after desktop is ready

Additional Software Packages installation is currently done in GDM's PostLogin,
which locks opening of the desktop. To provide better user experience, this
commit starts the installation after the desktop startup.

This is done by a systemd user unit, triggered by desktop.target that starts a
system unit of the same name, which itself calls Additional Software Packages
installation.

To ensure that the upgrade started by a NetworkManager hook is not racy with
the installation, the hook triggers a systemd system unit which is ordered
after the installation, and waits for its state file to appear.

To have all this actually stops locking opening of the desktop, it needs a
tails-greeter version that includes commit ffc6971.

Will-fix: #9059

Revision cfd9c114 (diff)
Added by alant 2 months ago

Enable the bugfix-9059-additional-software-after-session APT overlay (refs: #9059).

Revision 5c336069 (diff)
Added by alant about 2 months ago

Use systemd.path to trigger upgrade of additional software packages

Will-fix: #9059

Revision 537cc6d8 (diff)
Added by alant about 2 months ago

Manage additional software upgrade dependency at systemd unit level

Using /run/live-additional-software/activated had a couple issues:

- it was created before the additional software are actually installed,
while we want to wait until after they are installed to avoid lock
conflicts;
- it was created in code written in imperative language, which makes it
harder to reason about than if it was part of the dependency &
ordering we are otherwise managing with systemd.

Will-fix: #9059

Revision 7663481a (diff)
Added by alant about 2 months ago

Create empty live-additional-software.conf in live-persist

This way, we don't need to start python of Additional Software Packages are disabled just to create an empty file

Will-fix: #9059

Revision 644b3c27 (diff)
Added by alant about 2 months ago

Remove activation logic from tails-additional-software

Activation is now handeled by systemd.

Will-fix: #9059

Revision 5a56309f (diff)
Added by alant about 2 months ago

Remove configuration file creation from tails-additional-software

The creation of the empty file is now handled by live-persist. This prevents
launching a Python interpreter only for creating a file.

Will-fix: #9059

Revision d5a4f979 (diff)
Added by alant about 2 months ago

Notify the user about additional software installation

Will-fix: #9059

Revision ddff0657 (diff)
Added by alant about 2 months ago

Clarify additional software upgrade fail notification

Better distinguish them from install notofications and give more information.

Will-fix: #9059

Revision b811ce24 (diff)
Added by alant about 2 months ago

Fix live-additional-software file detection

Will-fix: #9059

Revision 16c6a28d (diff)
Added by alant about 1 month ago

ASP: add systemd service hardening (refs: #9059)

Revision 53bdfcdb (diff)
Added by bertagaz about 1 month ago

Test suite: add additional software packages feature.

This first iteration implements test of the automatic installation of
additional software packages when Tails is booted in offline mode.

Refs: #14572, #14570, #9059

Revision 8671c64e (diff)
Added by alant about 1 month ago

Install additional software after desktop is ready

Additional Software Packages installation is currently done in GDM's PostLogin,
which locks opening of the desktop. To provide better user experience, this
commit starts the installation after the desktop startup.

This is done by a systemd user unit, triggered by desktop.target that starts a
system unit of the same name, which itself calls Additional Software Packages
installation.

To ensure that the upgrade started by a NetworkManager hook is not racy with
the installation, the hook triggers a systemd system unit which is ordered
after the installation, and waits for its state file to appear.

To have all this actually stops locking opening of the desktop, it needs a
tails-greeter version that includes commit ffc6971.

Will-fix: #9059

Revision b86f1b84 (diff)
Added by alant about 1 month ago

Enable the bugfix-9059-additional-software-after-session APT overlay (refs: #9059).

Revision 38d78e59 (diff)
Added by alant about 1 month ago

Use systemd.path to trigger upgrade of additional software packages

Will-fix: #9059

Revision 9ad5314f (diff)
Added by alant about 1 month ago

Manage additional software upgrade dependency at systemd unit level

Using /run/live-additional-software/activated had a couple issues:

- it was created before the additional software are actually installed,
while we want to wait until after they are installed to avoid lock
conflicts;
- it was created in code written in imperative language, which makes it
harder to reason about than if it was part of the dependency &
ordering we are otherwise managing with systemd.

Will-fix: #9059

Revision fafc6230 (diff)
Added by alant about 1 month ago

Create empty live-additional-software.conf in live-persist

This way, we don't need to start python of Additional Software Packages are disabled just to create an empty file

Will-fix: #9059

Revision ffb129c0 (diff)
Added by alant about 1 month ago

Remove activation logic from tails-additional-software

Activation is now handeled by systemd.

Will-fix: #9059

Revision 58fc00a4 (diff)
Added by alant about 1 month ago

Remove configuration file creation from tails-additional-software

The creation of the empty file is now handled by live-persist. This prevents
launching a Python interpreter only for creating a file.

Will-fix: #9059

Revision d5558dc4 (diff)
Added by alant about 1 month ago

Notify the user about additional software installation

Will-fix: #9059

Revision a0e928f9 (diff)
Added by alant about 1 month ago

Clarify additional software upgrade fail notification

Better distinguish them from install notofications and give more information.

Will-fix: #9059

Revision c7a196bf (diff)
Added by alant about 1 month ago

Fix live-additional-software file detection

Will-fix: #9059

Revision 6d199212 (diff)
Added by alant about 1 month ago

ASP: add systemd service hardening (refs: #9059)

Revision c0556ee3
Added by anonym 30 days ago

Merge remote-tracking branch 'origin/bugfix/9059-additional-software-after-session' into devel

Fix-committed: #9059, #9819

History

#2 Updated by intrigeri almost 3 years ago

From PostLogin.default in the Greeter, tails-additional-software install is run when the desktop is started. This can take several minutes during which the desktop is unresponsive. This should instead be done in the background.

Two bits of historical information: the reason why we've designed this two-stages process (install before starting the session, upgrade after the network is up) is that some software needs to be installed before the session is started to be useful at all, e.g. things that wrap the call to gnome-session. It's certainly debatable whether the benefits (for very few users, likely, if any at all) are worth the annoyance (for many others), but it's worth making it clear that this proposal breaks previously supported usecases, at least in theory.

The obvious (?) solution to that would be to split the additional software config file into two, one that would list software installed before session startup, the other one would list software installed post-session-startup. I find it doubtful that it would be worth the effort.

I've no strong opinion regarding what we should do, but well, now you have more information to make up your mind :)

(Note that Alan's Redmine notifications are apparently disabled, so I'm not sure he'll see this ticket.)

#3 Updated by sajolida almost 3 years ago

Thanks for the additional information, I didn't know about that.

  • Could we come up with some examples of useful packages that would be broken by this? What could they be?

Regarding your possible solution, we could:

  • Disable pre-login by default.
  • Keep `live-additional-software.conf` as the post-login list, so that most people don't have to change anything.
  • Try to document a workaround and ask people to complain.
  • See if people complain.
  • Then implement a special pre-login list if we really feel the need either by identificating useful packages that would be broken or by having people report on issues for them.

#4 Updated by intrigeri almost 3 years ago

  • Could we come up with some examples of useful packages that would be broken by this? What could they be?

I'm too busy to add this to my plate, but at least I can help you find the answer yourself:

  • There are things that wrap the X session (e.g. all kinds of agents such as monkeysphere-validation-agent, gpg-agent, ssh-agent, some input methods and accessibility stuff). Most of the files that land into /etc/X11/Xsession.d/ are affected: http://codesearch.debian.net/results/etc%2FX11%2FXsession.d/page_0. These ones would be entirely broken if we install packages in a non-blocking way.
  • There are things that start automatically with the X session. They generally live in /etc/xdg/autostart/: http://codesearch.debian.net/results/etc%2Fxdg%2Fautostart/page_0. These ones will have surprising, racy behaviour if we install packages in a non-blocking way: sometimes the program will be automatically started, sometimes not.

Now, one should check in a real Tails the theory I'm coming up with here. It might be that the current implementation of additional software doesn't fully support one of these use cases.

Regarding your possible solution, we could:

  • Disable pre-login by default.
  • Keep `live-additional-software.conf` as the post-login list, so that most people don't have to change anything.
  • Try to document a workaround and ask people to complain.
  • See if people complain.
  • Then implement a special pre-login list if we really feel the need either by identificating useful packages that would be broken or by having people report on issues for them.

ACK. And hopefully we won't have to implement the pre-login list thingie, ever.

In any case, implementation-wise I'd like to avoid:

  • Introducing race conditions -prone behaviour, but I didn't look at the patch so I don't know if it's the case. Off the top of my head, we should install additional packages e.g. once GNOME has started.
  • Avoid triggering more "not enough memory available" errors from Tails Upgrader => likely one must make either Tails Upgrader wait for the additional software to be installed, or the opposite.

#5 Updated by sajolida almost 3 years ago

  • Assignee deleted (alant)
  • Type of work changed from Code to Research

I'm too busy to work on this actively right now, but that's a lot for all the useful info.

I'm marking this as a Research ticket. It's bothering me enough so I'll keep it in mind even if not assigned to me. I'll try to dig into codesearch but I would be very happy if someone do that faster than me.

#6 Updated by alant almost 3 years ago

sajolida wrote:

See a patch in attachment that corrects this behavior, but I'm not knowledgeable about the GNOME internal to know if that's a good idea.

This patch is likely to break any software that is autostarted or relies on session-wide environnment.

As already written above, the question here is: do we want startup speed or compatibility with more software.

#7 Updated by intrigeri almost 3 years ago

alant wrote:

As already written above, the question here is: do we want startup speed or compatibility with more software.

I'm now in favour of startup speed, especially with the careful plan sajolida described in #9059#note-3.

#8 Updated by sajolida over 2 years ago

I'm bringing in some data on this issue. These are tests I made on a X200 with Tails 1.4.1 on a fast SD card:

  • 55 seconds between boot menu and Tails Greeter
  • 16 seconds between Greeter and GNOME without additional packages
  • 63 (+47) seconds between Greeter and GNOME with only `sm` as additional package
  • 114 (+51) seconds with my list of additional packages

Having even a single additional package more than double the waiting time at boot. Most of this is probably due to unpackaging and building the APT database again. Could we do that earlier? In parallel with the Greeter? Could we store this information in persistence and save us some time then?

#9 Updated by sajolida over 2 years ago

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

We did some more testing and found out that /var/cache/apt/archives/ was stored in persistence while we could store all /var/cache/apt/ and hope for some performance improvements. Unfortunately this didn't change much and we couldn't fine any other low-hanging fruit.

Maybe installing the additional software packages could be started right after persistence is unlock, so we could gain a few seconds while the user after that might choose other options in the Greeter (like a root password).

As a next step, I'll provide with a syslog for installing a single additional package.

#10 Updated by sajolida over 2 years ago

  • Description updated (diff)
  • Target version set to 2016

#11 Updated by sajolida over 2 years ago

  • Related to Bug #9050: Persistence preset: additional software added

#12 Updated by sajolida over 2 years ago

  • Related to Feature #5996: Additional software configuration GUI added

#13 Updated by sajolida over 2 years ago

  • Related to Feature #9819: Check for updates of "additional software" only once added

#14 Updated by intrigeri over 2 years ago

Related to Feature #5996: Additional software configuration GUI added

Do you want a new "Affected tool" for this feature? It would be cleaner than added tons of "Related to" relationships.

#15 Updated by sajolida over 2 years ago

Do you want a new "Affected tool" for this feature? It would be cleaner than added tons of "Related to" relationships.

Yes please, that would be sweet :)

#16 Updated by sajolida over 2 years ago

  • Description updated (diff)

#17 Updated by intrigeri over 2 years ago

Yes please, that would be sweet :)

Done => please apply it instead of these "related to" relationships :)

#18 Updated by sajolida over 2 years ago

  • Related to deleted (Feature #5996: Additional software configuration GUI)

#19 Updated by sajolida over 2 years ago

  • Related to deleted (Feature #9819: Check for updates of "additional software" only once)

#20 Updated by sajolida over 2 years ago

  • Related to deleted (Bug #9050: Persistence preset: additional software)

#21 Updated by sajolida over 2 years ago

  • Affected tool changed from Greeter to Additional Software Packages

#22 Updated by sajolida over 2 years ago

  • Description updated (diff)

#23 Updated by Dr_Whax over 1 year ago

  • Description updated (diff)
  • Target version changed from 2016 to 2017

#24 Updated by sajolida over 1 year ago

  • Related to Bug #11319: There should be user feedback when the session is loading added

#25 Updated by BitingBird 6 months ago

  • Target version changed from 2017 to 2018

#26 Updated by BitingBird 6 months ago

  • Related to Feature #9819: Check for updates of "additional software" only once added

#28 Updated by sajolida 6 months ago

  • Assignee changed from sajolida to alant

Alan will solve this :)

#29 Updated by u 6 months ago

  • Parent task set to #14568

#30 Updated by u 6 months ago

  • Parent task changed from #14568 to #5551

#31 Updated by u 6 months ago

  • Target version changed from 2018 to Tails_3.5

Setting a target version - is this solvable while working on the Offline mode feature? Otherwise, change target version.

#32 Updated by intrigeri 6 months ago

Setting a target version - is this solvable while working on the Offline mode feature? Otherwise, change target version.

FWIW I think it can be solved independently from the offline mode.

#33 Updated by intrigeri 5 months ago

  • Parent task changed from #5551 to #14568

#34 Updated by intrigeri 5 months ago

(I don't think this has anything to do with #5551.)

#35 Updated by u 4 months ago

  • Target version changed from Tails_3.5 to Tails_3.6

#36 Updated by u 4 months ago

  • Target version changed from Tails_3.6 to Tails_3.5

#37 Updated by alant 4 months ago

  • Due date set to 12/15/2017

I plan to work on that before mid december 2017, whith the objective to sort the proposals and requirements and make a plan. If possible this will include implementing a first step to deliver as soon as possible.

#38 Updated by alant 3 months ago

I propose that :

  • the additional software installation would be started after GNOME has started, so that it doesn't block the opening of the desktop but doesn't introduce race conditions.
  • tails-security-check would wait for the additional software installation to finish, in order not to compete for RAM. Currently il already waits for Tor bootstrap, so it wouldn't make sense to make additional software wait for the security check while we are working of offline additional software.

It seems me that the easier way to implement this would be to start tails-additional-software from systemd --user, but we need to be root. Any better idea to solve this than adding sudo configuration, which looks to me like a security risk by improving attack surface of root software?

#39 Updated by intrigeri 3 months ago

It's not clear to me who you're asking. If that's me, fine; ETA?

#40 Updated by alant 2 months ago

  • QA Check changed from Info Needed to Dev Needed
  • Blueprint set to https://tails.boum.org/blueprint/additional_software_packages/dont_block_desktop_startup

#41 Updated by segfault 2 months ago

It seems me that the easier way to implement this would be to start tails-additional-software from systemd --user, but we need to be root. Any better idea to solve this than adding sudo configuration, which looks to me like a security risk by improving attack surface of root software?

How about running tails-additional-software as its own user and adding a polkit rule to allow it to use packagekit? I'm doing the same in the redesigned Tails Server. The rule could look like this:

Identity=unix-user:tails-additional-software
Action=org.freedesktop.packagekit.package-install
ResultAny=no
ResultInactive=no
ResultActive=yes

#42 Updated by intrigeri 2 months ago

segfault wrote:

How about running tails-additional-software as its own user and adding a polkit rule to allow it to use packagekit?

It would be lovely but sadly this won't work: that script needs to do other operations as root, that are not available through the PackageKit API.

#43 Updated by alant 2 months ago

  • Status changed from Confirmed to In Progress

#44 Updated by alant 2 months ago

  • Feature Branch set to bugfix/9059-additional-software-after-session
  • Type of work changed from Research to Code

#45 Updated by alant 2 months ago

Basic implementation is ready to build and test. An addition of debugging, polishing could include :

  • notifying user when the isntall is done
  • systemd unit hardening

#46 Updated by intrigeri 2 months ago

  • Type of work changed from Code to Research

Also, the handling of the flag file we rely on (/run/live-additional-software/activated) has a couple issues:

  • it's created before the ASP are installed, while we want to wait until after they are installed to avoid lock conflicts; that's why it's called "activated"; once we have a flag file that indicates what we care about here, it should be called differently
  • it's created in code written in imperative language, which makes it harder to reason about than if it was part of the dependency & ordering we are otherwise managing with systemd; I believe this can easily confuse us in a way that leads to bugs of the same kind as the one reported in the previous bullet point. So I recommend creating this flag file with ExecStartPost= instead.

I've pushed a few fixed on top of the branch. I have no other comment at this point :)

#47 Updated by alant 2 months ago

  • Type of work changed from Research to Code

intrigeri wrote:

Also, the handling of the flag file we rely on (/run/live-additional-software/activated) has a couple issues:

Current flag file is there to prevent the upgrade to be launched (and use bandwidth) if there are no additional software packages activated.

This can be replaced by a flag file managed by systemd unit if the live-additional-software.conf were created outside of tails-additional-software.

#48 Updated by intrigeri 2 months ago

  • Type of work changed from Code to Research

Dear Alan, you'll be pleased to learn that there might be a way to eliminate the dreaded while .. sleep loop: see systemd.path(5). I've not checked if it's available in Stretch, but my understanding is that it's exactly what we wanted (but we forgot to check if it existed, or perhaps I looked in the list of directives and not in the list of unit types; whatever).

#49 Updated by intrigeri 2 months ago

This can be replaced by a flag file managed by systemd unit if the live-additional-software.conf were created outside of tails-additional-software.

i.e. in live-persist, which would be more consistent with how we're initializing the persistent volume :)

#50 Updated by intrigeri 2 months ago

  • % Done changed from 0 to 10

#51 Updated by alant about 2 months ago

Current ISO has not the version of tails-greeter prepared for this branch. As a result, tails-additional-software is still launched in PostLogin. I'll prepare a new ISO and test again.

#52 Updated by alant about 2 months ago

  • Blocked by Bug #15132: devel branch FTBFS since aufs-dkms 4.14 is in sid added

#53 Updated by intrigeri about 2 months ago

Blocked by Bug #15132: devel branch FTBFS since aufs-dkms 4.14 is in sid added

I suggest you merge the branch for #15132 into yours, so your work is not blocked (only merging will actually be blocked :)

#54 Updated by alant about 2 months ago

Basic functionnality works as expected. I'll investigate the flag file and if possible user notifications and hardening .

#55 Updated by alant about 2 months ago

  • % Done changed from 10 to 50
  • Type of work changed from Research to Code

I implemented:

  • systemd path activation
  • handeling flag file with ExecStartPost
  • handeling creation of empty live-additional-software.conf in live-persist
  • user notifications

I'm gonna build and test that. Remaining to do is systemd unit hardening.

#56 Updated by alant about 2 months ago

  • % Done changed from 50 to 60

I just pushed a few fixes, it should be working now.

#57 Updated by alant about 1 month ago

alant wrote:

Remaining to do is systemd unit hardening.

Done in commit 16c6a28. I'm not fully convinced as the complexity seems me error-prone :

PrivateDevices=yes
PrivateTmp=yes
# Capabilities needed by tails-additional-software
CapabilityBoundingSet=CAP_DAC_READ_SEARCH
# Capabilities needed by apt/dpkg
CapabilityBoundingSet=CAP_CHOWN CAP_DAC_OVERRIDE CAP_FOWNER CAP_FSETID
CapabilityBoundingSet=CAP_SETGID CAP_SETUID
ProtectSystem=no
# Capabilities needed by tails-notify-user
CapabilityBoundingSet=CAP_SYS_PTRACE CAP_AUDIT_WRITE CAP_SYS_RESOURCE
ProtectHome=no

Gonna build and test this and see what happens.

#58 Updated by alant about 1 month ago

  • Assignee changed from alant to segfault
  • % Done changed from 60 to 70
  • QA Check changed from Dev Needed to Ready for QA

It works fine for me. Please review and merge if you're fine with it.

I'm not really convinced about the capability listing in commit 16c6a28. I'm afraid it may be fragile and I'm not sure we gain a lot form it. What do you think?

#59 Updated by alant about 1 month ago

  • Related to Feature #14571: Code review for Implement Offline Mode added

#60 Updated by intrigeri about 1 month ago

Please review and merge if you're fine with it.

s/and merge/and reassign to anonym for another quick review & merging/

#61 Updated by segfault about 1 month ago

  • Assignee changed from segfault to alant
  • QA Check changed from Ready for QA to Info Needed

It works fine for me. Please review and merge if you're fine with it.

Looks good to me! Just a few comments:

1. During my test, tails-additional-software-install.service failed, because APT didn't have any package lists and therefore failed to install any packages. After running apt update, it worked as expected. I don't know if this is a regression, I guess this is also the case in the current version (I didn't check), but I think it should be fixed anyway. A fix could be to run apt update if the installation fails and run it again afterwards.

2. From commit 4e609909:

+# XXX:Buster this sudo rule should be replaced by a polkit rule once we have
+# policykit >= 0.106. The rule is already in
+# [[blueprint/additional_software_packages/org.boum.tails.additional-software.rules]]
+# and should be installed in /usr/share/polkit-1/rules.d/

I doubt that we will ever have polkit >= 0.106. polkit 0.106 has been release in 2012, and it seems like Debian does not want to move from the old .pkla to the new .rules format (I looked everywhere, but couldn't find any discussion or reasoning for this. I suspect they don't want to break compatibility with custom .pkla rules.). Unfortunately, we can't specify the "unit" and "verb" in .pkla format. So we could either keep the sudoers solution, or run tails-additional-software as its own user and allow it to manage all systemd services.

3. This one is nitpicking, but it made it harder for me to understand the option: In system/tails-additional-software-install.service, user/tails-additional-software-install.service, and tails-additional-software-upgrade.service, you set this TimeoutStartSec=0. According to the manual, it should be TimeoutStartSec=infinity (using 0 instead seems to work for compatibility reasons), which is also the default value for oneshot type services, so it could just be removed.

4. From commit 6e649379:

+        subprocess.check_output([cmd, title, body], stderr=subprocess.STDOUT)

subprocess.check_call() should be used instead of check_output if the output is not used.

5. I think the user visible strings can be improved. I would recommend:

s/Your additional software installation failed/The additional software installation failed/
s/The installation failed. Please check your additional software configuration, or read the system log to understand better the problem./The additional software installation failed. Please check the additional software configuration and the system log (journalctl) for errors./
s/Your additional software are installed/Additional software installed successfully/
s/Your additional software are ready to use./The additional software was installed successfully and is ready to use./
s/Your additional software upgrade failed/Failed to update the additional software/
s/The check for upgrades failed. This might be due to a network problem. Please check your network connection, try to restart Tails, or read the system log to understand better the problem./Updating the additional software failed. Please check your network connection, check the system log (journalctl) for errors, or try restarting Tails./
s/Your additional software are up to date/Additional software successfully updated/
s/The upgrade was successful./The additional software was successfully updated./

But I'm sure this could still be improved by someone more into technical writing.

6.

I'm not really convinced about the capability listing in commit 16c6a28. I'm afraid it may be fragile and I'm not sure we gain a lot form it. What do you think?

To be able to review these capabilities, I would like to know how you got this list. Trial and error? Might there be capabilities missing which might for example only be required for some packages? One solution to get rid of at least the capabilities for APT/DPKG would be to use PackageKit for package installation. I think we could use this code based on what I use in onionkit (formerly known as Tails Server):

# Based on isenkram, copyright (C) 2013,2015-2016 Petter Reinholdtsen <pere@debian.org>

from typing import List
from pydbus import SystemBus

import gi
from gi.repository import GLib
gi.require_version('PackageKitGlib', '1.0')
from gi.repository import PackageKitGlib as packagekit

class PackageKitManager(object):
    """Helper to install and uninstall packages using PackageKit.
    Reference for PackageKit DBus API: https://www.freedesktop.org/software/PackageKit/gtk-doc/api-reference.html""" 
    def __init__(self):        
        self.client = SystemBus().get("org.freedesktop.PackageKit").Client()
        self.client.set_interactive(False)

    def get_package_ids(self, package_names: List[str], package_filter: int) -> List[object]:
        results = self.client.resolve(
            package_filter,  # package filter flags (for example to resolve only installed packages)
            tuple(package_names),  # names of packages to resolve
            None,  # cancellable
            self.progress_callback,  # progress callback
            None  # progress callback user data
        )
        self.assert_success(results)

        packages_resolved = {package.get_name(): package for package in results.get_package_array()}
        for package_name in package_names:
            if package_name not in packages_resolved or not packages_resolved[package_name]:
                raise PackageException("Package %r not found" % package_name)

        return [packages_resolved[package_name].get_id() for package_name in package_names]

    def refresh_cache(self):
        """Refresh package cache from all enabled repositories""" 
        results = self.client.refresh_cache(
            False,  # don't enforce reloading if there is valid, up to date data
            None,  # cancellable
            self.progress_callback,  # progress callback
            None  # progress callback user data
        )
        self.assert_success(results)

    def install(self, package_names: List[str]):
        """Run a PackageKit transaction to install given packages.""" 
        package_ids = self.get_package_ids(package_names, package_filter=0)

        # Start package installation
        results = self.client.install_packages(
            1 << packagekit.TransactionFlagEnum.ONLY_TRUSTED,  # only install signed packages
            package_ids,  # packages to install
            None,  # cancellable
            self.progress_callback,  # progress callback
            None  # progress callback user data
        )
        self.assert_success(results)

    @staticmethod
    def assert_success(results):
        """Check that the most recent operation was a success.""" 
        if results and results.get_error_code() is not None:
            error = results.get_error_code()
            error_code = error.get_code() if error else None
            error_string = packagekit.ErrorEnum.to_string(error_code) if error_code else None
            error_details = error.get_details() if error else None
            raise PackageException(error_string, error_details)

Then we would have to execute tails-additional-software as its own user and use a polkit rule to allow it to use PackageKit:

# /etc/polkit-1/localauthority/10-vendor.d/org.boum.tails.additional-software.pkla
[Allow tails-additional-software to install packages]
Identity=unix-user:tails-additional-software
Action=org.freedesktop.packagekit.package-install
ResultAny=no
ResultInactive=no  # or maybe this required for systemd services?
ResultActive=yes

#62 Updated by intrigeri about 1 month ago

I doubt that we will ever have polkit >= 0.106. polkit 0.106 has been release in 2012, and it seems like Debian does not want to move from the old .pkla to the new .rules format (I looked everywhere,

At least I see lots of work on packaging newer release in Debian experimental in the last few years. So there's definitely some interest.
Perhaps one of you should ask them what's blocking? :)

but couldn't find any discussion or reasoning for this.

FYI the Debian+Ubuntu GNOME team mostly communicates on IRC. I'm not a fan, but that's how it is.

#63 Updated by alant about 1 month ago

  • Assignee changed from alant to segfault

segfault wrote:

1. During my test, tails-additional-software-install.service failed, because APT didn't have any package lists and therefore failed to install any packages. After running apt update, it worked as expected. I don't know if this is a regression, I guess this is also the case in the current version (I didn't check), but I think it should be fixed anyway. A fix could be to run apt update if the installation fails and run it again afterwards.

Did you actually installed the packages manually before configuring it as an additional software? That is the only way it is working, and that is what the GUI to be designed will do. As tails-additional-software-install.service should work offline, it can't rely on apt update. This is tails-additional-software-upgrade.service role.

Anyway I think this is no regression and doesn't need to be fixed.

2. From commit 4e609909:
[...]

I doubt that we will ever have polkit >= 0.106. polkit 0.106 has been release in 2012, and it seems like Debian does not want to move from the old .pkla to the new .rules format (I looked everywhere, but couldn't find any discussion or reasoning for this. I suspect they don't want to break compatibility with custom .pkla rules.). Unfortunately, we can't specify the "unit" and "verb" in .pkla format. So we could either keep the sudoers solution, or run tails-additional-software as its own user and allow it to manage all systemd services.

I'm in favor of keeping the sudo solution for now. The service you speak about is a systemd --user one as it relies on the session being launched, so it must run as amnesia.

3. This one is nitpicking, but it made it harder for me to understand the option: In system/tails-additional-software-install.service, user/tails-additional-software-install.service, and tails-additional-software-upgrade.service, you set this TimeoutStartSec=0. According to the manual, it should be TimeoutStartSec=infinity (using 0 instead seems to work for compatibility reasons), which is also the default value for oneshot type services, so it could just be removed.

OK, replaced it by infinity

4. From commit 6e649379:
[...]

subprocess.check_call() should be used instead of check_output if the output is not used.

0K.

5. I think the user visible strings can be improved. I would recommend:
[...]

But I'm sure this could still be improved by someone more into technical writing.

I'd like to wait UX input on that, especially as some of these string have been written by sajolida. I sent an email to tails-ux mailing list with your proposals.

6.

I'm not really convinced about the capability listing in commit 16c6a28. I'm afraid it may be fragile and I'm not sure we gain a lot form it. What do you think?

To be able to review these capabilities, I would like to know how you got this list. Trial and error?

Yes, trial and error.

Might there be capabilities missing which might for example only be required for some packages?

Maybe. What cases are you thinking about?

One solution to get rid of at least the capabilities for APT/DPKG would be to use PackageKit for package installation. I think we could use this code based on what I use in onionkit (formerly known as Tails Server):

I don't think we should do that as part of this task, because this is completly rewriting tails-additional-software and I don't think it is time to do that. We'd need to think more about it, and if we end up deciding it's a good idea, find time to do it properly.

#64 Updated by segfault about 1 month ago

  • Assignee changed from segfault to anonym
  • QA Check changed from Info Needed to Ready for QA

alant wrote:

segfault wrote:

1. During my test, tails-additional-software-install.service failed, because APT didn't have any package lists and therefore failed to install any packages. After running apt update, it worked as expected. I don't know if this is a regression, I guess this is also the case in the current version (I didn't check), but I think it should be fixed anyway. A fix could be to run apt update if the installation fails and run it again afterwards.

Did you actually installed the packages manually before configuring it as an additional software? That is the only way it is working, and that is what the GUI to be designed will do.

I didn't look at what the GUI will do, but currently this is evidently not the only way it is working, because it also worked for me after running apt update, without installing the packages manually.

As tails-additional-software-install.service should work offline, it can't rely on apt update. This is tails-additional-software-upgrade.service role.

What I proposed is a fallback for when the installation cannot be performed offline.

Anyway I think this is no regression and doesn't need to be fixed.

Ok, maybe not in context of this ticket. But, from my point of view, this is an easily fixable issue, which, at least with the current workflow, users probably run into.

2. From commit 4e609909:
[...]

I doubt that we will ever have polkit >= 0.106. polkit 0.106 has been release in 2012, and it seems like Debian does not want to move from the old .pkla to the new .rules format (I looked everywhere, but couldn't find any discussion or reasoning for this. I suspect they don't want to break compatibility with custom .pkla rules.). Unfortunately, we can't specify the "unit" and "verb" in .pkla format. So we could either keep the sudoers solution, or run tails-additional-software as its own user and allow it to manage all systemd services.

I'm in favor of keeping the sudo solution for now. The service you speak about is a systemd --user one as it relies on the session being launched, so it must run as amnesia.

Well, the process could still run as another user, if we allow amnesia to run the executable as another user. But I'm fine with keeping the sudoers solution.

5. I think the user visible strings can be improved. I would recommend:
[...]

But I'm sure this could still be improved by someone more into technical writing.

I'd like to wait UX input on that, especially as some of these string have been written by sajolida. I sent an email to tails-ux mailing list with your proposals.

Ok.

6.

I'm not really convinced about the capability listing in commit 16c6a28. I'm afraid it may be fragile and I'm not sure we gain a lot form it. What do you think?

To be able to review these capabilities, I would like to know how you got this list. Trial and error?

Yes, trial and error.

Ok. I would like to not redo this work and trust you on this, because it seems effortful and I already spent more time on this review than I can clock.

Might there be capabilities missing which might for example only be required for some packages?

Maybe. What cases are you thinking about?

No specific case in mind, but I think that packages do different things in their installation scripts, and by testing only some packages, we could miss capabilities required by other packages.

One solution to get rid of at least the capabilities for APT/DPKG would be to use PackageKit for package installation. I think we could use this code based on what I use in onionkit (formerly known as Tails Server):

I don't think we should do that as part of this task, because this is completly rewriting tails-additional-software and I don't think it is time to do that. We'd need to think more about it, and if we end up deciding it's a good idea, find time to do it properly.

Ok.

Assigning to anonym now.

#65 Updated by intrigeri about 1 month ago

Anyway I think this is no regression and doesn't need to be fixed.

Ok, maybe not in context of this ticket. But, from my point of view, this is an easily fixable issue, which, at least with the current workflow, users probably run into.

I think this idea would be worth a dedicated ticket.

#66 Updated by anonym about 1 month ago

This branch is based on devel. I'll try to rebase it on stable...

#67 Updated by anonym about 1 month ago

  • Feature Branch changed from bugfix/9059-additional-software-after-session to bugfix/9059-additional-software-after-session OR bugfix/9059-additional-software-after-session-rebased-on-stable
git checkout -b bugfix/9059-additional-software-after-session-rebased-on-stable origin/bugfix/9059-additional-software-after-session
git rebase --onto origin/stable origin/devel
git push -u origin bugfix/9059-additional-software-after-session-rebased-on-stable

#68 Updated by anonym about 1 month ago

  • Target version changed from Tails_3.5 to Tails_3.6

#69 Updated by anonym 30 days ago

  • Feature Branch changed from bugfix/9059-additional-software-after-session OR bugfix/9059-additional-software-after-session-rebased-on-stable to bugfix/9059-additional-software-after-session

#70 Updated by alant 30 days ago

anonym wrote:

Target version changed from Tails_3.5 to Tails_3.6

Was there an issue preventing the merge?

#71 Updated by anonym 30 days ago

alant wrote:

anonym wrote:

Target version changed from Tails_3.5 to Tails_3.6

Was there an issue preventing the merge?

No, it just looked like a too big change for a bugfix release. I mean, we have lived with these short-comings/bugs for years, users can wait another six weeks. :) That way this will get some exposure to our testers, in Tails 3.6~rc1.

Meanwhile I wasted an hour investigating what I thought was a bug, but then turned out to be that the branch at the moment builds with Tails Greeter 1.0.4, so APS is started in the Greeter's PostLogin hook... grr...

#72 Updated by anonym 29 days ago

  • Status changed from In Progress to Fix committed
  • Assignee deleted (anonym)
  • % Done changed from 70 to 100
  • QA Check changed from Ready for QA to Pass
  • Feature Branch changed from bugfix/9059-additional-software-after-session to bugfix/9059-additional-software-after-session greeter:bugfix/9059-additional-software-after-session

anonym wrote:

Meanwhile I wasted an hour investigating what I thought was a bug, but then turned out to be that the branch at the moment builds with Tails Greeter 1.0.4, so APS is started in the Greeter's PostLogin hook... grr...

I built an updated package and uploaded it. Testing went super fine, and the code looks good. I fixed my only complaint myself (031a66becf84ee85e5e408b8c7f7a57311b3337a). Merged!

#73 Updated by alant 27 days ago

anonym wrote:

anonym wrote:

Meanwhile I wasted an hour investigating what I thought was a bug, but then turned out to be that the branch at the moment builds with Tails Greeter 1.0.4, so APS is started in the Greeter's PostLogin hook... grr...

Yes, it needs its own greeter.

I built an updated package and uploaded it. Testing went super fine, and the code looks good. I fixed my only complaint myself (031a66becf84ee85e5e408b8c7f7a57311b3337a). Merged!

Sorry, I used python style without even thinking, and it looked good im my editor.

Also available in: Atom PDF