Verify container TLS, support custom CA #14019

cben · 2017-02-22T14:12:32Z

Support verifying kubernetes/openshift TLS certificates, including self-signed certificates.

The UI PR ManageIQ/manageiq-ui-classic#450 will consistently set endpoint (security_protocol, verify_ssl, certificate_authority) for containers, with 3 modes:

(ssl-with-validation, 1, nil)
(ssl-with-validation-custom-ca, 1, certificate_authority text)
(ssl-without-validation, 0, nil)
[Some providers use just ssl but with inconsistent meanings, I opted for long but unambiguous.]

REST API passing these options already possible on master.

Compatibility:

Existing providers, both via UI and API had no security_protocol, defaulted to verify_ssl == 1 but did not enforce it.

If they have a globally-trusted cert, they'll silently become secure;
If they have self-signed cert, their auth will become "Error" until users Edit the provider to configure custom CA to trust (or disable validation):

Is this OK or too aggressive?
Verified behavior without hawkular endpoint (kubernetes, openshifts created before darga(?)). Defaults to secure as expected.

Verified that various connections still work:

Refresh.
Metrics (Hawkular endpoint).
Event watcher.
SSA scanning.
- Only 70% sure MiqFS part worked, want to re-check.
fetch_hawk_inv (tested without real data, returned [] but connection worked no errors)

Links

Container & oVirt combined UI changes: TLS verification & custom CA UI for oVirt and Container providers manageiq-ui-classic#450
Similar oVirt backend change: Verify oVirt TLS certificates #14004
Similar Hawkular changes: backed Be able to use tls when connecting to Hawkular #14054, UI Hawkular/add ssl support manageiq-ui-classic#460
This builds on Per-endpoint configurable certificate_authority #13567 [merged] for storing the certificate_authority field,
and MiqContainerGroup, WebDAV: take net/http options hash manageiq-gems-pending#51 [merged] for passing https options to container MiqFS.

@miq-bot add-label providers/containers, security, bug, enhancement

@Fryguy @blomquisg @moolitayer please review. cc @jhernand @simon3z

durandom

LGTM 👍

@cben will it require a data migration?
I remember these two issues making a provider defunct after upgrade

#10758
#10736

cben · 2017-02-23T10:43:55Z

It should not require a migration.

I'm assuming we're OK with defaulting to secure existing providers, breaking some until edited by user?
(If we wanted to grandfather existing insecurity, that may need a migration — existing providers have verify_ssl == 1 in DB.)
I'm relying on existing containers providers not having security_protocol set and future UI-created ones having it.
- API users might continue creating providers without setting security_protocol; this should work, respecting verify_ssl if set, defaulting to secure if not.

I tried to test these in container_manager_spec.rb

Thanks for the 2 pointers, I'll dig into those.
One thing I didn't yet but should try is going back before containers had multiple endpoints (pre-darga I think), creating a provider, migrating forward, and seeing what endpoints I get and how this code deals.

[kicking Travis — failure unrelated, got fixed yesterday UPDATE: now another unrelated MiqReport failure]

durandom · 2017-02-23T13:00:40Z

I'm assuming we're OK with defaulting to secure existing providers, breaking some until edited by user?

@blomquisg maybe this needs release notes or other docs?

blomquisg · 2017-02-23T13:29:40Z

maybe this needs release notes or other docs?

Right, that's honestly the only way I can think to handle this. I think @cben is right in saying that if we don't include a migration, it errs on the side of being more secure, or breaking providers that were intended to be less secure. Then, if a user wants to continue in a less secure configuration, they will have to go out of their way to enforce that configuration.

simon3z · 2017-02-23T22:19:28Z

...els/manageiq/providers/kubernetes/container_manager/metrics_capture/hawkular_client_mixin.rb

@@ -10,8 +10,12 @@ def hawkular_client
      @hawkular_entrypoint, @hawkular_credentials, @hawkular_options)
  end

+  # may be nil
+  def hawkular_endpoint


@cben it seems that having two similar methods as hawkular_endpoint and hawkular_entrypoint is confusing. Any way to improve this part?

cben · 2017-02-27T23:25:30Z

app/models/manageiq/providers/openshift/container_manager_mixin.rb

