Apply the right permissions to /etc/hosts

In the current Ganeti version when modifying /etc/hosts we mistakenly
give it the permissions of the temporary file we create to define its
content, which is by default 0600. This breaks most non-root
applications, and thus must be corrected. This patch forces the mode to
be 0644 (but we might decide to just use the mode of the previous
/etc/hosts, if we want to be more polite against any eventual
administrative choice). We also add a new assertFileMode() method for
unit tests and actually check in the SetEtcHostsEntry and
RemoveEtcHostsEntry tests that the mode is correct, to be sure not to
reintroduce this bug again. Also, a FIXME is added in the original
functions stating that it would be nice to use WriteFile+fn() rather
than reimplementing its functionality again.

Reviewed-by: iustinp

diff --git a/lib/utils.py b/lib/utils.py
index f887303..8bd1000 100644 (file)
--- a/lib/utils.py
+++ b/lib/utils.py
@@ -895,6 +895,7 @@ def SetEtcHostsEntry(file_name, ip, hostname, aliases):
   @param aliases: the list of aliases to add for the hostname
 
   """
+  # FIXME: use WriteFile + fn rather than duplicating its efforts
   # Ensure aliases are unique
   aliases = UniqueSequence([hostname] + aliases)[1:]
 
@@ -917,6 +918,7 @@ def SetEtcHostsEntry(file_name, ip, hostname, aliases):
 
         out.flush()
         os.fsync(out)
+        os.chmod(tmpname, 0644)
         os.rename(tmpname, file_name)
       finally:
         f.close()
@@ -950,6 +952,7 @@ def RemoveEtcHostsEntry(file_name, hostname):
   @param hostname: the hostname to be removed
 
   """
+  # FIXME: use WriteFile + fn rather than duplicating its efforts
   fd, tmpname = tempfile.mkstemp(dir=os.path.dirname(file_name))
   try:
     out = os.fdopen(fd, 'w')
@@ -971,6 +974,7 @@ def RemoveEtcHostsEntry(file_name, hostname):
 
         out.flush()
         os.fsync(out)
+        os.chmod(tmpname, 0644)
         os.rename(tmpname, file_name)
       finally:
         f.close()

diff --git a/test/ganeti.utils_unittest.py b/test/ganeti.utils_unittest.py
index 1050002..1c2992c 100755 (executable)
--- a/test/ganeti.utils_unittest.py
+++ b/test/ganeti.utils_unittest.py
@@ -527,6 +527,7 @@ class TestEtcHosts(testutils.GanetiTestCase):
       "127.0.0.1\tlocalhost\n"
       "192.168.1.1 router gw\n"
       "1.2.3.4\tmyhost.domain.tld myhost\n")
+    self.assertFileMode(self.tmpname, 0644)
 
   def testSettingExistingIp(self):
     SetEtcHostsEntry(self.tmpname, '192.168.1.1', 'myhost.domain.tld',
@@ -536,6 +537,7 @@ class TestEtcHosts(testutils.GanetiTestCase):
       "# This is a test file for /etc/hosts\n"
       "127.0.0.1\tlocalhost\n"
       "192.168.1.1\tmyhost.domain.tld myhost\n")
+    self.assertFileMode(self.tmpname, 0644)
 
   def testSettingDuplicateName(self):
     SetEtcHostsEntry(self.tmpname, '1.2.3.4', 'myhost', ['myhost'])
@@ -545,6 +547,7 @@ class TestEtcHosts(testutils.GanetiTestCase):
       "127.0.0.1\tlocalhost\n"
       "192.168.1.1 router gw\n"
       "1.2.3.4\tmyhost\n")
+    self.assertFileMode(self.tmpname, 0644)
 
   def testRemovingExistingHost(self):
     RemoveEtcHostsEntry(self.tmpname, 'router')
@@ -553,6 +556,7 @@ class TestEtcHosts(testutils.GanetiTestCase):
       "# This is a test file for /etc/hosts\n"
       "127.0.0.1\tlocalhost\n"
       "192.168.1.1 gw\n")
+    self.assertFileMode(self.tmpname, 0644)
 
   def testRemovingSingleExistingHost(self):
     RemoveEtcHostsEntry(self.tmpname, 'localhost')
@@ -560,6 +564,7 @@ class TestEtcHosts(testutils.GanetiTestCase):
     self.assertFileContent(self.tmpname,
       "# This is a test file for /etc/hosts\n"
       "192.168.1.1 router gw\n")
+    self.assertFileMode(self.tmpname, 0644)
 
   def testRemovingNonExistingHost(self):
     RemoveEtcHostsEntry(self.tmpname, 'myhost')
@@ -568,6 +573,7 @@ class TestEtcHosts(testutils.GanetiTestCase):
       "# This is a test file for /etc/hosts\n"
       "127.0.0.1\tlocalhost\n"
       "192.168.1.1 router gw\n")
+    self.assertFileMode(self.tmpname, 0644)
 
   def testRemovingAlias(self):
     RemoveEtcHostsEntry(self.tmpname, 'gw')
@@ -576,6 +582,7 @@ class TestEtcHosts(testutils.GanetiTestCase):
       "# This is a test file for /etc/hosts\n"
       "127.0.0.1\tlocalhost\n"
       "192.168.1.1 router\n")
+    self.assertFileMode(self.tmpname, 0644)
 
 
 class TestShellQuoting(unittest.TestCase):

diff --git a/test/testutils.py b/test/testutils.py
index f2980dd..c0932cc 100644 (file)
--- a/test/testutils.py
+++ b/test/testutils.py
@@ -22,6 +22,7 @@
 """Utilities for unit testing"""
 
 import os
+import stat
 import tempfile
 import unittest
 
@@ -46,7 +47,7 @@ class GanetiTestCase(unittest.TestCase):
         pass
 
   def assertFileContent(self, file_name, expected_content):
-    """Checks the content of a file is what we expect.
+    """Checks that the content of a file is what we expect.
 
     @type file_name: str
     @param file_name: the file whose contents we should check
@@ -57,6 +58,19 @@ class GanetiTestCase(unittest.TestCase):
     actual_content = utils.ReadFile(file_name)
     self.assertEqual(actual_content, expected_content)
 
+  def assertFileMode(self, file_name, expected_mode):
+    """Checks that the mode of a file is what we expect.
+
+    @type file_name: str
+    @param file_name: the file whose contents we should check
+    @type expected_mode: int
+    @param expected_mode: the mode we expect
+
+    """
+    st = os.stat(file_name)
+    actual_mode = stat.S_IMODE(st.st_mode)
+    self.assertEqual(actual_mode, expected_mode)
+
   @staticmethod
   def _TestDataFilename(name):
     """Returns the filename of a given test data file.