launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/fix-dns-config-perm into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~rvb/maas/fix-dns-config-perm/+merge/122531

This branch fixes the permissions of the files written by the class DNSZoneConfig: the zone files and the reverse zone files.

= Pre-imp =

No pre-imp per se for this I'm afraid but I found this while trying to diagnose bug 1044210 and I discussed this with Julian who confirmed that he had to "massage" the permissions of the zone files when testing.

= Notes =

I've simply added a parameter to incremental_write (that parameter is passed down to atomic_write) and set that parameter to 0644 in the config-writing methods in DNSZoneConfig.

You'll notice that I've changed the permissions from 0744 to 0644.  I don't see why these config files should be executed.
-- 
https://code.launchpad.net/~rvb/maas/fix-dns-config-perm/+merge/122531
Your team MAAS Maintainers is requested to review the proposed merge of lp:~rvb/maas/fix-dns-config-perm into lp:maas.

=== modified file 'src/provisioningserver/dns/config.py'
--- src/provisioningserver/dns/config.py	2012-08-31 07:17:29 +0000
+++ src/provisioningserver/dns/config.py	2012-09-03 14:28:23 +0000
@@ -128,6 +128,11 @@
         return TEMPLATES_PATH
 
     @property
+    def access_permissions(self):
+        """The access permissions for the config file."""
+        return 0644
+
+    @property
     def target_dir(self):
         return conf.DNS_CONFIG_DIR
 
@@ -152,7 +157,8 @@
         kwargs.update(self.get_context())
         rendered = self.render_template(template, **kwargs)
         atomic_write(
-            rendered, self.target_path, overwrite=overwrite, mode=0744)
+            rendered, self.target_path, overwrite=overwrite,
+            mode=self.access_permissions)
 
 
 class DNSConfig(DNSConfigBase):
@@ -322,11 +328,13 @@
         template = self.get_template()
         kwargs.update(self.get_context())
         rendered = self.render_template(template, **kwargs)
-        incremental_write(rendered, self.target_path)
+        incremental_write(
+            rendered, self.target_path, mode=self.access_permissions)
 
     def write_reverse_config(self, **kwargs):
         """Write out the DNS reverse config file for this zone."""
         template = self.get_template()
         kwargs.update(self.get_reverse_context())
         rendered = self.render_template(template, **kwargs)
-        incremental_write(rendered, self.target_reverse_path)
+        incremental_write(
+            rendered, self.target_reverse_path, mode=self.access_permissions)

=== modified file 'src/provisioningserver/dns/tests/test_config.py'
--- src/provisioningserver/dns/tests/test_config.py	2012-08-31 07:17:29 +0000
+++ src/provisioningserver/dns/tests/test_config.py	2012-09-03 14:28:23 +0000
@@ -382,3 +382,26 @@
                 )
             )
         )
+
+    def test_DNSZoneConfig_config_file_is_world_readable(self):
+        self.patch(DNSConfig, 'target_dir', self.make_dir())
+        dns_zone_config = DNSZoneConfig(
+            factory.getRandomString(), serial=random.randint(1, 100),
+            dns_ip=factory.getRandomIPAddress(),
+            **network_infos(factory.getRandomNetwork()))
+        dns_zone_config.write_config()
+        self.assertEqual(
+            stat.S_IROTH,
+            os.stat(dns_zone_config.target_path).st_mode & stat.S_IROTH)
+
+    def test_DNSZoneConfig_reverse_config_file_is_world_readable(self):
+        self.patch(DNSConfig, 'target_dir', self.make_dir())
+        dns_zone_config = DNSZoneConfig(
+            factory.getRandomString(), serial=random.randint(1, 100),
+            dns_ip=factory.getRandomIPAddress(),
+            **network_infos(factory.getRandomNetwork()))
+        dns_zone_config.write_reverse_config()
+        self.assertEqual(
+            stat.S_IROTH,
+            os.stat(
+                dns_zone_config.target_reverse_path).st_mode & stat.S_IROTH)

=== modified file 'src/provisioningserver/tests/test_utils.py'
--- src/provisioningserver/tests/test_utils.py	2012-09-03 10:50:27 +0000
+++ src/provisioningserver/tests/test_utils.py	2012-09-03 14:28:23 +0000
@@ -175,6 +175,12 @@
         self.assertAlmostEqual(
             os.stat(filename).st_mtime, old_mtime + 1, delta=0.01)
 
+    def test_incremental_write_sets_permissions(self):
+        atomic_file = self.make_file()
+        mode = 0323
+        incremental_write(factory.getRandomString(), atomic_file, mode=mode)
+        self.assertEqual(mode, stat.S_IMODE(os.stat(atomic_file).st_mode))
+
 
 class TestGetMTime(TestCase):
     """Test `get_mtime`."""

=== modified file 'src/provisioningserver/utils.py'
--- src/provisioningserver/utils.py	2012-08-31 23:16:59 +0000
+++ src/provisioningserver/utils.py	2012-09-03 14:28:23 +0000
@@ -101,12 +101,14 @@
             os.remove(temp_file)
 
 
-def incremental_write(content, filename):
+def incremental_write(content, filename, mode=0600):
     """Write the given `content` into the file `filename` and
     increment the modification time by 1 sec.
+
+    :param mode: Access permissions for the file.
     """
     old_mtime = get_mtime(filename)
-    atomic_write(content, filename)
+    atomic_write(content, filename, mode=mode)
     new_mtime = pick_new_mtime(old_mtime)
     os.utime(filename, (new_mtime, new_mtime))