Project

General

Profile

Feature #11753

Feature #11198: Port complex shell scripts into Python

Port complex shell scripts shipped in /usr/local to Python

Added by intrigeri over 1 year ago. Updated about 10 hours ago.

Status:
In Progress
Priority:
Elevated
Assignee:
Category:
-
Target version:
Start date:
08/27/2016
Due date:
% Done:

40%

QA Check:
Dev Needed
Feature Branch:
segfault:feature/11198-ready-for-merge
Type of work:
Code


Related issues

Blocked by Tails - Bug #15270: devel branch FTBFS since torbrowser-launcher 0.2.9 entered sid Fix committed 01/30/2018

History

#1 Updated by intrigeri over 1 year ago

  • Description updated (diff)

#2 Updated by intrigeri over 1 year ago

  • Blueprint set to https://tails.boum.org/blueprint/Port_shell_scripts_to_Python/

#3 Updated by intrigeri over 1 year ago

Note that we already had some preliminary work in feature/11198-python-scripting, but IIRC it was about converting some build time hook, while the pull request from goodcrypto is about scripts shipped to the user in /usr/local/{lib,*bin}.

#4 Updated by intrigeri over 1 year ago

  • Target version changed from Tails_2.6 to Tails_2.7

Sorry, we were not able to review this during the 2.6 cycle again. I'm postponing to 2.7 so that it's on top of anonym's review plate, even though this cannot go into 2.7 since it's a bugfix-only release.

#5 Updated by bertagaz over 1 year ago

  • Target version changed from Tails_2.7 to Tails_2.9.1

#6 Updated by anonym about 1 year ago

  • Target version changed from Tails_2.9.1 to Tails 2.10

#7 Updated by anonym about 1 year ago

  • Target version changed from Tails 2.10 to Tails_2.11

#8 Updated by intrigeri about 1 year ago

  • Priority changed from Normal to Elevated

#9 Updated by anonym 12 months ago

  • Target version changed from Tails_2.11 to Tails_2.12

#10 Updated by anonym 10 months ago

  • Target version changed from Tails_2.12 to Tails_3.0

#11 Updated by intrigeri 10 months ago

I don't think I want to merge this between 3.0~rc1 and 3.0, so I'll assume that "Target version = 3.0" means "I want to look into it between 3.0~rc1 and 3.0" so this can be merged in 3.2. So if we want (some of?) this in 3.0 final, please set target version to 3.0~rc1.

#12 Updated by anonym 10 months ago

intrigeri wrote:

"I want to look into it between 3.0~rc1 and 3.0" so this can be merged in 3.2.

This is what it means.

#13 Updated by intrigeri 9 months ago

  • Target version changed from Tails_3.0 to Tails_3.1

Our plan is to create one subtask per script, share the review work between anonym and I, and spread it over the next months, e.g. we would aim at tackling 1 script each for each Tails release cycle. anonym will create these tickets during the 3.1 cycle.

#14 Updated by intrigeri 8 months ago

#15 Updated by anonym 8 months ago

  • Target version changed from Tails_3.1 to Tails_3.2

#16 Updated by anonym 6 months ago

  • Assignee changed from anonym to segfault

Assigning to you per your interest. Let me know what you want me to do!

#17 Updated by BitingBird 6 months ago

  • Description updated (diff)

#18 Updated by segfault 5 months ago

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

#19 Updated by intrigeri 5 months ago

#20 Updated by segfault 3 months ago

  • Feature Branch changed from goodcrypto:feature/11198 to segfault:feature/11198

I started reviewing the portings done by goodcrypto. I cherry-picked some of the Python scripts, and updated them to include the modifications of the bash scripts since the initial portings. I also found and fixed some bugs and improved the code style a bit. The portings of goodcrypto are very close to the original bash scripts - which is convenient for reviewing, but sometimes results in unnecessary complex code, because some things could be done more elegantly in Python than in bash. This especially applies to the portings of the tailslib scripts, which I tend to reimplement at least partly (I already started with tailslib/tor.py).

