Add TLS support to HTTP Server

[annanay25]

Part of cortexproject/cortex#2350

Changes

• Adds six new config flags which will accept path to the Cert, Key and ClientCA, for HTTP and GRPC servers.
  • server.http-cert-path, server.http-key-path, server.http-ca-path
  • server.grpc-cert-path, server.grpc-key-path, server.grpc-ca-path
• Either both server and client auth will be enabled, or neither.
• Added tests, and scripts to generate certs at runtime.
• Changes in dependencies are because of the following addition:

github.com/prometheus/node_exporter v1.0.0-rc.0.0.20200417100735-fa4edd700ebc

• ConfigToTLSConfig, which is used from the prometheus node_exporter library and generates a *tls.Config struct from the config options, is awaiting security review.
• The following breaking change has been made to the prometheus client_golang library:

github.com/prometheus/client_golang v1.4.1

/cc @bboreham

[bboreham]

Q: Should dummy certs for testing be included in this PR, or a script to generate certs at test runtime?

I don't know; what are the trade-offs? What did someone else do when faced with this question?

[annanay25]

Adding certs to the repo for testing purposes seems like the most common solution, the only trade-off being that the test will fail once the certs expire.

[halcyondude]

I would like to confirm that both variants (enabled, not enabled) of this functionality will be tested in CI.

Scenario

Now that cortex is 1.0+ we plan to begin using it in production. We have been rolling out a service mesh across our infra, for a few reasons - one of which is consistent mTLS across all k8s services which are on the mesh. We expect this to be how we handle service <-> service encryption.

While The motivation to bake this into the project is rational, I would hope that it doesn’t over time become mandatory as it adds another layer/place where certificates need to be managed. The mesh we are using is able to integrate with external CA’s, and cert rotation/renewal is handled by cert-manager (using LetsEncrypt).

We are planning to run Cortex atop this existing infra, which in addition to providing mTLS, also enables observability, a layer of resiliency, and (I’m hoping) the ability to have blue-greens via traffic split for cortex service versions.

If I’m misunderstanding the use case here or intent apologies! We are still learning about the project, code, and operational aspects of running cortex in our infra.

[annanay25]

Thanks for your feedback, I completely agree with your reasoning @halcyondude.

The PR has been designed to remain backward compatible. TLS remains optional, and can be setup via a service mesh as you have mentioned. If the relevant flags are not initialised, tls will not be initiated and the server will work exactly as before.

That said, some users may not want to add a service mesh dependency and IMO it makes sense to bake this feature into the server :)

[tomwilkie]

Q: Should dummy certs for testing be included in this PR, or a script to generate certs at test runtime?

I think having certs (even dummy certs) checked in is undesirable; it makes it looks like a leak of secrets, even if it isn't. Can we easily generate them in the unit test?

[tomwilkie]

Can we reuse https://github.com/prometheus/node_exporter/blob/master/https/tls_config.go instead of building it ourself? This also has more options for client cert - which might be a good thing.

I think we'll need to be able to specify different TLS settings for gRPC and HTTP - for instance, not needing client cert verification on HTTP but needing it on gRPC sounds reasonable to me.

[gouthamve]

I tried exposing the function upstream but I am not sure it will be done anytime soon prometheus/node_exporter#1677 (comment)

I don't think we should block this PR on that but we should add a TODO in the code and in an issue to move to the upstream function when its exposed. I also see that upstream is waiting for a security review, and I don't think we should block on that imo. The upstream code has already gone through robust code review here: prometheus/node_exporter#1277 and from what I can see, by reusing that code, we're getting a free security review :)

[bboreham]

A few more thoughts below.

[annanay25]

Updated PR description as well. Please let me know if this is sufficient to move forward. :)

[bboreham]

lgtm, thanks!

[bboreham]

Note this is under Apache 2 licence.
(also it's more future-proof to use a commit hash than master in the URL)

[annanay25]

Ah, right. I could send a follow up PR to change this.

[annanay25]

Thanks!