Merge ~pzakha/cloud-init:userdata into cloud-init:master

[Pavel Zakharov]

Add config for ssh-key import and consuming user-data

This patch enables control over SSH public-key import and
discarding supplied user-data (both disabled by default).

  allow-userdata: false
  ssh:
    allow_public_ssh_keys: false

This feature enables closed appliances to prevent customers
from unintentionally breaking the appliance which were
not designed for user interaction.

The downstream change for this is here:
  https://github.com/delphix/cloud-init/pull/4

[Dan Watkins (oddbloke)]

Hi Pavel, thanks for the contribution to cloud-init! I'm flitting between meetings, so I'm not able to assess whether these changes are broadly appropriate. That said, I do have an inline comment on how the changes could be expressed a little more concisely. I'll revisit this later today for a more full review.

[Scott Moser (smoser)]

I'm not opposed to this in principal. But I'd like to warn that this is not "drm". The end user can probably find a way of getting root on your image if they really want. You'll just be making it more difficult to do so.

That said, I do understand the desire to not have a user "break" your supported functionality unintentionally and then asking for support.

[Pavel Zakharov (pzakha)]

Thanks for the reviews.

Yes I understand that if the user really wants to access the system, they will find a way to do so. The intent of this is to make it non-trivial. Your reasoning is spot on.

[Pavel Zakharov (pzakha)]

I think I've addressed the review feedback, let me know what are the next steps.

[Ryan Harper (raharper)]

Thanks for updating the branch. I didn't see a unittest for the change. Let me know if you need help there.

I definitely would like to see us document this behavior. At a min, this branch can update:

 cloudinit/config/cc_ssh.py

The large block comment at the top is in RTD format, please add the allow_public_keys field, default value and describe what it does.

And I think the best location to mention disabling user-data is in:

 doc/rtd/topics/boot.rst

under the Network section. It mentions that this stage consumes all user-data.
Here you could add a