Nevertheless, the porting was definitely a big effort, and I think it's shame that we didn't manage to use the results before (and give feedback to goodcrypto).

I pushed my preliminary results to the feature/11198 branch in my gitlab repository (https://gitlab.com/segfault_/tails).

#21 Updated by segfault 3 months ago

  • % Done changed from 20 to 40

#22 Updated by intrigeri 3 months ago

Thanks a lot, segfault, for taking this over! :)

#23 Updated by anonym 29 days ago

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

#24 Updated by segfault 8 days ago

I'm not sure when I will find the time to review the remaining scripts, but I don't think that this is necessarily a blocker for shipping the scripts I already reviewed. And the longer we wait with this, the more additional work will be necessary, in form of porting additional patches we applied to the shell scripts over to the Python scripts. How do you think we should continue on this?

#25 Updated by intrigeri 8 days ago

I'm not sure when I will find the time to review the remaining scripts, but I don't think that this is necessarily a blocker for shipping the scripts I already reviewed. And the longer we wait with this, the more additional work will be necessary, in form of porting additional patches we applied to the shell scripts over to the Python scripts.

Agreed.

How do you think we should continue on this?

Prepare a branch that has only the changes you deem ready for merging, ensure they pass our test suite, assign for review to the RM (currently bertagaz) or to anonym/intrigeri if you prefer more eyes to look at it first.

#26 Updated by segfault 5 days ago

  • Blocked by Bug #12679: Sandbox Tor Browser's content renderer processes more strictly added

#27 Updated by segfault 3 days ago

  • Assignee changed from segfault to bertagaz
  • Feature Branch changed from segfault:feature/11198 to segfault:feature/11198-ready-for-merge

intrigeri wrote:

Prepare a branch that has only the changes you deem ready for merging,

did that now in feature/11198-ready-for-merge in https://gitlab.com/segfault_/tails.git.

ensure they pass our test suite,

the test suite fails with various strange errors on my machine, both when testing this branch or the 3.5 ISO. But I tested all the ported scripts manually.

assign for review to the RM (currently bertagaz) or to anonym/intrigeri if you prefer more eyes to look at it first.

assigning to bertagaz now to let him decide whether he wants to do the review.

#28 Updated by intrigeri 3 days ago

ensure they pass our test suite,

the test suite fails with various strange errors on my machine, both when testing this branch or the 3.5 ISO. But I tested all the ported scripts manually.

I've pushed your branch to the official tails.git so Jenkins runs the test suite. Hopefully it will help bertagaz review this in a timely manner :)

#29 Updated by segfault 3 days ago

For the record: This branch requires updates in submodules/pythonlib which I pushed to branch feature/11198 in https://gitlab.com/segfault_/tails-pythonlib.git.

#30 Updated by intrigeri 3 days ago

For the record: This branch requires updates in submodules/pythonlib which I pushed to branch feature/11198 in https://gitlab.com/segfault_/tails-pythonlib.git.

I've pushed that branch to our main pythonlib repo in the hope that it fixes the build of the branch on Jenkins.

#31 Updated by intrigeri 2 days ago

  • Blocked by Bug #15270: devel branch FTBFS since torbrowser-launcher 0.2.9 entered sid added

