SSH extension #1363

rlrossiter · 2020-03-06T17:08:33Z

Add an extension used for AAD-based SSH for Azure VMs.

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

General Guidelines

Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

My extension description/summary conforms to the Extension Summary Guidelines.

azuresdkci · 2020-03-06T17:08:55Z

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it microsoft/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/rlrossiter/azure-cli-extensions.git@ssh-extension#subdirectory=src/$EXT&egg=$EXT"

yonzhan · 2020-03-07T01:41:35Z

SSH for Azure VM

yungezz · 2020-03-07T01:50:53Z

hi @rlrossiter we'll look at the issue and keep offline syncing with you.

CLI arguments were changed from extra to argument with None defaults in custom. Changed resource_group to resource_group_name argument for auto-population from cli-core. Removed Python 2 and added new Python 3s to setup.

src/ssh/setup.py

src/ssh/azext_ssh/__init__.py

src/ssh/azext_ssh/azext_metadata.json

arrownj · 2020-04-23T02:24:02Z

src/ssh/azext_ssh/ssh_utils.py

+def _get_ssh_path():
+    ssh_path = "ssh"
+
+    if platform.system() == 'Windows':


Is it possible there is no ssh in other platform ?

arrownj · 2020-04-27T07:44:31Z

src/ssh/azext_ssh/tests/latest/test_custom.py

+    @mock.patch('azext_ssh.custom._do_ssh_op')
+    @mock.patch('azext_ssh.custom.ssh_utils')
+    @mock.patch('functools.partial')
+    def test_ssh_config(self, mock_partial, mock_ssh_utils, mock_do_op):


Hi @rlrossiter , which python version are you using to run this test ? It always failed from my side (python 3.8).

The version isn't the problem, the code changed and broke the unit tests. A lot of them are now failing for me too because of the code changes (Python 3.6).

Hi @rlrossiter, what other tests also failed for you ? As you can see from the CI log, all other tests are passed. Did you use the latest azure-cli dev branch and rerun azdev setup ? It should be only this test failed. And the reason has something to do with functions.partial. I'm not sure the root cause currently, will investigate deeper.

I guess I didn't have all of the commits you added to this branch. Will re-pull and take a look if I get a chance.

arrownj · 2020-04-27T07:45:34Z

src/ssh/setup.py

+]
+
+DEPENDENCIES = [
+    'paramiko==2.6.0',


Hi @rlrossiter, seems we do not use these dependencies in our code any more ? Can we remove them ?

arrownj · 2020-04-27T08:21:50Z

src/ssh/azext_ssh/custom.py

+    op_call(ssh_ip, username, cert_file, private_key_file)
+
+
+def _prepare_jwk_data(public_key_file):


These codes were previously in azure-cli-core. I move it here because we think it is ssh certificate specific. In azure-cli-core, we expose a function get_msal_token in _profile.Profile to make it simple and clean. @rlrossiter @danybeam, could you please help review it ?

I don't like the use of the _profile module from core outside of core because modules that start with _ usually denote that it's a private module that shouldn't be used outside of the repo it lives in. That's why I put this in azure-cli-core to start

Yes, I agree with you about the private module part. For this _profile case, as there are lots of examples in extensions to use _profile directly, so I think we can just follow it. From cli-core perspective, we have an internal discussion, and we think we'd better not include SSH specific logic in it.

arrownj · 2020-04-27T08:32:48Z

src/ssh/azext_ssh/custom.py

+    profile = Profile(cli_ctx=cmd.cli_ctx)
+    username, certificate = profile.get_msal_token(scopes, data)
+
+    cert_file = _write_cert_file(public_key_file, username, certificate)


Hi @rlrossiter , previously there is a fixed [email protected] in certificate head, which is weird to me. I replace it with username, could you please help check whether it is right ?

Previous code in azure-cli-core

class SSHCredentials(object): def __init__(self, username, cert): self.username = username self.certificate = "[email protected] " + cert

The original was correct as that was the type of certificate the file was (it's required by SSH). The username is encoded as part of the base64 bytes written to the file. The username was passed back alongside the credentials but not part of the certificate in order to know who to SSH as

Thanks for your explain. Yes, I decode the certificate and find [email protected] is in it. I have change it back. You can have a look.

Could you please share more about this? Is there any document which explain it? It's still weird to me that why it is a fixed name in the certificate. Thanks in advance.

…sions into ssh-extension

Ryan Rossiter added 7 commits January 21, 2020 08:29

Add SSH extension

d701968

Add unit tests and break things down a bit

3961659

Add ssh config command

6dd88e2

URL-encode modulus and exponent

c254d55

Make az ssh vm start interactive

56534d9

Finalize ssh commands

1c2908d

Updated help docs

20137a3

Reorder imports

53b764b

yonzhan requested a review from qwordy March 7, 2020 01:41

yonzhan assigned qwordy and yungezz Mar 7, 2020

yungezz requested review from arrownj and removed request for qwordy March 7, 2020 01:49

yungezz assigned arrownj and unassigned qwordy Mar 7, 2020

Ryan Rossiter added 2 commits March 31, 2020 10:06

Updated arguments

9987156

CLI arguments were changed from extra to argument with None defaults in custom. Changed resource_group to resource_group_name argument for auto-population from cli-core. Removed Python 2 and added new Python 3s to setup.

Fix lint error

423de41

arrownj reviewed Apr 23, 2020

View reviewed changes

arrownj added 4 commits April 24, 2020 13:27

move ssh certificate part from core to extension

444ba0d

follow CLI extension guide

9940587

update CODEOWNERS

fe1216a

Merge branch 'master' into ssh-extension

b2ead61

arrownj reviewed Apr 27, 2020

View reviewed changes

arrownj added 3 commits April 27, 2020 16:49

update CODEOWNERS

46c9303

roll back to [email protected] for certificate file

2138bb5

update --name to --vm-name

0c0284f

arrownj added 3 commits August 14, 2020 14:44

Merge branch 'master' into ssh-extension

a1c123c

fix sytle error

018f014

Merge branch 'ssh-extension' of github.com:rlrossiter/azure-cli-exten…

524dbf3

…sions into ssh-extension

arrownj approved these changes Aug 17, 2020

View reviewed changes

arrownj merged commit 92a2530 into Azure:master Aug 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SSH extension #1363

SSH extension #1363

rlrossiter commented Mar 6, 2020

azuresdkci commented Mar 6, 2020

yonzhan commented Mar 7, 2020

yungezz commented Mar 7, 2020

arrownj Apr 23, 2020

arrownj Apr 27, 2020

rlrossiter Apr 27, 2020

arrownj Apr 27, 2020

rlrossiter Apr 27, 2020

arrownj Apr 27, 2020

arrownj Apr 27, 2020

rlrossiter Apr 27, 2020

arrownj Apr 27, 2020

arrownj Apr 27, 2020

rlrossiter Apr 27, 2020

arrownj Apr 27, 2020

		op_call(ssh_ip, username, cert_file, private_key_file)


		def _prepare_jwk_data(public_key_file):

SSH extension #1363

SSH extension #1363

Conversation

rlrossiter commented Mar 6, 2020

General Guidelines

azuresdkci commented Mar 6, 2020

yonzhan commented Mar 7, 2020

yungezz commented Mar 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment