cloud-init-dev team mailing list archive

[Chad Smith <chad.smith@xxxxxxxxxxxxx>]

Chad Smith has proposed merging ~chad.smith/cloud-init:bug/1798189-instance-data-fallback into cloud-init:master.

Commit message:
instance-data: root user can fallback to instace-data.json when sensitive data absent

On cloud-init upgrade path from 18.3 to 18.4 cloud-init changed how instance-data
is written. Cloud-init changes instance-data.json from root read-only to redacted
world-readable content, and provided a separate unredacted instance-data-sensitive.json
which is read-only root. Since instance-data is only rewritten from cache on
reboot, the query and render tools needed fallback to use the 'old'
instance-data.json  if the new sensitive file isn't yet present.

This avoids error messages from tools about an absebt /run/instance-data-sensitive.json
file.

LP: #1798189

Requested reviews:
  cloud-init commiters (cloud-init-dev)
Related bugs:
  Bug #1798189 in cloud-init: "cloud-init query: /run/cloud/instance-data-sensitive.json not generated on upgrade"
  https://bugs.launchpad.net/cloud-init/+bug/1798189

For more details, see:
https://code.launchpad.net/~chad.smith/cloud-init/+git/cloud-init/+merge/357793
-- 
Your team cloud-init commiters is requested to review the proposed merge of ~chad.smith/cloud-init:bug/1798189-instance-data-fallback into cloud-init:master.

diff --git a/cloudinit/cmd/devel/render.py b/cloudinit/cmd/devel/render.py
index 2ba6b68..4d3ec95 100755
--- a/cloudinit/cmd/devel/render.py
+++ b/cloudinit/cmd/devel/render.py
@@ -8,11 +8,10 @@ import sys
 
 from cloudinit.handlers.jinja_template import render_jinja_payload_from_file
 from cloudinit import log
-from cloudinit.sources import INSTANCE_JSON_FILE
+from cloudinit.sources import INSTANCE_JSON_FILE, INSTANCE_JSON_SENSITIVE_FILE
 from . import addLogHandlerCLI, read_cfg_paths
 
 NAME = 'render'
-DEFAULT_INSTANCE_DATA = '/run/cloud-init/instance-data.json'
 
 LOG = log.getLogger(NAME)
 