#32 Updated by intrigeri 2 days ago

  • Blocked by deleted (Bug #12679: Sandbox Tor Browser's content renderer processes more strictly)

#33 Updated by intrigeri 2 days ago

  • Assignee changed from bertagaz to intrigeri

(As per discussion with anonym, in order to make bertagaz' review plate lighter and increase the chances he can deal with what's left there in a timely manner.)

#34 Updated by intrigeri 2 days ago

  • Assignee changed from intrigeri to segfault
  • QA Check changed from Ready for QA to Dev Needed
  • CONTROL_SOCKET_PATH = '/var/run/tor/control': I think you mean /run/tor/control (we've dropped the obsolete /var prefix some time ago)
  • It looks like some shell code that was rewritten to Python still exists in your branch and thus we have data/code duplication. I didn't look in detail but I think that at least config/chroot_local-includes/usr/local/lib/tails-shell-library/tor-browser.sh has duplicated data/code. Is there a reason not to delete it?
  • reg_ex = r'^langpack-([^]+)firefox.mozilla.org.xpi$': you've added the "firefox.mozilla.org.xpi" bit, I assume to be stricter; fine, but then escape the "." in there :)
  • Now that we have tests in our pythonlib (great!) I think we should run them on our CI. If you agree, please file a ticket and there we can discuss what should be tested and how :)
  • Regarding "Make the python library available in Tails": we do this kind of stuff in auto/config; I don't remember why but it would be nice to keep this consistent.
  • Why doesn't config/chroot_local-hooks/19-install-tor-browser-AppArmor-profile raise an exception when if error.stderr?
  • Please teach me some Python: what's the advantage of '{}'.format(launch_text) compared to launch_text?
  • I'm somewhat confused to see e.g. the trivial config/chroot_local-includes/usr/local/bin/tails-get-bootinfo converted to a twice as long Python script, on a branch that's about "complex shell scripts". In that case I really wonder if the maintenance benefit is worth the runtime performance & resources consumption cost. What do you think?
  • I'm surprised by the manual parsing of /proc/meminfo. I would expect this info can be queried in a much nicer way in Python. Same in various other places but you warned us! So let's see that as a first iteration and then I hope we'll make the code more pythonic in the future :)
  • I'm not convinced by the porting of tails-upgrade-frontend-wrapper to Python: the main reason why this code was written in a shell wrapper, and not directly in the Upgrader, is to ensure we can report to the user if the system lacks RAM to check for upgrades. If the system hasn't enough memory, it's possible that it can't run this Python script either. So in this specific case I see a strong argument in favour of keeping the wrapper in shell. Moreover, the Python wrapper keeps running while the Upgrader runs, so we're actually increasing the chances that the Upgrader cannot do an upgrade due to lack of RAM (and then the user has to do a manual upgrade); I don't think the maintenance benefit (for us) is worth the cost for the user.
  • I think os.path.exists('/usr/local/sbin/tor-has-bootstrapped') is wrong: we want to run that script, not merely check whether it exists.
  • In config/chroot_local-includes/usr/local/sbin/tails-debugging-info I'm not sure I see the value of adding partial checks for the conditions we require for security. At least add a comment that makes it clear that these checks are not sufficient, pointing to the longer comment on top of the file?

This:

diff --git a/config/chroot_local-includes/usr/local/sbin/restart-tor b/config/chroot_local-includes/usr/local/sbin/restart-tor
old mode 100755
new mode 100644

… seems weird, no?

Otherwise, looks good, great work! I've not run the full test suite on this branch yet and I see that some scripts for which we have no test are modified. Let's do that once we reach "code review: pass!" :)

#35 Updated by segfault 2 days ago

intrigeri wrote:

  • CONTROL_SOCKET_PATH = '/var/run/tor/control': I think you mean /run/tor/control (we've dropped the obsolete /var prefix some time ago)

Actually, tails-shell-library/tor.sh uses the control port, not the socket file. And I didn't know that the /var prefix is obsolete, the socket file can still be found at that path. Fixed in commit 2577ba6a1c4283c4092a16a930bc98d1c8d884ed of the pythonlib submodule.

  • It looks like some shell code that was rewritten to Python still exists in your branch and thus we have data/code duplication. I didn't look in detail but I think that at least config/chroot_local-includes/usr/local/lib/tails-shell-library/tor-browser.sh has duplicated data/code. Is there a reason not to delete it?

Yes, it is still used by some scripts that are not ported to Python yet, including scripts in chroot_local-hooks/, the Thunderbird wrapper, the unsafe browser wrapper, and more. I see that it will make it harder to maintain two the code with this duplication. I can prioritize porting the scripts that still use tor-browser.sh.

  • reg_ex = r'^langpack-([^]+)firefox.mozilla.org.xpi$': you've added the "firefox.mozilla.org.xpi" bit, I assume to be stricter; fine,

I added this because I hope that it makes the regex more readable. goodcrypto left the following comment in the code, and it also took me a while to realize that the @ doesnt have a special meaning, but is really only matching an @:

# !! did these '@' chars have special meaning in bash or sed?

but then escape the "." in there :)

