compute: Generate SSH keypairs ourselves

Starting with the 2.92 microversion, nova will no longer generate SSH
keys. Avoid breaking users by generating keypairs ourselves using the
cryptography library, which was already an indirect dependency through
openstacksdk.

Change-Id: I3ad2732f70854ab72da0947f00847351dda23944
Implements: blueprint keypair-generation-removal

Author: stephenfin

openstackclient/compute/v2/keypair.py

@@ -15,11 +15,13 @@
 
 """Keypair action implementations"""
 
+import collections
 import io
 import logging
 import os
-import sys
 
+from cryptography.hazmat.primitives.asymmetric import ed25519
+from cryptography.hazmat.primitives import serialization
 from openstack import utils as sdk_utils
 from osc_lib.command import command
 from osc_lib import exceptions
@@ -30,6 +32,27 @@
 
 
 LOG = logging.getLogger(__name__)
+Keypair = collections.namedtuple('Keypair', 'private_key public_key')
+
+
+def _generate_keypair():
+    """Generate a Ed25519 keypair in OpenSSH format.
+
+    :returns: A `Keypair` named tuple with the generated private and public
+    keys.
+    """
+    key = ed25519.Ed25519PrivateKey.generate()
+    private_key = key.private_bytes(
+        serialization.Encoding.PEM,
+        serialization.PrivateFormat.OpenSSH,
+        serialization.NoEncryption()
+    ).decode()
+    public_key = key.public_key().public_bytes(
+        serialization.Encoding.OpenSSH,
+        serialization.PublicFormat.OpenSSH
+    ).decode()
+
+    return Keypair(private_key, public_key)
 
 
 def _get_keypair_columns(item, hide_pub_key=False, hide_priv_key=False):
@@ -59,30 +82,37 @@ def get_parser(self, prog_name):
         key_group.add_argument(
             '--public-key',
             metavar='<file>',
-            help=_("Filename for public key to add. If not used, "
-                   "creates a private key.")
+            help=_(
+                "Filename for public key to add. "
+                "If not used, generates a private key in ssh-ed25519 format. "
+                "To generate keys in other formats, including the legacy "
+                "ssh-rsa format, you must use an external tool such as "
+                "ssh-keygen and specify this argument."
+            ),
         )
         key_group.add_argument(
             '--private-key',
             metavar='<file>',
-            help=_("Filename for private key to save. If not used, "
-                   "print private key in console.")
+            help=_(
+                "Filename for private key to save. "
+                "If not used, print private key in console."
+            )
         )
         parser.add_argument(
             '--type',
             metavar='<type>',
             choices=['ssh', 'x509'],
             help=_(
-                "Keypair type. Can be ssh or x509. "
-                "(Supported by API versions '2.2' - '2.latest')"
+                'Keypair type '
+                '(supported by --os-compute-api-version 2.2 or above)'
             ),
         )
         parser.add_argument(
             '--user',
             metavar='<user>',
             help=_(
-                'The owner of the keypair. (admin only) (name or ID). '
-                'Requires ``--os-compute-api-version`` 2.10 or greater.'
+                'The owner of the keypair (admin only) (name or ID) '
+                '(supported by --os-compute-api-version 2.10 or above)'
             ),
         )
         identity_common.add_user_domain_option_to_parser(parser)
@@ -96,19 +126,43 @@ def take_action(self, parsed_args):
             'name': parsed_args.name
         }
 
-        public_key = parsed_args.public_key
-        if public_key:
+        if parsed_args.public_key:
+            generated_keypair = None
             try:
                 with io.open(os.path.expanduser(parsed_args.public_key)) as p:
                     public_key = p.read()
             except IOError as e:
                 msg = _("Key file %(public_key)s not found: %(exception)s")
                 raise exceptions.CommandError(
-                    msg % {"public_key": parsed_args.public_key,
-                           "exception": e}
+                    msg % {
+                        "public_key": parsed_args.public_key,
+                        "exception": e,
+                    }
                 )
 
             kwargs['public_key'] = public_key
+        else:
+            generated_keypair = _generate_keypair()
+            kwargs['public_key'] = generated_keypair.public_key
+
+            # If user have us a file, save private key into specified file
+            if parsed_args.private_key:
+                try:
+                    with io.open(
+                        os.path.expanduser(parsed_args.private_key), 'w+'
+                    ) as p:
+                        p.write(generated_keypair.private_key)
+                except IOError as e:
+                    msg = _(
+                        "Key file %(private_key)s can not be saved: "
+                        "%(exception)s"
+                    )
+                    raise exceptions.CommandError(
+                        msg % {
+                            "private_key": parsed_args.private_key,
+                            "exception": e,
+                        }
+                    )
 
         if parsed_args.type:
             if not sdk_utils.supports_microversion(compute_client, '2.2'):
@@ -136,32 +190,17 @@ def take_action(self, parsed_args):
 
         keypair = compute_client.create_keypair(**kwargs)
 
