Tails

#1 Updated by alant 2015-01-02 01:37:59

• blocks Feature #8513: Implement PGP/MIME in WhisperBack added

#2 Updated by alant 2015-01-02 01:38:24

• Subject changed from Replace @WhisperBack.mail_appended_info@ by a dictionnary to Replace WhisperBack.mail_appended_info by a dictionnary

#3 Updated by intrigeri 2015-01-02 09:44:55

• Subject changed from Replace WhisperBack.mail_appended_info by a dictionnary to Replace WhisperBack.mail_appended_info with a dictionnary

#4 Updated by intrigeri 2015-01-02 09:46:33

• Subject changed from Replace WhisperBack.mail_appended_info with a dictionnary to Replace WhisperBack.mail_appended_info with a dictionary
• Status changed from New to Confirmed

#5 Updated by alant 2015-01-18 18:30:32

• blocked by deleted (Feature #8513: Implement PGP/MIME in WhisperBack)

#6 Updated by alant 2015-01-18 18:39:27

• blocks Feature #8722: Create MIME/Multipart message with attachments in WhisperBack added

#7 Updated by sascha.markus@gmail.com 2018-05-30 14:22:50

• File 0001-feature-8514-Replace-WhisperBack.mail_appended_info-.patch added
• File 0002-feature-8514-Replace-WhisperBack.mail_appended_info-.patch added
• Assignee set to alant
• QA Check set to Info Needed
• Feature Branch set to https://github.com/saschamarkus/whisperback/tree/feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary

Hi alant
Here is a draft version to have dicts instead of a big string.
In config/chroot_local-includes/usr/local/sbin/tails-debugging-info the debug info is created as a dict and printed out serialized as json.
https://github.com/saschamarkus/tails/commit/1a52d1a9b063ca47fe999249178b6d4438a66356

This json is deserialized in the client code in whisperBack/whisperback.py
https://github.com/saschamarkus/whisperback/commit/240414182f6329c6f43bd5c17d100248c26ed873

This draft doesn’t contain the output of /bin/journalctl because the deserialization failed.
I’ll check how to solve this if you decide that this approach is OK.

#8 Updated by intrigeri 2018-05-30 15:14:49

• Assignee changed from alant to intrigeri

(I doubt Alan will have time to look into this any time soon so I will.)

#9 Updated by intrigeri 2018-06-02 08:43:34

• Feature Branch changed from https://github.com/saschamarkus/whisperback/tree/feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary to saschamarkus/whisperback:feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary

#10 Updated by intrigeri 2018-06-02 09:11:24

• Status changed from Confirmed to In Progress
• Assignee changed from intrigeri to sascha.markus@gmail.com
• % Done changed from 0 to 10
• QA Check changed from Info Needed to Dev Needed

Your branch contains quite some unrelated changes such as your Feature #7180 work and your Bug #6432 patch (that was applied in master with a different commit ID). Can you please rebase it on current master and ensure it contains only changes related to this ticket? If for some reason this work has to be based on top of Feature #7180, fine: then let’s mark this ticket as blocked by Feature #7180 so that we review and merge them in the right order :)

#12 Updated by sascha.markus@gmail.com 2018-06-02 22:29:27

• Assignee changed from sascha.markus@gmail.com to intrigeri

Sanitizing the hardware info broke the json. Solved now.

tails-debugging-info in the tails repo now “returns” a list of dicts instead a dict of dicts to keep the order of the infos.
https://github.com/saschamarkus/tails/tree/feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary

The client code was updated to match this change.
https://github.com/saschamarkus/whisperback/tree/feature/8514-Replace-mail_appended_info-with-a-dictionary

#13 Updated by intrigeri 2018-06-03 06:41:25

• QA Check changed from Dev Needed to Ready for QA
• Feature Branch changed from saschamarkus/whisperback:feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary to saschamarkus/whisperback:feature/8514-Replace-mail_appended_info-with-a-dictionary,saschamarkus/tails:feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary

#14 Updated by intrigeri 2018-06-03 07:39:10

• Assignee changed from intrigeri to sascha.markus@gmail.com
• Target version set to Tails_3.9
• % Done changed from 10 to 20
• QA Check changed from Ready for QA to Dev Needed

Great! I’m glad we’re making progress towards Feature #6183, which will incidentally fix nasty problems such as Bug #15470 (that makes our help desk’s and users’ life more painful than it should be). Once we’re done here it would be awesome if you tackled Feature #8722, ideally by the end of July.

Code review (not tested the code yet):

• Any reason why https://github.com/saschamarkus/tails/commit/1a52d1a9b063ca47fe999249178b6d4438a66356 removes the automated tests embedded in the script?
• In “Return the command and it’s result as dict”, maybe s/result/standard output/?
• At least of one your commits introduces trailing whitespace and very long lines. Please fix this. (I recommend using linting helpers such as pep8, pylint and friends :)
• Here we’re changing the config API (mail_appended_info must now return valid JSON) so please update doc/config.py.sample and the docstring in config/chroot_local-includes/etc/whisperback/config.py.

Other than this, looks good! Let’s aim to have this merged in time for 3.9 :)