Fixed in commit 7065e748d0a262f443965a91d10d85c8b603480a of the pythonlib submodule.

  • Now that we have tests in our pythonlib (great!) I think we should run them on our CI. If you agree, please file a ticket and there we can discuss what should be tested and how :)

Done, see #15330.

  • Regarding "Make the python library available in Tails": we do this kind of stuff in auto/config; I don't remember why but it would be nice to keep this consistent.

Ok, but then we can't use the setup script, because auto/config is not run in the chroot, right? Should we manually copy tailslib/ to /usr/local/lib/python3.5/dist-packages/?

  • Why doesn't config/chroot_local-hooks/19-install-tor-browser-AppArmor-profile raise an exception when if error.stderr?

Because I thought it could be too verbose to print both the truncated stderr printed by the sh.ErrorReturnCode exception and the full stderr we print manually. But it is a bug that we don't exit without an error return code, I intended to sys.exit(1) in that case. Anyway, after thinking about this again, I think that having a stacktrace (by raising an exception) beats the verbose error message, so I'm now raising an exception in commit 5c74b181cacfc261dfbdc6d6d2175761428fa52d.

  • Please teach me some Python: what's the advantage of '{}'.format(launch_text) compared to launch_text?

Haha, there is none (given that launch_text is a string). This code comes from goodcrypto's commit 2f0c795373626902291da1d4c270762e7dc1e279. Fixed in b66afbcbf42e2f374ef2a6a0e915cb6affa0cd4e.

  • I'm somewhat confused to see e.g. the trivial config/chroot_local-includes/usr/local/bin/tails-get-bootinfo converted to a twice as long Python script, on a branch that's about "complex shell scripts". In that case I really wonder if the maintenance benefit is worth the runtime performance & resources consumption cost. What do you think?

I wouldn't have started the porting with these very small shell scripts, but with the actually complex ones. And I'm not sure if it was worth the time we spent on fixing and reviewing these, but that's almost done now. I'm not sure if performance and resource consumption are an issue though. I tested both the old and new tails-get-bootinfo scripts: shell executed in 0.005 seconds, Python in 0.1, and the Python script uses 4 MiB of RAM.

  • I'm surprised by the manual parsing of /proc/meminfo. I would expect this info can be queried in a much nicer way in Python. Same in various other places but you warned us! So let's see that as a first iteration and then I hope we'll make the code more pythonic in the future :)

Yes, many things are not very pythonic in these scripts (although I already spent a lot of time on improving the original scripts from goodcrypto). The Python standard library doesn't actually support determining the free RAM, but the psutil module supports it. I just noticed that we already ship this because it's needed by onion-grater.

  • I'm not convinced by the porting of tails-upgrade-frontend-wrapper to Python: the main reason why this code was written in a shell wrapper, and not directly in the Upgrader, is to ensure we can report to the user if the system lacks RAM to check for upgrades. If the system hasn't enough memory, it's possible that it can't run this Python script either. So in this specific case I see a strong argument in favour of keeping the wrapper in shell.

The Python script uses about 6.6 MiB of RAM (determined with the same method used by tails-upgrade-frontend-wrapper). If we used psutil, it would use about 9.4 MiB. Anyway, I think if a system doesn't have enough memory to execute this script, it's not usable at all.

