Host validation #22336

kbrock · 2023-02-02T20:52:03Z

part of of ManageIQ/manageiq-ui-classic#8608

depends upon:

make default credentials verify ws manageiq-providers-vmware#859

dependent of:

In the ui (read: api), we need to test new credentials.
These are not the credentials stored in the host.

So we need to pass these new credentials over the wire to the
ems_operations worker to test. validate_credentials? pulls those credentials out and applies to the host (without save)

Updated host to call a default authentication method so different providers like vmware and openstack can choose different defaults for verify credentials

/cc @MelsHyrule

agrare · 2023-02-10T19:03:10Z

app/models/host.rb

    # Prevent the connection details, including the password, from being leaked into the logs
    # and MiqQueue by only returning true/false
-    !!verify_credentials(*args)
+    auth = options.delete('credentials') || options.delete(:credentials)


Do we have to handle either or could we enforce one-or-the-other?

agrare · 2023-02-10T19:05:44Z

app/models/host.rb

+  # some providers (i.e.: openstack) want to use something other than ws as a default
+  def verify_credentials_default(auth_type, _options)
+    verify_credentials_with_ws(auth_type)
+  end
+


Would it be fair to say the inverse, that vmware wants to use ws as the default?

Also am I reading this wrong or does openstack "default" to ws as well?

case auth_type.to_s when 'remote', 'default', 'ssh_keypair' then verify_credentials_with_ssh(auth_type, options) when 'ws' then verify_credentials_with_ws(auth_type) when 'ipmi' then verify_credentials_with_ipmi(auth_type) else verify_credentials_with_ws(auth_type) end

no. openstack does not implement ws.
so while I agree with you that they fall through to ws, that means they will throw an error.

They modified this method so default and ssh_keypair to go to ssh. If they had not added those cases, those would have fallen through to the else (the default). Swapping that to be ssh seems more in spirit with what we want and is less code change.

So I interpret this as they default to ssh, but they just left the else code because they didn't understand the implications.

In the end, I am extracting credentials outside of this method. Too many methods override this. I no longer have to make this change, but it is still weighing on my mind.

I do wonder what the default should be for the base class (and openstack).
Still think HostEsx should default to ws, but think the others should be ssh

you read this correctly.

vmware EsxHost is the only one that wants ws as the default
but we have it setup that ws is the default for all

Most of the providers have overwritten verify_credentials so I'm unsure if this method helps us that much.

@agrare I just understood your question. I like the idea of using ssh as the default and changing the default for VMware EsxHost - probably the only one that actually implements ws

👍 yes if VMware is the only different one seems better to flip the default

kbrock · 2023-02-13T23:01:51Z

update:

rebased master
just using :credentials rather than both string and symbol

verification is transforming to using task Changing the tests to make it more obvious when the values are getting set

the logic was interpreting (:save => false) as the authtype

kbrock · 2023-02-28T15:12:56Z

app/models/host.rb

@@ -639,10 +639,13 @@ def verify_credentials_task(userid, auth_type = nil, options = {})
    MiqTask.generic_action_with_callback(task_opts, queue_opts)
  end

-  def verify_credentials?(*args)
+  def verify_credentials?(auth_type = nil, options = {})
    # Prevent the connection details, including the password, from being leaked into the logs
    # and MiqQueue by only returning true/false


Hmm, this comment is no longer true.

We are now putting the new password into the queue. It is the only way I could think of passing the new password to the ems_operations worker (who is actually verifying the password)

@agrare are you ok with putting the prospective credentials onto the queue?

Yes as long as it is filtered out in the logs as a ManageIQ::Password object (this is the same for the EMS verify_credentials method)

I just remembered why we have this comment (was reviewing the VerifyCredentialsMixin that ExtManagementSystem uses). It was so that the results weren't leaked into the logs, not the parameters.

In the ui we need to test new credentials. These are not the credentials stored in the host. So we need to pass these new credentials over the wire to the ems_operations worker to test introduced a default case. vmware will override with ws, but others prefer ssh

kbrock · 2023-03-01T02:01:03Z

update:

default for verify_credentials is now ssh
- updated vmware and openstack to reflect this change (prs linked above)

miq-bot · 2023-03-01T02:02:38Z

Checked commits kbrock/manageiq@ee4c1b9~...d039070 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 🏆

agrare · 2023-03-01T14:22:56Z

@miq-bot cross-repo-tests ManageIQ/manageiq-api#1202, ManageIQ/manageiq-providers-openstack#839, ManageIQ/manageiq-providers-vmware#859

From Pull Request: ManageIQ/manageiq#22336

agrare · 2023-03-01T14:27:20Z

app/models/host.rb

+    auth = options.delete(:credentials)
+    update_authentication(auth, :save => false) if auth.present?


Hm this doesn't looks right, if we're verifying new credentials for an existing host and the verification fails then we've just overwritten the old credentials right?

Why did you have to add this? Or was this already done elsewhere that you had to move?

This is the way it has always been done (not a fan)

We set the credentials, we pass in :save => false so they are not changed
Then we call verify on the host.
It uses the temporary values.