#15 Updated by sascha.markus@gmail.com 2018-06-03 21:53:28

• Assignee changed from sascha.markus@gmail.com to intrigeri
• QA Check changed from Dev Needed to Ready for QA

The branches in both repos are updated.
The files are checked with pep8. Some lines still have >79 chars but aren’t very long anymore.
I moved the documentation for mail_prepended_info and mail_append_info from the comments to the docstring part.
The tests for main and this_command are back. I had to add the empty print() again to make them pass because I didn’t see another way to test for “anything”.

#16 Updated by intrigeri 2018-06-05 10:50:57

• Assignee changed from intrigeri to sascha.markus@gmail.com
• % Done changed from 20 to 30
• QA Check changed from Ready for QA to Dev Needed

Almost there! Another code review (still not tested):

• I think it's standard input is wrong: you mean “standard output” instead, right?
• OrderedDict is not used anymore, is it? then let’s not import it :)
• config.py.sample: please keep docstrings generic (you’ve added Tails-specific requirements in there while the idea is that whoever uses WhisperBack can override these functions to prepend/append/etc. whatever info they want, which might not be related to Tails)

Notes for the future regarding your Git history:

• atomic commits would be so muuuuch nicer (to review, to bisect, etc.)
• please don’t include in a commit message anything that is not about that commit: e.g. 1fdaa434827d201370f6c91ad99e9d58510a04a4 says “Replace big string with dictionaries” which was already implemented in a previous commit

#17 Updated by sascha.markus@gmail.com 2018-06-05 20:48:00

• Assignee changed from sascha.markus@gmail.com to intrigeri
• QA Check changed from Dev Needed to Ready for QA

Both repos updated with your suggested changes.

I tried to merge two commits into one. But it didn’t make it any better :-(
Clearly something I have practice.

#18 Updated by intrigeri 2018-06-06 10:30:05

• % Done changed from 30 to 60

Code review passes! I’ll test soon.

#20 Updated by intrigeri 2018-06-11 09:19:19

• Assignee changed from intrigeri to sascha.markus@gmail.com
• QA Check deleted (Ready for QA)

Ouch, I noticed a new blocker! We need to update the “Scenario: tails-debugging-info does not leak information” automated test since the way it parses (yes!) /usr/local/sbin/tails-debugging-info is broken with the new implementation. I think best way to do this would be to first extract the info about all the debug commands, files and directories we want to a machine-readable config file; best would be /etc/whisperback/debugging-info.json (that I could read directly from the test suite that’s in Ruby) but worst case, I guess that moving them to a variable in /etc/whisperback/config.py would be OK, as long as one can load that config.py as a library without triggering any side effect.

I recommend an array (to preserve ordering) of arrays. Each such array would contain TYPE and then any type-specific info, e.g.:

[
  ['command', { 'args' => '/usr/sbin/dmidecode', '-s', 'system-manufacturer' }],
  ['file', { 'user': 'root', 'path': '/etc/X11/xorg.conf' }],
  etc.
]

Makes sense?

Once this is implemented on your side I’m happy to adjust the test suite accordingly :)

#25 Updated by sascha.markus@gmail.com 2018-06-11 21:18:05

• File debug.json added

