default umask of 0077 too restrictive for scripts

[Andreas Hasenack]

For some reason (still investigating), smart's config file was mode 0600 in five installations:
drwxr-xr-x 4 root root 4096 2008-08-07 20:19 .
drwxr-xr-x 25 root root 4096 2008-08-06 20:05 ..
-rw-r--r-- 1 root root 6056528 2008-08-07 20:17 cache
drwxr-xr-x 2 root root 12288 2008-08-07 20:19 channels
-rw------- 1 root root 1722 2008-08-07 20:19 config
-rw-r--r-- 1 root root 1914 2008-08-07 20:17 config.old
drwxr-xr-x 2 root root 4096 2008-08-06 21:06 packages

This prevents regular users from using it even for just read access.

[Andreas Hasenack (ahasenack)]

This happened because a "smart update" command was sent to the machine via Landscape's script execution plugin. That plugin sets the umask to 0077 before running the script.

Smart moves the config file to .old and creates a new one, so this operation is affected by the current umask. That's how the config file got mode 0600 while the previous one was 0644.

[Andreas Hasenack (ahasenack)]

After a brief discussion on IRC, we think it would be best to just set the umask to the default value in ubuntu before running scripts: 0022.

[Andreas Hasenack (ahasenack)]

Moved to thames-pre-7.

[Kevin McDermott (bigkevmcd)]

I've added a test and fix which sets the umask before executing the script, and then restores it when we finish removing the script.

[Thomas Herve (therve)]

OK cool. The only problem is that the test doesn't check the umask *during* the script, only that os.umask is called. Instead of 'echo hi', you could call umask and check the result?

Otherwise, +1.

[Jamu Kakar (jkakar)]

[1]

+ old_umask = os.umask(0022)
         env = {}
         attachment_dir = ""
         if attachments:
@@ -183,9 +184,10 @@
         if time_limit is not None:
             pp.schedule_cancel(time_limit)
         result = pp.result_deferred
- return result.addBoth(self._remove_script, filename, attachment_dir)
+ return result.addBoth(self._remove_script, filename, attachment_dir,
+ old_umask)

I think this whole block of code should be protected in a try/except like:

        old_umask = os.umask(0022)
        try:
            ...
            return result.addBoth(...)
        except:
            os.umask(old_umask)
            raise

Short and sweet, the best kind of branch. +1!

[Andreas Hasenack (ahasenack)]

umask is fine now!

qa + 1

[Andreas Hasenack (ahasenack)]

This was released into production.