Addressing -ssh-key-gen bug. by adewaleo · Pull Request #7105 · Azure/azure-cli

adewaleo · 2018-08-21T02:15:56Z

Fixes #4725 and #6780 by using id_rsa as the private key file (if it exists) to generate a new public key instead of generating a new private-public key pair (which overwrites id-rsa).

I tested this fix by deleting my public key and running az vm create ... --ssh-key-gen.

A new public key was generated in id_rsa.pub and I was able to ssh into the newly created VM. I was able to create another vm using the existing keys pair and ssh into it.

Please let me know your thoughts on this PR.

This checklist is used to make sure that common guidelines for a pull request are followed.

The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.

williexu

Some comments

williexu · 2018-08-21T16:50:50Z

src/command_modules/azure-cli-vm/azure/cli/command_modules/vm/_validators.py

    content = string_or_file
    if os.path.exists(string_or_file):
-        logger.info('Use existing SSH public key file: %s', string_or_file)
+        logger.info('Reusing existing SSH public key from %s', string_or_file)


I would leave the original logging msg, at this point, the CLI only knows the file exists and the key has not been verified yet.

Okay, I will leave the original logging message.

src/azure-cli-core/azure/cli/core/keys.py

tjprescott

LGTM

williexu

Some comments, also check CI

williexu · 2018-08-24T17:04:43Z

src/azure-cli-core/azure/cli/core/keys.py

You only want to catch a FileNotFoundError right?
It may be better to do the existence check like you do with the public key.

Okay, I will change this to check the path instead

src/azure-cli-core/azure/cli/core/tests/test_keys.py

adewaleo · 2018-08-24T18:27:53Z

I made the requested changes, can someone please look at the version numbers and make sure they look ok

troydai · 2018-08-24T21:31:09Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

Maybe use the absolute namespace.

troydai · 2018-08-24T21:31:47Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

Use str.format()

troydai · 2018-08-24T21:32:40Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

Put this in the tearDown otherwise, it is not executed when the test fails.

troydai · 2018-08-24T21:33:33Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

str.format

src/azure-cli-core/azure/cli/core/keys.py

adewaleo · 2018-08-28T16:56:07Z

So is it okay to merge this?

troydai · 2018-08-28T17:44:09Z

Please address my comments.

williexu · 2018-09-04T19:57:41Z

@adewaleo you might need to rebase your branch, check the changes for the PR

…" is run and id_rsa exists but id_rsa.pub does not exist.

williexu · 2018-09-04T21:46:39Z

src/azure-cli-core/azure/cli/core/keys.py

+                           public_key_filepath, pub_ssh_dir)
+
+            return public_key
+    except IOError:


FileNotFoundError, IOError spans too many scenarios

Unfortunately FileNotFoundError is not defined in Python 2. IOError handles the case that the file isn't found and that the file is unreadable for some other reason.

I wonder if I should check if the file exists and handle the IOError separately?

You want different behaviors for a non-existing public file vs other unreadable file errors(no permission, password-protected, etc) correct?

In that case, you need to differentiate between the two; if FileNotFound is not available in Python2, the best option seems to simply check for existence.

I think I have addressed your comments, thanks for the feedback.

troydai · 2018-09-04T21:43:14Z

src/azure-cli-core/azure/cli/core/keys.py

+    try:
+        with open(public_key_filepath, 'r') as public_key_file:
+            public_key = public_key_file.read()
+            pub_ssh_dir, _ = os.path.split(public_key_filepath)


Since the basename is not used here just use os.path.dirname

https://docs.python.org/3/library/os.path.html?highlight=os%20path%20dirname#os.path.dirname

troydai · 2018-09-04T21:44:45Z

src/azure-cli-core/azure/cli/core/keys.py

+        key = paramiko.RSAKey(filename=private_key_filepath)
+        logger.warning("Private SSH key file '%s' found in dir '%s'."
+                       " Generating public key file '%s'",
+                       private_key_filepath, ssh_dir, public_key_filepath)


What's the fallback here when it fails to load the private key?

troydai · 2018-09-04T21:46:20Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

+        cls.key.write_private_key(keyOutput)
+
+        cls.private_key = keyOutput.getvalue()
+        cls.public_key = '{} {}'.format(cls.key.get_name(), cls.key.get_base64())


Why not just leave it in the setUp?

troydai · 2018-09-04T21:47:51Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

+            if os.path.isfile(fp):
+                os.remove(fp)
+            else:
+                shutil.rmtree(fp)