sascha.markus@gmail.com wrote:
> intrigeri wrote:
> > Ouch, I noticed a new blocker! We need to update the “Scenario: tails-debugging-info does not leak information” automated test since the way it parses (yes!) /usr/local/sbin/tails-debugging-info is broken with the new implementation. I think best way to do this would be to first extract the info about all the debug commands, files and directories we want to a machine-readable config file; best would be /etc/whisperback/debugging-info.json (that I could read directly from the test suite that’s in Ruby) but worst case, I guess that moving them to a variable in /etc/whisperback/config.py would be OK, as long as one can load that config.py as a library without triggering any side effect.
> >
> > I recommend an array (to preserve ordering) of arrays. Each such array would contain TYPE and then any type-specific info, e.g.:
> >
> > […]
> >
> > Makes sense?
> >
> > Once this is implemented on your side I’m happy to adjust the test suite accordingly :)
> The method aleady returns an array of dicts to keep the order. What about adding the info needed for the test to the dict?
> Like
> [{"type": "file", "path":"/proc/cmdline", "content":["line1", "line2"]}, {"type": "command", "user": "root", "args":["/usr/sbin/dmidecode", "-s", "system-manufacturer"], "content":["line1", "line2"]}, ]
>
> I don’t know if we really need args as an array or if it’s ok to join them into one string. And if we can unify “path” and “args” to a generic “key”
>
> in the test it would be something like (pseudo ruby ahead…)
> @ info = JSON.parse($vm.file_content(“sudo /usr/local/sbin/tails-debugging-info”, :user => LIVE_USER)
> for i in 0…info.length > if '#{secret_contents}' == info[i].content
> raise “The secret was leaked by tails-debugging-info via ‘#{@info[i].path}’”
> @

Proposed output

#26 Updated by sascha.markus@gmail.com 2018-06-11 21:19:45

• Assignee changed from sascha.markus@gmail.com to intrigeri
• QA Check set to Info Needed

#29 Updated by intrigeri 2018-06-14 11:53:29

• Assignee changed from intrigeri to sascha.markus@gmail.com
• QA Check deleted (Info Needed)

Interesting, I had not thought of embedding in the output the info we need for the automated test. This would work and surely would be the cheapest solution to the problem we’re tackling, at least on the short term. Now, it feels rather uncommon to run a script (and ask it to do 100% of its job) merely to extract its configuration from its output. That’s not a common pattern so I’m a bit worried about how maintainable it will be.

In contrast, the approach I’ve proposed initially (extracting that config to an external file that can be read by another program without having to run tails-debugging-info) feels much cleaner and less surprising to me.

Now, if you’d rather stick to your proposal, I guess I can live with it, but then please ensure this interface (and the reason why we have it) is properly documented in the script, so nobody breaks this test next time they work on this script again :)

#30 Updated by sascha.markus@gmail.com 2018-06-14 22:27:42

• Assignee changed from sascha.markus@gmail.com to intrigeri

I made tails-debugging-info configurable and added the configuration.
“user” and “type” are removed again from the output json.
Please have a look if this is OK.
https://github.com/saschamarkus/tails/commit/c6bff024257c86abcbbb220591f430dc9b4aeb56#diff-a0a47c9466b7bed2392053662e153ae8

#31 Updated by intrigeri 2018-06-15 06:03:47

• QA Check set to Ready for QA

#32 Updated by intrigeri 2018-06-18 06:42:47

• Assignee changed from intrigeri to sascha.markus@gmail.com
• QA Check changed from Ready for QA to Info Needed

Looks good but in:

+    for _type, _args in config:
+        if _type == 'command':
+            info.append(debug_command(_args['args'][0], *_args['args'][1:]))
+        else:
+            info.append(debug_file(_args['user'], _args['path']))

… what about the “directory” type?

#33 Updated by sascha.markus@gmail.com 2018-06-18 08:28:20

• Assignee changed from sascha.markus@gmail.com to intrigeri
• QA Check changed from Info Needed to Ready for QA

Sorry!
I updated the code. Please pull again.

#36 Updated by intrigeri 2018-06-28 16:33:28

• blocked by Bug #15472: Rebase our Tor Browser AppArmor policy on top of torbrowser-launcher 0.2.9-2's added

