search

ManageIQ / manageiq-providers-hawkular

ManageIQ plugin for the Hawkular provider
8 stars 33 forks source link

Add optional validate param to raw_connect #100

Closed tumido closed 6 years ago

tumido commented 6 years ago

When provider is added the credentials validation is newly done via raw_connect (class method) instead of verify_credentials (needed EMS object). Further reading on this change can be found here: https://github.com/ManageIQ/manageiq-ui-classic/pull/1580

Related to: https://github.com/ManageIQ/manageiq-ui-classic/pull/2577

Similar PRs for other providers:

This PR enables the new workflow on provider. With this PR both workflows are available.

tumido commented 6 years ago

cc @aljesusg, @abonas, @jntullo, please review

tumido commented 6 years ago

@miq-bot add_label gaprindashvili/yes bug

miq-bot commented 6 years ago

@tumido Cannot apply the following label because they are not recognized: gaprindashvili/yes bug

abonas commented 6 years ago

@agrare @chessbyte I can't add for some reason @jntullo as a reviewer. any idea why?

aljesusg commented 6 years ago

With Correct credentials should be correct I am getting Invalid Credentials should_be_correct

With incorrect credentials go OK ok_other_credential

With a Incorrect Host go OK ok_host_random

tumido commented 6 years ago

@aljesusg it's not working because the mapping in your https://github.com/ManageIQ/manageiq-ui-classic/pull/2577 is sending password instead of params[:default_password], when you fix it, it's gonna work.

aljesusg commented 6 years ago

@tumido in get_task_args the first line is user, password = params[:default_userid], MiqPassword.encrypt(params[:default_password]) So are we working without encryption password?

tumido commented 6 years ago

@aljesusg I know.. It seems so. Without encryption it works for me. But I'm not sure how it behaves when the ssl is enabled, I haven't checked that.

aljesusg commented 6 years ago

Ok I tested and work without encryption I am gonna test this with SSL and CA @tumido Maybe we should desyncript in our validate if no ssl because the actual solution maybe don't work with SSL let me a second to test this. Thanks

aljesusg commented 6 years ago

This fail with SSL Credential validation was not successful: wrong argument (String)! (Expected kind of OpenSSL::X509::Store) thisis running in local with curl maybe we are missing something in the step of create ssl_cert_store, I tested with encrypted password and is failing too, after the call I'll debug this.

credentials object

{:username=>"jdoe", :password=>"password"}

options object

{:tenant=>"hawkular", :verify_ssl=>1, :ssl_cert_store=>"-----BEGIN CERTIFICATE-----\r\nMIIC/TCCAeWgAwIBAgIEZi3DSDANBgkqhkiG9w0BAQsFADAcMRowGAYDVQQDExFo\r\nYXdrdWxhci1zZXJ2aWNlczAgFw0xNzExMDIwOTQxMzVaGA8yMTE3MTAwOTA5NDEz\r\nNVowHDEaMBgGA1UEAxMRaGF3a3VsYXItc2VydmljZXMwggEiMA0GCSqGSIb3DQEB\r\nAQUAA4IBDwAwggEKAoIBAQCYWbkxaDx6e9mMK/T87QSJ6NS5KtlPzx3RItmegeOs\r\n8Z9vxHMC6rDKr+qlX3Ed/OhIk36az8rSrOYHT3ONdVVOK/em0JnOq8+lFyL1sfDT\r\nnN63s3ON4p8JS1jUxVcIafTJu12h//xf/nVbW5q3pcE242AXL4q0LIDIGczm2cV5\r\nYjeofTLJl/ra+8+yAygUm6+kT1MTkzb8TtNiJJDxAUTDDBF+iuISV4R5adLGmCmz\r\njiDZPxiwHI4TLO0I02jIzpQ9Rql+7Sd7jnEzea9oFJT7dr3E+CbknvUO1bso+Jwt\r\ns7T9CeYzKxBswW83pPTI44iYmydtWwenzPqUivMIwio3AgMBAAGjRTBDMCIGA1Ud\r\nEQQbMBmHBH8AAAGCEWhhd2t1bGFyLXNlcnZpY2VzMB0GA1UdDgQWBBTfPluQkZHh\r\nCsKExlfsrrzpNhJpNzANBgkqhkiG9w0BAQsFAAOCAQEAM5MDnDcgz5XKc6FL/fBr\r\nQ4TIgNG9T7qFn2lLEkOkqh4kDAKfyFa5bBRTrDRglQoGQBfdAJD4nnS/mxBsLrBD\r\neW8bS9+1aGIyUnZK8fYIVR7wXSvb3U/+D0foqJJDEtyBftSsscrL1dYNEaOMrhAO\r\nECj7xl4638zdjp3klxlcIvJ9NINdPON4ltr796nhZIoHs8OTDW04M3sztzdpMkiH\r\ncBz1seRQlPAmWSK29y1fpXNrr0PmlYijgMYVv59o7zm4/Gvv2PcTzWh9ykFK/WnZ\r\nY1MooB0peBI4X3vWx/VAuX1ybUyPthx/gzbtZks6ztVjJOgs50eawUz3Q7S3BtWv\r\n5g==\r\n-----END CERTIFICATE-----"}