Moreover, the Python wrapper keeps running while the Upgrader runs, so we're actually increasing the chances that the Upgrader cannot do an upgrade due to lack of RAM (and then the user has to do a manual upgrade); I don't think the maintenance benefit (for us) is worth the cost for the user.

That's true, but we could simply use os.exec to replace the Python process with the Upgrader. I already did this in various of the other scripts (by replacing the calls to os.spawn used by goodcrypto), but obviously I forgot it in the most important case, where the memory consumption is actually important. I fixed this in 33b483e6b1073f92c162082b3d784b0c94ae06f4.

  • I think os.path.exists('/usr/local/sbin/tor-has-bootstrapped') is wrong: we want to run that script, not merely check whether it exists.

Oh, I missed that one. Fixed in 97044feb6eaf4a6dd97375d10a9b09f78bc46253.

  • In config/chroot_local-includes/usr/local/sbin/tails-debugging-info I'm not sure I see the value of adding partial checks for the conditions we require for security. At least add a comment that makes it clear that these checks are not sufficient, pointing to the longer comment on top of the file?

What do you mean by "partial checks for the conditions we require for security"? The doctests currently don't test any conditions required for security, but only test if the functions do what they are supposed to do.

This:

[...]

… seems weird, no?

Yes. And in the case of tails-debugging-info too. Fixed both in ab1a509bcfa7040157aedcf4e3ba234c65f4c51f.

Otherwise, looks good, great work! I've not run the full test suite on this branch yet and I see that some scripts for which we have no test are modified. Let's do that once we reach "code review: pass!" :)

Thanks for reviewing it so soon, I was afraid that I would have to spend more work on updating the scripts again in a few months.

#36 Updated by intrigeri 1 day ago

segfault:

intrigeri wrote:

  • CONTROL_SOCKET_PATH = '/var/run/tor/control': I think you mean /run/tor/control (we've dropped the obsolete /var prefix some time ago)

Actually, tails-shell-library/tor.sh uses the control port, not the socket file. And I didn't know that the /var prefix is obsolete, the socket file can still be found at that path. Fixed in commit 2577ba6a1c4283c4092a16a930bc98d1c8d884ed of the pythonlib submodule.

LGTM

  • It looks like some shell code that was rewritten to Python still exists in your branch and thus we have data/code duplication. I didn't look in detail but I think that at least config/chroot_local-includes/usr/local/lib/tails-shell-library/tor-browser.sh has duplicated data/code. Is there a reason not to delete it?

Yes, it is still used by some scripts that are not ported to Python yet, including scripts in chroot_local-hooks/, the Thunderbird wrapper, the unsafe browser wrapper, and more. I see that it will make it harder to maintain two the code with this duplication. I can prioritize porting the scripts that still use tor-browser.sh.

Ouch. Yeah, indeed, let's not have two different implementations of the same thing. If parts of the shell library are obsolete already, or can realistically be made obsolete in time for the 3.6 freeze, let's focus on them.

but then escape the "." in there :)

Fixed in commit 7065e748d0a262f443965a91d10d85c8b603480a of the pythonlib submodule.

I think you forgot to push that.

  • Now that we have tests in our pythonlib (great!) I think we should run them on our CI. If you agree, please file a ticket and there we can discuss what should be tested and how :)

Done, see #15330.

Great, I'll follow up there one of these days (not a priority for 3.6 though).

  • Regarding "Make the python library available in Tails": we do this kind of stuff in auto/config; I don't remember why but it would be nice to keep this consistent.

Ok, but then we can't use the setup script, because auto/config is not run in the chroot, right? Should we manually copy tailslib/ to /usr/local/lib/python3.5/dist-packages/?

Argh, that's one of these times when I hate we lack a proper code review tool.

