Merge lp:~jtv/maas/bug-1042865 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Fix permissions for ephemeral ("commissioning") PXE image directory in TFTP.

If the import scripts run with an overly strict umask, maas-import-ephemerals may produce a "commissioning" image directory that the TFTP server is not allowed to read.

The reason seems to be that the download code creates a temporary directory to download in, then sets a more permissive umask, and finally calls into maas-provision to install the new image — which copies the temporary directory including its permissions from the time with the stricter umask.

In this branch I address that in two ways:

1. The umask really didn't belong in the inner loop, since it affects global script state. I hoisted it up to a global level, so that the temporary directory is created with the new umask.

2. Before copying the image, I make it world-readable. The "X" flag on chmod pertains to the "search" permission. It sets the "x" bit on directories ("search") but not on files ("execute").

One open question is what happens to systems that had a non-world-readable directory created by an import script run from an older maas version. That might be worth fixing up in postinst for now. We don't need to carry it around forever: a permissions problem with the image go away as soon as the script downloads the first image update. (Also, chmod -R a+rX is much easier to do in shell than in python.)

Sadly, the ephemerals import script has not been made testable and is not tested. I can't even run it locally. We'll have to see in Q/A whether this really fixes the problem.

Jeroen

[Gavin Panella (allenap)]

Nice.

review: Approve

[Scott Moser (smoser)]

I really think you fixed this in the wrong place.

 install_tftp_image "$wd" "$r_arch" "generic" "$r_release"

is what is called to "take this directory and install it to tftp".

it is *that* program that should take care to make sure it is consumable.
Not the caller.

[Scott Moser (smoser)]

Yeah.
This patch is wrong.
You should not globally change umask, which changes programs invoked by this program also.

Additionally, your change to umask is not even necessary as
 a.) mktemp is going to still do the right thing and make temporary directories with secure permissions
     $ sh -c 'umask a+r; d=$(mktemp -d); ls -ld $d; rmdir $d;'
     drwx------ 2 smoser smoser 4096 Aug 31 10:00 /tmp/tmp.Ez8zyx2rh9

 b.) you're doing a 'chmod -R' there anyway to open up permissions.

=== modified file 'scripts/maas-import-ephemerals'
--- scripts/maas-import-ephemerals	2012-08-15 12:54:49 +0000
+++ scripts/maas-import-ephemerals	2012-08-30 06:22:19 +0000
@@ -295,10 +295,6 @@
         cp -- "$src/$filename" "$tmpdir/"
     done
 
-    # All files we create here are public.  The TFTP user will need to be
-    # able to read them.
-    umask a+r
-
     debug 1 "maas-provision install-pxe-image --arch=$arch --subarch=$subarch --release=$release --purpose=commissioning --image=$tmpdir"
     maas-provision install-pxe-image \
         --arch=$arch --subarch=$subarch --release=$release \
@@ -346,6 +342,12 @@
 EOF
 fi
 
+
+# All files we create here are public.  The TFTP user will need to be
+# able to read them.
+umask a+r
+
+
 updates=0
 for release in $RELEASES; do
     for arch in $ARCHES; do
@@ -372,6 +374,7 @@
                 "$r_serial" "$r_arch" "$r_url" "$r_name" ||
                 fail "failed to prepare image for $release/$arch"
 
+            chmod -R a+rX "$wd"
             install_tftp_image "$wd" "$r_arch" "generic" "$r_release"
 
             target_name="${TARGET_NAME_PREFIX}${r_name}"