Merge "compute: Generate SSH keypairs ourselves"

Zuul · openstack-gerrit · commit 43f0e1090a20 · 2023-05-04T15:06:03.000Z
diff --git a/openstackclient/compute/v2/keypair.py b/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 ({}, {})
diff --git a/openstackclient/tests/functional/compute/v2/test_keypair.py b/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):
diff --git a/openstackclient/tests/unit/compute/v2/fakes.py b/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)
diff --git a/openstackclient/tests/unit/compute/v2/test_keypair.py b/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 = (
diff --git a/releasenotes/notes/keypair-create-client-side-generation-73d8dd36192f70c9.yaml b/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.
diff --git a/requirements.txt b/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
