Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Per-endpoint configurable certificate_authority #13567

Merged
merged 4 commits into from
Jan 20, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented Jan 18, 2017

Purpose

To securely connect to providers using self-signed TLS certificates, it should be possible to configure ManageIQ to trust specific CA(s).
Currently this generally requires adding them to the system CA bundle on the MIQ machine; Openstack looks at settings ssl.ssl_ca_file and ssl.ssl_ca_path but you still have to place the file(s) on the machine first.
This PR adds a way to to store the CA in the DB + a bit of support logic + REST API.
It remains up to specific providers to use it (exact use will vary by network lib but something like passing :ssl_cert_store => endpoint.ssl_cert_store).

  • There can be a secondary use case of "pinning a CA": my provider has a globally trusted cert from a TopSecureCA™, and I want to trust only them, not the standard bundle of over 150 CAs.
    I'm assuming here that whenever you specify a custom CA, you no longer want to trust the system bundle, so this case is also covered.

Rationale: Global / Per Provider / Per EMS / Per Endpoint ?

  • A global pool of trusted CAs would I think be less secure (I trust Alice to administrate provider A and Bob to administrate provider B, but I don't want Bob to be able to impersonate A) and pragmatically inconvenient to manage — hard to know which certs can be removed when.

  • Per Endpoint may be best since different endpoints could have different host, port and maybe certs. YAGNI?

  • Per Provider or EMS can be OK too, especially if we allow multiple certs.

=> I've chosen per-Endpoint because it logically belongs with verify_ssl and security_protocol columns that Endpoint already has.

Rationale: text column holding PEM cert(s)

A few endpoints might end up storing the same cert, but seemed to me much simpler than doing a new table with 1:many association...
I'm storing certs in PEM format — essentially the ASN-1 binary encoded in base64. Opaque, but it's what essentially all network libs take.
The parsing logic supports multiple concatenated PEM certs. There is no strong need, but it's a common thing people do with trust roots, so I figured it's easier to make it work than forbid...

REST API

Same as other endpoint attributes (see #13454):

  • Returned in endpoints (when non-nil).
  • For default endpoint can be set as a top-level attribute.
  • Can be set on endpoints inside connection_configurations.

@miq-bot add-label enhancement, providers, sql migration, api

cc @simon3z @blomquisg @Fryguy

# Returns a list, to support concatenated PEM certs.
def parse_certificate_authority
return [] if certificate_authority.blank?
certificate_authority.split(/(?=-----BEGIN)/).reject(&:blank?).collect do |pem_fragment|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a test for concatenated PEM certs? It seems the test only have 1 cert

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, at the end of endpoint_spec.rb: https://github.com/ManageIQ/manageiq/pull/13567/files#diff-763829b60a325924ed156fcba4a726e4R123
(in API test I didn't bother)

@Fryguy
Copy link
Member

Fryguy commented Jan 18, 2017

Aside from my one comment on the testing of multiple certs, LGTM 👍

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM...I'll wait on @simon3z

@miq-bot
Copy link
Member

miq-bot commented Jan 18, 2017

Checked commits cben/manageiq@4459137~...de73b1c with ruby 2.2.6, rubocop 0.46.0, and haml-lint 0.19.0
6 files checked, 0 offenses detected
Everything looks good. 🍪

@Fryguy Fryguy closed this Jan 18, 2017
@Fryguy Fryguy reopened this Jan 18, 2017
@cben cben closed this Jan 19, 2017
@cben cben reopened this Jan 19, 2017
@cben
Copy link
Contributor Author

cben commented Jan 19, 2017

Interesting, this now fails a migration spec from #13324:
https://travis-ci.org/ManageIQ/manageiq/jobs/193401493#L647
at https://github.com/ManageIQ/manageiq/pull/13324/files#diff-230601d4d50b88deb0491bee98291b9dR8
Apparently using FactoryGirl uses app/models/, which ends up running the validation I added here, which can't work before the (later) migration here created the certificate_authority column.
@moolitayer is working on a fix.

moolitayer pushed a commit to moolitayer/manageiq that referenced this pull request Jan 19, 2017
A refrence to factory girl can fail due to validations on columns
that are added in app/model after this migration.

Seen in ManageIQ#13567
@simon3z
Copy link
Contributor

simon3z commented Jan 19, 2017

@Fryguy my only comment here is that I would expect that a CA per provider would be enough (actually you generally have one CA per company... so even per-provider would be already overkill).

If this approach is simpler, more flexible and requires only some magic in the UI to copy the same certificate in all the endpoints for the vast majority of the cases... then I'm OK with it. LGTM 👍

@cben
Copy link
Contributor Author

cben commented Jan 20, 2017

There will now be 3 related fields: (security_protocol, verify_ssl, certificate_authority).
Code will typically look at some combination of them, so I think it's simplest to store them together and set them together.

  • The exact semantics varies by provider. Some check security_protocol, some check verify_ssl, some neither; I think SCVMM uses security_protocol == "ssl" sanely but several use "ssl-with-validation" for that and security_protocol == "ssl" means insecure...
  • From UI perspective it'll be natural to add CA field that only shows for a specific selection in security_protocol dropdown.

Toggling for Travis.

@cben cben closed this Jan 20, 2017
@cben cben reopened this Jan 20, 2017
@Fryguy Fryguy merged commit caa745c into ManageIQ:master Jan 20, 2017
@Fryguy Fryguy added this to the Sprint 53 Ending Jan 30, 2017 milestone Jan 20, 2017
terciodemelo pushed a commit to maas-ufcg/manageiq that referenced this pull request Feb 1, 2017
A refrence to factory girl can fail due to validations on columns
that are added in app/model after this migration.

Seen in ManageIQ#13567
@Ladas
Copy link
Contributor

Ladas commented Feb 21, 2017

@tzumainn @aufi it's something you could be interested in guys. Defining cert in the UI, rather than Settings.ssl.ssl_ca_file and Settings.ssl.ssl_ca_path

Fryguy pushed a commit to ManageIQ/manageiq-schema that referenced this pull request Jun 20, 2017
A refrence to factory girl can fail due to validations on columns
that are added in app/model after this migration.

Seen in ManageIQ/manageiq#13567

(transferred from ManageIQ/manageiq@a66497d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants