TLS verification & custom CA UI for oVirt and Container providers #450

cben · 2017-02-22T14:14:50Z

Combined PR with @jhernand — UI side for oVirt's ManageIQ/manageiq#14004 and containers' ManageIQ/manageiq#14019.
Depends on the core PR (at least the containers one), should be merged soon after.

[Travis can't pass until https://github.com/ManageIQ/manageiq/pull/14019 lands, passes locally with it.]

These build on ManageIQ/manageiq#13567 [merged] for storing & parsing the certificate_authority #field.

oVirt

(core part: ManageIQ/manageiq#14004)

Currently the oVirt provider doesn't check the validity of the TLS certificates presented by the oVirt server. This patch adds to the form used to add and modify the oVirt provider connection details two new controls. The first is a checkbox to enable or disable checking the TLS certificates of the oVirt server:

Verify TLS Certificates [Yes | No]

The second will only be enabled when the value of the first is 'Yes', and it is is a text box where the user can optionally paste a set of trusted CA certificates, in PEM format:

Trusted CA Certificates ┌────────────────────────────────────────┐
                        │-----BEGIN CERTIFICATE-----             │
                        │MIIDxjCCAq6gAwIBAgICEAAwDQYJKoZIhvc     │
                        │NAQELBQAwSTELMAkGA1UEBhMCVVMxMCVVMx     │
                        │...                                     │
                        │-----END CERTIFICATE-----               │
                        └────────────────────────────────────────┘
                        Paste here the trusted CA certificates, in
                        PEM format.

The value of the first control will be stored in the 'verify_ssl' column of the 'endpoints' table.
The value of the second control will be stored in the certificate_authority column of the endpoints table.

See screenshots below

Containers

(core part: ManageIQ/manageiq#14019)

Container provider: add Security Protocol, Trusted CA Certificates fields

Both on default & hawkular endpoints, added dropdown:

SSL
SSL trusting custom CA, which additionally shows CA field.
SSL without validation

This UI always sets (security_protocol, certificate_authority, verify_ssl) consisently.
Container backend will use security_protocol if set, ignoring verify_ssl.
See core PR for backward-compat details.

Hawkular endpoint has same dropdown and hiding/appearing CA field:
(independent control is useful because default openshift installs have bad SSL config, trying to address in openshift/openshift-ansible#3226 but no immediate relief)

@miq-bot add-label compute/containers, security, enhancement, pending core

@h-kataria @AparnaKarve @yaacov please review. cc @jhernand @simon3z

miq-bot · 2017-02-22T14:15:10Z

@cben Cannot apply the following label because they are not recognized: security

cben · 2017-02-22T16:15:04Z

Remaining lint warnings hard to address without breaking consistency.
I'll do a separate later PR to address postfix modifier ifs in ems_common_angular.rb, they bug me too.

jhernand

The changes look good to me. In addition I verified that they work correctly with oVirt. Thanks!

simon3z · 2017-02-23T09:38:20Z

app/controllers/ems_common.rb

+  def retrieve_container_security_protocols
+    [[_('SSL'), 'ssl-with-validation'],
+     [_('SSL trusting custom CA'), 'ssl-with-validation-custom-ca'],
+     [_('SSL without validation'), 'ssl-without-validation']]


@cben @jhernand these seems a little verbose to me. They could be: 'ssl', 'ssl-custom-ca' and 'ssl-insecure'.
I think it's not required to specify with-validation in general because SSL in fact has validation by default. Is the 'ssl' value clashing with a pre-existing one that implied no validation? Are these values the ones ending up in the db?
Anyway this is just a suggestion, the rest LGTM.

Yes, but. Alas, Openstack uses the values

[[_('SSL without validation'), 'ssl'], [_('SSL'), 'ssl-with-validation'], [_('Non-SSL'), 'non-ssl']]

where just ssl means insecure :-(
And AFAICT some other providers use ssl to mean secure.
It'd be good to fix, but would require migration(s), I don't want to block on that, and I don't want to add to the confusion now.
That only left the option of using 2 verbose but explicit values.

(If we do migrate openstack later, it'll be safer to never redefine ssl but switch to ssl-{with,without}-validation too.)

simon3z · 2017-02-23T09:38:46Z

LGTM 👍

@miq-bot assign dclarizio

jhernand · 2017-02-23T09:55:00Z

This is how the user interface looks when adding an oVirt provider, checking TLS certificates, and not providing the required trusted CA certificates:

This is adding the same provider, but disabling checking TLS certificates:

And this is adding the same provider, enabling checking TLS certificates, and providing the required trusted CA certificates:

jhernand · 2017-02-23T15:31:45Z

SSL host verification - finilizing

cben · 2017-02-27T12:06:16Z

@dclarizio @AparnaKarve Please review.
This is soft-blocking core PRs ManageIQ/manageiq#14004 ManageIQ/manageiq#14019. (The core must be merged first, but then users with insecure providers will be unable to Edit them to work again until this lands.)

dclarizio · 2017-02-27T15:43:31Z

@cben I added @AparnaKarve to review . . . can you look at the spec failures? Thx, Dan

cben · 2017-02-27T19:16:37Z

Specs are failing because the core PR wasn't merged. They pass locally with given both PRs.

AparnaKarve

@cben Overall, the changes look good.
Had one question though.

Before this PR, we did not have the concept of security protocol for Containers.
So when we open an existing Container Provider in the Edit mode, the dropdown for Security Protocol says "Nothing Selected" (and rightly so).
Should we default the Security Protocol for these containers to ssl-without-validation if the @ems.default_endpoint.security_protocol is nil ?

cben · 2017-02-28T14:21:57Z

The default should be ssl-with-validation. That's what the core PR defaults to, potentially breaking the provider, until you edit it.
The dropdown in main endpoint defaults to "Nothing Selected", while for Hawkular endpoint it shows <Choose>. ~~I can't understand why.~~ Found, fixing...
It seems the <Choose> also forces the Hawkular endpoint to be invalid from start, until filled & validated, which is a problem if user doesn't need Hawkular. @AparnaKarve Any tips how to debug? When is [Save] button enabled?

AparnaKarve · 2017-03-01T00:53:32Z

@cben Try this --

-                       :default_security_protocol  => default_security_protocol,
+                       :default_security_protocol  => default_endpoint.security_protocol ? default_endpoint.security_protocol : 'ssl-with-validation',

-        hawkular_security_protocol = @ems.connection_configurations.hawkular.endpoint.security_protocol
+        hawkular_security_protocol = @ems.connection_configurations.hawkular.endpoint.security_protocol ?
+          @ems.connection_configurations.hawkular.endpoint.security_protocol : 'ssl-with-validation'

This should populate the dropdowns with the SSL value for default and hawkular

Currently the oVirt provider doesn't check the validity of the TLS certificates presented by the oVirt server. This patch adds to the form used to add and modify the oVirt provider connection details two new controls. The first is a checkbox to enable or disable checking the TLS certificates of the oVirt server: Verify TLS Certificates [Yes | No] The second will only be enabled when the value of the first is 'Yes', and it is is a text box where the user can optionally paste a set of trusted CA certificates, in PEM format: Trusted CA Certificates ┌────────────────────────────────────────┐ │-----BEGIN CERTIFICATE----- │ │MIIDxjCCAq6gAwIBAgICEAAwDQYJKoZIhvc │ │NAQELBQAwSTELMAkGA1UEBhMCVVMxMCVVMx │ │... │ │-----END CERTIFICATE----- │ └────────────────────────────────────────┘ Paste here the trusted CA certificates, in PEM format. The value of the first control will be stored in the 'verify_ssl' column of the 'endpoints' table. The value of the second control will be stored in the 'certificate_authority' column of the 'endpoints' table. Signed-off-by: Juan Hernandez <[email protected]>

…elds Both on default & hawkular endpoints, added dropdown: - SSL - SSL trusting custom CA, which additionally shows CA field. - SSL without validation Always sets (security_protocol, certificate_authority, verify_ssl) consisently.

miq-bot · 2017-03-02T23:33:13Z

Checked commits cben/manageiq-ui-classic@d8156d9~...856a9a3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 5 offenses detected

app/controllers/mixins/ems_common_angular.rb

❗ - Line 278, Col 7 - Style/MultilineIfModifier - Favor a normal if-statement over a modifier clause in a multiline statement.

app/views/layouts/angular-bootstrap/_endpoints_angular.html.haml

⚠️ - Line 119 - Prefer to_s over string interpolation.
⚠️ - Line 119 - Use hash rockets syntax.
⚠️ - Line 133 - Prefer to_s over string interpolation.
⚠️ - Line 151 - Prefer to_s over string interpolation.

simon3z · 2017-03-03T16:30:39Z

@dclarizio @cben given that the backend part was merged ManageIQ/manageiq#14019 it is extremely problematic not having the UI (this PR) because currently the addition of a new provider fails 100%.

Any chance we can merge this soon?
cc @chessbyte @Loicavenel

dclarizio · 2017-03-03T17:27:49Z

@cben going to merge, but should follow up on @AparnaKarve's comments above about the the default in another PR. Thx, Dan

cben · 2017-03-05T14:39:33Z

Thanks. Will definitely follow up ASAP.
I'm still trying to tweak this to avoid setErrorOnTab on Hawkular endpoint when editing existing providers without hawkular. I've added logging all over the place but still don't understand what exactly goes into setErrorOnTab before I edit anything...
Thanks @AparnaKarve these 2 places will need provider-dependent defaults but yes. Found some more places where I need to copy around the new fields (eg. to postValidationModel).

miq-bot added compute/containers enhancement pending core labels Feb 22, 2017

cben mentioned this pull request Feb 22, 2017

Verify container TLS, support custom CA ManageIQ/manageiq#14019

Merged

7 tasks

cben force-pushed the ovirt-container-ca branch from d2ea016 to d79c934 Compare February 22, 2017 15:53

jhernand approved these changes Feb 23, 2017

View reviewed changes

simon3z reviewed Feb 23, 2017

View reviewed changes

simon3z approved these changes Feb 23, 2017

View reviewed changes

miq-bot assigned dclarizio Feb 23, 2017

josejulio mentioned this pull request Feb 23, 2017

Hawkular/add ssl support #460

Merged

1 task

dclarizio requested a review from AparnaKarve February 27, 2017 15:42

AparnaKarve reviewed Feb 27, 2017

View reviewed changes

cben closed this Mar 2, 2017

cben reopened this Mar 2, 2017

jhernand and others added 4 commits March 3, 2017 01:21

split tls_verify_hide, tls_ca_certs_hide

0cf0c3b

[cosmetic] align =>

856a9a3

cben force-pushed the ovirt-container-ca branch from d79c934 to 856a9a3 Compare March 2, 2017 23:30

dclarizio merged commit ea900e9 into ManageIQ:master Mar 3, 2017

dclarizio added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 3, 2017

This was referenced Mar 7, 2017

The TLS/SSL verify_peer setting is about to change ManageIQ/manageiq-documentation#244

Closed

Container TLS fixes #670

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TLS verification & custom CA UI for oVirt and Container providers #450

TLS verification & custom CA UI for oVirt and Container providers #450

cben commented Feb 22, 2017 •

edited

Loading

miq-bot commented Feb 22, 2017

cben commented Feb 22, 2017

jhernand left a comment

simon3z Feb 23, 2017

cben Feb 23, 2017

simon3z commented Feb 23, 2017

jhernand commented Feb 23, 2017

jhernand commented Feb 23, 2017

cben commented Feb 27, 2017 •

edited

Loading

dclarizio commented Feb 27, 2017

cben commented Feb 27, 2017

AparnaKarve left a comment •

edited

Loading

cben commented Feb 28, 2017 •

edited

Loading

AparnaKarve commented Mar 1, 2017

miq-bot commented Mar 2, 2017

simon3z commented Mar 3, 2017

dclarizio commented Mar 3, 2017

cben commented Mar 5, 2017

TLS verification & custom CA UI for oVirt and Container providers #450

TLS verification & custom CA UI for oVirt and Container providers #450

Conversation

cben commented Feb 22, 2017 • edited Loading

oVirt

Containers

miq-bot commented Feb 22, 2017

cben commented Feb 22, 2017

jhernand left a comment

Choose a reason for hiding this comment

simon3z Feb 23, 2017

Choose a reason for hiding this comment

cben Feb 23, 2017

Choose a reason for hiding this comment

simon3z commented Feb 23, 2017

jhernand commented Feb 23, 2017

jhernand commented Feb 23, 2017

cben commented Feb 27, 2017 • edited Loading

dclarizio commented Feb 27, 2017

cben commented Feb 27, 2017

AparnaKarve left a comment • edited Loading

Choose a reason for hiding this comment

cben commented Feb 28, 2017 • edited Loading

AparnaKarve commented Mar 1, 2017

miq-bot commented Mar 2, 2017

simon3z commented Mar 3, 2017

dclarizio commented Mar 3, 2017

cben commented Mar 5, 2017

cben commented Feb 22, 2017 •

edited

Loading

cben commented Feb 27, 2017 •

edited

Loading

AparnaKarve left a comment •

edited

Loading

cben commented Feb 28, 2017 •

edited

Loading