I was talking about the bits you do in auto/build, not about config/chroot_local-hooks/00-install-tailslib. See similar code we have at the end of auto/config.

  • Why doesn't config/chroot_local-hooks/19-install-tor-browser-AppArmor-profile raise an exception when if error.stderr?

Because I thought it could be too verbose to print both the truncated stderr printed by the sh.ErrorReturnCode exception and the full stderr we print manually. But it is a bug that we don't exit without an error return code, I intended to sys.exit(1) in that case. Anyway, after thinking about this again, I think that having a stacktrace (by raising an exception) beats the verbose error message, so I'm now raising an exception in commit 5c74b181cacfc261dfbdc6d6d2175761428fa52d.

LGTM

  • Please teach me some Python: what's the advantage of '{}'.format(launch_text) compared to launch_text?

Haha, there is none (given that launch_text is a string). This code comes from goodcrypto's commit 2f0c795373626902291da1d4c270762e7dc1e279. Fixed in b66afbcbf42e2f374ef2a6a0e915cb6affa0cd4e.

Looks good but there are quite a few other instances of that weird thing in other scripts.

  • I'm somewhat confused to see e.g. the trivial config/chroot_local-includes/usr/local/bin/tails-get-bootinfo converted to a twice as long Python script, on a branch that's about "complex shell scripts". In that case I really wonder if the maintenance benefit is worth the runtime performance & resources consumption cost. What do you think?

I wouldn't have started the porting with these very small shell scripts, but with the actually complex ones. And I'm not sure if it was worth the time we spent on fixing and reviewing these, but that's almost done now. I'm not sure if performance and resource consumption are an issue though. I tested both the old and new tails-get-bootinfo scripts: shell executed in 0.005 seconds, Python in 0.1, and the Python script uses 4 MiB of RAM.

OK, let's keep it the way it is on your branch then!

  • I'm surprised by the manual parsing of /proc/meminfo. I would expect this info can be queried in a much nicer way in Python. Same in various other places but you warned us! So let's see that as a first iteration and then I hope we'll make the code more pythonic in the future :)

Yes, many things are not very pythonic in these scripts (although I already spent a lot of time on improving the original scripts from goodcrypto). The Python standard library doesn't actually support determining the free RAM, but the psutil module supports it. I just noticed that we already ship this because it's needed by onion-grater.

Interesting!

  • I'm not convinced by the porting of tails-upgrade-frontend-wrapper to Python: the main reason why this code was written in a shell wrapper, and not directly in the Upgrader, is to ensure we can report to the user if the system lacks RAM to check for upgrades. If the system hasn't enough memory, it's possible that it can't run this Python script either. So in this specific case I see a strong argument in favour of keeping the wrapper in shell.

The Python script uses about 6.6 MiB of RAM (determined with the same method used by tails-upgrade-frontend-wrapper). If we used psutil, it would use about 9.4 MiB. Anyway, I think if a system doesn't have enough memory to execute this script, it's not usable at all.

I'm not 200% convinced (e.g. Tor Browser may already be running just fine but the user won't be told they lack memory to check for upgrades; admittedly that'll happen in extremely rare cases and opening two more tabs may be enough for the user to realize there's a problem), by OK, deal! Then I say go for psutil iff. other blockers are resolved in time for the freeze. Clearly not a blocker for merging though.

Moreover, the Python wrapper keeps running while the Upgrader runs, so we're actually increasing the chances that the Upgrader cannot do an upgrade due to lack of RAM (and then the user has to do a manual upgrade); I don't think the maintenance benefit (for us) is worth the cost for the user.

That's true, but we could simply use os.exec to replace the Python process with the Upgrader. I already did this in various of the other scripts (by replacing the calls to os.spawn used by goodcrypto), but obviously I forgot it in the most important case, where the memory consumption is actually important. I fixed this in 33b483e6b1073f92c162082b3d784b0c94ae06f4.