Way too complicated. One setUp tearDown pair and execute the same logic for each test method. There is no need to share the construction between test methods.

troydai · 2018-09-04T21:50:33Z

...odules/azure-cli-vm/azure/cli/command_modules/vm/tests/profile_2017_03_09/test_vm_actions.py

        self.assertTrue(os.path.isfile(public_key_file4))

+        # delete temporary directory and its files
+        shutil.rmtree(temp_dir_name)


Throw it in self.AddCleanup - https://docs.python.org/3/library/unittest.html?highlight=unittest%20addcleanup#unittest.TestCase.addCleanup

I think I have addressed your comments, thanks for the feedback.

yugangw-msft

LGTM overall. Thanks for the change! I added a few comments, hope they help you remove some code.

yugangw-msft · 2018-09-05T17:21:27Z

src/azure-cli-core/azure/cli/core/keys.py

    import paramiko
+    from paramiko.ssh_exception import PasswordRequiredException, SSHException
+
+    if os.path.isfile(public_key_filepath):


We probably don't need this as the caller is supposed to check the public key file not existing and then invoke this. Check out the code here

I hear you Yugang, but I was asked by @williexu to update this method independent of the current caller's behavior. I imagine this is to make sure the method doesn't fail if a future caller of this method fails to make the check.

I am fine with this idea, but then you also need to remove the redundant file check in VM and ACS modules. I thought we can do step by step through the next PR as this piece is pretty important so risk is a concern, but up to you.

yugangw-msft · 2018-09-05T17:26:30Z

src/azure-cli-core/HISTORY.rst

 ++++++
 * Minor fixes.
