cloud-init-dev team mailing list archive

[Scott Moser <smoser@xxxxxxxxxx>]

Scott Moser has proposed merging ~smoser/cloud-init:bug/1707222-use-run-for-tmpdir into cloud-init:master.

Commit message:
Use /run/cloud-init for tempfile operations.

During boot, the usage of /tmp is not safe.  In systemd systems,
systemd-tmpfiles-clean may run at any point and clear out a temp file
while cloud-init is using it.  The solution was to use
/run/cloud-init/tmp.

LP: #1707222


Requested reviews:
  cloud-init commiters (cloud-init-dev)
Related bugs:
  Bug #1707222 in cloud-init: "usage of /tmp during boot is not safe due to systemd-tmpfiles-clean"
  https://bugs.launchpad.net/cloud-init/+bug/1707222

For more details, see:
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/329811
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~smoser/cloud-init:bug/1707222-use-run-for-tmpdir into cloud-init:master.

diff --git a/cloudinit/config/cc_bootcmd.py b/cloudinit/config/cc_bootcmd.py
index 604f93b..9c0476a 100644
--- a/cloudinit/config/cc_bootcmd.py
+++ b/cloudinit/config/cc_bootcmd.py
@@ -37,6 +37,7 @@ specified either as lists or strings. For invocation details, see ``runcmd``.
 import os
 
 from cloudinit.settings import PER_ALWAYS
+from cloudinit import temp_utils
 from cloudinit import util
 
 frequency = PER_ALWAYS
@@ -49,7 +50,7 @@ def handle(name, cfg, cloud, log, _args):
                    " no 'bootcmd' key in configuration"), name)
         return
 
-    with util.ExtendedTemporaryFile(suffix=".sh") as tmpf:
+    with temp_utils.ExtendedTemporaryFile(suffix=".sh") as tmpf:
         try:
             content = util.shellify(cfg["bootcmd"])
             tmpf.write(util.encode_text(content))
diff --git a/cloudinit/config/cc_chef.py b/cloudinit/config/cc_chef.py
index 02c70b1..c192dd3 100644
--- a/cloudinit/config/cc_chef.py
+++ b/cloudinit/config/cc_chef.py
@@ -71,6 +71,7 @@ import itertools
 import json
 import os
 
+from cloudinit import temp_utils
 from cloudinit import templater
 from cloudinit import url_helper
 from cloudinit import util
@@ -303,7 +304,7 @@ def install_chef(cloud, chef_cfg, log):
                                                  "omnibus_url_retries",
                                                  default=OMNIBUS_URL_RETRIES))
         content = url_helper.readurl(url=url, retries=retries).contents
-        with util.tempdir() as tmpd:
+        with temp_utils.tempdir() as tmpd:
             # Use tmpdir over tmpfile to avoid 'text file busy' on execute
             tmpf = "%s/chef-omnibus-install" % tmpd
             util.write_file(tmpf, content, mode=0o700)
diff --git a/cloudinit/config/cc_snappy.py b/cloudinit/config/cc_snappy.py
index a9682f1..eecb817 100644
--- a/cloudinit/config/cc_snappy.py
+++ b/cloudinit/config/cc_snappy.py
@@ -63,11 +63,11 @@ is ``auto``. Options are:
 
 from cloudinit import log as logging
 from cloudinit.settings import PER_INSTANCE
+from cloudinit import temp_utils
 from cloudinit import util
 
 import glob
 import os
-import tempfile
 
 LOG = logging.getLogger(__name__)
 
@@ -183,7 +183,7 @@ def render_snap_op(op, name, path=None, cfgfile=None, config=None):
             #      config
             # Note, however, we do not touch config files on disk.
             nested_cfg = {'config': {shortname: config}}
-            (fd, cfg_tmpf) = tempfile.mkstemp()
+            (fd, cfg_tmpf) = temp_utils.mkstemp()
             os.write(fd, util.yaml_dumps(nested_cfg).encode())
             os.close(fd)
             cfgfile = cfg_tmpf
diff --git a/cloudinit/net/dhcp.py b/cloudinit/net/dhcp.py
index c7febc5..c842c83 100644
--- a/cloudinit/net/dhcp.py
+++ b/cloudinit/net/dhcp.py
@@ -9,6 +9,7 @@ import os
 import re
 
 from cloudinit.net import find_fallback_nic, get_devicelist