Wait, but what about allow_x_connection? It seems unused at the moment so I wonder how the Upgrader UI can work. Also, I don't see how we can exec while still dropping X privs after the Upgrader exit… without introducing yet another wrapper (let's please not).

  • I think os.path.exists('/usr/local/sbin/tor-has-bootstrapped') is wrong: we want to run that script, not merely check whether it exists.

Oh, I missed that one. Fixed in 97044feb6eaf4a6dd97375d10a9b09f78bc46253.

Good.

  • In config/chroot_local-includes/usr/local/sbin/tails-debugging-info I'm not sure I see the value of adding partial checks for the conditions we require for security. At least add a comment that makes it clear that these checks are not sufficient, pointing to the longer comment on top of the file?

What do you mean by "partial checks for the conditions we require for security"?

I mean the two instances of if owner != user, not the doctests.

This:

[...]

… seems weird, no?

Yes. And in the case of tails-debugging-info too. Fixed both in ab1a509bcfa7040157aedcf4e3ba234c65f4c51f.

LGTM

Otherwise, looks good, great work! I've not run the full test suite on this branch yet and I see that some scripts for which we have no test are modified. Let's do that once we reach "code review: pass!" :)

Thanks for reviewing it so soon,

You're welcome.

I was afraid that I would have to spend more work on updating the scripts again in a few months.

I always prioritize reviews above doing work of my own (my Redmine view for this is called "Unblock other people's work"). IMO that's the only sensible way to work as a team efficiently, otherwise we waste too much energy in context switching and bringing stuff back into our hot cache!

#37 Updated by segfault about 10 hours ago

intrigeri wrote:

segfault:

intrigeri wrote:

  • It looks like some shell code that was rewritten to Python still exists in your branch and thus we have data/code duplication. I didn't look in detail but I think that at least config/chroot_local-includes/usr/local/lib/tails-shell-library/tor-browser.sh has duplicated data/code. Is there a reason not to delete it?

Yes, it is still used by some scripts that are not ported to Python yet, including scripts in chroot_local-hooks/, the Thunderbird wrapper, the unsafe browser wrapper, and more. I see that it will make it harder to maintain two the code with this duplication. I can prioritize porting the scripts that still use tor-browser.sh.

Ouch. Yeah, indeed, let's not have two different implementations of the same thing. If parts of the shell library are obsolete already, or can realistically be made obsolete in time for the 3.6 freeze, let's focus on them.

Ok, I will look into this later today.

but then escape the "." in there :)

Fixed in commit 7065e748d0a262f443965a91d10d85c8b603480a of the pythonlib submodule.

I think you forgot to push that.

Indeed, pushed it now.

  • Regarding "Make the python library available in Tails": we do this kind of stuff in auto/config; I don't remember why but it would be nice to keep this consistent.

Ok, but then we can't use the setup script, because auto/config is not run in the chroot, right? Should we manually copy tailslib/ to /usr/local/lib/python3.5/dist-packages/?

Argh, that's one of these times when I hate we lack a proper code review tool.

I was talking about the bits you do in auto/build, not about config/chroot_local-hooks/00-install-tailslib. See similar code we have at the end of auto/config.

Ah ok. I didn't even see the code in auto/build, this commit was done by anonym. I moved the part in auto/build to auto/config now. I didn't test this yet, but will do so today.

  • Please teach me some Python: what's the advantage of '{}'.format(launch_text) compared to launch_text?

Haha, there is none (given that launch_text is a string). This code comes from goodcrypto's commit 2f0c795373626902291da1d4c270762e7dc1e279. Fixed in b66afbcbf42e2f374ef2a6a0e915cb6affa0cd4e.

Looks good but there are quite a few other instances of that weird thing in other scripts.

I now fixed all other instances I could find in c7fd8f4c832f40f8d1e9ca39bd0c48de9b549ec2.

  • I'm surprised by the manual parsing of /proc/meminfo. I would expect this info can be queried in a much nicer way in Python. Same in various other places but you warned us! So let's see that as a first iteration and then I hope we'll make the code more pythonic in the future :)