+* Fixed issue where `az vm create --generate-ssh-keys` overwrites private key
+  file if public key file is missing. (#4725, #6790)


should be #6780

Nice catch! Will update!

My bad, thanks

yugangw-msft · 2018-09-05T17:29:52Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

+    def _create_new_temp_key_file(self, key_data, suffix=""):
+        with tempfile.NamedTemporaryFile(mode='w', dir=self._tempdirName, delete=False, suffix=suffix) as f:
+            f.write(key_data)
+            file_path = f.name


you can just do return f.name

Thanks, will do.

yugangw-msft · 2018-09-05T17:37:30Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

+        # delete temporary directory to be used for temp files.
+        shutil.rmtree(self._tempdirName)
+
+    def test_public_key_file_exists(self):


suggest name like test_when_public_key_file_exists? also this test can removed though as generate_ssh_keys should only be invoked when public key file doesn't exist

I will update the test names.

I agree that the user should not call generate_ssh_keys even though they have a valid public key file. But I think it would still be good to handle and test this situation just in case this method is called by a future caller that fails to check this case. Again, I believe I was asked to handle all the different cases of public / private key being present or not present. ,

yugangw-msft · 2018-09-05T17:39:46Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

+            new_private_key = f.read()
+            self.assertEqual(self.private_key, new_private_key)
+
+    def test_public_key_file_exists_no_permissions(self):


again, we don't need this test

If you would like me to remove this test, I can. Although @troydai asked me to test different cornercases that come to mind.

yugangw-msft · 2018-09-05T17:40:59Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

+        with self.assertRaises(CLIError):
+            generate_ssh_keys("", public_key_path)
+
+    def test_private_key_file_exists_no_permissions(self):


suggest test_error_raised_when_private_key_file_exists_no_permissions

yugangw-msft · 2018-09-05T17:41:17Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

+            public_key_path = private_key_path + ".pub"
+            generate_ssh_keys(private_key_path, public_key_path)
+
+    def test_private_key_file_exists_encrypted(self):


same: test_error_raised_when_private_key_file_exists_no_permissions

will update, thanks

yugangw-msft · 2018-09-05T17:41:55Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

+            public_key_path = private_key_path + ".pub"
+            generate_ssh_keys(private_key_path, public_key_path)
+
+    def test_private_key_file_exists(self):


suggest: test_generate_public_key_from_existing_private_key_file

yugangw-msft · 2018-09-05T17:43:35Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

+            private_key = f.read()
+            self.assertEqual(self.private_key, private_key)
+
+    def test_private_key_file_new(self):


suggest: `test_generate_new_private_key_files

yugangw-msft · 2018-09-05T17:47:41Z

src/azure-cli-core/azure/cli/core/tests/test_keys.py

+        # Check that public key returned is same as public key in public key path
+        with open(public_key_path, 'r') as f:
+            public_key = f.read()
+            self.assertEqual(public_key, new_public_key)


Just a question: so you are sure that the keys generated at different time in the same machine would be the same?

So, this method asserts that the new_public_key returned by generate_ssh_keys is the same as the one that the method wrote to the public key file it created under public_key_path.

Thanks for all the feedback, btw.

adewaleo · 2018-09-05T23:54:01Z

So I undid my last commit. Someone should let me know if it looks good so I can merge.

williexu

Looks good for the most part, just some small requested changes.

williexu · 2018-09-06T18:18:22Z

src/azure-cli-core/HISTORY.rst

@@ -6,6 +6,8 @@ Release History
 2.0.46
 ++++++
 * Minor fixes.


Replace the Minor fixes entry

williexu · 2018-09-06T18:19:26Z

src/azure-cli-core/azure/cli/core/keys.py

-    os.chmod(private_key_filepath, 0o600)
+    if os.path.isfile(private_key_filepath):
+        # try to use existing private key if it exists.
+


remove this empty line

williexu · 2018-09-06T18:19:44Z

src/azure-cli-core/azure/cli/core/keys.py

+            logger.warning("Private SSH key file '%s' found in dir '%s'. "
+                           "Generating new Public key file '%s'",
+                           private_key_filepath, ssh_dir, public_key_filepath)
+


remove this empty line

Okay, I just addressed these comments.

williexu · 2018-09-06T21:05:09Z

Merge when ci is green

adewaleo requested review from tjprescott, williexu and yugangw-msft August 21, 2018 02:15

williexu suggested changes Aug 21, 2018

View reviewed changes

adewaleo force-pushed the ova-GenSSHKeysVMUnsafe branch from 8a4968f to ebc2b43 Compare August 23, 2018 18:36

tjprescott approved these changes Aug 24, 2018

View reviewed changes

williexu suggested changes Aug 24, 2018

View reviewed changes

troydai suggested changes Aug 24, 2018

View reviewed changes

src/azure-cli-core/azure/cli/core/keys.py Outdated Show resolved Hide resolved

Oluwatosin Adewale added 2 commits September 4, 2018 14:00

Logic to handle situation where "az vm create ... --generate-ssh-keys…

9d43aa3

…" is run and id_rsa exists but id_rsa.pub does not exist.

SSH-key-gen fixes.

7c67fdb

adewaleo force-pushed the ova-GenSSHKeysVMUnsafe branch from 38b88b6 to 7c67fdb Compare September 4, 2018 21:38

Fixed Pep8 issue.

285a90a

williexu suggested changes Sep 4, 2018

View reviewed changes

troydai suggested changes Sep 4, 2018

View reviewed changes

troydai added this to the Sprint 45 - Ignite milestone Sep 4, 2018

Addressed PR comments. Added new test methods to TestGenerateSSHKeys.

71f15f3

yugangw-msft reviewed Sep 5, 2018

View reviewed changes

troydai approved these changes Sep 5, 2018

View reviewed changes

Addressed more comments.

4983dc5

adewaleo force-pushed the ova-GenSSHKeysVMUnsafe branch from 30209f3 to 4983dc5 Compare September 5, 2018 23:49

williexu suggested changes Sep 6, 2018

View reviewed changes

Addressed some more comments.

0cb24eb

williexu approved these changes Sep 6, 2018

View reviewed changes

williexu merged commit 1fe7cde into Azure:dev Sep 6, 2018

This was referenced Sep 6, 2018

vm create --generate-ssh-keys is unsafe #6780

Closed

Remove redundant SSH public file checks in callers of core/keys.py's generate_ssh_keys #7267

Closed

adewaleo mentioned this pull request Nov 27, 2018

Cli nukes existing SSH id_rsa key when creating AKS cluster #7926

Closed

Conversation

adewaleo commented Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

williexu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tjprescott left a comment

Choose a reason for hiding this comment

Uh oh!

williexu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adewaleo commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adewaleo commented Aug 28, 2018

Uh oh!

troydai commented Aug 28, 2018

Uh oh!

williexu commented Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adewaleo Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adewaleo Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yugangw-msft Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adewaleo commented Aug 21, 2018 •

edited

Loading

williexu commented Sep 4, 2018 •

edited

Loading

adewaleo Sep 4, 2018 •

edited

Loading

adewaleo Sep 5, 2018 •

edited

Loading

yugangw-msft Sep 5, 2018 •

edited

Loading