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

Proposed by Newell Jensen
Status: Merged
Approved by: Newell Jensen
Approved revision: no longer in the source branch.
Merged at revision: 5069
Proposed branch: lp:~newell-jensen/maas/rack_register_command
Merge into: lp:~maas-committers/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
Gavin Panella (community) Abstain
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.
Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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
Revision history for this message
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.

Revision history for this message
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

Revision history for this message
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
Revision history for this message
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

Revision history for this message
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

Revision history for this message
Newell Jensen (newell-jensen) wrote :

This is ready for review.

Revision history for this message
Mike Pontillo (mpontillo) wrote :

One comment below.

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

review: Needs Fixing
Revision history for this message
Mike Pontillo (mpontillo) wrote :

Another comment below.

Revision history for this message
Newell Jensen (newell-jensen) wrote :

Replied inline.

Revision history for this message
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
=== modified file 'src/provisioningserver/__main__.py'
--- src/provisioningserver/__main__.py 2016-05-11 19:01:48 +0000
+++ src/provisioningserver/__main__.py 2016-06-03 22:32:53 +0000
@@ -8,6 +8,7 @@
8import provisioningserver.boot.install_bootloader8import provisioningserver.boot.install_bootloader
9import provisioningserver.boot.install_grub9import provisioningserver.boot.install_grub
10import provisioningserver.cluster_config_command10import provisioningserver.cluster_config_command
11import provisioningserver.register_command
11import provisioningserver.support_dump12import provisioningserver.support_dump
12import provisioningserver.upgrade_cluster13import provisioningserver.upgrade_cluster
13from provisioningserver.utils.script import (14from provisioningserver.utils.script import (
@@ -24,6 +25,7 @@
24 'config': provisioningserver.cluster_config_command,25 'config': provisioningserver.cluster_config_command,
25 'install-shared-secret': security.InstallSharedSecretScript,26 'install-shared-secret': security.InstallSharedSecretScript,
26 'install-uefi-config': provisioningserver.boot.install_grub,27 'install-uefi-config': provisioningserver.boot.install_grub,
28 'register': provisioningserver.register_command,
27 'support-dump': provisioningserver.support_dump,29 'support-dump': provisioningserver.support_dump,
28 'upgrade-cluster': provisioningserver.upgrade_cluster,30 'upgrade-cluster': provisioningserver.upgrade_cluster,
29}31}
3032
=== added file 'src/provisioningserver/register_command.py'
--- src/provisioningserver/register_command.py 1970-01-01 00:00:00 +0000
+++ src/provisioningserver/register_command.py 2016-06-03 22:32:53 +0000
@@ -0,0 +1,124 @@
1# Copyright 2016 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Register rack controller.
5
6 A MAAS region controller URL where this rack controller should connect
7 to will be prompted for when it has not been supplied.
8
9 Additionially, a shared secret required for communications with the region
10 controller will be prompted for (or read from stdin in a non-interactive
11 shell, see below) if one has not previously been installed. You can find it
12 at /var/lib/maas/secret on the region.
13
14 Only the shared secret can be supplied via stdin (non-interactive shell).
15 When this is the case, the user will need to supply the MAAS region
16 controller URL.
17 """
18
19__all__ = [
20 'add_arguments',
21 'run',
22]
23
24from sys import stdin
25from textwrap import dedent
26
27from provisioningserver.config import ClusterConfiguration
28from provisioningserver.security import (
29 InstallSharedSecretScript,
30 set_shared_secret_on_filesystem,
31 to_bin,
32)
33
34
35all_arguments = (
36 '--url',
37 '--secret',
38)
39
40
41def add_arguments(parser):
42 """Add this command's options to the `ArgumentParser`.
43
44 Specified by the `ActionScript` interface.
45 """
46 parser.description = dedent("""\
47 Examples of command usage (with interactive input):
48
49 - Supplying both URL and shared secret (not prompted for either):
50
51 # maas-rack register --secret <shared-secret> --url <your-url>
52
53 - Supplying URL but not shared secret (prompted for shared secret):
54
55 # maas-rack register --url <your-url>
56 Secret (hex/base16 encoded): <shared-secret>
57 Secret installed to /var/lib/maas/secret.
58
59 - Supplying shared secret but not URL (prompted for URL):
60
61 # maas-rack register --secret <shared-secret>
62 MAAS region controller URL: <your-url>
63 MAAS region controller URL saved as <your-url>
64
65 - Not supplying URL or shared secret (prompted for both):
66
67 # maas-rack register
68 MAAS region controller URL: <your-url>
69 MAAS region controller URL saved as <your-url>
70 Secret (hex/base16 encoded): <shared-secret>
71 Secret installed to /var/lib/maas/secret.
72
73 - Supplying secret via stdin but not URL
74 (error message printed as this is non-interactive shell):
75
76 # echo <shared-secret> | maas-rack register
77 MAAS region controller URL must be passed as an argument when supplying
78 the shared secret via stdin with a non-interactive shell.
79
80 - Supplying secret via stdin and URL (not prompted):
81
82 # echo <shared-secret> | maas-rack register --url <your-url>
83 Secret installed to /var/lib/maas/secret.
84 """)
85 parser.add_argument(
86 '--url', action='store', required=False,
87 help=(
88 'URL of the region controller where to connect this '
89 'rack controller.'))
90 parser.add_argument(
91 '--secret', action='store', required=False,
92 help=(
93 'The shared secret required to connect to the region controller. '
94 'You can find it on /var/lib/maas/secret on the region. '
95 'The secret must be hex/base16 encoded.'))
96
97
98def run(args):
99 """Register the rack controller with a region controller."""
100 # If stdin supplied to program URL must be passed as argument.
101 if not stdin.isatty() and args.url is None:
102 print(
103 "MAAS region controller URL must be given when supplying the "
104 "shared secret via stdin with a non-interactive shell.")
105 raise SystemExit(1)
106 if args.url is not None:
107 with ClusterConfiguration.open_for_update() as config:
108 config.maas_url = args.url
109 else:
110 try:
111 url = input("MAAS region controller URL: ")
112 except EOFError:
113 print() # So that the shell prompt appears on the next line.
114 raise SystemExit(1)
115 except KeyboardInterrupt:
116 print() # So that the shell prompt appears on the next line.
117 raise
118 with ClusterConfiguration.open_for_update() as config:
119 config.maas_url = url
120 print("MAAS region controller URL saved as %s." % url)
121 if args.secret is not None:
122 set_shared_secret_on_filesystem(to_bin(args.secret))
123 else:
124 InstallSharedSecretScript.run(args)
0125
=== added file 'src/provisioningserver/tests/test_register_command.py'
--- src/provisioningserver/tests/test_register_command.py 1970-01-01 00:00:00 +0000
+++ src/provisioningserver/tests/test_register_command.py 2016-06-03 22:32:53 +0000
@@ -0,0 +1,172 @@
1# Copyright 2016 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Tests for register command code."""
5
6__all__ = []
7
8from argparse import ArgumentParser
9import io
10from itertools import combinations
11import pprint
12from unittest.mock import Mock
13
14from maastesting.factory import factory
15from maastesting.matchers import MockCalledOnceWith
16from maastesting.testcase import MAASTestCase
17from provisioningserver import register_command
18from provisioningserver.config import ClusterConfiguration
19from provisioningserver.security import (
20 get_shared_secret_from_filesystem,
21 set_shared_secret_on_filesystem,
22 to_hex,
23)
24from provisioningserver.testing.config import ClusterConfigurationFixture
25from testtools.matchers import Equals
26
27
28class TestAddArguments(MAASTestCase):
29
30 def test_accepts_all_args(self):
31 all_test_arguments = register_command.all_arguments
32
33 default_arg_values = {
34 '--url': None,
35 '--secret': None,
36 }
37
38 failures = {}
39
40 # Try all cardinalities of combinations of arguments
41 for r in range(len(all_test_arguments) + 1):
42 for test_arg_names in combinations(all_test_arguments, r):
43 test_values = {
44 '--url': factory.make_simple_http_url(),
45 '--secret': factory.make_name('secret')
46 }
47
48 # Build a query dictionary for the given combination of args
49 args_under_test = []
50 for param_name in test_arg_names:
51 args_under_test.append(param_name)
52 args_under_test.append(test_values[param_name])
53
54 parser = ArgumentParser()
55 register_command.add_arguments(parser)
56
57 observed_args = vars(
58 parser.parse_args(args_under_test))
59
60 expected_args = {}
61 for param_name in all_test_arguments:
62 parsed_param_name = param_name[2:].replace('-', '_')
63
64 if param_name not in test_arg_names:
65 expected_args[parsed_param_name] = (
66 default_arg_values[param_name])
67 else:
68 expected_args[parsed_param_name] = (
69 observed_args[parsed_param_name])
70
71 if expected_args != observed_args:
72 failures[str(test_arg_names)] = {
73 'expected_args': expected_args,
74 'observed_args': observed_args,
75 }
76
77 error_message = io.StringIO()
78 error_message.write(
79 "One or more key / value argument list(s) passed in the query "
80 "string (expected_args) to the API do not match the values in "
81 "the returned query string. This means that some arguments were "
82 "dropped / added / changed by the the function, which is "
83 "incorrect behavior. The list of incorrect arguments is as "
84 "follows: \n")
85 pp = pprint.PrettyPrinter(depth=3, stream=error_message)
86 pp.pprint(failures)
87 self.assertDictEqual({}, failures, error_message.getvalue())
88
89
90class TestRegisterMAASRack(MAASTestCase):
91
92 def setUp(self):
93 super(TestRegisterMAASRack, self).setUp()
94 self.useFixture(ClusterConfigurationFixture())
95
96 def make_args(self, **kwargs):
97 args = Mock()
98 args.__dict__.update(kwargs)
99 return args
100
101 def test____sets_url(self):
102 secret = factory.make_bytes()
103 expected_url = factory.make_simple_http_url()
104 register_command.run(
105 self.make_args(url=expected_url, secret=to_hex(secret)))
106 with ClusterConfiguration.open() as config:
107 observed = config.maas_url
108 self.assertEqual(expected_url, observed)
109
110 def test___prompts_user_for_url(self):
111 expected_url = factory.make_simple_http_url()
112 secret = factory.make_bytes()
113
114 stdin = self.patch(register_command, "stdin")
115 stdin.isatty.return_value = True
116
117 input = self.patch(register_command, "input")
118 input.return_value = expected_url
119
120 register_command.run(self.make_args(url=None, secret=to_hex(secret)))
121 with ClusterConfiguration.open() as config:
122 observed = config.maas_url
123
124 self.expectThat(
125 input, MockCalledOnceWith("MAAS region controller URL: "))
126 self.expectThat(expected_url, Equals(observed))
127
128 def test___sets_secret(self):
129 url = factory.make_simple_http_url()
130 expected = factory.make_bytes()
131 register_command.run(self.make_args(url=url, secret=to_hex(expected)))
132 observed = get_shared_secret_from_filesystem()
133 self.assertEqual(expected, observed)
134
135 def test__prompts_user_for_secret(self):
136 url = factory.make_simple_http_url()
137 expected_previous_value = factory.make_bytes()
138 set_shared_secret_on_filesystem(expected_previous_value)
139 InstallSharedSecretScript_mock = self.patch(
140 register_command, "InstallSharedSecretScript")
141 args = self.make_args(url=url, secret=None)
142 register_command.run(args)
143 observed = get_shared_secret_from_filesystem()
144
145 self.expectThat(expected_previous_value, Equals(observed))
146 self.expectThat(
147 InstallSharedSecretScript_mock.run, MockCalledOnceWith(args))
148
149 def test__errors_out_when_piped_stdin_and_url_not_supplied(self):
150 args = self.make_args(url=None)
151 stdin = self.patch(register_command, "stdin")
152 stdin.isatty.return_value = False
153 self.assertRaises(
154 SystemExit, register_command.run, args)
155
156 def test__crashes_on_eoferror(self):
157 args = self.make_args(url=None)
158 stdin = self.patch(register_command, "stdin")
159 stdin.isatty.return_value = True
160 input = self.patch(register_command, "input")
161 input.side_effect = EOFError
162 self.assertRaises(
163 SystemExit, register_command.run, args)
164
165 def test__crashes_on_keyboardinterrupt(self):
166 args = self.make_args(url=None)
167 stdin = self.patch(register_command, "stdin")
168 stdin.isatty.return_value = True
169 input = self.patch(register_command, "input")
170 input.side_effect = KeyboardInterrupt
171 self.assertRaises(
172 KeyboardInterrupt, register_command.run, args)
0173
=== modified file 'src/provisioningserver/utils/script.py'
--- src/provisioningserver/utils/script.py 2015-12-03 23:28:19 +0000
+++ src/provisioningserver/utils/script.py 2016-06-03 22:32:53 +0000
@@ -1,4 +1,4 @@
1# Copyright 2014-2015 Canonical Ltd. This software is licensed under the1# Copyright 2014-2016 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""Utilities for adding sub-commands to the MAAS management commands."""4"""Utilities for adding sub-commands to the MAAS management commands."""
@@ -9,7 +9,10 @@
9 'MainScript',9 'MainScript',
10 ]10 ]
1111
12from argparse import ArgumentParser12from argparse import (
13 ArgumentParser,
14 RawDescriptionHelpFormatter,
15)
13import io16import io
14import signal17import signal
15from subprocess import CalledProcessError18from subprocess import CalledProcessError
@@ -61,7 +64,8 @@
61 release/2.7/library/argparse.html#sub-commands64 release/2.7/library/argparse.html#sub-commands
62 """65 """
63 parser = self.subparsers.add_parser(66 parser = self.subparsers.add_parser(
64 name, *args, help=handler.run.__doc__, **kwargs)67 name, *args, help=handler.run.__doc__,
68 formatter_class=RawDescriptionHelpFormatter, **kwargs)
65 parser.set_defaults(handler=handler)69 parser.set_defaults(handler=handler)
66 handler.add_arguments(parser)70 handler.add_arguments(parser)
67 return parser71 return parser