With SSL withpout validation is running ok so maybe is a problem working with the CA ssl_without_validation

tumido commented 6 years ago

@aljesusg ok, do you think this should be covered in this PR? Is it caused by me or just exposed?

josejulio commented 6 years ago

I think you should take a look at the PR[1] that did that work. The certificate is parsed to a OpenSSL::X509::Store by the method: ssl_cert_store

Edit, this [2] is could also be helpful.

I'm uncertain on how currently is handled, but these calls are needed to transform the text into an actual X509::Store.

[1] https://github.com/ManageIQ/manageiq/pull/13721/files#diff-5d3f1a5ab701ca4a756d3c921e36e9f6R22 [2] https://github.com/ManageIQ/manageiq-ui-classic/pull/450/files#diff-f402e411cd6dc158c724b14d72ead612R463

aljesusg commented 6 years ago

So why is getting this error @josejulio? I need to debug this.

josejulio commented 6 years ago

I suppose that call needs to be done when passing the arguments on manageiq-ui-classic.

aljesusg commented 6 years ago

But in MiddlewareManager the connect call to default_endpoint.ssl_cert_store that do this transform

josejulio commented 6 years ago

From the error mentioned before [1], that function isn't being called. Certainly debug is needed there, sorry for not having more information.

[1] https://github.com/ManageIQ/manageiq-providers-hawkular/pull/100#issuecomment-342205355

tumido commented 6 years ago

@israel-hdez I'll work on the tests, I'm not sure if it should block this to be merged though.

jshaughn commented 6 years ago

By applying locally this PR, and also https://github.com/ManageIQ/manageiq-ui-classic/pull/2577, I was finally able to create a non-ssl provider and so hopefully can now make some progress. It seems it's been a week being unable to create a provider, that is a long time. If a merge is not imminent I'd suggest maybe posting in dev-list how to work around this problem.

aljesusg commented 6 years ago

Now It's working again after https://github.com/ManageIQ/manageiq-ui-classic/pull/2643 with normal servers and SSL with CA. Tested !!! Do we need this PR?

tumido commented 6 years ago

@aljesusg if we want to be prepared for the new dialog, we need this PR. It's not breaking the "old way" it's just enabling the "new way". I vote for being prepared and have your and mine PRs merged.

aljesusg commented 6 years ago

Now its working with SSL and CA too @tumido

aljesusg commented 6 years ago

Tested with:

https://github.com/ManageIQ/manageiq-ui-classic/pull/2577 https://github.com/ManageIQ/manageiq-providers-hawkular/pull/100 https://github.com/ManageIQ/manageiq-providers-hawkular/pull/101

and revert

https://github.com/ManageIQ/manageiq-ui-classic/pull/2643

and it's working with hawkular server, ssl and CA

abonas commented 6 years ago

By applying locally this PR, and also ManageIQ/manageiq-ui-classic#2577, I was finally able to create a non-ssl provider and so hopefully can now make some progress. It seems it's been a week being unable to create a provider, that is a long time. If a merge is not imminent I'd suggest maybe posting in dev-list how to work around this problem.

@jshaughn there was a regression due to a broader change in core. core team fixed it yesterday and merged. so now it should be ok on master. in order to fix it to better fix a new dialog behavior for the future, there are several PRs in the works (incl this one).

miq-bot commented 6 years ago

Checked commits https://github.com/tumido/manageiq-providers-hawkular/compare/b2d24b8a6ec5113aed66087b9ffdfd2f1ecd9b82~...d1efd7b64701f19e86afa02f98dedb1a914b7f80 with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0 1 file checked, 1 offense detected

app/models/manageiq/providers/hawkular/middleware_manager.rb

tumido commented 6 years ago

@israel-hdez test coverage is available here https://github.com/ManageIQ/manageiq-providers-hawkular/pull/102

tumido commented 6 years ago

Closing, won't merge. Repo abandoned. :no_entry:

miq-bot commented 6 years ago

@tumido Cannot apply the following label because they are not recognized: gaprindashvili/yes bug