Bug #8950: Test suite fails if features/misc_files/* or parent directories have strict permissions

Bug #8950

Test suite fails if features/misc_files/* or parent directories have strict permissions

Added by intrigeri 2015-02-24 15:13:06 . Updated 2015-05-12 18:43:42 .

Status:

Resolved

Priority:

Normal

Assignee:

Category:

Test suite

Target version:

Tails_1.4

Start date:

2015-02-24

Due date:

% Done:

100%

Feature Branch:

test/8950-fs-shares-from-temp-dir

Type of work:

Code

Blueprint:

Starter:

Affected tool:

Deliverable for:

Description

We share this directory (with libvirt FS sharing) and then do stuff like I copy the sample videos to "/home/amnesia/Persistent" as user "amnesia". If the Git repository was cloned or updated with a strict umask, then I see:

      Failed to copy file:

      cp: cannot open `/tmp/shared_video_dir/video.mp4' for reading: Permission denied
       (MiniTest::Assertion)
      ./features/step_definitions/common_steps.rb:755:in `/^I copy "([^"]+)" to "([^"]+)" as user "([^"]+)"$/'

I’ve also been told that the 9p FS sharing fails if features/misc_files itself isn’t readable by libvirt (possibly because of parent directories’ permission, by the way).

I think this requirement should at least be written down somewhere, and ideally the test suite would check if it’s satisfied before running stuff that will fail otherwise. I don’t think we can automatically “fix” those permissions without doing dangerous things like recursively making all parent directories world-readable.

This just hit me on isotester1.lizard, so I’m making this a subticket of ~~Feature #8539~~ => kytv, anonym: we need an assignee on it.

Subtasks

History

#1 Updated by intrigeri 2015-02-24 15:20:13

blocks #8538 added

#2 Updated by intrigeri 2015-03-18 11:06:47

Hey anonym & kytv! We need an assignee for this ticket, don’t we?

#3 Updated by anonym 2015-03-18 16:31:39

Assignee set to kytv
QA Check set to Info Needed

intrigeri wrote:
> Hey anonym & kytv! We need an assignee for this ticket, don’t we?

If you don’t want it, kytv, please reassign it to me.

#4 Updated by kytv 2015-04-12 18:36:27

QA Check changed from Info Needed to Dev Needed

#5 Updated by kytv 2015-04-20 18:05:49

Assignee changed from kytv to anonym
QA Check changed from Dev Needed to Ready for QA
Feature Branch set to kytv:test/8950-check_misc_files-permissions

Perhaps this is acceptable.

root@testsuite:/home/kytv/tails# chmod 640 features/misc_files/sample.pdf 
root@testsuite:/home/kytv/tails# ./run_test_suite --iso tails-i386-1.3.2.iso --view -- features/ssh.feature 
ERROR: libvirt-qemu cannot access "/home/kytv/tails/features/misc_files/sample.pdf".
root@testsuite:/home/kytv/tails# chmod 644 features/misc_files/sample.pdf 
root@testsuite:/home/kytv/tails# ./run_test_suite --iso tails-i386-1.3.2.iso --view -- features/ssh.feature 
Virtual X framebuffer started on display :0
VNC server running on: localhost:5900
[info] Sikuli vision engine loaded.
Testing ISO image: tails-i386-1.3.2.iso
........
........

#6 Updated by kytv 2015-04-20 18:06:00

% Done changed from 0 to 30

#7 Updated by BitingBird 2015-04-21 14:10:11

Status changed from Confirmed to In Progress

#8 Updated by anonym 2015-05-05 12:06:03

Assignee changed from anonym to kytv
% Done changed from 30 to 40
QA Check changed from Ready for QA to Dev Needed

I hate to do this but:

+        if ! sudo -u libvirt-qemu test -r ${f}; then

(First of all, you should quote ${f} since it’s a path.)

This makes us either depend on running the test suite as root, or as a user with passwordless sudo access to the libvirt-qemu user. I would really like a better reason to introduce such dependencies than a simple access rights check. Instead, we can fix this with something like this (untested):

--- a/run_test_suite
+++ b/run_test_suite
@@ -61,9 +61,23 @@ check_dependency() {
     fi
 }

+# Check if file $1 is readable by user $2 or group $3 (or if its world
+# readable).
+file_readable_by_user() {
+    local owner="$(stat -L --print=%U "${1}")"
+    local group_owner="$(stat -L --print=%G "${1}")"
+    local access_rights="$(stat -L --print=%A "${1}")"
+    local access_owner_r=$(echo "${access_rights}" | cut -c 2)
+    local access_group_r=$(echo "${access_rights}" | cut -c 5)
+    local access_world_r=$(echo "${access_rights}" | cut -c 8)
+    [ "${owner}" = "${2}" -a "${access_owner_r}" = r ] || \
+    [ "${group_owner}" = "${3}" -a "${access_group_r}" = r ] || \
+    [ "${access_world_r}" = r ]
+}
+
 check_permissions() {
     for f in features/misc_files features/misc_files/*; do
-        if ! sudo -u libvirt-qemu test -r ${f}; then
+        if ! file_readable_by_user "${f}" libvirt-qemu libvirt-qemu; then
             echo "ERROR: libvirt-qemu cannot access \"`pwd`/$f\"." >&2
             local FAIL=1
         fi

Sorry to request this, it’s not very pretty. At least I provided a patch! :) What do you think?

#9 Updated by kytv 2015-05-05 23:12:50

Assignee changed from kytv to anonym
QA Check changed from Dev Needed to Ready for QA

Indeed, this is better. (Tested and verified working fine)

#10 Updated by intrigeri 2015-05-06 09:43:38

> [...]
> +file_readable_by_user() {
> +    local owner="$(stat -L --print=%U "${1}")"
> +    local group_owner="$(stat -L --print=%G "${1}")"
> +    local access_rights="$(stat -L --print=%A "${1}")"
> +    local access_owner_r=$(echo "${access_rights}" | cut -c 2)
> +    local access_group_r=$(echo "${access_rights}" | cut -c 5)
> +    local access_world_r=$(echo "${access_rights}" | cut -c 8)
> +    [ "${owner}" = "${2}" -a "${access_owner_r}" = r ] || \
> +    [ "${group_owner}" = "${3}" -a "${access_group_r}" = r ] || \
> +    [ "${access_world_r}" = r ]
> +}
> [...]
>

Nitpicking: this doesn’t take ACLs into account. Something like would, and is nicer at least to my eyes:

file_readable_by_user() {
   perl -E "use filetest 'access'; -r '$1' or exit 1"
}

Note that this add a dependency on perl-modules to the test suite, but I’m pretty sure that this package is pulled in many other ways already on a working tester system.

#11 Updated by anonym 2015-05-07 11:46:17

Assignee changed from anonym to kytv
QA Check changed from Ready for QA to Dev Needed

What intrigeri suggests is much better! Please rewrite the commit to do that instead! :)

#12 Updated by anonym 2015-05-08 11:18:33

Assignee changed from kytv to anonym
% Done changed from 40 to 0
Feature Branch deleted (~~kytv:test/8950-check_misc_files-permissions~~)

anonym wrote:
> What intrigeri suggests is much better! Please rewrite the commit to do that instead! :)

This is embarrassing, but it seems I just took intrigeri’s suggestion for granted, and can’t have looked at it at all. :S I don’t see how it possibly could do the check for a particular user and group (other than the one executing the code) so it won’t work. The issue with ACLs are real concerns, though, and I doubt we want to start parsing the output of getfacl -e…

Solving the actual permission issue (instead of just checking if it’s the case and then error out) would “work around” this issue, and since I’m the only one with an issue, I’ll do it. Actually, for some time I’ve been annoyed by us writing stuff into this Git folder and then share it when we have a temporary dir more suitable for that. IMHO we should instead share folders from $config["TMPDIR"], and ensure that they have the correct perms (e.g. we can set libvirt-qemu as group and make it group readable/accessible). We then copy or generate the needed files there instead. I’m on to this now…

Sorry for having wasted your time!

#13 Updated by intrigeri 2015-05-08 12:12:01

> This is embarrassing, but it seems I just took intrigeri’s suggestion for granted, and can’t have looked at it at all. :S I don’t see how it possibly could do the check for a particular user and group (other than the one executing the code) so it won’t work.

Indeed, filetest uses the access(2) syscall, that doesn’t provide the needed functionality. Sorry :/

> The issue with ACLs are real concerns, though, and I doubt we want to start parsing the output of getfacl -e…

Indeed, please let’s not do that.

> Solving the actual permission issue (instead of just checking if it’s the case and then error out) would “work around” this issue, and since I’m the only one with an issue, I’ll do it.

Well, I’ve reported this bug since I was hit by it on isotester1.lizard. And earlier, I’ve seen Alan go mad when facing pretty obscure error messages caused by wrong permissions.

> Actually, for some time I’ve been annoyed by us writing stuff into this Git folder and then share it when we have a temporary dir more suitable for that. IMHO we should instead share folders from $config["TMPDIR"], and ensure that they have the correct perms (e.g. we can set libvirt-qemu as group and make it group readable/accessible). We then copy or generate the needed files there instead. I’m on to this now…

Excellent idea. Would be a much nicer design.

#14 Updated by anonym 2015-05-08 14:24:40

Applied in changeset commit:3a2a280fcf22e7c99849010bce70ec8afd4f2b1b.

#15 Updated by anonym 2015-05-08 14:26:07

Assignee changed from anonym to kytv
% Done changed from 0 to 50
QA Check changed from Dev Needed to Ready for QA
Feature Branch set to test/8950-fs-shares-from-temp-dir

So I think this is a better solution, in the end, and it’s still possible to run the test suite as non-root.

#16 Updated by kytv 2015-05-08 18:42:29

Assignee changed from kytv to anonym
QA Check changed from Ready for QA to Dev Needed

To test this I set the permissions on the files in features/misc_files to 600, owned by me. I run the test suite is run as root user. ls -l shows the following:

-rw------- 1 kytv kytv 22K Feb 25 22:24 sample.pdf
-rw------- 1 kytv kytv 184 Feb 25 22:24 sample.tex

Running scenarios that use the shares yielded the following results:

  Given a computer                                                     # features/step_definitions/common_steps.rb:66
    And I setup a filesystem share containing a sample PDF               # features/step_definitions/checks.rb:131
[log] CLICK on (1024,384)
[log] TYPE "    "
[log] TYPE " autotest_never_use_this_option 
"
calling as root: echo 'hello?'
call returned: [0, "hello?\n", ""]
calling as root: mkdir -p /tmp/shared_pdf_dir
call returned: [0, "", ""]
calling as root: mount -t 9p -o trans=virtio /tmp/shared_pdf_dir /tmp/shared_pdf_dir
call returned: [0, "", ""]
[log] CLICK on (638,448)
    And I start Tails from DVD with network unplugged and I login        # features/step_definitions/common_steps.rb:149
calling as amnesia: cp "/tmp/shared_pdf_dir/sample.pdf" "/home/amnesia/sample.pdf"
call returned: [1, "", "cp: cannot open `/tmp/shared_pdf_dir/sample.pdf' for reading: Permission denied\n"]
    Then MAT can clean some sample PDF file                              # features/step_definitions/checks.rb:148
      Failed to copy file:

      cp: cannot open `/tmp/shared_pdf_dir/sample.pdf' for reading: Permission denied
      .
      <false> is not true. (Test::Unit::AssertionFailedError)
      ./features/step_definitions/common_steps.rb:772:in `/^I copy "([^"]+)" to "([^"]+)" as user "([^"]+)"$/'
      ./features/step_definitions/checks.rb:153:in `block (2 levels) in <top (required)>'
      ./features/step_definitions/checks.rb:150:in `each'
      ./features/step_definitions/checks.rb:150:in `/^MAT can clean some sample PDF file$/'
      features/checks.feature:82:in `Then MAT can clean some sample PDF file'

    Given a computer                                                         # features/step_definitions/common_steps.rb:66
    And I setup a filesystem share containing sample videos                  # features/step_definitions/totem.rb:14
    And I start Tails from DVD with network unplugged and I login            # features/step_definitions/common_steps.rb:149
    And I copy the sample videos to "/home/amnesia" as user "amnesia"        # features/step_definitions/totem.rb:19
    When I open "/home/amnesia/video.mp4" with Totem                         # features/step_definitions/totem.rb:42
    Then I see "SampleLocalMp4VideoFrame.png" after at most 10 seconds       # features/step_definitions/common_steps.rb:429
      FindFailed: can not find SampleLocalMp4VideoFrame.png on the screen.
      Line ?, in File ? (RuntimeError)
      features/totem.feature:21:in `Then I see "SampleLocalMp4VideoFrame.png" after at most 10 seconds'
    Given I close Totem                                                      # features/step_definitions/totem.rb:47
    And I copy the sample videos to "/home/amnesia/.gnupg" as user "amnesia" # features/step_definitions/totem.rb:19
    When I try to open "/home/amnesia/.gnupg/video.mp4" with Totem           # features/step_definitions/totem.rb:42
    Then I see "TotemUnableToOpen.png" after at most 10 seconds

I see “Location not found” in the totem scenario.

I fixed the permissions, setting them to 644 and the mat scenario passed. The totem one still fails with a Location not found error on the screen.

#17 Updated by anonym 2015-05-08 19:16:34

Assignee changed from anonym to kytv
QA Check changed from Dev Needed to Ready for QA

kytv wrote:
> To test this I set the permissions on the files in features/misc_files to 600, owned by me.

That’s not how to test this, I think. All files in there are stored in Git and should have the permissions stored in Git, which include that they are world readable. The problem before was that we generated files there, and depending on the umask they could be non-world-readable and hence no readable by the libvirt-qemu user. Right?

#18 Updated by kytv 2015-05-08 23:03:38

Assignee changed from kytv to intrigeri

With respect to the totem failure I wasn’t able to reproduce it after remounting the tmpfs I have tmpdir for the test suite set to. I don’t know what exactly happened there.

Anyhow, the code looks good, the test scenarios are now passing, and I do think this is an improvement…but this doesn’t seem to solve the problem.

anonym wrote:
> That’s not how to test this, I think.

If the expectation is that the user running the test suite owns the git tree (as I gathered from IRC), that doesn’t seem to be sufficient in this case.

I run the test suite as the root user. (Normally) My git checkout is owned by kytv:kytv, but the tree is world-readable. Everything works with this configuration.

As a test I ran the following:

chown -R root:root $CLONED_REPOSITORY
chmod 600 $CLONED_REPOSITORY/features/misc_files/*

root owns the checkout. root can read everything, including features/misc_files/*, but the scenario still fails with

  Scenario: MAT can clean a PDF file                              # features/checks.feature:4
    Given a computer                                              # features/step_definitions/common_steps.rb:66
    And I setup a filesystem share containing a sample PDF        # features/step_definitions/checks.rb:131
    And I start Tails from DVD with network unplugged and I login # features/step_definitions/common_steps.rb:149
    Then MAT can clean some sample PDF file                       # features/step_definitions/checks.rb:148
      Failed to copy file:

      cp: cannot open `/tmp/shared_pdf_dir/sample.pdf' for reading: Permission denied
      .
      <false> is not true. (Test::Unit::AssertionFailedError)
      ./features/step_definitions/common_steps.rb:772:in `/^I copy "([^"]+)" to "([^"]+)" as user "([^"]+)"$/'
      ./features/step_definitions/checks.rb:153:in `block (2 levels) in <top (required)>'
      ./features/step_definitions/checks.rb:150:in `each'
      ./features/step_definitions/checks.rb:150:in `/^MAT can clean some sample PDF file$/'
      features/checks.feature:8:in `Then MAT can clean some sample PDF file'

and

# features/scripts/vm-execute "ls -l /tmp"
Return status: 1
STDOUT:
total 0
drwx------ 2 amnesia    amnesia     60 May  8 22:38 gpg-T58Vwh
drwx------ 2 root       root        40 May  8 22:38 pulse-2L9K88eMlGn7
drwx------ 2 Debian-gdm Debian-gdm  40 May  8 22:39 pulse-opbz88UQYjqz
drwx------ 2 root       root        40 May  8 22:38 pulse-PKdhtXMmr18n
drwx------ 2 amnesia    amnesia    100 May  8 22:38 pulse-wB4NzQ9gvL2g
d????????? ? ?          ?            ?            ? shared_pdf_dir
STDERR:
ls: cannot access /tmp/shared_pdf_dir: Invalid argument

#19 Updated by kytv 2015-05-08 23:26:38

Assignee changed from intrigeri to kytv
QA Check changed from Ready for QA to Dev Needed

#20 Updated by kytv 2015-05-08 23:35:19

Assignee changed from kytv to anonym

This one byte change fixes it.

diff --git a/features/support/helpers/vm_helper.rb b/features/support/helpers/vm_helper.rb
index 07725ec..95231b6 100644
--- a/features/support/helpers/vm_helper.rb
+++ b/features/support/helpers/vm_helper.rb
@@ -253,7 +253,7 @@ class VM
     # The complete source directory must be group readable by the user
     # running the virtual machine.
     FileUtils.chown_R(nil, "libvirt-qemu", source)
-    FileUtils.chmod_R("g+rX", source)
+    FileUtils.chmod_R("og+rX", source)
     xml = REXML::Document.new(File.read("#{@xml_path}/fs_share.xml"))
     xml.elements['filesystem/source'].attributes['dir'] = source
     xml.elements['filesystem/target'].attributes['dir'] = tag

After this, I’m satisfied that this branch does what it should. :)

(Even though it seems to be obvious to me it may, however, be worthwhile to document that the user running the test suite needs read-access to the Git clone.)

#21 Updated by intrigeri 2015-05-09 01:01:13

> (Even though it seems to be obvious to me it may, however, be worthwhile to document that the user running the test suite needs read-access to the Git clone.)

As said on IRC earlier, I think it’s a must, and basically a one-line change somewhere. Would take less time to just do it than to argue whether it’s needed :)

#22 Updated by anonym 2015-05-10 19:08:49

Assignee changed from anonym to intrigeri
QA Check changed from Dev Needed to Ready for QA

Ok, I’ve fixed these last two things now.

#23 Updated by intrigeri 2015-05-11 11:24:14

% Done changed from 50 to 60

Code review passes, except:

@shared_iso_dir_on_guest = "/tmp/shared_pdf_dir"

… that either anonym or I will fix before merging.

#24 Updated by intrigeri 2015-05-11 12:22:25

Status changed from In Progress to Fix committed
% Done changed from 60 to 100

Applied in changeset commit:756a27ee4bb55bb33195bd70d23b13ed294d01d0.

#25 Updated by intrigeri 2015-05-11 13:03:57

Assignee deleted (~~intrigeri~~)
QA Check changed from Ready for QA to Pass

#26 Updated by BitingBird 2015-05-12 18:43:42

Status changed from Fix committed to Resolved