Tails

#3 Updated by intrigeri 2016-08-31 01:15:59

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

#11 Updated by intrigeri 2017-04-20 05:56:57

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 2017-04-20 09:15:40

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.

#22 Updated by intrigeri 2017-11-15 12:37:49

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

#24 Updated by segfault 2018-02-13 18:42:43

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 2018-02-13 19:44:18

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

#28 Updated by intrigeri 2018-02-18 11:13:32

>> 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 2018-02-18 14:13:31

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 2018-02-18 21:07:15

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

#35 Updated by segfault 2018-02-19 20:11:48

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 Feature #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 2018-02-20 18:37:21

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 Feature #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!

#38 Updated by segfault 2018-02-21 22:12:18

segfault wrote:
> 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.

It seems like we would have to port at least tails-shell-library/chroot-browser.sh and sbin/unsafe-browser, to prevent any code duplication created by the functions used in tor-launcher and tor-browser. I think I could do this until the end of the week. Would this be soon enough for you to able to review it in time for 3.6?

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

Seems to work.

#39 Updated by sajolida 2018-02-23 13:36:19

Once we merged something from GoodCrypt, I would find it super nice to let them know about this and maybe even answer them on tails-dev:

https://mailman.boum.org/pipermail/tails-dev/2016-December/011097.html

#40 Updated by intrigeri 2018-02-24 12:37:07

> I think I could do this until the end of the week. Would this be soon enough for you to able to review it in time for 3.6?

I can do some reviewing on Monday and Tuesday so if stuff works fine there’s still a chance we can merge it!

#41 Updated by intrigeri 2018-02-24 12:46:10

By the way, please merge current devel into your branch to make sure it still works with our current code :)

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

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 now fixed all other instances I could find in c7fd8f4c832f40f8d1e9ca39bd0c48de9b549ec2.

LGTM

>> 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?

>

<code class="python">
> 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))
> </code>

Wrapper hell! … but probably the best we can do.

>> >> * 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 lack time to look deeper into this during this cycle so let’s focus on porting ⇒ please either drop the partial check or add a comment as suggested above.

#42 Updated by segfault 2018-02-24 20:40:25

intrigeri wrote:
> By the way, please merge current devel into your branch to make sure it still works with our current code :)

Argh, there is merge conflict with 755813ec19b2d06132eda814b5bbcf909d257a87 + 6d458ec8f0de728a922d778509db24cb9bc5b30e, which add ensure_hook_dependency_is_installed from tails-shell-library/build.sh to 19-install-tor-browser-AppArmor-profile. Now I think we either have to (1) reimplement this function in Python, and then have duplicate code again, or (2) don’t use the Python port of 19-install-tor-browser-AppArmor-profile, or (3) drop this call to ensure_hook_dependency_is_installed.

> > What do you think about using this, instead of allow_x_connection?
>
> > […]
>
> Wrapper hell! … but probably the best we can do.

Implemented in 6fc943df36d005e0f0a6b4f0611c0add2397a54e. There I also implemented the memory check using psutil.

> >> >> * 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 lack time to look deeper into this during this cycle so let’s focus on porting ⇒ please either drop the partial check or add a comment as suggested above.

Added a comment in e802f60b9ab55bebfbcc49f29b295bb360c7f209.

I also improved the doctests of tails-upgrade-frontend-wrapper in 91473b62853e0ec167cbf9bd174ac42f9e866a74.

#43 Updated by intrigeri 2018-02-27 11:54:15

> Argh, there is merge conflict with 755813ec19b2d06132eda814b5bbcf909d257a87 + 6d458ec8f0de728a922d778509db24cb9bc5b30e, which add ensure_hook_dependency_is_installed from tails-shell-library/build.sh to 19-install-tor-browser-AppArmor-profile. Now I think we either have to (1) reimplement this function in Python, and then have duplicate code again, or (2) don’t use the Python port of 19-install-tor-browser-AppArmor-profile, or (3) drop this call to ensure_hook_dependency_is_installed.

