Merge ~pzakha/cloud-init:userdata into cloud-init:master
- Git
- lp:~pzakha/cloud-init
- userdata
- Merge into master
Status: | Merged |
---|---|
Approved by: | Ryan Harper |
Approved revision: | 67f7991583a71105e07c7555a4d2e06d51d6bb73 |
Merge reported by: | Server Team CI bot |
Merged at revision: | not available |
Proposed branch: | ~pzakha/cloud-init:userdata |
Merge into: | cloud-init:master |
Diff against target: |
254 lines (+103/-12) 5 files modified
cloudinit/config/cc_ssh.py (+13/-2) cloudinit/config/tests/test_ssh.py (+37/-9) cloudinit/stages.py (+5/-1) doc/rtd/topics/format.rst (+8/-0) tests/unittests/test_data.py (+40/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Ryan Harper | Approve | ||
Server Team CI bot | continuous-integration | Approve | |
Review via email: mp+367721@code.launchpad.net |
Commit message
Description of the change
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_
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:/
Dan Watkins (oddbloke) wrote : | # |
Scott Moser (smoser) wrote : | # |
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) wrote : | # |
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.
Ryan Harper (raharper) : | # |
Pavel Zakharov (pzakha) wrote : | # |
I think I've addressed the review feedback, let me know what are the next steps.
Ryan Harper (raharper) wrote : | # |
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/
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/
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/
user-data via the following key: allow_userdata , etc
And document the default value and what it does.
Pavel Zakharov (pzakha) wrote : | # |
Alright, I'm a bit busy right now, but I should be able to post an update in a few weeks.
Ryan Harper (raharper) wrote : | # |
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) wrote : | # |
I've updated the code, looking for another round of reviews!
Pavel Zakharov (pzakha) wrote : | # |
Could someone take another look at this?
Ryan Harper (raharper) wrote : | # |
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) wrote : | # |
FAILED: Continuous integration, rev:8913c393d58
https:/
Executed test runs:
SUCCESS: Checkout
FAILED: Unit & Style Tests
Click here to trigger a rebuild:
https:/
Ryan Harper (raharper) wrote : | # |
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>
-
Review feedback, fix tests
Pavel Zakharov (pzakha) wrote : | # |
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) wrote : | # |
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>
-
Reword the Disable User-Data section
Server Team CI bot (server-team-bot) wrote : | # |
PASSED: Continuous integration, rev:67f7991583a
https:/
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:/
Preview Diff
1 | diff --git a/cloudinit/config/cc_ssh.py b/cloudinit/config/cc_ssh.py |
2 | index fdd8f4d..050285a 100755 |
3 | --- a/cloudinit/config/cc_ssh.py |
4 | +++ b/cloudinit/config/cc_ssh.py |
5 | @@ -56,9 +56,13 @@ root login is disabled, and root login opts are set to:: |
6 | no-port-forwarding,no-agent-forwarding,no-X11-forwarding |
7 | |
8 | Authorized keys for the default user/first user defined in ``users`` can be |
9 | -specified using `ssh_authorized_keys``. Keys should be specified as a list of |
10 | +specified using ``ssh_authorized_keys``. Keys should be specified as a list of |
11 | public keys. |
12 | |
13 | +Importing ssh public keys for the default user (defined in ``users``)) is |
14 | +enabled by default. This feature may be disabled by setting |
15 | +``allow_publish_ssh_keys: false``. |
16 | + |
17 | .. note:: |
18 | see the ``cc_set_passwords`` module documentation to enable/disable ssh |
19 | password authentication |
20 | @@ -91,6 +95,7 @@ public keys. |
21 | ssh_authorized_keys: |
22 | - ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAGEA3FSyQwBI6Z+nCSjUU ... |
23 | - ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEA3I7VUf2l5gSn5uavROsc5HRDpZ ... |
24 | + allow_public_ssh_keys: <true/false> |
25 | ssh_publish_hostkeys: |
26 | enabled: <true/false> (Defaults to true) |
27 | blacklist: <list of key types> (Defaults to [dsa]) |
28 | @@ -207,7 +212,13 @@ def handle(_name, cfg, cloud, log, _args): |
29 | disable_root_opts = util.get_cfg_option_str(cfg, "disable_root_opts", |
30 | ssh_util.DISABLE_USER_OPTS) |
31 | |
32 | - keys = cloud.get_public_ssh_keys() or [] |
33 | + keys = [] |
34 | + if util.get_cfg_option_bool(cfg, 'allow_public_ssh_keys', True): |
35 | + keys = cloud.get_public_ssh_keys() or [] |
36 | + else: |
37 | + log.debug('Skipping import of publish ssh keys per ' |
38 | + 'config setting: allow_public_ssh_keys=False') |
39 | + |
40 | if "ssh_authorized_keys" in cfg: |
41 | cfgkeys = cfg["ssh_authorized_keys"] |
42 | keys.extend(cfgkeys) |
43 | diff --git a/cloudinit/config/tests/test_ssh.py b/cloudinit/config/tests/test_ssh.py |
44 | index e778984..0c55441 100644 |
45 | --- a/cloudinit/config/tests/test_ssh.py |
46 | +++ b/cloudinit/config/tests/test_ssh.py |
47 | @@ -5,6 +5,9 @@ import os.path |
48 | from cloudinit.config import cc_ssh |
49 | from cloudinit import ssh_util |
50 | from cloudinit.tests.helpers import CiTestCase, mock |
51 | +import logging |
52 | + |
53 | +LOG = logging.getLogger(__name__) |
54 | |
55 | MODPATH = "cloudinit.config.cc_ssh." |
56 | |
57 | @@ -87,7 +90,7 @@ class TestHandleSsh(CiTestCase): |
58 | cc_ssh.PUBLISH_HOST_KEYS = False |
59 | cloud = self.tmp_cloud( |
60 | distro='ubuntu', metadata={'public-keys': keys}) |
61 | - cc_ssh.handle("name", cfg, cloud, None, None) |
62 | + cc_ssh.handle("name", cfg, cloud, LOG, None) |
63 | options = ssh_util.DISABLE_USER_OPTS.replace("$USER", "NONE") |
64 | options = options.replace("$DISABLE_USER", "root") |
65 | m_glob.assert_called_once_with('/etc/ssh/ssh_host_*key*') |
66 | @@ -103,6 +106,31 @@ class TestHandleSsh(CiTestCase): |
67 | @mock.patch(MODPATH + "glob.glob") |
68 | @mock.patch(MODPATH + "ug_util.normalize_users_groups") |
69 | @mock.patch(MODPATH + "os.path.exists") |
70 | + def test_dont_allow_public_ssh_keys(self, m_path_exists, m_nug, |
71 | + m_glob, m_setup_keys): |
72 | + """Test allow_public_ssh_keys=False ignores ssh public keys from |
73 | + platform. |
74 | + """ |
75 | + cfg = {"allow_public_ssh_keys": False} |
76 | + keys = ["key1"] |
77 | + user = "clouduser" |
78 | + m_glob.return_value = [] # Return no matching keys to prevent removal |
79 | + # Mock os.path.exits to True to short-circuit the key writing logic |
80 | + m_path_exists.return_value = True |
81 | + m_nug.return_value = ({user: {"default": user}}, {}) |
82 | + cloud = self.tmp_cloud( |
83 | + distro='ubuntu', metadata={'public-keys': keys}) |
84 | + cc_ssh.handle("name", cfg, cloud, LOG, None) |
85 | + |
86 | + options = ssh_util.DISABLE_USER_OPTS.replace("$USER", user) |
87 | + options = options.replace("$DISABLE_USER", "root") |
88 | + self.assertEqual([mock.call(set(), user), |
89 | + mock.call(set(), "root", options=options)], |
90 | + m_setup_keys.call_args_list) |
91 | + |
92 | + @mock.patch(MODPATH + "glob.glob") |
93 | + @mock.patch(MODPATH + "ug_util.normalize_users_groups") |
94 | + @mock.patch(MODPATH + "os.path.exists") |
95 | def test_handle_no_cfg_and_default_root(self, m_path_exists, m_nug, |
96 | m_glob, m_setup_keys): |
97 | """Test handle with no config and a default distro user.""" |
98 | @@ -115,7 +143,7 @@ class TestHandleSsh(CiTestCase): |
99 | m_nug.return_value = ({user: {"default": user}}, {}) |
100 | cloud = self.tmp_cloud( |
101 | distro='ubuntu', metadata={'public-keys': keys}) |
102 | - cc_ssh.handle("name", cfg, cloud, None, None) |
103 | + cc_ssh.handle("name", cfg, cloud, LOG, None) |
104 | |
105 | options = ssh_util.DISABLE_USER_OPTS.replace("$USER", user) |
106 | options = options.replace("$DISABLE_USER", "root") |
107 | @@ -140,7 +168,7 @@ class TestHandleSsh(CiTestCase): |
108 | m_nug.return_value = ({user: {"default": user}}, {}) |
109 | cloud = self.tmp_cloud( |
110 | distro='ubuntu', metadata={'public-keys': keys}) |
111 | - cc_ssh.handle("name", cfg, cloud, None, None) |
112 | + cc_ssh.handle("name", cfg, cloud, LOG, None) |
113 | |
114 | options = ssh_util.DISABLE_USER_OPTS.replace("$USER", user) |
115 | options = options.replace("$DISABLE_USER", "root") |
116 | @@ -165,7 +193,7 @@ class TestHandleSsh(CiTestCase): |
117 | cloud = self.tmp_cloud( |
118 | distro='ubuntu', metadata={'public-keys': keys}) |
119 | cloud.get_public_ssh_keys = mock.Mock(return_value=keys) |
120 | - cc_ssh.handle("name", cfg, cloud, None, None) |
121 | + cc_ssh.handle("name", cfg, cloud, LOG, None) |
122 | |
123 | self.assertEqual([mock.call(set(keys), user), |
124 | mock.call(set(keys), "root", options="")], |
125 | @@ -196,7 +224,7 @@ class TestHandleSsh(CiTestCase): |
126 | cfg = {} |
127 | expected_call = [self.test_hostkeys[key_type] for key_type |
128 | in ['ecdsa', 'ed25519', 'rsa']] |
129 | - cc_ssh.handle("name", cfg, cloud, None, None) |
130 | + cc_ssh.handle("name", cfg, cloud, LOG, None) |
131 | self.assertEqual([mock.call(expected_call)], |
132 | cloud.datasource.publish_host_keys.call_args_list) |
133 | |
134 | @@ -225,7 +253,7 @@ class TestHandleSsh(CiTestCase): |
135 | cfg = {'ssh_publish_hostkeys': {'enabled': True}} |
136 | expected_call = [self.test_hostkeys[key_type] for key_type |
137 | in ['ecdsa', 'ed25519', 'rsa']] |
138 | - cc_ssh.handle("name", cfg, cloud, None, None) |
139 | + cc_ssh.handle("name", cfg, cloud, LOG, None) |
140 | self.assertEqual([mock.call(expected_call)], |
141 | cloud.datasource.publish_host_keys.call_args_list) |
142 | |
143 | @@ -252,7 +280,7 @@ class TestHandleSsh(CiTestCase): |
144 | cloud.datasource.publish_host_keys = mock.Mock() |
145 | |
146 | cfg = {'ssh_publish_hostkeys': {'enabled': False}} |
147 | - cc_ssh.handle("name", cfg, cloud, None, None) |
148 | + cc_ssh.handle("name", cfg, cloud, LOG, None) |
149 | self.assertFalse(cloud.datasource.publish_host_keys.call_args_list) |
150 | cloud.datasource.publish_host_keys.assert_not_called() |
151 | |
152 | @@ -282,7 +310,7 @@ class TestHandleSsh(CiTestCase): |
153 | 'blacklist': ['dsa', 'rsa']}} |
154 | expected_call = [self.test_hostkeys[key_type] for key_type |
155 | in ['ecdsa', 'ed25519']] |
156 | - cc_ssh.handle("name", cfg, cloud, None, None) |
157 | + cc_ssh.handle("name", cfg, cloud, LOG, None) |
158 | self.assertEqual([mock.call(expected_call)], |
159 | cloud.datasource.publish_host_keys.call_args_list) |
160 | |
161 | @@ -312,6 +340,6 @@ class TestHandleSsh(CiTestCase): |
162 | 'blacklist': []}} |
163 | expected_call = [self.test_hostkeys[key_type] for key_type |
164 | in ['dsa', 'ecdsa', 'ed25519', 'rsa']] |
165 | - cc_ssh.handle("name", cfg, cloud, None, None) |
166 | + cc_ssh.handle("name", cfg, cloud, LOG, None) |
167 | self.assertEqual([mock.call(expected_call)], |
168 | cloud.datasource.publish_host_keys.call_args_list) |
169 | diff --git a/cloudinit/stages.py b/cloudinit/stages.py |
170 | index 5012988..18ca3b9 100644 |
171 | --- a/cloudinit/stages.py |
172 | +++ b/cloudinit/stages.py |
173 | @@ -549,7 +549,11 @@ class Init(object): |
174 | with events.ReportEventStack("consume-user-data", |
175 | "reading and applying user-data", |
176 | parent=self.reporter): |
177 | - self._consume_userdata(frequency) |
178 | + if util.get_cfg_option_bool(self.cfg, 'allow_userdata', True): |
179 | + self._consume_userdata(frequency) |
180 | + else: |
181 | + LOG.debug('allow_userdata = False: discarding user-data') |
182 | + |
183 | with events.ReportEventStack("consume-vendor-data", |
184 | "reading and applying vendor-data", |
185 | parent=self.reporter): |
186 | diff --git a/doc/rtd/topics/format.rst b/doc/rtd/topics/format.rst |
187 | index 7605040..f9f4ba6 100644 |
188 | --- a/doc/rtd/topics/format.rst |
189 | +++ b/doc/rtd/topics/format.rst |
190 | @@ -196,6 +196,14 @@ Example |
191 | |
192 | Also this `blog`_ post offers another example for more advanced usage. |
193 | |
194 | +Disabling User-Data |
195 | +=================== |
196 | + |
197 | +Cloud-init can be configured to ignore any user-data provided to instance. |
198 | +This allows custom images to prevent users from accidentally breaking closed |
199 | +appliances. Setting ``allow_userdata: false`` in the configuration will disable |
200 | +cloud-init from processing user-data. |
201 | + |
202 | .. [#] See your cloud provider for applicable user-data size limitations... |
203 | .. _blog: http://foss-boss.blogspot.com/2011/01/advanced-cloud-init-custom-handlers.html |
204 | .. vi: textwidth=78 |
205 | diff --git a/tests/unittests/test_data.py b/tests/unittests/test_data.py |
206 | index 3efe7ad..97d1dc2 100644 |
207 | --- a/tests/unittests/test_data.py |
208 | +++ b/tests/unittests/test_data.py |
209 | @@ -524,6 +524,46 @@ c: 4 |
210 | self.assertEqual(cfg.get('password'), 'gocubs') |
211 | self.assertEqual(cfg.get('locale'), 'chicago') |
212 | |
213 | + @mock.patch('cloudinit.util.read_conf_with_confd') |
214 | + def test_dont_allow_user_data(self, mock_cfg): |
215 | + mock_cfg.return_value = {"allow_userdata": False} |
216 | + |
217 | + # test that user-data is ignored but vendor-data is kept |
218 | + user_blob = ''' |
219 | +#cloud-config-jsonp |
220 | +[ |
221 | + { "op": "add", "path": "/baz", "value": "qux" }, |
222 | + { "op": "add", "path": "/bar", "value": "qux2" } |
223 | +] |
224 | +''' |
225 | + vendor_blob = ''' |
226 | +#cloud-config-jsonp |
227 | +[ |
228 | + { "op": "add", "path": "/baz", "value": "quxA" }, |
229 | + { "op": "add", "path": "/bar", "value": "quxB" }, |
230 | + { "op": "add", "path": "/foo", "value": "quxC" } |
231 | +] |
232 | +''' |
233 | + self.reRoot() |
234 | + initer = stages.Init() |
235 | + initer.datasource = FakeDataSource(user_blob, vendordata=vendor_blob) |
236 | + initer.read_cfg() |
237 | + initer.initialize() |
238 | + initer.fetch() |
239 | + initer.instancify() |
240 | + initer.update() |
241 | + initer.cloudify().run('consume_data', |
242 | + initer.consume_data, |
243 | + args=[PER_INSTANCE], |
244 | + freq=PER_INSTANCE) |
245 | + mods = stages.Modules(initer) |
246 | + (_which_ran, _failures) = mods.run_section('cloud_init_modules') |
247 | + cfg = mods.cfg |
248 | + self.assertIn('vendor_data', cfg) |
249 | + self.assertEqual('quxA', cfg['baz']) |
250 | + self.assertEqual('quxB', cfg['bar']) |
251 | + self.assertEqual('quxC', cfg['foo']) |
252 | + |
253 | |
254 | class TestConsumeUserDataHttp(TestConsumeUserData, helpers.HttprettyTestCase): |
255 |
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.