Feature #14598

Code review for Additional Software packages GUI

Added by Anonymous 2017-09-04 17:17:34 . Updated 2018-11-02 13:19:20 .

Status:
Resolved
Priority:
Elevated
Assignee:
Category:
Target version:
Start date:
2017-09-04
Due date:
2018-04-15
% Done:

100%

Feature Branch:
bugfix/14598-asp-review-bugs
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Additional Software Packages
Deliverable for:
299

Description


Subtasks


Related issues

Blocked by Tails - Bug #15838: Fix non-blocking issues identified during ASP code review Resolved 2018-09-26

History

#1 Updated by Anonymous 2017-09-04 17:18:07

  • blocked by Feature #14596: Write automated tests for Additional Software GUI added

#3 Updated by Anonymous 2017-09-04 17:19:03

  • Description updated

#4 Updated by Anonymous 2017-09-07 08:38:18

  • Description updated
  • Due date set to 2018-04-15
  • Target version changed from 2018 to Tails_3.7

#5 Updated by intrigeri 2018-03-06 14:28:45

You can start reviewing my bits (Feature #14595). IMO you don’t need to wait for the blocking ticket (automated tests) for a static code review:

I know it’s Perl (and even Modern Perl with a full-blown OOP system) which is not your strongest skill. But you’re the reviewer for this project and we have nobody else on the team who’s learnt enough Perl to feel more comfortable doing code reviews so well: let’s say the goal of this review is to ensure I did not sneak in any semi-obvious security hole and that my commits seem to do what their commit message pretends; I doubt we can realistically set the bar much higher.

For now all these changes are imported into tails.git (feature/14594-asp-gui branch) as patches for easier testing: config/chroot_local-patches/*-14594-asp-gui.diff. IIRC there are a few other corresponding changes in that branch but they’re mostly trivial, or temporary hacks until the topic branches get merged into these 3 repos, or in the test suite i.e. outside of your realm. So for now I think you can ignore that topic branch in tails.git, until Alan lets you know that his work (that’s also in that topic branch) is ready for your code review.

If you have questions, you know where you can find me :)

#6 Updated by segfault 2018-03-10 10:21:20

intrigeri wrote:
> You can start reviewing my bits (Feature #14595). IMO you don’t need to wait for the blocking ticket (automated tests) for a static code review:
>
> * the bulk of the work is in https://git-tails.immerda.ch/persistence-setup/log/?h=feature/14594-asp-gui
> * the shared library had to be adjusted a bit, mostly to follow persistence-setup’s migration from the Moose OO system to the lighter Moo one: https://git-tails.immerda.ch/perl5lib/log/?h=feature/14594-asp-gui
> * the upgrader had to be adjusted to changes in the shared library and while I was at it I’ve dropped code that is now available in our perl5lib: https://git-tails.immerda.ch/iuk/log/?h=feature/14594-asp-gui
>
> I know it’s Perl (and even Modern Perl with a full-blown OOP system) which is not your strongest skill.

This is an understatement, I’ve never done anything with Perl and always turned away in horror when I saw Perl code somewhere.

> But you’re the reviewer for this project and we have nobody else on the team who’s learnt enough Perl to feel more comfortable doing code reviews so well:

I was surprised when I read this, because I was sure that I said that I would not like to review Perl code when you asked me if I could review this project. I now understand that this is the part I took over from u when she asked me in January - I didn’t realize back then that this would be in Perl.

> let’s say the goal of this review is to ensure I did not sneak in any semi-obvious security hole and that my commits seem to do what their commit message pretends; I doubt we can realistically set the bar much higher.

I will try to do the best I can do within the 8 hours I have for reviewing this.

> For now all these changes are imported into tails.git (feature/14594-asp-gui branch) as patches for easier testing: config/chroot_local-patches/*-14594-asp-gui.diff. IIRC there are a few other corresponding changes in that branch but they’re mostly trivial, or temporary hacks until the topic branches get merged into these 3 repos, or in the test suite i.e. outside of your realm. So for now I think you can ignore that topic branch in tails.git, until Alan lets you know that his work (that’s also in that topic branch) is ready for your code review.

Ok, so I will completely ignore tails.git for this code review and only review the three repositories you listed above, right?

#7 Updated by alant 2018-03-13 13:52:07

You may want to have start looking at the code in tails:feature/14594-asp-gui in a few days. I didn’t test last changes (I’m waiting for a new ISO) and there are a few details remaining, but I expect the diff to be short and would love to have feedback on the work already done.

#8 Updated by intrigeri 2018-03-19 11:58:46

>> But you’re the reviewer for this project and we have nobody else on the team who’s learnt enough Perl to feel more comfortable doing code reviews so well:

> I was surprised when I read this, because I was sure that I said that I would not like to review Perl code when you asked me if I could review this project.

I think you’re right :)

> I now understand that this is the part I took over from u when she asked me in January

I doubt it.

> I will try to do the best I can do within the 8 hours I have for reviewing this.

Please focus on Alan’s bits first: I think your time+energy will be more useful there.
And if you have time left then you can take a look at the Perl components.

>> For now all these changes are imported into tails.git (feature/14594-asp-gui branch) as patches for easier testing: config/chroot_local-patches/*-14594-asp-gui.diff. IIRC there are a few other corresponding changes in that branch but they’re mostly trivial, or temporary hacks until the topic branches get merged into these 3 repos, or in the test suite i.e. outside of your realm. So for now I think you can ignore that topic branch in tails.git, until Alan lets you know that his work (that’s also in that topic branch) is ready for your code review.

> Ok, so I will completely ignore tails.git for this code review and only review the three repositories you listed above, right?

Yes, except since then Alan requested a review of his bits too (tails.git + pythonlib).

#9 Updated by segfault 2018-03-21 07:32:39

alant wrote:
> You may want to have start looking at the code in tails:feature/14594-asp-gui in a few days. I didn’t test last changes (I’m waiting for a new ISO) and there are a few details remaining, but I expect the diff to be short and would love to have feedback on the work already done.

Whoops, totally missed this, I’m so sorry! I’m super busy this week, but will try to do it on the weekend.

#10 Updated by alant 2018-03-22 23:35:01

segfault wrote:
> Whoops, totally missed this, I’m so sorry! I’m super busy this week, but will try to do it on the weekend.

Thanks! I plan to work on this on sunday, so I’ll be available for questions on XMPP.

#11 Updated by segfault 2018-03-29 23:49:20

  • Assignee changed from segfault to alant

Here is my review of branch feature/14594-asp-gui, up to commit ed9795e3bb288b0ea8c7206ad25e93cfe660c526. Note that I didn’t review each commit on its own, but the whole diff between this branch and devel, minus the changes in po/, wiki/, features/, and config/chroot_local-patches/*-14594-asp-gui.diff.

+++ b/config/chroot_local-includes/lib/systemd/system/tails-synchronize-data-to-new-persistent-volume.service
@@ -0,0 +1,19 @@
+[Unit]
+Description=Synchronize data to newly created persistent volume
+
+[Service]
+RemainAfterExit=yes
+Type=oneshot
+ExecStop=/bin/sh -c '                                                        \
+    if mountpoint -q /media/tails-persistence-setup/TailsData                \
+            && test ! -d /media/tails-persistence-setup/TailsData/apt; then  \
+        echo "Copy APT data to newly created persistent volume";             \
+        mkdir /media/tails-persistence-setup/TailsData/apt/ &&               \
+        cp -a /var/cache/apt/archives                                        \
+                   /media/tails-persistence-setup/TailsData/apt/cache;       \
+        cp -a /var/lib/apt/lists                                             \
+                   /media/tails-persistence-setup/TailsData/apt/;            \
+    fi'
+
+[Install]
+WantedBy=multi-user.target

I think it should be made clear - at least in the description, and IMO also in the unit name - that the data is synchronized on shutdown, and not when the service is started.

Btw, I couldn’t find the code where /media/tails-persistence-setup/TailsData is created and mounted. Can you point me to that?

+++ b/config/chroot_local-includes/usr/local/bin/tails-additional-software-config
@@ -0,0 +1,170 @@
[...]
+    def get_asp_configuration(self):
+        try:
+            additional_packages = get_additional_packages()
+        except OSError:
+            # XXX: notify
+            pass
+        apt_cache = apt.cache.Cache()
+
+        packages_with_description = []
+        for package in sorted(additional_packages):

The additional_packages variable might be referenced before assignment here. Maybe set it to an empty list when an exception is caught, or simply return?

+++ b/config/chroot_local-includes/usr/local/lib/tails-additional-software-notify
@@ -0,0 +1,98 @@
[...]
+        if not (accept_label or deny_label or documentation_target):
+            sys.exit(3)
[...]
+        "Returns: 0 if the button with <accept_label> is selected\n"
+        "         2 if the number of arguments is wrong\n"
+        "         3 if the button with <deny_label> is selected\n"

I don’t think the exit code for “the user clicked deny” should be returned in the case of “there are no buttons to be shown (which shouldn’t happen because we checked the number of arguments before)”. I think raising an exception with a descriptive error message would be more sensible in that case.

+++ b/config/chroot_local-includes/usr/local/sbin/live-persist
@@ -241,9 +241,9 @@ other::r-x"
+       if ! persistence_conf_file_has_correct_access_rights "$f" "644"

I think it would be better if the config file was not world-readable. Have you considered creating a tails-additional-software user/group and only allowing this user/group access to the config file?

+++ b/tailslib/additionalsoftware/config.py
@@ -0,0 +1,110 @@
+def get_persistence_path(search_new_persistence=False):
+    """Return the path of the (newly created) persistence.
+
+    Return PERSISTENCE_DIR if it exists.  If search_new_persistence is True,
+    also try NEWLY_CREATED_PERSISTENCE_DIR.
+
+    If no persistence directory exists, raise FileNotFoundError.
+    """
+    if os.path.isdir(PERSISTENCE_DIR):
+        return PERSISTENCE_DIR
+    elif (search_new_persistence
+          and os.path.isdir(NEWLY_CREATED_PERSISTENCE_DIR)):
+        return NEWLY_CREATED_PERSISTENCE_DIR
+    else:
+        raise FileNotFoundError(
+            "No persistence directory found. Neither {dir} not {alt_dir} "
+            "exist.".format(dir=PERSISTENCE_DIR,
+                            alt_dir=NEWLY_CREATED_PERSISTENCE_DIR))

The error message says that alt_dir doesn’t exist, but it might not have been checked.

+++ b/config/chroot_local-includes/usr/local/sbin/tails-additional-software
@@ -1,18 +1,32 @@
+def _spawn_daemon(func):
+    """Spawn func after double-forking.
+
+    Do the UNIX double-fork magic, see Stevens' "Advanced
+    Programming in the UNIX Environment" for details (ISBN 0201563177).
+
+    From https://stackoverflow.com/questions/6011235/run-a-program-from-
+    python-and-have-it-continue-to-run-after-the-script-is-kille
+    """
+    try:
+        pid = os.fork()
+        if pid > 0:
+            # parent process, return and keep running
+            return
+    except OSError as e:
+        syslog.syslog("fork #1 failed: %d (%s)" % (e.errno, e.strerror))
+        sys.exit(1)
+
+    os.setsid()
+
+    # do second fork
+    try:
+        pid = os.fork()
+        if pid > 0:
+            # exit from second parent
+            sys.exit(0)
+    except OSError as e:
+        syslog.syslog("fork #2 failed: %d (%s)" % (e.errno, e.strerror))
+        sys.exit(1)
+
+    # do stuff
+    func()

I read the comments on Bug #15382, but I don’t get why a double fork is needed here and a simple subprocess.Popen() wouldn’t suffice.

These are the only issues I could find, it’s nice code! I also learned about some new GTK classes which I’m now going to use in my GTK projects, yay! :)

Just as a sidenote: I find the type hints introduced in Python 3.5 (and 3.6) super helpful (at least when using an IDE which recognizes them), and try to use them in all new code I write - but that’s a personal preference I guess.

#12 Updated by alant 2018-04-05 17:42:57

  • % Done changed from 0 to 20

segfault wrote:
> Here is my review of branch feature/14594-asp-gui, up to commit ed9795e3bb288b0ea8c7206ad25e93cfe660c526.

> I think it should be made clear - at least in the description, and IMO also in the unit name - that the data is synchronized on shutdown, and not when the service is started.
>
Done in commit 8b3c48feb5

> Btw, I couldn’t find the code where /media/tails-persistence-setup/TailsData is created and mounted. Can you point me to that?
>
mount_persistence_partition in persistence-setup:lib/Tails/Persistence/Setup.pm.

> The additional_packages variable might be referenced before assignment here. Maybe set it to an empty list when an exception is caught, or simply return?
>
Fixed in b38d913811 and ddb65fb506

> I don’t think the exit code for “the user clicked deny” should be returned in the case of “there are no buttons to be shown (which shouldn’t happen because we checked the number of arguments before)”. I think raising an exception with a descriptive error message would be more sensible in that case.
>
We can’t really raise an exception because that’s run through sudo. I fixed the exit code in commit 7bb0b09196.

> I think it would be better if the config file was not world-readable. Have you considered creating a tails-additional-software user/group and only allowing this user/group access to the config file?
>
The config file should be readable by amnesia for the configuration window to read it. And the installed packages can be get using apt-cache by an unprivileged user, so I don’t see the point of making this file not world-readable. It could however be tails-persistence-setup/anmesia if that makes real sense.

> The error message says that alt_dir doesn’t exist, but it might not have been checked.
>
Fixed in tailslib commit 9ca838b.

> I read the comments on Bug #15382, but I don’t get why a double fork is needed here and a simple subprocess.Popen() wouldn’t suffice.
>
I tried and it doesn’t suffice. “The first process in the process group becomes the process group leader and the first process in the session becomes the session leader. Every session can have one TTY associated with it. Only a session leader can take control of a TTY. For a process to be truly daemonized (ran in the background) we should ensure that the session leader is killed so that there is no possibility of the session ever taking control of the TTY. ” (https://stackoverflow.com/questions/881388/what-is-the-reason-for-performing-a-double-fork-when-creating-a-daemon)

> Just as a sidenote: I find the type hints introduced in Python 3.5 (and 3.6) super helpful (at least when using an IDE which recognizes them), and try to use them in all new code I write - but that’s a personal preference I guess.

I didn’t read about that, thanks for the hint!

#13 Updated by alant 2018-04-05 21:41:01

  • Status changed from Confirmed to In Progress

Applied in changeset commit:8b3c48feb57f01014c9ba2795d499cbb2b70a141.

#14 Updated by segfault 2018-04-13 09:51:01

alant wrote:
> segfault wrote:
> > I think it should be made clear - at least in the description, and IMO also in the unit name - that the data is synchronized on shutdown, and not when the service is started.
> >
> Done in commit 8b3c48feb5

Looks good.

> > Btw, I couldn’t find the code where /media/tails-persistence-setup/TailsData is created and mounted. Can you point me to that?
> >
> mount_persistence_partition in persistence-setup:lib/Tails/Persistence/Setup.pm.

Ah right. I didn’t review that, because it’s perl.

> > The additional_packages variable might be referenced before assignment here. Maybe set it to an empty list when an exception is caught, or simply return?
> >
> Fixed in b38d913811 and ddb65fb506

Same issue: Now the packages variable in update_packages_list() might be referenced before assignment, so in case of an error, a NameError will be raised in the if packages: line, and I don’t think that’s what you want.

> > I don’t think the exit code for “the user clicked deny” should be returned in the case of “there are no buttons to be shown (which shouldn’t happen because we checked the number of arguments before)”. I think raising an exception with a descriptive error message would be more sensible in that case.
> >
> We can’t really raise an exception because that’s run through sudo.

Ah, I see, you want to be able to distinguish if sudo failed or the program failed. I don’t see a better solution, so I’m ok with that.

> I fixed the exit code in commit 7bb0b09196.

Looks good.

> > I think it would be better if the config file was not world-readable. Have you considered creating a tails-additional-software user/group and only allowing this user/group access to the config file?
> >
> The config file should be readable by amnesia for the configuration window to read it. And the installed packages can be get using apt-cache by an unprivileged user, so I don’t see the point of making this file not world-readable. It could however be tails-persistence-setup/anmesia if that makes real sense.

I think it would be better to change it to tails-persistence-setup:amnesia. I understand the argument that the same information can be retrieved through apt-cache, but I think - as a general security practice - access should be restricted as much as possible, and it wouldn’t be much effort in this case, right?

> > The error message says that alt_dir doesn’t exist, but it might not have been checked.
> >
> Fixed in tailslib commit 9ca838b.

Looks good.

> > I read the comments on Bug #15382, but I don’t get why a double fork is needed here and a simple subprocess.Popen() wouldn’t suffice.
> >
> I tried and it doesn’t suffice. “The first process in the process group becomes the process group leader and the first process in the session becomes the session leader. Every session can have one TTY associated with it. Only a session leader can take control of a TTY. For a process to be truly daemonized (ran in the background) we should ensure that the session leader is killed so that there is no possibility of the session ever taking control of the TTY. ” (https://stackoverflow.com/questions/881388/what-is-the-reason-for-performing-a-double-fork-when-creating-a-daemon)

I don’t understand why it would be a problem if the daemon would take control of a TTY - at least that shouldn’t cause the parent to block, which was the issue in Bug #15382. But since you say that you tested it with Popen and - for reasons unknown to me - it didn’t work, I don’t insist on spending more time to investigate this further. But if you already understand it, please try to explain it to me.

Please reassign this ticket to me when you want me to review the remaining commits (preferably when your work on this is finished, so that I won’t have to do another review after that).

#15 Updated by alant 2018-04-28 18:02:55

segfault wrote:
> alant wrote:
> > segfault wrote:
> > > I think it would be better if the config file was not world-readable. Have you considered creating a tails-additional-software user/group and only allowing this user/group access to the config file?
> > >
> > The config file should be readable by amnesia for the configuration window to read it. And the installed packages can be get using apt-cache by an unprivileged user, so I don’t see the point of making this file not world-readable. It could however be tails-persistence-setup/anmesia if that makes real sense.
>
> I think it would be better to change it to tails-persistence-setup:amnesia. I understand the argument that the same information can be retrieved through apt-cache, but I think - as a general security practice - access should be restricted as much as possible, and it wouldn’t be much effort in this case, right?

I tried to do that, but it’s a lot more complicated than it seems. Let’s see if it works in a build.

> > > I read the comments on Bug #15382, but I don’t get why a double fork is needed here and a simple subprocess.Popen() wouldn’t suffice.
> > >
> > I tried and it doesn’t suffice. “The first process in the process group becomes the process group leader and the first process in the session becomes the session leader. Every session can have one TTY associated with it. Only a session leader can take control of a TTY. For a process to be truly daemonized (ran in the background) we should ensure that the session leader is killed so that there is no possibility of the session ever taking control of the TTY. ” (https://stackoverflow.com/questions/881388/what-is-the-reason-for-performing-a-double-fork-when-creating-a-daemon)
>
> I don’t understand why it would be a problem if the daemon would take control of a TTY - at least that shouldn’t cause the parent to block, which was the issue in Bug #15382. But since you say that you tested it with Popen and - for reasons unknown to me - it didn’t work, I don’t insist on spending more time to investigate this further. But if you already understand it, please try to explain it to me.
>
Experience shows that it’s needed. I can’t explain better than the link I provided.

#16 Updated by alant 2018-04-28 21:52:15

  • % Done changed from 20 to 50

#17 Updated by alant 2018-05-01 18:41:39

alant wrote:
> segfault wrote:
> > alant wrote:
> > > segfault wrote:
> > > > I think it would be better if the config file was not world-readable. Have you considered creating a tails-additional-software user/group and only allowing this user/group access to the config file?
> > > >
> > > The config file should be readable by amnesia for the configuration window to read it. And the installed packages can be get using apt-cache by an unprivileged user, so I don’t see the point of making this file not world-readable. It could however be tails-persistence-setup/anmesia if that makes real sense.
> >
> > I think it would be better to change it to tails-persistence-setup:amnesia. I understand the argument that the same information can be retrieved through apt-cache, but I think - as a general security practice - access should be restricted as much as possible, and it wouldn’t be much effort in this case, right?
>
> I tried to do that, but it’s a lot more complicated than it seems. Let’s see if it works in a build.
>
To have the config file 0640 tails-persistence-setup:amnesia, we would need persistence setup to have the right to chgrp to amnesia. So we’d need to grant more privileges to persistence setup, lowering security. All this to make a config file that contain information any user can infer (from apt-cache and diff) not world readeable. It doesn’t look wise to do. Add to that the fact that I already spend a few hours on this problem, and that intrigeri has no remaining time, it dosen’t looks worth more effort. So I reverted the related commits.

#18 Updated by bertagaz 2018-05-10 11:09:27

  • Target version changed from Tails_3.7 to Tails_3.8

#19 Updated by Anonymous 2018-06-07 10:23:55

Please reassign to segfault once more code review is needed on this matter.

#20 Updated by intrigeri 2018-06-26 16:28:01

  • Target version changed from Tails_3.8 to Tails_3.9

#21 Updated by alant 2018-07-30 23:02:17

  • Assignee changed from alant to segfault

It should be ready at commit 9942abd. Please note I just pushed a few minor changes that might fail testing, but if you want to start reviewing, there will at worst be 2 more commits.

Thanks by advance!

#22 Updated by Anonymous 2018-08-07 13:20:49

  • Priority changed from Normal to Elevated

Hi segfault, this is meant to be merged for the upcoming release in a week (August 15th). Please review asap, and reassign early enough to Alan if necessary. Thanks.

#23 Updated by intrigeri 2018-08-08 14:52:25

  • blocks deleted (Feature #14596: Write automated tests for Additional Software GUI)

#24 Updated by intrigeri 2018-08-08 14:52:56

(With my RM hat on I’ve deleted the relationship with Feature #14596: we need to merge the code within 7 days and cannot afford to wait for the test suite anymore.)

#25 Updated by intrigeri 2018-08-08 15:23:56

alant wrote:
> To have the config file 0640 tails-persistence-setup:amnesia, we would need persistence setup to have the right to chgrp to amnesia. So we’d need to grant more privileges to persistence setup, lowering security. All this to make a config file that contain information any user can infer (from apt-cache and diff) not world readeable. It doesn’t look wise to do. Add to that the fact that I already spend a few hours on this problem, and that intrigeri has no remaining time, it dosen’t looks worth more effort.

Fully agreed. Let’s not spend time on improvements whose cost/benefit ratio is close to ∞.

#26 Updated by alant 2018-08-09 20:38:29

alant wrote:
> Please note I just pushed a few minor changes that might fail testing.

This commit passed testing so for me everything is fine.

#27 Updated by segfault 2018-08-12 17:32:09

My first batch of remarks:

I’d suggest you set `PERSISTENCE_SETUP_USERNAME = “tails-persistence-setup”` in tailslib/init.py and use it in the two occurrances where this string is currently used.

tails-additional-software-config:

  • There are a lot of untranslatable strings in update_packages_list()

configuration-window.ui:

  • Untranslatable strings: header bar title,
  • Maybe remove the install_label text, because it’s overwritten in update_packages_list() anyway

tails-additional-software:

  • Why did you change the shebang from /usr/bin/env python3 to /usr/bin/python3? Usually using /usr/bin/env is preferred
  • Use logging.warning instead of deprecated logging.warn
  • _notify_failure():
    * incomplete sentence in docstring: “The user has the option to edit the configuration of to view the system log”
    * second “details =” line uses .format() but the string doesn’t contain any replacement fields
  • show_system_log(): the log file is not readable by amnesia, so gedit is not able to show it
  • apt_hook_pre(): you shouldn’t use `assert` to check for conditions that could happen, because it is only evaluated if __debug__ is True (see https://docs.python.org/3/reference/simple_stmts.html)
  • main(): What do we need the syslog handler for? Wouldn’t it be better to just log to stderr (i.e. use logging.StreamHandler()) instead of logging to /dev/log? This way, if the program is run in a terminal, we get the output there directly, as would be expected, and else the output to stderr should go to the syslog anyway.

config.py:

  • there is a race condition between multiple calls of add_additional_packages and/or remove_additional_packages: if the config file is written between when the packages are read and when the packages are written, this change will be overwritten by the subsequent write. You could fix this by acquiring a file lock before reading the file, for example with fcntl.flock(f, fcntl.LOCK_EX), and only releasing the lock once the file was written (example: https://gitlab.com/segfault3/onionkit/blob/master/onionkit/util.py#L21).
  • it would be good to do the resetting of uid/gid in a `finally:` block, or even better, using a context manager
  • both add_additional_packages() and remove_additional_packages() are only ever called with search_new_persistence=True, so why is this argument even needed?
  • as a result of the above, get_additional_packages() and _write_config are also only called with search_new_persistence=True

org.boum.tails.additional-software.policy:

  • is not translatable

#28 Updated by segfault 2018-08-12 17:34:18

> there is a race condition between multiple calls of add_additional_packages and/or remove_additional_packages: if the config file is written between when the packages are read and when the packages are written, this change will be overwritten by the subsequent write. You could fix this by acquiring a file lock before reading the file, for example with fcntl.flock(f, fcntl.LOCK_EX), and only releasing the lock once the file was written (example: https://gitlab.com/segfault3/onionkit/blob/master/onionkit/util.py#L21).

This way you could also get rid of the python3-atomicwrites package

#29 Updated by segfault 2018-08-12 17:48:08

persistence.py:

  • get_persistence_path(): only ever called with search_new_persistence=True, return_nonexistent=False
  • has_unlocked_persistence(): only called with search_new_persistence=True

#30 Updated by segfault 2018-08-12 17:52:59

  • Assignee changed from segfault to alant
  • QA Check set to Dev Needed

That’s it, I’m done with the review of the diff between devel and 1ac10aa5cbe9486d8f59bdae72f2661d2ad52231, excluding po/, wiki/, features/, and perl code.

I don’t think there are any blockers for merging this, but I guess it would be good to fix show_system_log() and the untranslatable strings before 3.9.

#31 Updated by alant 2018-08-12 22:00:24

  • % Done changed from 50 to 60

segfault wrote:
> That’s it, I’m done with the review of the diff between devel and 1ac10aa5cbe9486d8f59bdae72f2661d2ad52231

Thanks for the review! I’m surprised you didn’t do the review only since your last review. Next time it may save you some time, unless you want to have everything in mind.

> I don’t think there are any blockers for merging this

Great

> but I guess it would be good to fix show_system_log()

How did you test this? In my tests it worked.

> and the untranslatable strings before 3.9.

Yes sure I’ll look at this.

#32 Updated by segfault 2018-08-12 22:12:16

alant wrote:
> segfault wrote:
> > That’s it, I’m done with the review of the diff between devel and 1ac10aa5cbe9486d8f59bdae72f2661d2ad52231
>
> Thanks for the review! I’m surprised you didn’t do the review only since your last review. Next time it may save you some time, unless you want to have everything in mind.

I tried that, but when comparing the old commit until which I reviewed with HEAD, it includes all the changes merged from devel. I also tried merging devel into the old commit on a new branch, to compare HEAD with that, but then I would have had to fix a lot of merge conflicts :/

> > but I guess it would be good to fix show_system_log()
>
> How did you test this? In my tests it worked.

I built an image, booted it and changed the code to raise an exception that would trigger _notify_failure() (in the try-except block in handle_removed_packages()). The result was that gedit opened, but told me that it couldn’t open /run/live-additional-software/log.

#33 Updated by segfault 2018-08-12 22:15:14

segfault wrote:
> alant wrote:
> > Thanks for the review! I’m surprised you didn’t do the review only since your last review. Next time it may save you some time, unless you want to have everything in mind.
>
> I tried that, but when comparing the old commit until which I reviewed with HEAD, it includes all the changes merged from devel. I also tried merging devel into the old commit on a new branch, to compare HEAD with that, but then I would have had to fix a lot of merge conflicts :/

And the branch had ~250 new commits since my last review, so reviewing commit by commit was not an option.

#34 Updated by segfault 2018-08-12 22:17:19

> And the branch had ~250 new commits since my last review, so reviewing commit by commit was not an option.

183 actually

#35 Updated by alant 2018-08-15 06:43:21

I can work on the requested changes after RC, but haven’t completed them yet. What is the next deadline?

#36 Updated by intrigeri 2018-08-15 06:57:00

> I can work on the requested changes after RC, but haven’t completed them yet. What is the next deadline?

See the “August 18-30” bullet point on https://mailman.boum.org/pipermail/tails-dev/2018-July/012219.html. I strongly advise isolating “small, targeted, not too risky fixes” that might be granted a freeze exception, from anything else that will have to wait post-3.9 to be merged.

#37 Updated by Anonymous 2018-08-19 11:04:36

Segfault said:

“I don’t think there are any blockers for merging this, but I guess it would be good to fix show_system_log() and the untranslatable strings before 3.9.”

plus comments in notes 27, 28 and 29 which could be fixed after August 30th if I understand correctly.

(Thanks for the fast review, segfault!)

#38 Updated by alant 2018-08-19 21:04:09

> tails-additional-software-config:
> * There are a lot of untranslatable strings in update_packages_list()
>
Should be fixed, thanks !

> configuration-window.ui:
> * Untranslatable strings: header bar title,

The fact that the string is not extracted is a bug in xgettext, which I can’t easily solve. But as there is the same string in the .desktop file, it goes in the catalog and should be used by GTK.

> * Maybe remove the install_label text, because it’s overwritten in update_packages_list() anyway
>
That would make the UI file less understandable for no benefit that I can see.

#39 Updated by intrigeri 2018-08-20 07:29:34

Alan, once all 3.9 items flagged by segfault are fixed, please:

  • reassign to him for another round of review
  • find some way to track the other things that are not blockers for 3.9

#40 Updated by alant 2018-08-25 00:05:52

  • Assignee changed from alant to segfault
  • QA Check changed from Dev Needed to Info Needed

> > > but I guess it would be good to fix show_system_log()
> >
> > How did you test this? In my tests it worked.
>
> I built an image, booted it and changed the code to raise an exception that would trigger _notify_failure() (in the try-except block in handle_removed_packages()). The result was that gedit opened, but told me that it couldn’t open /run/live-additional-software/log.

I can’t reproduce this issue. I booted an image, added a nonexisting package to live-additional-software.conf, rebooted, had the error notification, clicked “Show System Log” and had the log shown.

I belive the other blocers you raised issues are fixed, but Bug #15822 is remaining on my plate.

#41 Updated by alant 2018-08-26 12:21:10

  • Feature Branch set to bugfix/14598-asp-review-bugs

#42 Updated by alant 2018-08-26 14:12:53

> * find some way to track the other things that are not blockers for 3.9

I moved these to Bug #15838.

#43 Updated by alant 2018-08-26 14:22:27

  • related to Bug #15838: Fix non-blocking issues identified during ASP code review added

#44 Updated by alant 2018-08-26 16:49:12

I have been delayed by failing builds, so my last commits are not yet tested, but the blocking issues sould be fixed, if you want to start reviewing without being in a hurry. The branch is bugfix/14598-asp-review-bugs.

#45 Updated by alant 2018-08-26 23:41:50

  • QA Check changed from Info Needed to Ready for QA

#46 Updated by alant 2018-08-30 20:27:40

  • % Done changed from 60 to 70

The branch at commit 15642806697a07548a3ba6b140a042fbfaa34524 is my proposal for 3.9.

#47 Updated by alant 2018-08-31 14:38:02

I merged bugfix/15879-asp-2-notifications so please consider e9592c41d09f1277541a7f6abc0efdd75e53e04e.

The history of the bugfix/14598-asp-review-bugs branch is ugly due to sevray failed bugfix attempts. I can propose a rebased branch if you think it’s needed.

#48 Updated by segfault 2018-09-01 10:18:15

alant wrote:
> > tails-additional-software-config:
> > * There are a lot of untranslatable strings in update_packages_list()
> >
> Should be fixed, thanks !
>
> > configuration-window.ui:
> > * Untranslatable strings: header bar title,
>
> The fact that the string is not extracted is a bug in xgettext, which I can’t easily solve. But as there is the same string in the .desktop file, it goes in the catalog and should be used by GTK.

Did you try adding translatable="yes" to the title?

> > * Maybe remove the install_label text, because it’s overwritten in update_packages_list() anyway
> >
> That would make the UI file less understandable for no benefit that I can see.

Lets discuss this on Bug #15838.

#49 Updated by Anonymous 2018-09-03 11:02:15

  • Description updated

#50 Updated by segfault 2018-09-03 14:09:46

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

I reviewed up to 93bfdb98623216573f0f997c1cfe3b9b98632a79.

There are some commits from devel, which I guess shouldn’t be there, because the branch should be based on testing.

Not a blocker, but what I find quite confusing is the use of the variable name package_name - it is now mostly used as the actual package name, but sometimes (e.g. tails-additional-software-config:update_packages_list), it is used as the package name + target, version etc.

Everything else LGTM. But the branch FTBFS with Unable to locate package linux-image-4.17.0-2-amd64, so I couldn’t test it yet.

#51 Updated by Anonymous 2018-09-03 14:25:00

  • related to Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies added

#52 Updated by Anonymous 2018-09-03 14:26:24

segfault wrote:

> Everything else LGTM. But the branch FTBFS with Unable to locate package linux-image-4.17.0-2-amd64, so I couldn’t test it yet.

This is likely due to Bug #15846.

#53 Updated by segfault 2018-09-03 17:36:46

Oh, I forgot something: I think the merge of bugfix/15879-asp-2-notifications went wrong, the argument to install_additional_packages should be named upgrade_mode but it’s still called ignore_old_apt_lists.

#54 Updated by segfault 2018-09-03 18:14:13

My comments above (Feature #14598#note-50, Feature #14598#note-53) were about branch feature/14594-asp-gui. I now also reviewed bugfix/14598-asp-review-bugs, in which the argument to install_additional_packages has the correct name.

#55 Updated by alant 2018-09-03 23:24:15

  • QA Check changed from Dev Needed to Info Needed

So as far as I understand, this ticket which is about bugfix/14598-asp-review-bugs passes review. I pasted a comment about 15838 there. Thanks for the review!

#56 Updated by segfault 2018-09-04 09:28:28

  • Assignee changed from alant to intrigeri
  • QA Check changed from Info Needed to Pass

The reason I set this to “Dev Needed” was for you to reply to my questions and comments:

> Did you try adding translatable=“yes” to the title?

> Not a blocker, but what I find quite confusing is the use of the variable name package_name - it is now mostly used as the actual package name, but sometimes (e.g. tails-additional-software-config:update_packages_list), it is used as the package name + target, version etc.

But since we are late, lets move the discussions to Bug #15838 and say the review passed.

#57 Updated by intrigeri 2018-09-04 09:57:03

> The reason I set this to “Dev Needed” was for you to reply to my questions and comments:

(Which makes sense to me from a process PoV: it helps clarify whose court the ball is in.)

#58 Updated by intrigeri 2018-09-04 10:57:45

  • related to deleted (Bug #15846: devel and branches based on devel FTBFS: The following packages have unmet dependencies)

#59 Updated by intrigeri 2018-09-04 10:57:56

  • related to deleted (Bug #15838: Fix non-blocking issues identified during ASP code review)

#60 Updated by intrigeri 2018-09-04 10:58:04

  • blocked by Bug #15838: Fix non-blocking issues identified during ASP code review added

#61 Updated by intrigeri 2018-09-04 10:58:33

  • Assignee changed from intrigeri to segfault
  • Target version changed from Tails_3.9 to Tails_3.10.1
  • QA Check deleted (Pass)

Merged => let’s repurpose this for the review of the fixes that Alan will prepare for 3.10.

#62 Updated by bertagaz 2018-09-06 10:14:22

While finishing the last scenario of the automated test suite for that feature, I’ve stumble on a bug. The failure case in this code is never run:

576     if install_additional_packages(ignore_old_apt_lists=True):
577         logging.info("The upgrade was successful.")
578     else:
579         _notify_failure(_("The upgrade of your additional software failed"),
580                         _("Please check your network connection, "
581                           "restart Tails, or read the system log to "
582                           "understand the problem."))
583         return False

Indeed, it’s using install_additional_packages(), which has:

533     if apt_get_returncode:
534         _close_notification(installing_notification_id)
535         logging.warning("Warning: installation of %s failed"
536                         % " ".join(packages))
537         _notify_failure(_("The installation of your additional software "
538                           "failed"))
539         return False

So it seems the failure is catched at that moment and not passed along, resulting with “The installation of your additional software failed” notification when the upgrade failed.

#63 Updated by segfault 2018-09-06 15:08:29

  • Assignee changed from segfault to alant

Alan, can you look into the issue bertagaz found?

#64 Updated by bertagaz 2018-09-09 09:35:46

I’ve pushed a scenario to test upgrade failure recovery (https://git-tails.immerda.ch/tails/tree/features/additional_software_packages.feature?h=feature/14596-automated-tests-for-ASP-gui#n124). If you run it on an ISO of your branch, you’ll see the notification shown is not the right one (installation failure, and not upgrade failure in https://git-tails.immerda.ch/tails/tree/features/additional_software_packages.feature?h=feature/14596-automated-tests-for-ASP-gui#n145)

#65 Updated by alant 2018-09-11 23:53:18

segfault wrote:
> Alan, can you look into the issue bertagaz found?

Yes. However, this ticket is becoming too messy for me so I’d prefer that other issues are reported as subtickets.

#66 Updated by bertagaz 2018-09-12 08:41:45

alant wrote:
> segfault wrote:
> > Alan, can you look into the issue bertagaz found?
>
> Yes. However, this ticket is becoming too messy for me so I’d prefer that other issues are reported as subtickets.

Hold on, maybe I’ve reported that a bit too fast. In a branch based on stable, this bug I saw did not show up (see https://jenkins.tails.boum.org/job/test_Tails_ISO_feature-14596-automated-tests-for-asp-gui-on-stable/3/console). I’ll investigate a bit more now that devel isn’t FTBFS anymore. I’ve seen that on a branch which was based on feature/14594-asp-gui, which has newer commits.

#67 Updated by Anonymous 2018-10-15 11:55:33

  • QA Check set to Info Needed

I’m not sure about the current status of this ticket.

#69 Updated by Anonymous 2018-10-16 13:52:51

  • Assignee changed from alant to bertagaz

After re-reading all the comments:

- I think the code review on this ticket passed.

- Everything else has been moved to Bug #15838 and related tickets.
- The issue reported by bertagaz in https://labs.riseup.net/code/issues/14598#note-62 seems to be the only leftover on this ticket. I am reassigning this ticket to bertagaz so that he can check if this still happens or not. If not, please close this ticket @bertagaz.

#70 Updated by intrigeri 2018-10-24 17:03:41

  • Target version changed from Tails_3.10.1 to Tails_3.11

#71 Updated by bertagaz 2018-11-01 09:18:55

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

u wrote:
> - The issue reported by bertagaz in https://labs.riseup.net/code/issues/14598#note-62 seems to be the only leftover on this ticket. I am reassigning this ticket to bertagaz so that he can check if this still happens or not. If not, please close this ticket @bertagaz.

I’ve not seen this issue again since then, so I guess we can close this ticket.

#72 Updated by Anonymous 2018-11-02 13:19:20

  • Status changed from Fix committed to Resolved
  • QA Check deleted (Info Needed)

I’m marking this as resolved then. Thanks!