Project

General

Profile

Feature #14598

Feature #14568: Additional Software Packages

Code review for Additional Software packages GUI

Added by u 10 months ago. Updated 11 days ago.

Status:
In Progress
Priority:
Normal
Assignee:
Category:
-
Target version:
Start date:
09/04/2017
Due date:
04/15/2018
% Done:

50%

QA Check:
Feature Branch:
Type of work:
Code
Blueprint:
Starter:
Affected tool:
Additional Software Packages

Description

This needs to happen before April 15th 2018. (B4)


Related issues

Blocked by Tails - Feature #14596: Write automated tests for Additional Software GUI In Progress 09/04/2017 03/06/2018

Associated revisions

Revision 8b3c48fe (diff)
Added by alant 2 months ago

ASP: made clear that the data is synchronized on shutdown

Refs: #14598.

Revision ddb65fb5 (diff)
Added by alant 2 months ago

ASP: handle errors when reading config file

Refs: #14598

Revision 7bb0b091 (diff)
Added by alant 2 months ago

ASP: fix exit code on argument error

Refs: #14598

Revision 8d0780d1 (diff)
Added by alant 2 months ago

ASP: import tailslib fixing an exception message

Refs: #14598

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

ASP: import fixed tailslib

Refs: #14598

Revision 42fc109d (diff)
Added by alant about 2 months ago

ASP: import fixed tailslib

Refs: #15545 #14598

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

ASP: return on config file read failure

Refs: #14598

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

ASP: fix persistence-setup invocation from configuration window

Refs: #14598

History

#1 Updated by u 10 months ago

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

#3 Updated by u 10 months ago

  • Description updated (diff)

#4 Updated by u 9 months ago

  • Description updated (diff)
  • Due date set to 04/15/2018
  • Target version changed from 2018 to Tails_3.7

#5 Updated by intrigeri 3 months ago

You can start reviewing my bits (#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 3 months ago

intrigeri wrote:

You can start reviewing my bits (#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.

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 3 months ago

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 3 months ago

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 3 months ago

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 3 months ago

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 3 months ago

  • 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 #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 2 months ago

  • % 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 #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 2 months ago

  • Status changed from Confirmed to In Progress

#14 Updated by segfault 2 months ago

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 #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 #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 about 2 months ago

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 #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 #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 about 2 months ago

  • % Done changed from 20 to 50

#17 Updated by alant about 2 months ago

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 about 1 month ago

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

#19 Updated by u 11 days ago

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

Also available in: Atom PDF