Yes, many things are not very pythonic in these scripts (although I already spent a lot of time on improving the original scripts from goodcrypto). The Python standard library doesn't actually support determining the free RAM, but the psutil module supports it. I just noticed that we already ship this because it's needed by onion-grater.

Interesting!

  • I'm not convinced by the porting of tails-upgrade-frontend-wrapper to Python: the main reason why this code was written in a shell wrapper, and not directly in the Upgrader, is to ensure we can report to the user if the system lacks RAM to check for upgrades. If the system hasn't enough memory, it's possible that it can't run this Python script either. So in this specific case I see a strong argument in favour of keeping the wrapper in shell.

The Python script uses about 6.6 MiB of RAM (determined with the same method used by tails-upgrade-frontend-wrapper). If we used psutil, it would use about 9.4 MiB. Anyway, I think if a system doesn't have enough memory to execute this script, it's not usable at all.

I'm not 200% convinced (e.g. Tor Browser may already be running just fine but the user won't be told they lack memory to check for upgrades; admittedly that'll happen in extremely rare cases and opening two more tabs may be enough for the user to realize there's a problem), by OK, deal! Then I say go for psutil iff. other blockers are resolved in time for the freeze. Clearly not a blocker for merging though.

ACK. I will implement the memory check with psutil once we resolved the issue below.

Moreover, the Python wrapper keeps running while the Upgrader runs, so we're actually increasing the chances that the Upgrader cannot do an upgrade due to lack of RAM (and then the user has to do a manual upgrade); I don't think the maintenance benefit (for us) is worth the cost for the user.

That's true, but we could simply use os.exec to replace the Python process with the Upgrader. I already did this in various of the other scripts (by replacing the calls to os.spawn used by goodcrypto), but obviously I forgot it in the most important case, where the memory consumption is actually important. I fixed this in 33b483e6b1073f92c162082b3d784b0c94ae06f4.

Wait, but what about allow_x_connection? It seems unused at the moment so I wonder how the Upgrader UI can work. Also, I don't see how we can exec while still dropping X privs after the Upgrader exit… without introducing yet another wrapper (let's please not).

You're right, the allow_x_connection context manager wasn't used and exec won't work with it. I noticed that I didn't actually run an upgrade with the new wrapper yet, because there are no updates available for the branch, which is why I missed this in my tests. I'm a bit shocked about this sloppiness, I actually spent quite some time on this. I will test it on Tails 3.4 once we decided what we want to do with this script.

What do you think about using this, instead of allow_x_connection?

os.execv("/bin/sh", ("/bin/sh", "-c",
                     "xhost +SI:localuser:{user};" 
                     "sudo -u {user} /usr/bin/tails-upgrade-frontend;" 
                     "xhost -SI:localuser:{user}".format(user=RUN_AS_USER))
  • In config/chroot_local-includes/usr/local/sbin/tails-debugging-info I'm not sure I see the value of adding partial checks for the conditions we require for security. At least add a comment that makes it clear that these checks are not sufficient, pointing to the longer comment on top of the file?

What do you mean by "partial checks for the conditions we require for security"?

I mean the two instances of if owner != user, not the doctests.

Ah, ok. These are indeed partial checks. I would like to implement a complete check instead, but I'm afraid I don't understand parts of the comment on the top of the file. Why would it make a difference if there is only one non-root user involved in the ownership situation? I suspect there is something missing in the comment, also because you define "$USER", but then never use it.

I was afraid that I would have to spend more work on updating the scripts again in a few months.

I always prioritize reviews above doing work of my own (my Redmine view for this is called "Unblock other people's work"). IMO that's the only sensible way to work as a team efficiently, otherwise we waste too much energy in context switching and bringing stuff back into our hot cache!

Seems very reasonable :)

Also available in: Atom PDF