@@ -36,7 +36,7 @@ def openshift_connect(hostname, port, options)
      Kubeclient::Client.new(
        raw_api_endpoint(hostname, port, '/oapi'),
        options[:version] || api_version,
-        :ssl_options    => { :verify_ssl => verify_ssl_mode },
+        :ssl_options    => options[:ssl_options],


TODO: if not given this passes nil, which crashes kubeclient (it assumes @ssl_options is always a hash, the default hash is only used if :ssl_options it not passed at all to constructor).
This only affects calls get_hostname_from_routes, and will be masked by the UI PR which starts passing :ssl_options from get_hostname_from_routes, but I should fix this.

[same in kubernetes_connect]

Upcoming UI will consistently set endpoint (security_protocol, verify_ssl, certificate_authority). Existing providers, both via UI and API had no security_protocol, defaulted to verify_ssl == 1 but did not enforce it. - If they had a globally-trusted cert, they'll silently become secure; - If they had self-signed cert, their auth will become "Unreachable" until users Edit the provider to configure custom CA to trust.

To reduce confusion with hawkular_endpoint (which returns an Endpoint).

miq-bot · 2017-02-28T11:27:35Z

Checked commits cben/manageiq@7fb273d~...cc9d396 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks good. 👍

cben · 2017-03-01T14:26:16Z

@blomquisg This PR is ready. I believe oVirt #14004 is ready too.

Addressed comments, added one test, verified behavior with a pre-darga provider defaults to secure [https://gist.github.com/cben/684d5e36e068278b29de0309e669ff0e].

Still chasing an edge case in the UI PR ManageIQ/manageiq-ui-classic#450 — if your provider had no hawkular endpoint (kubernetes, openshifts created before darga(?)) — it's hard or impossible to edit the provider without adding a hawkular endpoint.

It could make sense to merge things as is and I'll follow up with another UI PR (strictly better than merging core without UI; the alternative is blocking all 3).

blomquisg · 2017-03-01T18:51:41Z

I've opened a docs issue discussing this situation so that we can craft appropriate documentation for users impacted by this change: ManageIQ/manageiq-documentation#244

miq-bot added bug enhancement providers/containers core/security labels Feb 22, 2017

cben mentioned this pull request Feb 22, 2017

TLS verification & custom CA UI for oVirt and Container providers ManageIQ/manageiq-ui-classic#450

Merged

cben force-pushed the container_verify_tls branch from 9545bf8 to 67ded83 Compare February 22, 2017 15:07

chessbyte assigned blomquisg Feb 22, 2017

chessbyte requested review from simon3z and Fryguy February 22, 2017 15:31

blomquisg requested a review from durandom February 22, 2017 19:37

durandom approved these changes Feb 23, 2017

View reviewed changes

cben closed this Feb 23, 2017

cben reopened this Feb 23, 2017

blomquisg closed this Feb 23, 2017

blomquisg reopened this Feb 23, 2017

simon3z reviewed Feb 23, 2017

View reviewed changes

cben closed this Feb 27, 2017

cben reopened this Feb 27, 2017

cben commented Feb 27, 2017

View reviewed changes

cben force-pushed the container_verify_tls branch from 67ded83 to 2257eb7 Compare February 28, 2017 11:11

cben added 2 commits February 28, 2017 13:22

s/hawkular_entrypoint/hawkular_uri/

cc9d396

To reduce confusion with hawkular_endpoint (which returns an Endpoint).

cben force-pushed the container_verify_tls branch from 2257eb7 to cc9d396 Compare February 28, 2017 11:22

blomquisg mentioned this pull request Mar 1, 2017

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

Closed

blomquisg merged commit efaca4a into ManageIQ:master Mar 1, 2017

blomquisg added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify container TLS, support custom CA #14019

Verify container TLS, support custom CA #14019

cben commented Feb 22, 2017 •

edited

Loading

durandom left a comment

cben commented Feb 23, 2017 •

edited

Loading

durandom commented Feb 23, 2017

blomquisg commented Feb 23, 2017

simon3z Feb 23, 2017

cben Feb 27, 2017

cben Feb 28, 2017

miq-bot commented Feb 28, 2017

cben commented Mar 1, 2017

blomquisg commented Mar 1, 2017

Verify container TLS, support custom CA #14019

Verify container TLS, support custom CA #14019

Conversation

cben commented Feb 22, 2017 • edited Loading

Compatibility:

Verified that various connections still work:

Links

durandom left a comment

Choose a reason for hiding this comment

cben commented Feb 23, 2017 • edited Loading

durandom commented Feb 23, 2017

blomquisg commented Feb 23, 2017

simon3z Feb 23, 2017

Choose a reason for hiding this comment

cben Feb 27, 2017

Choose a reason for hiding this comment

cben Feb 28, 2017

Choose a reason for hiding this comment

miq-bot commented Feb 28, 2017

cben commented Mar 1, 2017

blomquisg commented Mar 1, 2017

cben commented Feb 22, 2017 •

edited

Loading

cben commented Feb 23, 2017 •

edited

Loading