I think (2) is the only one that can realistically allow us to merge this in time for Tails 3.6. If we don’t succeed at that we can think about it again.

#47 Updated by intrigeri 2018-02-28 08:52:09

Full test suite results on my local Jenkins:

Failing Scenarios:
 features/time_syncing.feature:37 # Scenario: Boot with a hardware clock set way in the past and make sure that Tails sets the clock to the source date
 features/additional_software_packages.feature:8 # Scenario: Additional software packages are installed even without network

… but these scenarios always fail for me here on devel-based branches so it’s not the fault of this one.

#51 Updated by segfault 2018-02-28 10:31:12

intrigeri wrote:
> intrigeri wrote:
> > * I’ll manually test a couple things that this branch affects but that have no automated test coverage.
>
> Regression: the output of journalctl in WhisperBack reports includes control chars (for colored output), which is not the case on Tails 3.5. That merely noise but it makes it harder for help desk and developers to work with these reports. I think that’s because debug_command uses sh.Command which perhaps gives the command a tty. What about replacing sh.Command with another way to get the stdout of a command without running it under a tty?

Should be fixed in 4ba0e7986fb2df994355ab15cb16f7e5cebb0244.

I also noticed that because of the partial check, debug_command now refuses to open /live/persistence/TailsData_unlocked/{persistence.conf,live-additional-software.conf} and , because they are owned by tails-persistence-setup and not root. This means that a process running as tails-persistence-setup can replace one of these files with any file that is only readable as root, so that its content is included in tails-debugging-info. I don’t think this is critical, because amnesia can’t run arbitrary commands as tails-persistence-setup, but I think it would be cleaner to open these files as tails-persistence-setup instead of root. What do you think?

#52 Updated by intrigeri 2018-02-28 10:35:04

segfault wrote:
> Should be fixed in 4ba0e7986fb2df994355ab15cb16f7e5cebb0244.

Assuming you’ve tested it and confirmed it fixes the problem: great!

> I think it would be cleaner to open these files as tails-persistence-setup instead of root. What do you think?

Yes, good catch. Please implement and test it :)

#53 Updated by segfault 2018-02-28 10:40:42

intrigeri wrote:
> segfault wrote:
> > Should be fixed in 4ba0e7986fb2df994355ab15cb16f7e5cebb0244.
>
> Assuming you’ve tested it and confirmed it fixes the problem: great!

Yes, I tested it and it fixes the problem. As you requested, I rebased my branch on feature/11753-port-to-python, so now this commit 8c5a06778cceac0abad436f1c3cc26b9b3083a8c.

> > I think it would be cleaner to open these files as tails-persistence-setup instead of root. What do you think?
>
> Yes, good catch. Please implement and test it :)

Done and tested in 1ee5a550aedf0c7c19291cd03c03df504fe0b60b.

#56 Updated by bertagaz 2018-02-28 20:20:06

intrigeri wrote:
> I’m done with the code review, sending over to the RM’s plate for a last check. The reproducibility test you should look at is https://jenkins.tails.boum.org/view/Tails_ISO/job/reproducibly_build_Tails_ISO_feature-11753-port-to-python/3/ (job run 2 should be enough in theory: it’ll lack segfault’s last changes which should not impact reproducibility in theory, but to be on the safe side I suggest you wait for run 3).

I’ll do my best, but this is very last minute, so I’ll rely on intrigeri’s review, otherwise that’s too much to review for me before the freeze.

#57 Updated by intrigeri 2018-02-28 20:41:21

> I’ll do my best, but this is very last minute, so I’ll rely on intrigeri’s review, otherwise that’s too much to review for me before the freeze.

To clarify: the review was done already, you’re not being asked to do a full code review again. The only thing left to check is the reproducibility test results on Jenkins.

#59 Updated by intrigeri 2018-03-06 09:39:59

FTR I’ve just merged segfault/pythonlib:feature/11198-ready-for-merge into pythonlib:master, since this step was missed.