+from cloudinit import temp_utils
 from cloudinit import util
 
 LOG = logging.getLogger(__name__)
@@ -47,7 +48,7 @@ def maybe_perform_dhcp_discovery(nic=None):
     if not dhclient_path:
         LOG.debug('Skip dhclient configuration: No dhclient command found.')
         return {}
-    with util.tempdir(prefix='cloud-init-dhcp-') as tmpdir:
+    with temp_utils.tempdir(prefix='cloud-init-dhcp-') as tmpdir:
         return dhcp_discovery(dhclient_path, nic, tmpdir)
 
 
diff --git a/cloudinit/sources/helpers/azure.py b/cloudinit/sources/helpers/azure.py
index e22409d..28ed0ae 100644
--- a/cloudinit/sources/helpers/azure.py
+++ b/cloudinit/sources/helpers/azure.py
@@ -6,10 +6,10 @@ import os
 import re
 import socket
 import struct
-import tempfile
 import time
 
 from cloudinit import stages
+from cloudinit import temp_utils
 from contextlib import contextmanager
 from xml.etree import ElementTree
 
@@ -111,7 +111,7 @@ class OpenSSLManager(object):
     }
 
     def __init__(self):
-        self.tmpdir = tempfile.mkdtemp()
+        self.tmpdir = temp_utils.mkdtemp()
         self.certificate = None
         self.generate_certificate()
 
diff --git a/cloudinit/temp_utils.py b/cloudinit/temp_utils.py
new file mode 100644
index 0000000..118282a
--- /dev/null
+++ b/cloudinit/temp_utils.py
@@ -0,0 +1,77 @@
+import contextlib
+import os
+import shutil
+import tempfile
+
+_TMPDIR = None
+_ROOT_TMPDIR = "/run/cloud-init/tmp"
+
+
+def _tempfile_dir_arg(odir=None):
+    """Return the proper 'dir' argument for tempfile functions.
+
+    When root, cloud-init will use /run/cloud-init/tmp to avoid
+    any cleaning that would happen.
+"""
+
+    if odir is not None:
+        return odir
+
+    global _TMPDIR
+    if _TMPDIR is None:
+        if os.getuid() == 0:
+            tdir = _ROOT_TMPDIR
+        else:
+            tdir = os.environ.get('TMPDIR', '/tmp')
+        if not os.path.isdir(tdir):
+            os.makedirs(tdir)
+            os.chmod(tdir, 0o1777)
+    return _TMPDIR
+
+
+def ExtendedTemporaryFile(**kwargs):
+    kwargs['dir'] = _tempfile_dir_arg(kwargs.pop('dir', None))
+    fh = tempfile.NamedTemporaryFile(**kwargs)
+    # Replace its unlink with a quiet version
+    # that does not raise errors when the
+    # file to unlink has been unlinked elsewhere..
+
+    def _unlink_if_exists(path):
+        if not os.path.exists(path):
+            return
+        os.unlink(path)
+
+    fh.unlink = _unlink_if_exists
+
+    # Add a new method that will unlink
+    # right 'now' but still lets the exit
+    # method attempt to remove it (which will
+    # not throw due to our del file being quiet
+    # about files that are not there)
+    def unlink_now():
+        fh.unlink(fh.name)
+
+    setattr(fh, 'unlink_now', unlink_now)
+    return fh
+
+
+@contextlib.contextmanager
+def tempdir(**kwargs):
+    # This seems like it was only added in python 3.2
+    # Make it since its useful...
+    # See: http://bugs.python.org/file12970/tempdir.patch
+    tdir = mkdtemp(**kwargs)
+    try:
+        yield tdir
+    finally:
+        shutil.rmtree(tdir)
+
+
+def mkdtemp(**kwargs):
+    kwargs['dir'] = _tempfile_dir_arg(kwargs.pop('dir', None))
+    return tempfile.mkdtemp(**kwargs)
+
+
+def mkstemp(**kwargs):
+    kwargs['dir'] = _tempfile_dir_arg(kwargs.pop('dir', None))
+    return tempfile.mkstemp(**kwargs)
diff --git a/cloudinit/util.py b/cloudinit/util.py
index 609e94c..ae5cda8 100644
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -30,7 +30,6 @@ import stat
 import string
 import subprocess
 import sys
-import tempfile
 import time
 
 from errno import ENOENT, ENOEXEC
@@ -45,6 +44,7 @@ from cloudinit import importer
 from cloudinit import log as logging
 from cloudinit import mergers
 from cloudinit import safeyaml
