Merge lp:~newell-jensen/maas/rack_register_command into lp:maas/trunk

Proposed by Newell Jensen on 2016-05-18
Status: Merged
Approved by: Newell Jensen on 2016-06-03
Approved revision: 5034
Merged at revision: 5069
Proposed branch: lp:~newell-jensen/maas/rack_register_command
Merge into: lp:maas/trunk
Diff against target: 357 lines (+305/-3)
4 files modified
src/provisioningserver/__main__.py (+2/-0)
src/provisioningserver/register_command.py (+124/-0)
src/provisioningserver/tests/test_register_command.py (+172/-0)
src/provisioningserver/utils/script.py (+7/-3)
To merge this branch: bzr merge lp:~newell-jensen/maas/rack_register_command
Reviewer Review Type Date Requested Status
Mike Pontillo (community) Approve on 2016-06-03
Gavin Panella (community) 2016-05-18 Abstain on 2016-05-24
Review via email: mp+295101@code.launchpad.net

Commit message

Add rack register command.

Description of the change

Examples of command usage:

Supplying both URL and shared secret (not prompted for either):

$ sudo maas-rack register --secret e0de7c075986f6d7328ff52ebcc63576 --url http://192.168.122.63/MAAS

Supplying URL but not shared secret (prompted for shared secret):

$ sudo maas-rack register --url http://192.168.122.63/MAAS
Secret (hex/base16 encoded): e0de7c075986f6d7328ff52ebcc63576
Secret installed to /var/lib/maas/secret.

Supplying shared secret but not URL (prompted for URL):

$ sudo maas-rack register --secret e0de7c075986f6d7328ff52ebcc63576
MAAS region controller URL: http://192.168.122.63/MAAS
MAAS region controller URL saved as http://192.168.122.63/MAAS.

Not supplying URL or shared secret (prompted for both):

$ sudo maas-rack register
MAAS region controller URL: http://192.168.122.63/MAAS
MAAS region controller URL saved as http://192.168.122.63/MAAS.
Secret (hex/base16 encoded): e0de7c075986f6d7328ff52ebcc63576
Secret installed to /var/lib/maas/secret.

Supplying secret via stdin but not URL (error message printed as this is non-interactive shell):

$ echo e0de7c075986f6d7328ff52ebcc63576 | sudo maas-rack register
MAAS region controller URL must be passed as an argument when supplying the shared secret via stdin with a non-interactive shell.

Supplying secret via stdin and URL (not prompted):

$ echo e0de7c075986f6d7328ff52ebcc63576 | sudo maas-rack register --url http://192.168.122.63/MAAS
Secret installed to /var/lib/maas/secret.

To post a comment you must log in.
Lee Trager (ltrager) wrote :

If you delete a rack controller we stop and disable the maas-rackd service. I think the rack register should make sure maas-rackd running and enabled.

Lee Trager (ltrager) wrote :

It'd also be nice if /var/lib/maas/secret exists try it and only prompt if it doesn't work but I wouldn't make that a requirement.

Gavin Panella (allenap) wrote :

This looks good, but it should extend the install-shared-secret command.

The latter prompts the user for the shared secret and accepts it via
stdin. Accepting passwords and security tokens via command-line
arguments has always been problematic because any user of the system can
see them.

It could call InstallSharedSecretScript.run() like so:

  # In src/provisioningserver/register_command.py:

  """Register rack controller.

  A shared secret required for communications with the region controller
  will be prompted for (or read from stdin in a non-interactive shell)
  if one has not previously been installed. You can find it at
  /var/lib/maas/secret on the region.
  """

  from provisioningserver.security import (
      get_shared_secret_from_filesystem,
      InstallSharedSecretScript,
  )

  def add_arguments(parser):
      """Add this command's options to the `ArgumentParser`.

      Specified by the `ActionScript` interface.
      """
      parser.add_argument(
          '--url', action='store', required=False,
          help=('URL of the region controller where to connect '
                'this rack controller.'))
      InstallSharedSecretScript.add_arguments(parser)

  def run(args):
      """Update configuration settings."""
      if args.url is not None:
          with ClusterConfiguration.open_for_update() as config:
              config.maas_url = args.url
      if get_shared_secret_from_filesystem() is None:
          InstallSharedSecretScript.run(args)

review: Needs Fixing
Newell Jensen (newell-jensen) wrote :

Gavin,

