Tails

#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:

• 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. 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.

#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.

#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.

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

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

#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

#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

#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.

#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.

#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.

#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.

#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.)

#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.

#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.