+from cloudinit import temp_utils
 from cloudinit import type_utils
 from cloudinit import url_helper
 from cloudinit import version
@@ -349,26 +349,6 @@ class DecompressionError(Exception):
     pass
 
 
-def ExtendedTemporaryFile(**kwargs):
-    fh = tempfile.NamedTemporaryFile(**kwargs)
-    # Replace its unlink with a quiet version
-    # that does not raise errors when the
-    # file to unlink has been unlinked elsewhere..
-    LOG.debug("Created temporary file %s", fh.name)
-    fh.unlink = del_file
-
-    # Add a new method that will unlink
-    # right 'now' but still lets the exit
-    # method attempt to remove it (which will
-    # not throw due to our del file being quiet
-    # about files that are not there)
-    def unlink_now():
-        fh.unlink(fh.name)
-
-    setattr(fh, 'unlink_now', unlink_now)
-    return fh
-
-
 def fork_cb(child_cb, *args, **kwargs):
     fid = os.fork()
     if fid == 0:
@@ -790,18 +770,6 @@ def umask(n_msk):
         os.umask(old)
 
 
-@contextlib.contextmanager
-def tempdir(**kwargs):
-    # This seems like it was only added in python 3.2
-    # Make it since its useful...
-    # See: http://bugs.python.org/file12970/tempdir.patch
-    tdir = tempfile.mkdtemp(**kwargs)
-    try:
-        yield tdir
-    finally:
-        del_dir(tdir)
-
-
 def center(text, fill, max_len):
     return '{0:{fill}{align}{size}}'.format(text, fill=fill,
                                             align="^", size=max_len)
@@ -1587,7 +1555,7 @@ def mount_cb(device, callback, data=None, rw=False, mtype=None, sync=True):
         mtypes = ['']
 
     mounted = mounts()
-    with tempdir() as tmpd:
+    with temp_utils.tempdir() as tmpd:
         umount = False
         if os.path.realpath(device) in mounted:
             mountpoint = mounted[os.path.realpath(device)]['mountpoint']
diff --git a/tests/unittests/test_datasource/test_azure_helper.py b/tests/unittests/test_datasource/test_azure_helper.py
index b2d2971..69e9c49 100644
--- a/tests/unittests/test_datasource/test_azure_helper.py
+++ b/tests/unittests/test_datasource/test_azure_helper.py
@@ -275,7 +275,7 @@ class TestOpenSSLManager(TestCase):
                 mock.patch('builtins.open'))
 
     @mock.patch.object(azure_helper, 'cd', mock.MagicMock())
-    @mock.patch.object(azure_helper.tempfile, 'mkdtemp')
+    @mock.patch.object(azure_helper.temp_utils, 'mkdtemp')
     def test_openssl_manager_creates_a_tmpdir(self, mkdtemp):
         manager = azure_helper.OpenSSLManager()
         self.assertEqual(mkdtemp.return_value, manager.tmpdir)
@@ -292,7 +292,7 @@ class TestOpenSSLManager(TestCase):
         manager.clean_up()
 
     @mock.patch.object(azure_helper, 'cd', mock.MagicMock())
-    @mock.patch.object(azure_helper.tempfile, 'mkdtemp', mock.MagicMock())
+    @mock.patch.object(azure_helper.temp_utils, 'mkdtemp', mock.MagicMock())
     @mock.patch.object(azure_helper.util, 'del_dir')
     def test_clean_up(self, del_dir):
         manager = azure_helper.OpenSSLManager()
diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
index f251024..0b2e292 100644
--- a/tests/unittests/test_net.py
+++ b/tests/unittests/test_net.py
@@ -9,6 +9,7 @@ from cloudinit.net import network_state
 from cloudinit.net import renderers
 from cloudinit.net import sysconfig
 from cloudinit.sources.helpers import openstack
+from cloudinit import temp_utils
 from cloudinit import util
 
 from .helpers import CiTestCase
@@ -2150,7 +2151,7 @@ class TestCmdlineConfigParsing(CiTestCase):
         static['mac_address'] = macs['eth1']
 
         expected = {'version': 1, 'config': [dhcp, static]}
-        with util.tempdir() as tmpd:
+        with temp_utils.tempdir() as tmpd:
             for fname, content in pairs:
                 fp = os.path.join(tmpd, fname)
                 files.append(fp)