I had talked to Andres about doing exactly what you are mentioned with regards to using InstallSharedSecretScript.run but he said he didn't want us to go that route as user interaction for entering the shared key doesn't allow automation for using this command. I will run it by him and see if he still believes this to be the best route to take on this. Thanks for the review.

Gavin Panella (allenap) wrote :

> I had talked to Andres about doing exactly what you are mentioned with
> regards to using InstallSharedSecretScript.run but he said he didn't
> want us to go that route as user interaction for entering the shared
> key doesn't allow automation for using this command. I will run it by
> him and see if he still believes this to be the best route to take on
> this. Thanks for the review.

Fair enough, but both install-shared-secret and this new command are
trivial to automate:

  echo shared-secret | maas-rack register

echo is a builtin in both sh and bash so this avoids revealing the
secret in /proc/*/cmdline.

This isn't just me being paranoid, it's a common practice. PostgreSQL's
psql command, for example, takes a --password flag but it consumes no
arguments; its effect is to force the prompt for a password. It allows
the use of an environment variable, PGPASSWORD, but recommends against
it for security [1].

[1] http://www.postgresql.org/docs/current/static/libpq-envars.html

Gavin Panella (allenap) wrote :

The justifications given in the call yesterday for using a command-line
argument over the mechanism used by install-shared-secret were:

  1. It's easier to automate.

  2. MAAS accepts passwords via the command-line elsewhere.

#1 is incorrect. #2 says that it's broken elsewhere so we'll break it
here for the sake of #1. The example of install-shared-secret means that
#2 can also be used to argue exactly the opposite, i.e.:

  2b. MAAS does not accept passwords via the command-line elsewhere.

Newell, this is nothing personal — you've implemented what was requested
— but I can't approve this and remain honest.

review: Abstain
Andres Rodriguez (andreserl) wrote :

Maas-region createadmin --password your-clear-text-pass

On Tuesday, May 24, 2016, Gavin Panella <email address hidden> wrote:

> Review: Abstain
>
> The justifications given in the call yesterday for using a command-line
> argument over the mechanism used by install-shared-secret were:
>
> 1. It's easier to automate.
>
> 2. MAAS accepts passwords via the command-line elsewhere.
>
> #1 is incorrect. #2 says that it's broken elsewhere so we'll break it
> here for the sake of #1. The example of install-shared-secret means that
> #2 can also be used to argue exactly the opposite, i.e.:
>
> 2b. MAAS does not accept passwords via the command-line elsewhere.
>
> Newell, this is nothing personal — you've implemented what was requested
> — but I can't approve this and remain honest.
>
> --
>
> https://code.launchpad.net/~newell-jensen/maas/rack_register_command/+merge/295101
> You are subscribed to branch lp:maas.
>

--
Andres Rodriguez (RoAkSoAx)
Ubuntu Server Developer
MSc. Telecom & Networking
Systems Engineer

Gavin Panella (allenap) wrote :

> Maas-region createadmin --password your-clear-text-pass

Counterexamples in MAAS:

maas-region createsuperuser
maas-region changepassword
maas-rack install-shared-secret

Newell Jensen (newell-jensen) wrote :

This is ready for review.

Mike Pontillo (mpontillo) wrote :

One comment below.

Also, can you link this branch to bug #1588116?

review: Needs Fixing
Mike Pontillo (mpontillo) wrote :

Another comment below.

Newell Jensen (newell-jensen) wrote :

Replied inline.

5033. By Newell Jensen on 2016-06-03

Make unit test string closer to pep8 wideth of 79 columns per review comment.

5034. By Newell Jensen on 2016-06-03

Add examples to parser description.

Mike Pontillo (mpontillo) wrote :

This is great. Thanks for the fixes.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/provisioningserver/__main__.py'
2--- src/provisioningserver/__main__.py 2016-05-11 19:01:48 +0000
3+++ src/provisioningserver/__main__.py 2016-06-03 22:32:53 +0000
4@@ -8,6 +8,7 @@
5 import provisioningserver.boot.install_bootloader
6 import provisioningserver.boot.install_grub
7 import provisioningserver.cluster_config_command
8+import provisioningserver.register_command
9 import provisioningserver.support_dump
10 import provisioningserver.upgrade_cluster
11 from provisioningserver.utils.script import (
12@@ -24,6 +25,7 @@
13 'config': provisioningserver.cluster_config_command,
14 'install-shared-secret': security.InstallSharedSecretScript,
15 'install-uefi-config': provisioningserver.boot.install_grub,
16+ 'register': provisioningserver.register_command,
17 'support-dump': provisioningserver.support_dump,
18 'upgrade-cluster': provisioningserver.upgrade_cluster,
19 }
20
21=== added file 'src/provisioningserver/register_command.py'
22--- src/provisioningserver/register_command.py 1970-01-01 00:00:00 +0000
23+++ src/provisioningserver/register_command.py 2016-06-03 22:32:53 +0000
24@@ -0,0 +1,124 @@
25+# Copyright 2016 Canonical Ltd. This software is licensed under the
26+# GNU Affero General Public License version 3 (see the file LICENSE).
27+
28+"""Register rack controller.
29+
30+ A MAAS region controller URL where this rack controller should connect
31+ to will be prompted for when it has not been supplied.
32+
33+ Additionially, a shared secret required for communications with the region
34+ controller will be prompted for (or read from stdin in a non-interactive
35+ shell, see below) if one has not previously been installed. You can find it
36+ at /var/lib/maas/secret on the region.
37+
38+ Only the shared secret can be supplied via stdin (non-interactive shell).
39+ When this is the case, the user will need to supply the MAAS region
40+ controller URL.
41+ """
42+
43+__all__ = [
44+ 'add_arguments',
45+ 'run',
46+]
47+
48+from sys import stdin
49+from textwrap import dedent
50+
51+from provisioningserver.config import ClusterConfiguration
52+from provisioningserver.security import (
53+ InstallSharedSecretScript,
54+ set_shared_secret_on_filesystem,
55+ to_bin,
56+)
57+
58+
59+all_arguments = (
60+ '--url',
61+ '--secret',
62+)
63+
64+
65+def add_arguments(parser):
66+ """Add this command's options to the `ArgumentParser`.
67+
68+ Specified by the `ActionScript` interface.
69+ """
70+ parser.description = dedent("""\
71+ Examples of command usage (with interactive input):
72+
73+ - Supplying both URL and shared secret (not prompted for either):
74+
75+ # maas-rack register --secret <shared-secret> --url <your-url>
76+
77+ - Supplying URL but not shared secret (prompted for shared secret):
78+
79+ # maas-rack register --url <your-url>
80+ Secret (hex/base16 encoded): <shared-secret>
81+ Secret installed to /var/lib/maas/secret.
82+
83+ - Supplying shared secret but not URL (prompted for URL):
84+
85+ # maas-rack register --secret <shared-secret>
86+ MAAS region controller URL: <your-url>
87+ MAAS region controller URL saved as <your-url>
88+
89+ - Not supplying URL or shared secret (prompted for both):
90+
91+ # maas-rack register
92+ MAAS region controller URL: <your-url>
93+ MAAS region controller URL saved as <your-url>
94+ Secret (hex/base16 encoded): <shared-secret>
95+ Secret installed to /var/lib/maas/secret.
96+
97+ - Supplying secret via stdin but not URL
98+ (error message printed as this is non-interactive shell):
99+
100+ # echo <shared-secret> | maas-rack register
101+ MAAS region controller URL must be passed as an argument when supplying
102+ the shared secret via stdin with a non-interactive shell.
103+
104+ - Supplying secret via stdin and URL (not prompted):
105+
106+ # echo <shared-secret> | maas-rack register --url <your-url>
107+ Secret installed to /var/lib/maas/secret.
108+ """)
109+ parser.add_argument(
110+ '--url', action='store', required=False,
111+ help=(
112+ 'URL of the region controller where to connect this '
113+ 'rack controller.'))
114+ parser.add_argument(
115+ '--secret', action='store', required=False,
116+ help=(
117+ 'The shared secret required to connect to the region controller. '
118+ 'You can find it on /var/lib/maas/secret on the region. '
119+ 'The secret must be hex/base16 encoded.'))
120+
121+
122+def run(args):
123+ """Register the rack controller with a region controller."""
124+ # If stdin supplied to program URL must be passed as argument.
125+ if not stdin.isatty() and args.url is None:
126+ print(
127+ "MAAS region controller URL must be given when supplying the "
128+ "shared secret via stdin with a non-interactive shell.")
129+ raise SystemExit(1)
130+ if args.url is not None:
131+ with ClusterConfiguration.open_for_update() as config:
132+ config.maas_url = args.url
133+ else:
134+ try:
135+ url = input("MAAS region controller URL: ")
136+ except EOFError:
137+ print() # So that the shell prompt appears on the next line.
138+ raise SystemExit(1)
139+ except KeyboardInterrupt:
140+ print() # So that the shell prompt appears on the next line.
141+ raise
142+ with ClusterConfiguration.open_for_update() as config:
143+ config.maas_url = url
144+ print("MAAS region controller URL saved as %s." % url)
145+ if args.secret is not None:
146+ set_shared_secret_on_filesystem(to_bin(args.secret))
147+ else:
148+ InstallSharedSecretScript.run(args)
149
150=== added file 'src/provisioningserver/tests/test_register_command.py'
151--- src/provisioningserver/tests/test_register_command.py 1970-01-01 00:00:00 +0000
152+++ src/provisioningserver/tests/test_register_command.py 2016-06-03 22:32:53 +0000
153@@ -0,0 +1,172 @@
154+# Copyright 2016 Canonical Ltd. This software is licensed under the
155+# GNU Affero General Public License version 3 (see the file LICENSE).
156+
157+"""Tests for register command code."""
158+
159+__all__ = []
160+
161+from argparse import ArgumentParser
162+import io
163+from itertools import combinations
164+import pprint
165+from unittest.mock import Mock
166+
167+from maastesting.factory import factory
168+from maastesting.matchers import MockCalledOnceWith
169+from maastesting.testcase import MAASTestCase
170+from provisioningserver import register_command
171+from provisioningserver.config import ClusterConfiguration
172+from provisioningserver.security import (
173+ get_shared_secret_from_filesystem,
174+ set_shared_secret_on_filesystem,
175+ to_hex,
176+)
177+from provisioningserver.testing.config import ClusterConfigurationFixture
178+from testtools.matchers import Equals
179+
180+
181+class TestAddArguments(MAASTestCase):
182+
183+ def test_accepts_all_args(self):
184+ all_test_arguments = register_command.all_arguments
185+
186+ default_arg_values = {
187+ '--url': None,
188+ '--secret': None,
189+ }
190+
191+ failures = {}
192+
193+ # Try all cardinalities of combinations of arguments
194+ for r in range(len(all_test_arguments) + 1):
195+ for test_arg_names in combinations(all_test_arguments, r):
196+ test_values = {
197+ '--url': factory.make_simple_http_url(),
198+ '--secret': factory.make_name('secret')
199+ }
200+
201+ # Build a query dictionary for the given combination of args
202+ args_under_test = []
203+ for param_name in test_arg_names:
204+ args_under_test.append(param_name)
205+ args_under_test.append(test_values[param_name])
206+
207+ parser = ArgumentParser()
208+ register_command.add_arguments(parser)
209+
210+ observed_args = vars(
211+ parser.parse_args(args_under_test))
212+
213+ expected_args = {}
214+ for param_name in all_test_arguments:
215+ parsed_param_name = param_name[2:].replace('-', '_')
216+
217+ if param_name not in test_arg_names:
218+ expected_args[parsed_param_name] = (
219+ default_arg_values[param_name])
220+ else:
221+ expected_args[parsed_param_name] = (
222+ observed_args[parsed_param_name])
223+
224+ if expected_args != observed_args:
225+ failures[str(test_arg_names)] = {
226+ 'expected_args': expected_args,
227+ 'observed_args': observed_args,
228+ }
229+
230+ error_message = io.StringIO()
231+ error_message.write(
232+ "One or more key / value argument list(s) passed in the query "
233+ "string (expected_args) to the API do not match the values in "
234+ "the returned query string. This means that some arguments were "
235+ "dropped / added / changed by the the function, which is "
236+ "incorrect behavior. The list of incorrect arguments is as "
237+ "follows: \n")
238+ pp = pprint.PrettyPrinter(depth=3, stream=error_message)
239+ pp.pprint(failures)
240+ self.assertDictEqual({}, failures, error_message.getvalue())
241+
242+
243+class TestRegisterMAASRack(MAASTestCase):
244+
245+ def setUp(self):
246+ super(TestRegisterMAASRack, self).setUp()
247+ self.useFixture(ClusterConfigurationFixture())
248+
249+ def make_args(self, **kwargs):
250+ args = Mock()
251+ args.__dict__.update(kwargs)
252+ return args
253+
254+ def test____sets_url(self):
255+ secret = factory.make_bytes()
256+ expected_url = factory.make_simple_http_url()
257+ register_command.run(
258+ self.make_args(url=expected_url, secret=to_hex(secret)))
259+ with ClusterConfiguration.open() as config:
260+ observed = config.maas_url
261+ self.assertEqual(expected_url, observed)
262+
263+ def test___prompts_user_for_url(self):
264+ expected_url = factory.make_simple_http_url()
265+ secret = factory.make_bytes()
266+
267+ stdin = self.patch(register_command, "stdin")
268+ stdin.isatty.return_value = True
269+
270+ input = self.patch(register_command, "input")
271+ input.return_value = expected_url
272+
273+ register_command.run(self.make_args(url=None, secret=to_hex(secret)))
274+ with ClusterConfiguration.open() as config:
275+ observed = config.maas_url
276+
277+ self.expectThat(
278+ input, MockCalledOnceWith("MAAS region controller URL: "))
279+ self.expectThat(expected_url, Equals(observed))
280+
281+ def test___sets_secret(self):
282+ url = factory.make_simple_http_url()
283+ expected = factory.make_bytes()
284+ register_command.run(self.make_args(url=url, secret=to_hex(expected)))
285+ observed = get_shared_secret_from_filesystem()
286+ self.assertEqual(expected, observed)
287+
288+ def test__prompts_user_for_secret(self):
289+ url = factory.make_simple_http_url()
290+ expected_previous_value = factory.make_bytes()
291+ set_shared_secret_on_filesystem(expected_previous_value)
292+ InstallSharedSecretScript_mock = self.patch(
293+ register_command, "InstallSharedSecretScript")
294+ args = self.make_args(url=url, secret=None)
295+ register_command.run(args)
296+ observed = get_shared_secret_from_filesystem()
297+
298+ self.expectThat(expected_previous_value, Equals(observed))
299+ self.expectThat(
300+ InstallSharedSecretScript_mock.run, MockCalledOnceWith(args))
301+
302+ def test__errors_out_when_piped_stdin_and_url_not_supplied(self):
303+ args = self.make_args(url=None)
304+ stdin = self.patch(register_command, "stdin")
305+ stdin.isatty.return_value = False
306+ self.assertRaises(
307+ SystemExit, register_command.run, args)
308+
309+ def test__crashes_on_eoferror(self):
310+ args = self.make_args(url=None)
311+ stdin = self.patch(register_command, "stdin")
312+ stdin.isatty.return_value = True
313+ input = self.patch(register_command, "input")
314+ input.side_effect = EOFError
315+ self.assertRaises(
316+ SystemExit, register_command.run, args)
317+
318+ def test__crashes_on_keyboardinterrupt(self):
319+ args = self.make_args(url=None)
320+ stdin = self.patch(register_command, "stdin")
321+ stdin.isatty.return_value = True
322+ input = self.patch(register_command, "input")
323+ input.side_effect = KeyboardInterrupt
324+ self.assertRaises(
325+ KeyboardInterrupt, register_command.run, args)
326
327=== modified file 'src/provisioningserver/utils/script.py'
328--- src/provisioningserver/utils/script.py 2015-12-03 23:28:19 +0000
329+++ src/provisioningserver/utils/script.py 2016-06-03 22:32:53 +0000
330@@ -1,4 +1,4 @@
331-# Copyright 2014-2015 Canonical Ltd. This software is licensed under the
332+# Copyright 2014-2016 Canonical Ltd. This software is licensed under the
333 # GNU Affero General Public License version 3 (see the file LICENSE).
334
335 """Utilities for adding sub-commands to the MAAS management commands."""
336@@ -9,7 +9,10 @@
337 'MainScript',
338 ]
339
340-from argparse import ArgumentParser
341+from argparse import (
342+ ArgumentParser,
343+ RawDescriptionHelpFormatter,
344+)
345 import io
346 import signal
347 from subprocess import CalledProcessError
348@@ -61,7 +64,8 @@
349 release/2.7/library/argparse.html#sub-commands
350 """
351 parser = self.subparsers.add_parser(
352- name, *args, help=handler.run.__doc__, **kwargs)
353+ name, *args, help=handler.run.__doc__,
354+ formatter_class=RawDescriptionHelpFormatter, **kwargs)
355 parser.set_defaults(handler=handler)
356 handler.add_arguments(parser)
357 return parser