#37 Updated by intrigeri 2018-06-28 17:37:24

• QA Check deleted (Ready for QA)

Tested manually, looks good! Next step: update the test suite accordingly.

#41 Updated by intrigeri 2018-06-30 07:02:06

• related to #10333 added

#42 Updated by intrigeri 2018-06-30 07:04:56

• Feature Branch changed from saschamarkus/whisperback:feature/8514-Replace-mail_appended_info-with-a-dictionary,saschamarkus/tails:feature/8514-Replace-WhisperBack.mail_appended_info-with-a-dictionary to feature/8514-7180-whisperback-remove-right-pane-and-refactor,saschamarkus/whisperback:feature/8514-Replace-mail_appended_info-with-a-dictionary

Notes to the reviewer:

• I’m only asking to review 4e5c9785d149fc4daa73ed6cbc9df55393ed51bc.. on the feature/8514-7180-whisperback-remove-right-pane-and-refactor branch: I’ve already reviewed the other changes (implemented by Sascha Markus), both in their whisperback.git repo and in tails.git branch; I’ve imported Sascha’s WhisperBack changes via a snapshot package that’s imported into the feature-8514-7180-whisperback-remove-right-pane-and-refactor APT overlay which this branch enables; and my branch in tails.git has Sascha’s branch merged into it.
• The only relevant thing you need to know about the other changes is that the list of files/commands/directories printed by tails-debugging-info are now configured in config/chroot_local-includes/etc/whisperback/debugging-info.json.
• I’m pretty sure I got the logic right wrt. fixing the test case but I still have a slight doubt: it strikes me as odd that anonym could possibly have got it that wrong, so I wonder if I am getting it all wrong myself. That’s the main aspect I need another pair of eyes for. The comments on top of config/chroot_local-includes/usr/local/sbin/tails-debugging-info give some context. Sadly, the original bug it’s all about (#10333) is private and the other participant on that ticket is MIA currently so I can’t ask him his consent to make it public; but the corresponding changelog entry is “Prevent a symlink attack on ~/.xsession-errors via tails-debugging-info which could be used by the amnesia user to read the contents of any file, no matter the permissions”, which was mainly implemented in commit 0f8c07ce72e43ff4b48633d6ef45d44c532d8f3d.
• See the previous comment that demonstrates how the test rightfully fails if I revert my fix to the security check. I’ll paste the debug log of the test suite run on the latest version of the branch when I’ll submit it for review.
• The Gherkin (cucumber) text for the affected test is in features/regression_tests.feature. I know the scenario title poorly matches what we’re really testing but that’s orthogonal to this ticket.

#44 Updated by intrigeri 2018-06-30 11:04:29

• Assignee changed from intrigeri to lamby
• Estimated time set to 1 h
• QA Check set to Ready for QA

Chris, could you please take the part of this review that’s about the work I did on top of Sascha’s? See Feature #8514#note-42 to know what to review and what I’m expecting from it. Given you will need to catch up with the surrounding context I think that allocating 1 hour is fair, but if you’re faster than that to grasp it all, let me know, I won’t complain ;)

I can assign this to someone else if you prefer. I’m proposing it to you first because think you’re the person on the FT that is the most experienced wrt. automated testing.

#45 Updated by intrigeri 2018-06-30 11:05:34

• blocks Feature #13241: Core work: Test suite maintenance added

#52 Updated by lamby 2018-07-09 14:03:51

• Assignee changed from lamby to intrigeri

I’ve had a good look at this and I think I understand the private reasons behind 0f8c07ce72e43ff4b48633d6ef45d44c532d8f3d. :) I’ve reviewed and tested as much as possible, including running the new debugging output script using the JSON-powered stuff.

I remain somewhat lost on the Cucumber test - features/regression_tests.feature seems to contain no tests whatsoever here, just 6 lines of explanatory text.

#53 Updated by intrigeri 2018-07-10 11:50:52

• Assignee changed from intrigeri to lamby

lamby wrote:
> I’ve had a good look at this and I think I understand the private reasons behind 0f8c07ce72e43ff4b48633d6ef45d44c532d8f3d. :) I’ve reviewed and tested as much as possible, including running the new debugging output script using the JSON-powered stuff.

Good!

