(ccc2021 hackathon ) kvm: add hosts using cloudstack ssh private key #5684

weizhouapache · 2021-11-12T10:19:29Z

Description

This PR provides the option to add kvm hosts with empty or wrong password.
To support this, the cloudstack ssh public key needs to be added in the ~/.ssh/authorized_keys on host.

Thanks @rhtyd for the idea !

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Screenshots (if appropriate):

get cloudstack public key from management server (/var/lib/cloudstack/management/.ssh/id_rsa.pub)

add the key to ~/.ssh/authorized_keys on kvm hosts

add kvm host with empty password

How Has This Been Tested?

add host, with correct password, works as expected.
put host to maitenance, stop cloudstack-agent, cancel maintenance, remove the host. all work.
add host, with empty password (ssh public key is added to ~/.ssh/authorized_keys), works as expected.
put host to maitenance, stop cloudstack-agent, cancel maintenance. all work as expected.

weizhouapache · 2021-11-12T10:21:25Z

@blueorangutan package

blueorangutan · 2021-11-12T10:22:03Z

@weizhouapache a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

yadvr · 2021-11-12T10:48:00Z

server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java

+            final String privateKey = _configDao.getValue("ssh.privatekey");
+            if (!SSHCmdHelper.acquireAuthorizedConnectionWithPublicKey(sshConnection, username, privateKey)) {
+                s_logger.error("Failed to authenticate with ssh key");
+                if (!sshConnection.authenticateWithPassword(username, password)) {


Nit - maybe do a check if password was even passed (empty?)

yadvr · 2021-11-12T10:50:44Z

Left some comments but nice hack @weizhouapache !

yadvr · 2021-11-12T10:51:35Z

(of course your changes are closer to what maybe reviewed/tested merged so I'm being very thorough otherwise LGTM!)

yadvr

LGTM

yadvr · 2021-11-12T10:48:51Z

ui/src/views/infra/HostAdd.vue

-        <div class="form__label"><span class="required">* </span>{{ $t('label.password') }}</div>
-        <span class="required required-label">{{ $t('label.required') }}</span>
+      <div class="form__item" v-if="selectedClusterHyperVisorType !== 'VMware'">
+        <div class="form__label">{{ $t('label.password') }}</div>


Maybe be have it not required just for kvm? (for ex. required for xenserver?)

ping @weizhouapache when you've time - why not put explicit checks for example a radio/button or toggle to switch between password or ssh-key auth just for KVM (however for XenServer too this feature can be done).

@rhtyd
good idea. will make the UI change.
let's support kvm at first, then xenserver (this is not needed for vmware).

yadvr · 2021-11-12T10:50:26Z

utils/src/main/java/com/cloud/utils/ssh/SSHCmdHelper.java

+        if (StringUtils.isNotBlank(privateKey)) {
+            File privateKeyFile = null;
+            try {
+                privateKeyFile = File.createTempFile("cloudstack-host-", null);


@weizhouapache
This temp. hack may not be required as mgmt server saves the ssh private/pub keys at: /var/lib/cloudstack/management/.ssh/id_rsa on the mgmt server, why not just read/use that?

In fact if this is saved/assumed, then we don't even need to read the private key from db and pass it here. Thoughts?

Note: needs checking on all distros (this path), on centos7, centos/rocky8, ubuntu and suse. cc @davidjumani if you know/remember distro specific paths.

@weizhouapache This temp. hack may not be required as mgmt server saves the ssh private/pub keys at: /var/lib/cloudstack/management/.ssh/id_rsa on the mgmt server, why not just read/use that?

In fact if this is saved/assumed, then we don't even need to read the private key from db and pass it here. Thoughts?

@rhtyd
I have considered it. but at the end I decided to use temp file so that the process will not rely on any file on management server.

In that case let's fix the mod bits of the file to be just read-only (0400)

blueorangutan · 2021-11-12T11:08:13Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1699

DaanHoogland

looks good, just one attack vector (of questionable vulnerability admitted). We might need to revisit that part. Also documentation on how to use it/prepare hosts will be needed.
Good Job @weizhouapache

utils/src/main/java/com/cloud/utils/ssh/SSHCmdHelper.java

yadvr · 2021-11-12T11:40:25Z

utils/src/main/java/com/cloud/utils/ssh/SSHCmdHelper.java

+                return false;
+            }
+            try {
+                if (!sshConnection.authenticateWithPublicKey(username, privateKeyFile, null)) {


@weizhouapache can we pass the privatekey content to this method (or similar available methods)?

@rhtyd yes, I will use char[], testing it.

@rhtyd @DaanHoogland
updated this PR to use char[] in server authentication. thanks for your review !

weizhouapache · 2021-11-12T11:56:51Z

@blueorangutan package

weizhouapache · 2021-11-12T13:11:57Z

@blueorangutan test keepEnv

blueorangutan · 2021-11-12T13:13:03Z

@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

DaanHoogland · 2021-11-23T14:34:03Z

@weizhouapache this is not in draft, is it ready for merge? (just making sure)

weizhouapache · 2021-11-23T15:48:40Z

@weizhouapache this is not in draft, is it ready for merge? (just making sure)

@DaanHoogland
it is ready for review (not merge).

weizhouapache · 2021-11-24T10:24:34Z

@DaanHoogland @rhtyd
made a UI change as per Rohit's comment.

sureshanaparti · 2021-11-24T11:10:36Z

server/src/main/java/com/cloud/hypervisor/kvm/discoverer/LibvirtServerDiscoverer.java

-                s_logger.debug("Failed to authenticate");
-                throw new DiscoveredWithErrorException("Authentication error");
+
+            final String privateKey = _configDao.getValue("ssh.privatekey");


@weizhouapache I think, it is better to try with ssh key or password (based on the authentication mode), not with other when one of it fails. You can determine authentication mode, either from the user input or may be from password/privatekey config.

@sureshanaparti
isn't it better to support both ? hosts are still manageable if one of the authentication fails.

if one authentication fails, try other may not be good.

if one authentication fails, try other may not be good.

@sureshanaparti ok. let's ask other guys.

@rhtyd @DaanHoogland what's your opinion ?

sureshanaparti · 2021-11-24T11:14:45Z

@weizhouapache Does multiple kvm hosts in a cluster supports different authentication mechanism (few with passwords, and others with ssh key)? If so, is this info maintained in management server?

yadvr · 2021-12-03T10:25:06Z

LGTM, @weizhouapache is this ready for testing ? cc @borisstoyanov @vladimirpetrov
@blueorangutan package

blueorangutan · 2021-12-03T10:26:04Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

weizhouapache · 2021-12-03T10:58:10Z

LGTM, @weizhouapache is this ready for testing ? cc @borisstoyanov @vladimirpetrov @blueorangutan package

@rhtyd yes, it is ready for review and testing.

blueorangutan · 2021-12-03T11:00:03Z

@weizhouapache a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

weizhouapache · 2021-12-03T11:06:21Z

@weizhouapache Does multiple kvm hosts in a cluster supports different authentication mechanism (few with passwords, and others with ssh key)? If so, is this info maintained in management server?

@sureshanaparti yes, if host_details table has record with name=password for the host, the host supports password authentication (as well as ssh key authentication).
if not, the host supports ssh key authentication only.

I think the main issue for now is, password authentication does not work sometimes, for example by ssh protocol, incorrect ssh settings, wrong password, password is changed. it should not be a problem/risk to ssh from mgmt server to hosts by both options, as they all use internal IPs.

blueorangutan · 2021-12-03T11:43:05Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1828

weizhouapache · 2021-12-03T11:46:02Z

@blueorangutan test

blueorangutan · 2021-12-03T11:48:03Z

@weizhouapache a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-12-03T11:52:51Z

Trillian Build Failed (tid-2615)

blueorangutan · 2021-12-03T11:57:00Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 1829

weizhouapache · 2021-12-03T11:59:29Z

@blueorangutan test

sureshanaparti · 2021-12-03T12:54:48Z

@blueorangutan test

blueorangutan · 2021-12-03T12:55:03Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-12-03T21:45:55Z

Trillian test result (tid-2616)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30475 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5684-t2616-kvm-centos7.zip
Smoke tests completed. 91 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

kvm: add hosts using cloustack ssh private key

e9a25ba

weizhouapache requested review from DaanHoogland and yadvr November 12, 2021 10:19

weizhouapache changed the title ~~kvm: add hosts using cloustack ssh private key~~ (ccc2021 hachathon ) kvm: add hosts using cloustack ssh private key Nov 12, 2021

yadvr added the hackathon label Nov 12, 2021

yadvr reviewed Nov 12, 2021

View reviewed changes

yadvr approved these changes Nov 12, 2021

View reviewed changes

yadvr changed the base branch from main to 4.16 November 12, 2021 10:51

DaanHoogland approved these changes Nov 12, 2021

View reviewed changes

utils/src/main/java/com/cloud/utils/ssh/SSHCmdHelper.java Outdated Show resolved Hide resolved

yadvr reviewed Nov 12, 2021

View reviewed changes

Update PR#5684: use char[] in authentication

a2ab99b

weizhouapache changed the title ~~(ccc2021 hachathon ) kvm: add hosts using cloustack ssh private key~~ (ccc2021 hackathon ) kvm: add hosts using cloustack ssh private key Nov 12, 2021

weizhouapache changed the title ~~(ccc2021 hackathon ) kvm: add hosts using cloustack ssh private key~~ (ccc2021 hackathon ) kvm: add hosts using cloudstack ssh private key Nov 12, 2021

yadvr added this to the 4.16.1.0 milestone Nov 15, 2021

Update PR#5684: add radio group on UI

045fa5a

weizhouapache force-pushed the 4.16-add-kvm-host-ssh-privatekey branch from 382d6a8 to 045fa5a Compare November 24, 2021 10:24

sureshanaparti reviewed Nov 24, 2021

View reviewed changes

yadvr approved these changes Dec 30, 2021

View reviewed changes

yadvr merged commit 8952cd5 into apache:4.16 Dec 30, 2021

This was referenced Jun 24, 2022

Documentation needs instructions for adding a host using an SSH key #6498

Closed

When creating a new zone, the host password field should not be required when an SSH key is being used #6499

Closed

weizhouapache deleted the 4.16-add-kvm-host-ssh-privatekey branch December 9, 2022 08:24

(ccc2021 hackathon ) kvm: add hosts using cloudstack ssh private key #5684

(ccc2021 hackathon ) kvm: add hosts using cloudstack ssh private key #5684

Uh oh!

Conversation

weizhouapache commented Nov 12, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

weizhouapache commented Nov 12, 2021

Uh oh!

blueorangutan commented Nov 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented Nov 12, 2021

Uh oh!

yadvr commented Nov 12, 2021

Uh oh!

yadvr 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!

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!

blueorangutan commented Nov 12, 2021

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

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!

weizhouapache commented Nov 12, 2021

Uh oh!

weizhouapache commented Nov 12, 2021

Uh oh!

blueorangutan commented Nov 12, 2021

Uh oh!

DaanHoogland commented Nov 23, 2021

Uh oh!

weizhouapache commented Nov 23, 2021

Uh oh!

weizhouapache commented Nov 24, 2021 • edited by sureshanaparti 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!

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!

sureshanaparti commented Nov 24, 2021

Uh oh!

yadvr commented Dec 3, 2021

Uh oh!

blueorangutan commented Dec 3, 2021

Uh oh!

weizhouapache commented Dec 3, 2021

Uh oh!

weizhouapache commented Nov 24, 2021 •

edited by sureshanaparti

Loading