Merge lp:~stub/charm-helpers/bug-1478940-dont-reset-directory-perms into lp:charm-helpers

[Stuart Bishop]

Rendering a template should not make the target directory world readable as a side effect.

[Stuart Bishop (stub)]

I'm going to land this if there are no objections. The security bug has been pending for a few months.

=== modified file 'charmhelpers/core/templating.py'
--- charmhelpers/core/templating.py	2015-04-29 12:52:18 +0000
+++ charmhelpers/core/templating.py	2015-07-28 12:54:49 +0000
@@ -64,5 +64,9 @@
                     level=hookenv.ERROR)
         raise e
     content = template.render(context)
-    host.mkdir(os.path.dirname(target), owner, group, perms=0o755)
+    target_dir = os.path.dirname(target)
+    if not os.path.exists(target_dir):
+        # This is a terrible default directory permission, as the file
+        # or its siblings will often contain secrets.
+        host.mkdir(os.path.dirname(target), owner, group, perms=0o755)
     host.write_file(target, content.encode(encoding), owner, group, perms)

=== modified file 'tests/core/test_templating.py'
--- tests/core/test_templating.py	2015-04-29 12:52:18 +0000
+++ tests/core/test_templating.py	2015-07-28 12:54:49 +0000
@@ -53,6 +53,42 @@
             contents = open(fn2.name).read()
             self.assertRegexpMatches(contents, 'listen 80')
             self.assertEqual(fchown.call_count, 2)
+            # Not called, because the target directory exists. Calling
+            # it would make the target directory world readable and
+            # expose your secrets (!).
+            self.assertEqual(mkdir.call_count, 0)
+
+    @mock.patch.object(templating.os.path, 'exists')
+    @mock.patch.object(templating.host.os, 'fchown')
+    @mock.patch.object(templating.host, 'mkdir')
+    @mock.patch.object(templating.host, 'log')
+    def test_render_no_dir(self, log, mkdir, fchown, exists):
+        exists.return_value = False
+        with tempfile.NamedTemporaryFile() as fn1, \
+                tempfile.NamedTemporaryFile() as fn2:
+            context = {
+                'nats': {
+                    'port': '1234',
+                    'host': 'example.com',
+                },
+                'router': {
+                    'domain': 'api.foo.com'
+                },
+                'nginx_port': 80,
+            }
+            templating.render('fake_cc.yml', fn1.name,
+                              context, templates_dir=TEMPLATES_DIR)
+            contents = open(fn1.name).read()
+            self.assertRegexpMatches(contents, 'port: 1234')
+            self.assertRegexpMatches(contents, 'host: example.com')
+            self.assertRegexpMatches(contents, 'domain: api.foo.com')
+
+            templating.render('test.conf', fn2.name, context,
+                              templates_dir=TEMPLATES_DIR)
+            contents = open(fn2.name).read()
+            self.assertRegexpMatches(contents, 'listen 80')
+            self.assertEqual(fchown.call_count, 2)
+            # Target directory was created, world readable (!).
             self.assertEqual(mkdir.call_count, 2)
 
     @mock.patch.object(templating.host.os, 'fchown')