> I remain somewhat lost on the Cucumber test - features/regression_tests.feature seems to contain no tests whatsoever here, just 6 lines of explanatory text.

https://git-tails.immerda.ch/tails/tree/features/regression_tests.feature?h=feature/8514-7180-whisperback-remove-right-pane-and-refactor contains one test scenario, written in the Gherkin language used by Cucumber. Then the corresponding steps are defined in features/step_definitions/*.rb. In this case:

git diff --stat origin/devel...origin/feature/8514-7180-whisperback-remove-right-pane-and-refactor
 config/APT_overlays.d/feature-8514-7180-whisperback-remove-right-pane-and-refactor |   0
 config/chroot_local-includes/etc/whisperback/config.py                             |  61 +++++++++++++++++++++-------------------
 config/chroot_local-includes/etc/whisperback/debugging-info.json                   |  24 ++++++++++++++++
 config/chroot_local-includes/usr/local/sbin/tails-debugging-info                   | 126 +++++++++++++++++++++++++++++++++++------------------------------------------------
 features/step_definitions/checks.rb                                                |  21 ++++++++------
 5 files changed, 122 insertions(+), 110 deletions(-)

… the relevant bits are in features/step_definitions/checks.rb: Then /^tails-debugging-info is not susceptible to symlink attacks$/.

Meta: actually, I’d like you to become one more person who at least roughly understands the basics of how our test suite works. So if you feel like it and want to spend 1-2 extra hours on reading a bit about Cucumber and looking at stuff in our features/ directory, by all means, be my guest; I’ll gladly bump the “estimated time” value. Starting point: https://tails.boum.org/contribute/release_process/test/automated_tests/ :)

#54 Updated by lamby 2018-07-16 10:35:18

• Assignee changed from lamby to intrigeri

Thanks for sending over the file in question — I think I was misled somewhat by the generic filename combined the generic-sounding English tests (indeed, the does not leak information part is actually defined elsewhere).

With regards to getting up to speed on Cuumber, I’ve spent about an hour reading on it (shall I bump estimated time?) and having a quick play at the same time as trying to get the automated testsuite working locally (although hitting some issues there, but at least generated some related tickets which I’ve assigned to you, but more systemic issues when trying to run it in a container to get all the right dependencies..).

However, I just think that I have some somewhat-irrational dislike of the BDD method of testing. :/ Probably not the right place to discuss it, but I find it.. I dunno, really really opaque, misleading, ineffective and ugly. I try and have as few “hang-ups” as possible but I fear that BDD is just One Of Those Things.

So whilst I would totally agree we need need to increase the bus factor on the testsuite here I’m not sure I’m 100% the person to do that, especially if someone else is available. I could be persuaded otherwise if there was nobody else, but I just don’t think I would be most effective here. Does that make sense? Again, I try to have as few “not touching that” things in tech, but alas… :)

#56 Updated by intrigeri 2018-08-09 15:42:05

• Assignee changed from intrigeri to segfault

segfault, I’d like a second pair of eyes on this. By this I don’t mean the entire branch as I’ve already reviewed Sascha’s work. So before you dive into it, please read:

• Feature #8514#note-38
• Feature #8514#note-39
• Feature #8514#note-40
• Feature #8514#note-42

To be extra clear, the commits I need a review for are:

• 85cea6e62fc0714fa821187824a87153ca20df65
• 3460524dfc3496f0d5928c663bd0fcd646fb3aae
• e50d21a445ff859efc59d2de57da55a27c91e343
• 32c0fef4098e365302836e663c8b24fd19c50919
• e50e6e2c8d588f9b46eab9e24de8c9b49820a0a0
• 59f011ac3e921e04b3aa7f58606df0920bd15a3f

#57 Updated by segfault 2018-08-14 13:44:07

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

3460524dfc3496f0d5928c663bd0fcd646fb3aae:
The check was not actually meant as a security check, more as a check for whoever adds new files to tails-debugging-info to tell them that adding this file is not secure.

As the comment above the check already says, the check is not sufficient for security (it also only checks the ownership of the file itself, while for security it would also have to check the ownership of all directories).