We discussed this and this is required because previously the UI didn't verify host credentials over the queue, it called it directly after doing this update_authentications save => false

This is because the Host#verify_credentials* methods only work off of the authentications association not on new credentials passed in as arguments.

I'm 👍 with this for now so that we can get this running properly over the queue and then we can come back and improve the verify_credentials&friends methods to work more closely to how we verify ems creds.

agrare · 2023-03-01T15:02:27Z

@kbrock in a follow-up could we use the VerifyCredentialsMixin module here and delete the verify_credentials_task and verify_credentials? methods?

agrare · 2023-03-01T15:22:36Z

Cross-repo is only failing on the unrelated api authentication specs

kbrock · 2023-03-02T00:40:58Z

@kbrock in a follow-up could we use the VerifyCredentialsMixin module here and delete the verify_credentials_task and verify_credentials? methods?

The methods are just a tad different.
I started with fixing the ems and then adding the host, thinking they were both the same. For the ems case, it is a class method, and for host it is an instance method.

Also the internals seem a little different, but I will revisit.

kbrock · 2023-03-02T00:42:52Z

kicking now that vmware change is in.

kbrock requested review from agrare and Fryguy as code owners February 2, 2023 20:52

kbrock force-pushed the host_validation branch 2 times, most recently from 64acfc7 to fda6839 Compare February 7, 2023 02:49

kbrock mentioned this pull request Feb 7, 2023

Host Edit Form React Conversion ManageIQ/manageiq-ui-classic#8608

Merged

9 tasks

agrare reviewed Feb 10, 2023

View reviewed changes

kbrock force-pushed the host_validation branch from fda6839 to 865998f Compare February 13, 2023 23:00

kbrock mentioned this pull request Feb 13, 2023

Host verify_credentials ManageIQ/manageiq-api#1202

Merged

kbrock added enhancement core labels Feb 14, 2023

Fryguy assigned agrare Feb 15, 2023

kbrock added 2 commits February 27, 2023 21:32

for host verification specs, make passwords explicit

ee4c1b9

verification is transforming to using task Changing the tests to make it more obvious when the values are getting set

pass the correct parameters into authentication_check

a74eb0c

the logic was interpreting (:save => false) as the authtype

kbrock commented Feb 28, 2023

View reviewed changes

kbrock mentioned this pull request Mar 1, 2023

make default credentials verify ws ManageIQ/manageiq-providers-vmware#859

Merged

kbrock mentioned this pull request Mar 1, 2023

use default verify_credentials implementation ManageIQ/manageiq-providers-openstack#839

Merged

1 task

kbrock force-pushed the host_validation branch from 865998f to d039070 Compare March 1, 2023 02:00

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Mar 1, 2023

Running tests for agrare

e7a9c19

From Pull Request: ManageIQ/manageiq#22336

miq-bot mentioned this pull request Mar 1, 2023

[BOT] Cross repo test for ManageIQ/manageiq#22336 ManageIQ/manageiq-cross_repo-tests#756

Closed

agrare reviewed Mar 1, 2023

View reviewed changes

kbrock closed this Mar 2, 2023

kbrock reopened this Mar 2, 2023

agrare approved these changes Mar 2, 2023

View reviewed changes

agrare merged commit 7a2f4c0 into ManageIQ:master Mar 2, 2023

kbrock deleted the host_validation branch March 2, 2023 17:41

kbrock mentioned this pull request Mar 7, 2023

Use default host_verify_credentials ManageIQ/manageiq-providers-ovirt#634

Merged

kbrock mentioned this pull request Jan 18, 2024

Fix Host default_authentication_type ManageIQ/manageiq-providers-vmware#895

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Host validation #22336

Host validation #22336

kbrock commented Feb 2, 2023 •

edited

Loading

agrare Feb 10, 2023

agrare Feb 10, 2023

kbrock Feb 11, 2023 •

edited

Loading

kbrock Feb 11, 2023

kbrock Feb 17, 2023

kbrock Feb 28, 2023 •

edited

Loading

agrare Feb 28, 2023

kbrock commented Feb 13, 2023

kbrock Feb 28, 2023

agrare Feb 28, 2023

agrare Mar 1, 2023

kbrock commented Mar 1, 2023

miq-bot commented Mar 1, 2023

agrare commented Mar 1, 2023

agrare Mar 1, 2023

kbrock Mar 2, 2023

agrare Mar 2, 2023

agrare commented Mar 1, 2023

agrare commented Mar 1, 2023

kbrock commented Mar 2, 2023

kbrock commented Mar 2, 2023

		auth = options.delete(:credentials)
		update_authentication(auth, :save => false) if auth.present?

Host validation #22336

Host validation #22336

Conversation

kbrock commented Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Feb 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Feb 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Feb 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock commented Mar 1, 2023

miq-bot commented Mar 1, 2023

agrare commented Mar 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrare commented Mar 1, 2023

agrare commented Mar 1, 2023

kbrock commented Mar 2, 2023

kbrock commented Mar 2, 2023

kbrock commented Feb 2, 2023 •

edited

Loading

kbrock Feb 11, 2023 •

edited

Loading

kbrock Feb 28, 2023 •

edited

Loading