-        private_key = parsed_args.private_key
-        # Save private key into specified file
-        if private_key:
-            try:
-                with io.open(
-                    os.path.expanduser(parsed_args.private_key), 'w+'
-                ) as p:
-                    p.write(keypair.private_key)
-            except IOError as e:
-                msg = _("Key file %(private_key)s can not be saved: "
-                        "%(exception)s")
-                raise exceptions.CommandError(
-                    msg % {"private_key": parsed_args.private_key,
-                           "exception": e}
-                )
         # NOTE(dtroyer): how do we want to handle the display of the private
         #                key when it needs to be communicated back to the user
         #                For now, duplicate nova keypair-add command output
-        if public_key or private_key:
+        if parsed_args.public_key or parsed_args.private_key:
             display_columns, columns = _get_keypair_columns(
                 keypair, hide_pub_key=True, hide_priv_key=True)
             data = utils.get_item_properties(keypair, columns)
 
             return (display_columns, data)
         else:
-            sys.stdout.write(keypair.private_key)
+            self.app.stdout.write(generated_keypair.private_key)
             return ({}, {})
 
 
@@ -405,5 +444,5 @@ def take_action(self, parsed_args):
             data = utils.get_item_properties(keypair, columns)
             return (display_columns, data)
         else:
-            sys.stdout.write(keypair.public_key)
+            self.app.stdout.write(keypair.public_key)
             return ({}, {})

openstackclient/tests/functional/compute/v2/test_keypair.py

@@ -117,24 +117,28 @@ def test_keypair_create_private_key(self):
             self.assertIsNotNone(cmd_output.get('user_id'))
             self.assertIsNotNone(cmd_output.get('fingerprint'))
             pk_content = f.read()
-            self.assertInOutput('-----BEGIN RSA PRIVATE KEY-----', pk_content)
+            self.assertInOutput(
+                '-----BEGIN OPENSSH PRIVATE KEY-----', pk_content,
+            )
             self.assertRegex(pk_content, "[0-9A-Za-z+/]+[=]{0,3}\n")
-            self.assertInOutput('-----END RSA PRIVATE KEY-----', pk_content)
+            self.assertInOutput(
+                '-----END OPENSSH PRIVATE KEY-----', pk_content,
+            )
 
     def test_keypair_create(self):
         """Test keypair create command.
 
         Test steps:
         1) Create keypair in setUp
-        2) Check RSA private key in output
+        2) Check Ed25519 private key in output
         3) Check for new keypair in keypairs list
         """
         NewName = data_utils.rand_name('TestKeyPairCreated')
         raw_output = self.openstack('keypair create ' + NewName)
         self.addCleanup(self.openstack, 'keypair delete ' + NewName)
-        self.assertInOutput('-----BEGIN RSA PRIVATE KEY-----', raw_output)
+        self.assertInOutput('-----BEGIN OPENSSH PRIVATE KEY-----', raw_output)
         self.assertRegex(raw_output, "[0-9A-Za-z+/]+[=]{0,3}\n")
-        self.assertInOutput('-----END RSA PRIVATE KEY-----', raw_output)
+        self.assertInOutput('-----END OPENSSH PRIVATE KEY-----', raw_output)
         self.assertIn(NewName, self.keypair_list())
 
     def test_keypair_delete_not_existing(self):

openstackclient/tests/unit/compute/v2/fakes.py

@@ -793,7 +793,7 @@ class FakeKeypair(object):
     """Fake one or more keypairs."""
 
     @staticmethod