e50d21a445ff859efc59d2de57da55a27c91e343:
I think there is a misunderstanding. IIUC, the original author of the test (anonym) only wanted to check if, in the case that a file that is writable by amnesia (or any other non-privileged user), so that it could be replaced by a symlink, tails-debugging-info will not print the content of files only readable by root. The test assumes that `debug_file` commands that are called with user=root will also not be susceptible to symlink attacks. So actually what it tests is only whether passing user=amnesia will indeed run `cat` as amnesia.

What you understood, probably because we introduced a (insufficient) permission check when porting the script to Python, is that this test would check for symlink attacks in case that amnesia managed to replace a file that is supposed to be only writable by root with a symlink.

This is also what you implemented now in the test, but since the check is not sufficient to protect against symlink attacks, I don’t think it makes sense to test it. I would say we revert your commits, leave the test as it was and explain in a comment that it does assume that the ownership of the target files and all parent directories is described in the docstring of tails-debugging-info.

Later, when we have the time, we can actually add a security check in tails-debugging-info, which we can then write a test for.

#58 Updated by intrigeri 2018-08-14 20:34:51

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

> 3460524dfc3496f0d5928c663bd0fcd646fb3aae:
> The check was not actually meant as a security check, more as a check for whoever adds new files to tails-debugging-info to tell them that adding this file is not secure.

> As the comment above the check already says, the check is not sufficient for security (it also only checks the ownership of the file itself, while for security it would also have to check the ownership of all directories).

Indeed, my commit message shows that I did not get this right when I wrote it.

To recap, here’s the effect of this commit of mine:

• If the path is not a symlink, then it’s a noop.
• If the path is a symlink owned by user, then we lose the check on the owner of the symlink target and thus provide less guidance (or none at all) to the developer.
• If the path is a symlink not owned by user, then we give better guidance to the developer.

To provide at least as good guidance to the developer in all cases, and better guidance in the 3rd case, we would need to check ownership of both the symlink and its target, i.e. something like (untested):

<code class="diff">
--- a/config/chroot_local-includes/usr/local/sbin/tails-debugging-info
+++ b/config/chroot_local-includes/usr/local/sbin/tails-debugging-info
@@ -103,9 +103,10 @@ def debug_file(user, filename):
 # This check is not sufficient, see the comment at the top of the file
 # for the complete requirements required for security
owner = getpwuid(os.stat(filename, follow_symlinks=False).st_uid).pw_name
-    if owner != user:
+    target_owner = getpwuid(os.stat(filename, follow_symlinks=True).st_uid).pw_name
+    if owner != user or target_owner != user:
return {'key': filename, 'content': '''WARNING: not opening file {}, because it is '''
-                '''owned by {} instead of {}'''.format(filename, owner, user)}
+                '''owned, or is a symlink to a file owned, by {} instead of {}'''.format(filename, owner, user)}

file_content = []
with open(filename) as f:

Would this make sense to you?

> e50d21a445ff859efc59d2de57da55a27c91e343:
> I think there is a misunderstanding. IIUC, the original author of the test (anonym) only wanted to check if, in the case that a file that is writable by amnesia (or any other non-privileged user), so that it could be replaced by a symlink, tails-debugging-info will not print the content of files only readable by root. The test assumes that `debug_file` commands that are called with user=root will also not be susceptible to symlink attacks. So actually what it tests is only whether passing user=amnesia will indeed run `cat` as amnesia.

> What you understood, probably because we introduced a (insufficient) permission check when porting the script to Python, is that this test would check for symlink attacks in case that amnesia managed to replace a file that is supposed to be only writable by root with a symlink.

OK. This makes sense to me. I’m quite pissed off to have spent so much time on this because I was confused by the name of the “tails-debugging-info is not susceptible to symlink attacks” step, which was rather misleading since it did test a tiny subset of what it pretends to, but well, what is done is done :)

> This is also what you implemented now in the test, but since the check is not sufficient to protect against symlink attacks, I don’t think it makes sense to test it.
> I would say we revert your commits, leave the test as it was and explain in a comment that it does assume that the ownership of the target files and all parent directories is described in the docstring of tails-debugging-info.

OK, I’m convinced, let’s do something like that. I say “something like” because:

• As I explained in commit:e50d21a445ff859efc59d2de57da55a27c91e343 (and noticed when I wanted to validate that commit:85cea6e62fc0714fa821187824a87153ca20df65 worked as intended), in its present form this test does not test anything at all. All it does is waste CPU cycles, warm the climate, and make me waste tons of time trying to improve it. So I propose we simply remove it.
• If for some reason we decide to instead keep this test, then we do need commit:85cea6e62fc0714fa821187824a87153ca20df65 (that’s why I started working on this in the first place).

Thank you so much for checking this! I’m very glad a second pair of eyes identified the misunderstanding \o/

#59 Updated by segfault 2018-08-15 14:06:54

• Assignee changed from segfault to intrigeri

intrigeri wrote:
> OK, I’m convinced, let’s do something like that. I say “something like” because:
>
> * As I explained in commit:e50d21a445ff859efc59d2de57da55a27c91e343 (and noticed when I wanted to validate that commit:85cea6e62fc0714fa821187824a87153ca20df65 worked as intended), in its present form this test does not test anything at all. All it does is waste CPU cycles, warm the climate, and make me waste tons of time trying to improve it. So I propose we simply remove it.

I now understand why the test does not test anymore what it was designed for, i.e. whether debug_file($USER, $FILE) can only read files accessible by $USER:
Since porting tails-debugging-info to Python, we always read the file as root, before it was read as $USER (via sudo -u). I would say that’s a bug, and if we fix that, the test would again test what I think it was designed for. I could prepare a commit if you agree.

> * If for some reason we decide to instead keep this test, then we do need commit:85cea6e62fc0714fa821187824a87153ca20df65 (that’s why I started working on this in the first place).

Indeed.

#61 Updated by intrigeri 2018-08-15 14:54:14

• Assignee changed from intrigeri to segfault

#62 Updated by segfault 2018-08-15 14:57:57

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

intrigeri wrote:
> > I now understand why the test does not test anymore what it was designed for, i.e. whether debug_file($USER, $FILE) can only read files accessible by $USER:
> > Since porting tails-debugging-info to Python, we always read the file as root, before it was read as $USER (via sudo -u). I would say that’s a bug, and if we fix that, the test would again test what I think it was designed for. I could prepare a commit if you agree.
>
> Wow, indeed the user parameter is now unused (except for the incomplete check we’ve discussed already). It would be good to fix it some day (ticket ?) but let’s not block on this now because it’s almost orthogonal to this discussion.

OK.

> That’s not why I’m claiming this test does not test anything at all: it does not test anything because “the only files tails-debugging-info displays as non-root simply do not exist in the context where this test is run; they only exist if persistence is enabled or if the Greeter has logged errors. I guess this was not the case back when this test was implemented but that’s how it is currently”

Right, I completely forgot about that!

> (from the commit:e50d21a445ff859efc59d2de57da55a27c91e343 message), which I noticed when I implemented commit:85cea6e62fc0714fa821187824a87153ca20df65 and added debug output to check it worked fine. That’s why I’m proposing we remove it.

OK, lets remove it.

#63 Updated by intrigeri 2018-08-15 16:20:25

• QA Check changed from Pass to Ready for QA

I’ve removed the test. Since you did not comment on my proposal to improve test (Feature #8514#note-58) yet and the freeze deadline is now in the past, I’ve reverted my change so we’re back to square one there. I think we’re now good to merge. I’ll build and test one last time though.

#65 Updated by segfault 2018-08-15 18:05:17

• related to Feature #15793: Improve ownership test in tails-debugging-info added

#66 Updated by intrigeri 2018-08-15 19:23:48

• Status changed from In Progress to Fix committed
• Assignee deleted (intrigeri)
• % Done changed from 60 to 100
• QA Check changed from Ready for QA to Pass

#67 Updated by intrigeri 2018-08-16 09:14:36

• Status changed from Fix committed to In Progress

Applied in changeset commit:37213b93ae8c736f27c0618482346eb8c8256638.

#68 Updated by intrigeri 2018-08-16 09:14:36

• Status changed from In Progress to Fix committed

Applied in changeset commit:d24273ff9e11ab1f7cf35cb04659ead470d6c038.

#69 Updated by intrigeri 2018-09-05 16:19:04

• Status changed from Fix committed to Resolved