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

[Newell Jensen]

Add rack register command.

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.

[Lee Trager (ltrager)]

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)]

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)]

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)]

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)]

> 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)]

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)]

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)]

> 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)]

This is ready for review.

[Mike Pontillo (mpontillo)]

One comment below.

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

review: Needs Fixing

[Mike Pontillo (mpontillo)]

Another comment below.

[Newell Jensen (newell-jensen)]

Replied inline.

[Mike Pontillo (mpontillo)]

This is great. Thanks for the fixes.

review: Approve

=== 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 @@
 import provisioningserver.boot.install_bootloader
 import provisioningserver.boot.install_grub
 import provisioningserver.cluster_config_command
+import provisioningserver.register_command
 import provisioningserver.support_dump
 import provisioningserver.upgrade_cluster
 from provisioningserver.utils.script import (
@@ -24,6 +25,7 @@
     'config': provisioningserver.cluster_config_command,
     'install-shared-secret': security.InstallSharedSecretScript,
     'install-uefi-config': provisioningserver.boot.install_grub,
+    'register': provisioningserver.register_command,
     'support-dump': provisioningserver.support_dump,
     'upgrade-cluster': provisioningserver.upgrade_cluster,
 }

=== 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 @@
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Register rack controller.
+
+  A MAAS region controller URL where this rack controller should connect
+  to will be prompted for when it has not been supplied.
+
+  Additionially, a shared secret required for communications with the region
+  controller will be prompted for (or read from stdin in a non-interactive
+  shell, see below) if one has not previously been installed. You can find it
+  at /var/lib/maas/secret on the region.
+
+  Only the shared secret can be supplied via stdin (non-interactive shell).
+  When this is the case, the user will need to supply the MAAS region
+  controller URL.
+  """
+
+__all__ = [
+    'add_arguments',
+    'run',
+]
+
+from sys import stdin
+from textwrap import dedent
+
+from provisioningserver.config import ClusterConfiguration
+from provisioningserver.security import (
+    InstallSharedSecretScript,
+    set_shared_secret_on_filesystem,
+    to_bin,
+)
+
+
+all_arguments = (
+    '--url',
+    '--secret',
+)
+
+
+def add_arguments(parser):
+    """Add this command's options to the `ArgumentParser`.
+
+    Specified by the `ActionScript` interface.
+    """
+    parser.description = dedent("""\
+        Examples of command usage (with interactive input):
+
+        - Supplying both URL and shared secret (not prompted for either):
+
+        # maas-rack register --secret <shared-secret> --url <your-url>
+
+        - Supplying URL but not shared secret (prompted for shared secret):
+
+        # maas-rack register --url <your-url>
+        Secret (hex/base16 encoded): <shared-secret>
+        Secret installed to /var/lib/maas/secret.
+
+        - Supplying shared secret but not URL (prompted for URL):
+
+        # maas-rack register --secret <shared-secret>
+        MAAS region controller URL: <your-url>
+        MAAS region controller URL saved as <your-url>
+
+        - Not supplying URL or shared secret (prompted for both):
+
+        # maas-rack register
+        MAAS region controller URL: <your-url>
+        MAAS region controller URL saved as <your-url>
+        Secret (hex/base16 encoded): <shared-secret>
+        Secret installed to /var/lib/maas/secret.
+
+        - Supplying secret via stdin but not URL
+          (error message printed as this is non-interactive shell):
+
+        # echo <shared-secret> | 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 <shared-secret> | maas-rack register --url <your-url>
+        Secret installed to /var/lib/maas/secret.
+    """)
+    parser.add_argument(
+        '--url', action='store', required=False,
+        help=(
+            'URL of the region controller where to connect this '
+            'rack controller.'))
+    parser.add_argument(
+        '--secret', action='store', required=False,
+        help=(
+            'The shared secret required to connect to the region controller.  '
+            'You can find it on /var/lib/maas/secret on the region.  '
+            'The secret must be hex/base16 encoded.'))
+
+
+def run(args):
+    """Register the rack controller with a region controller."""
+    # If stdin supplied to program URL must be passed as argument.
+    if not stdin.isatty() and args.url is None:
+        print(
+            "MAAS region controller URL must be given when supplying the "
+            "shared secret via stdin with a non-interactive shell.")
+        raise SystemExit(1)
+    if args.url is not None:
+        with ClusterConfiguration.open_for_update() as config:
+            config.maas_url = args.url
+    else:
+        try:
+            url = input("MAAS region controller URL: ")
+        except EOFError:
+            print()  # So that the shell prompt appears on the next line.
+            raise SystemExit(1)
+        except KeyboardInterrupt:
+            print()  # So that the shell prompt appears on the next line.
+            raise
+        with ClusterConfiguration.open_for_update() as config:
+            config.maas_url = url
+        print("MAAS region controller URL saved as %s." % url)
+    if args.secret is not None:
+        set_shared_secret_on_filesystem(to_bin(args.secret))
+    else:
+        InstallSharedSecretScript.run(args)

=== 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 @@
+# Copyright 2016 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for register command code."""
+
+__all__ = []
+
+from argparse import ArgumentParser
+import io
+from itertools import combinations
+import pprint
+from unittest.mock import Mock
+
+from maastesting.factory import factory
+from maastesting.matchers import MockCalledOnceWith
+from maastesting.testcase import MAASTestCase
+from provisioningserver import register_command
+from provisioningserver.config import ClusterConfiguration
+from provisioningserver.security import (
+    get_shared_secret_from_filesystem,
+    set_shared_secret_on_filesystem,
+    to_hex,
+)
+from provisioningserver.testing.config import ClusterConfigurationFixture
+from testtools.matchers import Equals
+
+
+class TestAddArguments(MAASTestCase):
+
+    def test_accepts_all_args(self):
+        all_test_arguments = register_command.all_arguments
+
+        default_arg_values = {
+            '--url': None,
+            '--secret': None,
+        }
+
+        failures = {}
+
+        # Try all cardinalities of combinations of arguments
+        for r in range(len(all_test_arguments) + 1):
+            for test_arg_names in combinations(all_test_arguments, r):
+                test_values = {
+                    '--url': factory.make_simple_http_url(),
+                    '--secret': factory.make_name('secret')
+                }
+
+                # Build a query dictionary for the given combination of args
+                args_under_test = []
+                for param_name in test_arg_names:
+                    args_under_test.append(param_name)
+                    args_under_test.append(test_values[param_name])
+
+                parser = ArgumentParser()
+                register_command.add_arguments(parser)
+
+                observed_args = vars(
+                    parser.parse_args(args_under_test))
+
+                expected_args = {}
+                for param_name in all_test_arguments:
+                    parsed_param_name = param_name[2:].replace('-', '_')
+
+                    if param_name not in test_arg_names:
+                        expected_args[parsed_param_name] = (
+                            default_arg_values[param_name])
+                    else:
+                        expected_args[parsed_param_name] = (
+                            observed_args[parsed_param_name])
+
+                if expected_args != observed_args:
+                    failures[str(test_arg_names)] = {
+                        'expected_args': expected_args,
+                        'observed_args': observed_args,
+                    }
+
+        error_message = io.StringIO()
+        error_message.write(
+            "One or more key / value argument list(s) passed in the query "
+            "string (expected_args) to the API do not match the values in "
+            "the returned query string. This means that some arguments were "
+            "dropped / added / changed by the the function, which is "
+            "incorrect behavior. The list of incorrect arguments is as "
+            "follows: \n")
+        pp = pprint.PrettyPrinter(depth=3, stream=error_message)
+        pp.pprint(failures)
+        self.assertDictEqual({}, failures, error_message.getvalue())
+
+
+class TestRegisterMAASRack(MAASTestCase):
+
+    def setUp(self):
+        super(TestRegisterMAASRack, self).setUp()
+        self.useFixture(ClusterConfigurationFixture())
+
+    def make_args(self, **kwargs):
+        args = Mock()
+        args.__dict__.update(kwargs)
+        return args
+
+    def test____sets_url(self):
+        secret = factory.make_bytes()
+        expected_url = factory.make_simple_http_url()
+        register_command.run(
+            self.make_args(url=expected_url, secret=to_hex(secret)))
+        with ClusterConfiguration.open() as config:
+            observed = config.maas_url
+        self.assertEqual(expected_url, observed)
+
+    def test___prompts_user_for_url(self):
+        expected_url = factory.make_simple_http_url()
+        secret = factory.make_bytes()
+
+        stdin = self.patch(register_command, "stdin")
+        stdin.isatty.return_value = True
+
+        input = self.patch(register_command, "input")
+        input.return_value = expected_url
+
+        register_command.run(self.make_args(url=None, secret=to_hex(secret)))
+        with ClusterConfiguration.open() as config:
+            observed = config.maas_url
+
+        self.expectThat(
+            input, MockCalledOnceWith("MAAS region controller URL: "))
+        self.expectThat(expected_url, Equals(observed))
+
+    def test___sets_secret(self):
+        url = factory.make_simple_http_url()
+        expected = factory.make_bytes()
+        register_command.run(self.make_args(url=url, secret=to_hex(expected)))
+        observed = get_shared_secret_from_filesystem()
+        self.assertEqual(expected, observed)
+
+    def test__prompts_user_for_secret(self):
+        url = factory.make_simple_http_url()
+        expected_previous_value = factory.make_bytes()
+        set_shared_secret_on_filesystem(expected_previous_value)
+        InstallSharedSecretScript_mock = self.patch(
+            register_command, "InstallSharedSecretScript")
+        args = self.make_args(url=url, secret=None)
+        register_command.run(args)
+        observed = get_shared_secret_from_filesystem()
+
+        self.expectThat(expected_previous_value, Equals(observed))
+        self.expectThat(
+            InstallSharedSecretScript_mock.run, MockCalledOnceWith(args))
+
+    def test__errors_out_when_piped_stdin_and_url_not_supplied(self):
+        args = self.make_args(url=None)
+        stdin = self.patch(register_command, "stdin")
+        stdin.isatty.return_value = False
+        self.assertRaises(
+            SystemExit, register_command.run, args)
+
+    def test__crashes_on_eoferror(self):
+        args = self.make_args(url=None)
+        stdin = self.patch(register_command, "stdin")
+        stdin.isatty.return_value = True
+        input = self.patch(register_command, "input")
+        input.side_effect = EOFError
+        self.assertRaises(
+            SystemExit, register_command.run, args)
+
+    def test__crashes_on_keyboardinterrupt(self):
+        args = self.make_args(url=None)
+        stdin = self.patch(register_command, "stdin")
+        stdin.isatty.return_value = True
+        input = self.patch(register_command, "input")
+        input.side_effect = KeyboardInterrupt
+        self.assertRaises(
+            KeyboardInterrupt, register_command.run, args)

=== 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 @@
-# Copyright 2014-2015 Canonical Ltd.  This software is licensed under the
+# Copyright 2014-2016 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Utilities for adding sub-commands to the MAAS management commands."""
@@ -9,7 +9,10 @@
     'MainScript',
     ]
 
-from argparse import ArgumentParser
+from argparse import (
+    ArgumentParser,
+    RawDescriptionHelpFormatter,
+)
 import io
 import signal
 from subprocess import CalledProcessError
@@ -61,7 +64,8 @@
             release/2.7/library/argparse.html#sub-commands
         """
         parser = self.subparsers.add_parser(
-            name, *args, help=handler.run.__doc__, **kwargs)
+            name, *args, help=handler.run.__doc__,
+            formatter_class=RawDescriptionHelpFormatter, **kwargs)
         parser.set_defaults(handler=handler)
         handler.add_arguments(parser)
         return parser