**Note**: Cloud-init system config (/etc/cloud/cloud.cfg.d/*) may disable consumption of
user-data via the following key: allow_userdata , etc

And document the default value and what it does.

review: Needs Fixing

[Pavel Zakharov (pzakha)]

Alright, I'm a bit busy right now, but I should be able to post an update in a few weeks.

[Ryan Harper (raharper)]

I've marked this branch work-in-progress. Once you've updated this branch please move the state back to NeedsReview. Thanks!

[Pavel Zakharov (pzakha)]

I've updated the code, looking for another round of reviews!

[Pavel Zakharov (pzakha)]

Could someone take another look at this?

[Ryan Harper (raharper)]

Thanks for the update.

I'm wondering if these two config changes should be in separate commits.

I've some in-line comments, suggestions.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:8913c393d5878fd4d169565f7c2dacb7596ecdbe
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1199/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1199//rebuild

review: Needs Fixing (continuous-integration)

[Ryan Harper (raharper)]

If you can rebase this to master, that'll fix up the current CI issue unrelated to your changes.

33f9d36... by Pavel Zakharov <email address hidden> on 2019-10-11

Review feedback, fix tests

[Pavel Zakharov (pzakha)]

I've rebased the code on master, and fixed the failing unit tests.

@raharper, I think I fixed most of the phrasing, but I didn't quite understand your feedback regarding the "Disabling User-Data" section.

[Ryan Harper (raharper)]

Thanks for rebasing. I'll look at it again and comment. I think the important part was to not say

"cloud-init will silently ignore"

when your code *logs* that we're ignoring user-data as requested, which is what I want; we want to leave a breadcrumb in the log file for folks to see why their user-data was not processed.

67f7991... by Pavel Zakharov <email address hidden> on 2019-10-11

Reword the Disable User-Data section

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:67f7991583a71105e07c7555a4d2e06d51d6bb73
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1239/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/1239//rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Thanks!

review: Approve

diff --git a/cloudinit/config/cc_ssh.py b/cloudinit/config/cc_ssh.py
index fdd8f4d..050285a 100755
--- a/cloudinit/config/cc_ssh.py
+++ b/cloudinit/config/cc_ssh.py
@@ -56,9 +56,13 @@ root login is disabled, and root login opts are set to::
     no-port-forwarding,no-agent-forwarding,no-X11-forwarding
 
 Authorized keys for the default user/first user defined in ``users`` can be
-specified using `ssh_authorized_keys``. Keys should be specified as a list of
+specified using ``ssh_authorized_keys``. Keys should be specified as a list of
 public keys.
 
+Importing ssh public keys for the default user (defined in ``users``)) is
+enabled by default.  This feature may be disabled by setting
+``allow_publish_ssh_keys: false``.
+
 .. note::
     see the ``cc_set_passwords`` module documentation to enable/disable ssh
     password authentication
@@ -91,6 +95,7 @@ public keys.
     ssh_authorized_keys:
         - ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAGEA3FSyQwBI6Z+nCSjUU ...
         - ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA3I7VUf2l5gSn5uavROsc5HRDpZ ...
+    allow_public_ssh_keys: <true/false>
     ssh_publish_hostkeys:
         enabled: <true/false> (Defaults to true)
         blacklist: <list of key types> (Defaults to [dsa])
@@ -207,7 +212,13 @@ def handle(_name, cfg, cloud, log, _args):
         disable_root_opts = util.get_cfg_option_str(cfg, "disable_root_opts",
                                                     ssh_util.DISABLE_USER_OPTS)
 
-        keys = cloud.get_public_ssh_keys() or []
+        keys = []
+        if util.get_cfg_option_bool(cfg, 'allow_public_ssh_keys', True):
+            keys = cloud.get_public_ssh_keys() or []
+        else:
+            log.debug('Skipping import of publish ssh keys per '
+                      'config setting: allow_public_ssh_keys=False')
+
         if "ssh_authorized_keys" in cfg:
             cfgkeys = cfg["ssh_authorized_keys"]
             keys.extend(cfgkeys)
diff --git a/cloudinit/config/tests/test_ssh.py b/cloudinit/config/tests/test_ssh.py
index e778984..0c55441 100644
--- a/cloudinit/config/tests/test_ssh.py
+++ b/cloudinit/config/tests/test_ssh.py
@@ -5,6 +5,9 @@ import os.path
 from cloudinit.config import cc_ssh
 from cloudinit import ssh_util
 from cloudinit.tests.helpers import CiTestCase, mock
+import logging
+
+LOG = logging.getLogger(__name__)
 
 MODPATH = "cloudinit.config.cc_ssh."
 
@@ -87,7 +90,7 @@ class TestHandleSsh(CiTestCase):
         cc_ssh.PUBLISH_HOST_KEYS = False
         cloud = self.tmp_cloud(
             distro='ubuntu', metadata={'public-keys': keys})
-        cc_ssh.handle("name", cfg, cloud, None, None)
+        cc_ssh.handle("name", cfg, cloud, LOG, None)
         options = ssh_util.DISABLE_USER_OPTS.replace("$USER", "NONE")
         options = options.replace("$DISABLE_USER", "root")
         m_glob.assert_called_once_with('/etc/ssh/ssh_host_*key*')
@@ -103,6 +106,31 @@ class TestHandleSsh(CiTestCase):
     @mock.patch(MODPATH + "glob.glob")
     @mock.patch(MODPATH + "ug_util.normalize_users_groups")
     @mock.patch(MODPATH + "os.path.exists")
+    def test_dont_allow_public_ssh_keys(self, m_path_exists, m_nug,
+                                        m_glob, m_setup_keys):
+        """Test allow_public_ssh_keys=False ignores ssh public keys from
+           platform.
+        """
+        cfg = {"allow_public_ssh_keys": False}
+        keys = ["key1"]
+        user = "clouduser"
+        m_glob.return_value = []  # Return no matching keys to prevent removal
+        # Mock os.path.exits to True to short-circuit the key writing logic
+        m_path_exists.return_value = True
+        m_nug.return_value = ({user: {"default": user}}, {})
+        cloud = self.tmp_cloud(
+            distro='ubuntu', metadata={'public-keys': keys})
+        cc_ssh.handle("name", cfg, cloud, LOG, None)
+
+        options = ssh_util.DISABLE_USER_OPTS.replace("$USER", user)
+        options = options.replace("$DISABLE_USER", "root")
+        self.assertEqual([mock.call(set(), user),
+                          mock.call(set(), "root", options=options)],
+                         m_setup_keys.call_args_list)
+
+    @mock.patch(MODPATH + "glob.glob")
+    @mock.patch(MODPATH + "ug_util.normalize_users_groups")
+    @mock.patch(MODPATH + "os.path.exists")
     def test_handle_no_cfg_and_default_root(self, m_path_exists, m_nug,
                                             m_glob, m_setup_keys):
         """Test handle with no config and a default distro user."""
@@ -115,7 +143,7 @@ class TestHandleSsh(CiTestCase):
         m_nug.return_value = ({user: {"default": user}}, {})
         cloud = self.tmp_cloud(
             distro='ubuntu', metadata={'public-keys': keys})
-        cc_ssh.handle("name", cfg, cloud, None, None)
+        cc_ssh.handle("name", cfg, cloud, LOG, None)
 
         options = ssh_util.DISABLE_USER_OPTS.replace("$USER", user)
         options = options.replace("$DISABLE_USER", "root")
@@ -140,7 +168,7 @@ class TestHandleSsh(CiTestCase):
         m_nug.return_value = ({user: {"default": user}}, {})
         cloud = self.tmp_cloud(
             distro='ubuntu', metadata={'public-keys': keys})
-        cc_ssh.handle("name", cfg, cloud, None, None)
+        cc_ssh.handle("name", cfg, cloud, LOG, None)
 
         options = ssh_util.DISABLE_USER_OPTS.replace("$USER", user)
         options = options.replace("$DISABLE_USER", "root")
@@ -165,7 +193,7 @@ class TestHandleSsh(CiTestCase):
         cloud = self.tmp_cloud(
             distro='ubuntu', metadata={'public-keys': keys})
         cloud.get_public_ssh_keys = mock.Mock(return_value=keys)
-        cc_ssh.handle("name", cfg, cloud, None, None)
+        cc_ssh.handle("name", cfg, cloud, LOG, None)
 
         self.assertEqual([mock.call(set(keys), user),
                           mock.call(set(keys), "root", options="")],
@@ -196,7 +224,7 @@ class TestHandleSsh(CiTestCase):
         cfg = {}
         expected_call = [self.test_hostkeys[key_type] for key_type
                          in ['ecdsa', 'ed25519', 'rsa']]
-        cc_ssh.handle("name", cfg, cloud, None, None)
+        cc_ssh.handle("name", cfg, cloud, LOG, None)
         self.assertEqual([mock.call(expected_call)],
                          cloud.datasource.publish_host_keys.call_args_list)
 
@@ -225,7 +253,7 @@ class TestHandleSsh(CiTestCase):
         cfg = {'ssh_publish_hostkeys': {'enabled': True}}
         expected_call = [self.test_hostkeys[key_type] for key_type
                          in ['ecdsa', 'ed25519', 'rsa']]
-        cc_ssh.handle("name", cfg, cloud, None, None)
+        cc_ssh.handle("name", cfg, cloud, LOG, None)
         self.assertEqual([mock.call(expected_call)],
                          cloud.datasource.publish_host_keys.call_args_list)
 
@@ -252,7 +280,7 @@ class TestHandleSsh(CiTestCase):
         cloud.datasource.publish_host_keys = mock.Mock()
 
         cfg = {'ssh_publish_hostkeys': {'enabled': False}}
-        cc_ssh.handle("name", cfg, cloud, None, None)
+        cc_ssh.handle("name", cfg, cloud, LOG, None)
         self.assertFalse(cloud.datasource.publish_host_keys.call_args_list)
         cloud.datasource.publish_host_keys.assert_not_called()
 
@@ -282,7 +310,7 @@ class TestHandleSsh(CiTestCase):
                                         'blacklist': ['dsa', 'rsa']}}
         expected_call = [self.test_hostkeys[key_type] for key_type
                          in ['ecdsa', 'ed25519']]
-        cc_ssh.handle("name", cfg, cloud, None, None)
+        cc_ssh.handle("name", cfg, cloud, LOG, None)
         self.assertEqual([mock.call(expected_call)],
                          cloud.datasource.publish_host_keys.call_args_list)
 
@@ -312,6 +340,6 @@ class TestHandleSsh(CiTestCase):
                                         'blacklist': []}}
         expected_call = [self.test_hostkeys[key_type] for key_type
                          in ['dsa', 'ecdsa', 'ed25519', 'rsa']]
-        cc_ssh.handle("name", cfg, cloud, None, None)
+        cc_ssh.handle("name", cfg, cloud, LOG, None)
         self.assertEqual([mock.call(expected_call)],
                          cloud.datasource.publish_host_keys.call_args_list)
diff --git a/cloudinit/stages.py b/cloudinit/stages.py
index 5012988..18ca3b9 100644
--- a/cloudinit/stages.py
+++ b/cloudinit/stages.py
@@ -549,7 +549,11 @@ class Init(object):
         with events.ReportEventStack("consume-user-data",
                                      "reading and applying user-data",
                                      parent=self.reporter):
-            self._consume_userdata(frequency)
+            if util.get_cfg_option_bool(self.cfg, 'allow_userdata', True):
+                self._consume_userdata(frequency)
+            else:
+                LOG.debug('allow_userdata = False: discarding user-data')
+
         with events.ReportEventStack("consume-vendor-data",
                                      "reading and applying vendor-data",
                                      parent=self.reporter):
diff --git a/doc/rtd/topics/format.rst b/doc/rtd/topics/format.rst
index 7605040..f9f4ba6 100644
--- a/doc/rtd/topics/format.rst
+++ b/doc/rtd/topics/format.rst
@@ -196,6 +196,14 @@ Example
 
 Also this `blog`_ post offers another example for more advanced usage.
 
+Disabling User-Data
+===================
+
+Cloud-init can be configured to ignore any user-data provided to instance.
+This allows custom images to prevent users from accidentally breaking closed
+appliances. Setting ``allow_userdata: false`` in the configuration will disable
+cloud-init from processing user-data.
+
 .. [#] See your cloud provider for applicable user-data size limitations...
 .. _blog: http://foss-boss.blogspot.com/2011/01/advanced-cloud-init-custom-handlers.html
 .. vi: textwidth=78
diff --git a/tests/unittests/test_data.py b/tests/unittests/test_data.py
index 3efe7ad..97d1dc2 100644
--- a/tests/unittests/test_data.py
+++ b/tests/unittests/test_data.py
@@ -524,6 +524,46 @@ c: 4
         self.assertEqual(cfg.get('password'), 'gocubs')
         self.assertEqual(cfg.get('locale'), 'chicago')
 
+    @mock.patch('cloudinit.util.read_conf_with_confd')
+    def test_dont_allow_user_data(self, mock_cfg):
+        mock_cfg.return_value = {"allow_userdata": False}
+
+        # test that user-data is ignored but vendor-data is kept
+        user_blob = '''
+#cloud-config-jsonp
+[
+     { "op": "add", "path": "/baz", "value": "qux" },
+     { "op": "add", "path": "/bar", "value": "qux2" }
+]
+'''
+        vendor_blob = '''
+#cloud-config-jsonp
+[
+     { "op": "add", "path": "/baz", "value": "quxA" },
+     { "op": "add", "path": "/bar", "value": "quxB" },
+     { "op": "add", "path": "/foo", "value": "quxC" }
+]
+'''
+        self.reRoot()
+        initer = stages.Init()
+        initer.datasource = FakeDataSource(user_blob, vendordata=vendor_blob)
+        initer.read_cfg()
+        initer.initialize()
+        initer.fetch()
+        initer.instancify()
+        initer.update()
+        initer.cloudify().run('consume_data',
+                              initer.consume_data,
+                              args=[PER_INSTANCE],
+                              freq=PER_INSTANCE)
+        mods = stages.Modules(initer)
+        (_which_ran, _failures) = mods.run_section('cloud_init_modules')
+        cfg = mods.cfg
+        self.assertIn('vendor_data', cfg)
+        self.assertEqual('quxA', cfg['baz'])
+        self.assertEqual('quxB', cfg['bar'])
+        self.assertEqual('quxC', cfg['foo'])
+
 
 class TestConsumeUserDataHttp(TestConsumeUserData, helpers.HttprettyTestCase):