@@ -47,12 +46,22 @@ def handle_args(name, args):
     @return 0 on success, 1 on failure.
     """
     addLogHandlerCLI(LOG, log.DEBUG if args.debug else log.WARNING)
-    if not args.instance_data:
-        paths = read_cfg_paths()
-        instance_data_fn = os.path.join(
-            paths.run_dir, INSTANCE_JSON_FILE)
-    else:
+    if args.instance_data:
         instance_data_fn = args.instance_data
+    else:
+        paths = read_cfg_paths()
+        uid = os.getuid()
+        redacted_data_fn = os.path.join(paths.run_dir, INSTANCE_JSON_FILE)
+        if uid == 0:
+            instance_data_fn = os.path.join(
+                paths.run_dir, INSTANCE_JSON_SENSITIVE_FILE)
+            if not os.path.exists(instance_data_fn):
+                LOG.warning(
+                     'Missing root-readable %s. Using redacted %s instead.',
+                     instance_data_fn, redacted_data_fn)
+                instance_data_fn = redacted_data_fn
+        else:
+            instance_data_fn = redacted_data_fn
     if not os.path.exists(instance_data_fn):
         LOG.error('Missing instance-data.json file: %s', instance_data_fn)
         return 1
diff --git a/cloudinit/cmd/devel/tests/test_render.py b/cloudinit/cmd/devel/tests/test_render.py
index fc5d2c0..988bba0 100644
--- a/cloudinit/cmd/devel/tests/test_render.py
+++ b/cloudinit/cmd/devel/tests/test_render.py
@@ -6,7 +6,7 @@ import os
 from collections import namedtuple
 from cloudinit.cmd.devel import render
 from cloudinit.helpers import Paths
-from cloudinit.sources import INSTANCE_JSON_FILE
+from cloudinit.sources import INSTANCE_JSON_FILE, INSTANCE_JSON_SENSITIVE_FILE
 from cloudinit.tests.helpers import CiTestCase, mock, skipUnlessJinja
 from cloudinit.util import ensure_dir, write_file
 
@@ -63,6 +63,49 @@ class TestRender(CiTestCase):
             'Missing instance-data.json file: %s' % json_file,
             self.logs.getvalue())
 
+    def test_handle_args_root_fallback_from_sensitive_instance_data(self):
+        """When root user defaults to sensitive.json."""
+        user_data = self.tmp_path('user-data', dir=self.tmp)
+        run_dir = self.tmp_path('run_dir', dir=self.tmp)
+        ensure_dir(run_dir)
+        paths = Paths({'run_dir': run_dir})
+        self.add_patch('cloudinit.cmd.devel.render.read_cfg_paths', 'm_paths')
+        self.m_paths.return_value = paths
+        args = self.args(
+            user_data=user_data, instance_data=None, debug=False)
+        with mock.patch('sys.stderr', new_callable=StringIO):
+            with mock.patch('os.getuid') as m_getuid:
+                m_getuid.return_value = 0
+                self.assertEqual(1, render.handle_args('anyname', args))
+        json_file = os.path.join(run_dir, INSTANCE_JSON_FILE)
+        json_sensitive = os.path.join(run_dir, INSTANCE_JSON_SENSITIVE_FILE)
+        self.assertIn(
+            'WARNING: Missing root-readable %s. Using redacted %s' % (
+                json_sensitive, json_file), self.logs.getvalue())
+        self.assertIn(
+            'ERROR: Missing instance-data.json file: %s' % json_file,
+            self.logs.getvalue())
+
+    def test_handle_args_root_uses_sensitive_instance_data(self):
+        """When root user, and no instance-data arg, use sensitive.json."""
+        user_data = self.tmp_path('user-data', dir=self.tmp)
+        write_file(user_data, '##template: jinja\nrendering: {{ my_var }}')
+        run_dir = self.tmp_path('run_dir', dir=self.tmp)
+        ensure_dir(run_dir)
+        json_sensitive = os.path.join(run_dir, INSTANCE_JSON_SENSITIVE_FILE)
+        write_file(json_sensitive, '{"my-var": "jinja worked"}')
+        paths = Paths({'run_dir': run_dir})
+        self.add_patch('cloudinit.cmd.devel.render.read_cfg_paths', 'm_paths')
+        self.m_paths.return_value = paths
+        args = self.args(
+            user_data=user_data, instance_data=None, debug=False)
+        with mock.patch('sys.stderr', new_callable=StringIO):
+            with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout:
+                with mock.patch('os.getuid') as m_getuid:
+                    m_getuid.return_value = 0
+                    self.assertEqual(0, render.handle_args('anyname', args))
+        self.assertIn('rendering: jinja worked', m_stdout.getvalue())
+
     @skipUnlessJinja()
     def test_handle_args_renders_instance_data_vars_in_template(self):
         """If user_data file is a jinja template render instance-data vars."""
diff --git a/cloudinit/cmd/query.py b/cloudinit/cmd/query.py
index 7d2d4fe..ff03de9 100644
--- a/cloudinit/cmd/query.py
+++ b/cloudinit/cmd/query.py
@@ -79,22 +79,30 @@ def handle_args(name, args):
     uid = os.getuid()
     if not all([args.instance_data, args.user_data, args.vendor_data]):
         paths = read_cfg_paths()
-    if not args.instance_data:
+    if args.instance_data:
+        instance_data_fn = args.instance_data
+    else:
+        redacted_data_fn = os.path.join(paths.run_dir, INSTANCE_JSON_FILE)
         if uid == 0:
-            default_json_fn = INSTANCE_JSON_SENSITIVE_FILE
+            sensitive_data_fn = os.path.join(
+                paths.run_dir, INSTANCE_JSON_SENSITIVE_FILE)
+            if os.path.exists(sensitive_data_fn):
+                instance_data_fn = sensitive_data_fn
+            else:
+                LOG.warning(
+                     'Missing root-readable %s. Using redacted %s instead.',
+                     sensitive_data_fn, redacted_data_fn)
+                instance_data_fn = redacted_data_fn
         else:
-            default_json_fn = INSTANCE_JSON_FILE  # World readable
-        instance_data_fn = os.path.join(paths.run_dir, default_json_fn)
+            instance_data_fn = redacted_data_fn
+    if args.user_data:
+        user_data_fn = args.user_data
     else:
-        instance_data_fn = args.instance_data
-    if not args.user_data:
         user_data_fn = os.path.join(paths.instance_link, 'user-data.txt')
+    if args.vendor_data:
+        vendor_data_fn = args.vendor_data
     else:
-        user_data_fn = args.user_data
-    if not args.vendor_data:
         vendor_data_fn = os.path.join(paths.instance_link, 'vendor-data.txt')
-    else:
-        vendor_data_fn = args.vendor_data
 
     try:
         instance_json = util.load_file(instance_data_fn)
diff --git a/cloudinit/cmd/tests/test_query.py b/cloudinit/cmd/tests/test_query.py
index fb87c6a..241f541 100644
--- a/cloudinit/cmd/tests/test_query.py
+++ b/cloudinit/cmd/tests/test_query.py
@@ -7,7 +7,8 @@ import os
 from collections import namedtuple
 from cloudinit.cmd import query
 from cloudinit.helpers import Paths
-from cloudinit.sources import REDACT_SENSITIVE_VALUE, INSTANCE_JSON_FILE
+from cloudinit.sources import (
+    REDACT_SENSITIVE_VALUE, INSTANCE_JSON_FILE, INSTANCE_JSON_SENSITIVE_FILE)
 from cloudinit.tests.helpers import CiTestCase, mock
 from cloudinit.util import ensure_dir, write_file
 
@@ -76,6 +77,52 @@ class TestQuery(CiTestCase):
             'ERROR: Missing instance-data.json file: %s' % json_file,
             m_stderr.getvalue())
 
+    def test_handle_args_root_fallsback_to_instance_data(self):
+        """When no instance_data argument, root falls back to redacted json."""
+        args = self.args(
+            debug=False, dump_all=True, format=None, instance_data=None,
+            list_keys=False, user_data=None, vendor_data=None, varname=None)
+        run_dir = self.tmp_path('run_dir', dir=self.tmp)
+        ensure_dir(run_dir)
+        paths = Paths({'run_dir': run_dir})
+        self.add_patch('cloudinit.cmd.query.read_cfg_paths', 'm_paths')
+        self.m_paths.return_value = paths
+        with mock.patch('sys.stderr', new_callable=StringIO) as m_stderr:
+            with mock.patch('os.getuid') as m_getuid:
+                m_getuid.return_value = 0
+                self.assertEqual(1, query.handle_args('anyname', args))
+        json_file = os.path.join(run_dir, INSTANCE_JSON_FILE)
+        sensitive_file = os.path.join(run_dir, INSTANCE_JSON_SENSITIVE_FILE)
+        self.assertIn(
+            'WARNING: Missing root-readable %s. Using redacted %s instead.' % (
+                sensitive_file, json_file),
+            m_stderr.getvalue())
+
+    def test_handle_args_root_uses_instance_sensitive_data(self):
+        """When no instance_data argument, root uses semsitive json."""
+        user_data = self.tmp_path('user-data', dir=self.tmp)
+        vendor_data = self.tmp_path('vendor-data', dir=self.tmp)
+        write_file(user_data, 'ud')
+        write_file(vendor_data, 'vd')
+        run_dir = self.tmp_path('run_dir', dir=self.tmp)
+        sensitive_file = os.path.join(run_dir, INSTANCE_JSON_SENSITIVE_FILE)
+        write_file(sensitive_file, '{"my-var": "it worked"}')
+        ensure_dir(run_dir)
+        paths = Paths({'run_dir': run_dir})
+        self.add_patch('cloudinit.cmd.query.read_cfg_paths', 'm_paths')
+        self.m_paths.return_value = paths
+        args = self.args(
+            debug=False, dump_all=True, format=None, instance_data=None,
+            list_keys=False, user_data=vendor_data, vendor_data=vendor_data,
+            varname=None)
+        with mock.patch('sys.stdout', new_callable=StringIO) as m_stdout:
+            with mock.patch('os.getuid') as m_getuid:
+                m_getuid.return_value = 0
+                self.assertEqual(0, query.handle_args('anyname', args))
+        self.assertEqual(
+            '{\n "my_var": "it worked",\n "userdata": "vd",\n '
+            '"vendordata": "vd"\n}\n', m_stdout.getvalue())
+
     def test_handle_args_dumps_all_instance_data(self):
         """When --all is specified query will dump all instance data vars."""
         write_file(self.instance_data, '{"my-var": "it worked"}')