-    def create_one_keypair(attrs=None, no_pri=False):
+    def create_one_keypair(attrs=None):
         """Create a fake keypair
 
         :param dict attrs:
@@ -811,8 +811,6 @@ def create_one_keypair(attrs=None, no_pri=False):
             'public_key': 'dummy',
             'user_id': 'user'
         }
-        if not no_pri:
-            keypair_info['private_key'] = 'private_key'
 
         # Overwrite default attributes.
         keypair_info.update(attrs)

openstackclient/tests/unit/compute/v2/test_keypair.py

@@ -54,10 +54,10 @@ def setUp(self):
 
 class TestKeypairCreate(TestKeypair):
 
-    keypair = compute_fakes.FakeKeypair.create_one_keypair()
-
     def setUp(self):
-        super(TestKeypairCreate, self).setUp()
+        super().setUp()
+
+        self.keypair = compute_fakes.FakeKeypair.create_one_keypair()
 
         self.columns = (
             'fingerprint',
@@ -77,8 +77,11 @@ def setUp(self):
 
         self.sdk_client.create_keypair.return_value = self.keypair
 
-    def test_key_pair_create_no_options(self):
-
+    @mock.patch.object(
+        keypair, '_generate_keypair',
+        return_value=keypair.Keypair('private', 'public'),
+    )
+    def test_key_pair_create_no_options(self, mock_generate):
         arglist = [
             self.keypair.name,
         ]
@@ -90,18 +93,14 @@ def test_key_pair_create_no_options(self):
         columns, data = self.cmd.take_action(parsed_args)
 
         self.sdk_client.create_keypair.assert_called_with(
-            name=self.keypair.name
+            name=self.keypair.name,
+            public_key=mock_generate.return_value.public_key,
         )
 
         self.assertEqual({}, columns)
         self.assertEqual({}, data)
 
     def test_keypair_create_public_key(self):
-        # overwrite the setup one because we want to omit private_key
-        self.keypair = compute_fakes.FakeKeypair.create_one_keypair(
-            no_pri=True)
-        self.sdk_client.create_keypair.return_value = self.keypair
-
         self.data = (
             self.keypair.fingerprint,
             self.keypair.name,
@@ -135,7 +134,11 @@ def test_keypair_create_public_key(self):
             self.assertEqual(self.columns, columns)
             self.assertEqual(self.data, data)
 
-    def test_keypair_create_private_key(self):
+    @mock.patch.object(
+        keypair, '_generate_keypair',
+        return_value=keypair.Keypair('private', 'public'),
+    )
+    def test_keypair_create_private_key(self, mock_generate):
         tmp_pk_file = '/tmp/kp-file-' + uuid.uuid4().hex
         arglist = [
             '--private-key', tmp_pk_file,
@@ -156,19 +159,20 @@ def test_keypair_create_private_key(self):
 
             self.sdk_client.create_keypair.assert_called_with(
                 name=self.keypair.name,
+                public_key=mock_generate.return_value.public_key,
             )
 
             mock_open.assert_called_once_with(tmp_pk_file, 'w+')
-            m_file.write.assert_called_once_with(self.keypair.private_key)
+            m_file.write.assert_called_once_with(
+                mock_generate.return_value.private_key,
+            )
 
             self.assertEqual(self.columns, columns)
             self.assertEqual(self.data, data)
 
     @mock.patch.object(sdk_utils, 'supports_microversion', return_value=True)
     def test_keypair_create_with_key_type(self, sm_mock):
         for key_type in ['x509', 'ssh']:
-            self.keypair = compute_fakes.FakeKeypair.create_one_keypair(
-                no_pri=True)
             self.sdk_client.create_keypair.return_value = self.keypair
 
             self.data = (
@@ -233,8 +237,12 @@ def test_keypair_create_with_key_type_pre_v22(self, sm_mock):
                 '--os-compute-api-version 2.2 or greater is required',
                 str(ex))
 
+    @mock.patch.object(
+        keypair, '_generate_keypair',
+        return_value=keypair.Keypair('private', 'public'),
+    )
     @mock.patch.object(sdk_utils, 'supports_microversion', return_value=True)
-    def test_key_pair_create_with_user(self, sm_mock):
+    def test_key_pair_create_with_user(self, sm_mock, mock_generate):
         arglist = [
             '--user', identity_fakes.user_name,
             self.keypair.name,
@@ -250,6 +258,7 @@ def test_key_pair_create_with_user(self, sm_mock):
         self.sdk_client.create_keypair.assert_called_with(
             name=self.keypair.name,
             user_id=identity_fakes.user_id,
+            public_key=mock_generate.return_value.public_key,
         )
 
         self.assertEqual({}, columns)
@@ -673,9 +682,6 @@ def test_keypair_show_no_options(self):
                           self.cmd, arglist, verifylist)
 
     def test_keypair_show(self):
-        # overwrite the setup one because we want to omit private_key
-        self.keypair = compute_fakes.FakeKeypair.create_one_keypair(
-            no_pri=True)
         self.sdk_client.find_keypair.return_value = self.keypair
 
         self.data = (
@@ -704,7 +710,6 @@ def test_keypair_show(self):
         self.assertEqual(self.data, data)
 
     def test_keypair_show_public(self):
-
         arglist = [
             '--public-key',
             self.keypair.name
@@ -723,10 +728,6 @@ def test_keypair_show_public(self):
 
     @mock.patch.object(sdk_utils, 'supports_microversion', return_value=True)
     def test_keypair_show_with_user(self, sm_mock):
-
-        # overwrite the setup one because we want to omit private_key
-        self.keypair = compute_fakes.FakeKeypair.create_one_keypair(
-            no_pri=True)
         self.sdk_client.find_keypair.return_value = self.keypair
 
         self.data = (

releasenotes/notes/keypair-create-client-side-generation-73d8dd36192f70c9.yaml

@@ -0,0 +1,11 @@
+---
+features:
+  - |
+    The ``openstack keypair create`` command will now generate keypairs on the
+    client side in ssh-ed25519 format. The Compute service no longer supports
+    server-side key generation starting with ``--os-compute-api-version 2.92``
+    while the use of ssh-ed25519 is necessary as support for ssh-rsa has been
+    disabled by default starting in OpenSSH 8.8, which prevents its use in
+    guests using this version of OpenSSH in the default configuration.
+    ssh-ed25519 support is widespread and is supported by OpenSSH 6.5 or later
+    and Dropbear 2020.79 or later.

requirements.txt

@@ -4,6 +4,7 @@
 
 pbr!=2.1.0,>=2.0.0 # Apache-2.0
 
+cryptography>=2.7 # BSD/Apache-2.0
 cliff>=3.5.0 # Apache-2.0
 iso8601>=0.1.11 # MIT
 openstacksdk>=0.103.0 # Apache-2.0