Adding TLS to node exporter. Server Auth only.

[achiuBAE]

Following on from: https://groups.google.com/forum/#!topic/prometheus-developers/8sez091EtzY

Added a TLS listener to node exporter. Server authentication only (no client auth... for now?).
Certificate and private key .pem file passed in via flags:
--web.tls-cert="[path to pem]"
--web.tls-private-key="[path to pem]"
If certificate and private key is concatinated into a single pem, pass in via the --web.tls-cert flag.

If no flags passed, http listener starts up as normal.

http.Server struct used for future potential change to configure the TLS e.g. specifying the cipher suites.

@SuperQ

[SuperQ]

Nice, thanks for getting this started.

First question, how does this handle cert rotation?

[achiuBAE]

Hi, thanks for responding :) There is no certificate rotation handling in place so when a new cert is generated then it still maintains new connections with the old cert unless the server is restarted, pointing to the new file.
Are you also looking for certificate handling, so when a new key pair is generated, node exporter recognises this and uses this for new incoming connections without any downtime?

[SuperQ]

Yes, auto-reloading when a new cert file is written would be Ideal. I don't consider it a blocker. We're probably going to factor some of this code out into a library

[brian-brazil]

https://stackoverflow.com/questions/37473201/is-there-a-way-to-update-the-tls-certificates-in-a-net-http-server-without-any-d seems relevant

[achiuBAE]

Thanks for the link, that seems helpful.

[discordianfish]

I think if we want to handle this consistent across all prometheus project, it should be implemented as it's own package. Maybe in common? Maybe in client_golang?

[beorn7]

I would not like to see this in client_golang. I have started to do fairly regular releases there, ramping up to the planned 1.0.0 release. The more independent components we have in this repo, the more problematic releases are. We already have two completely independent things in there: the exposition client and the API client. Releases are mostly driven by changes in the exposition client. Syncing them with the API client is already mildly problematic.

prometheus/common might be the right place, as it is a mixed bag anyway. Not having versioning tags there is, however, also seen as problematic by now, especially given the rise of Go modules with the increased significance of semver tags. I think we should find a consensus in the community what the common repo should be used for and how it should be handled. Based on that, we can come up with a decision for TLS tooling.

[brian-brazil]

it should be implemented as it's own package. Maybe in common? Maybe in client_golang?

The plan of record is that we'll get it working here, and then move it out once it's stable and we want to use it elsewhere. I'd presume to common.

Not having versioning tags there is, however, also seen as problematic by now, especially given the rise of Go modules with the increased significance of semver tags.

It's more that noone has asked for them yet.

[discordianfish]

The plan of record is that we'll get it working here, and then move it out once it's stable and we want to use it elsewhere. I'd presume to common.

That makes sense. In that case though I would suggest implementing this as it's own package within this repo, e.g github.com/prometheus/node_exporter/http or something like that. Then we can easily move this to common.

[dioguerra]

I would like to see this merged...
Does anyone know the time schedule for such a feat??

[ksherryBAE]

Hello, I've been working with @achiuBAE on this.
We've been looking at the certificate rotation and have a question around the handling.
The method we're working on is to automatically reload the certs from the original filepath when the client connection is detected.
Does this solution sound reasonable?

[discordianfish]

@ksherryBAE So it would check if the file changed on each request? Not sure, might be fine for the node-exporter case but not necessary for others where we would like to use this. Maybe just don't do reload in the first take on this. I think it's more important to figure out the interface of the module so it will work for other parts of the prometheus project too.

[ksherryBAE]

@discordianfish The current method reloads the certs and keys on receiving a client hello. Only one hello is received per connection so it will not reload every time a scrape occurs.

[ksherryBAE]

The tests currently fail due to path issues. I belive this pull request will fix it: #1245

[brian-brazil]

The plan of record is not to offer anything below this line, just to stick with Go defaults.

[mxinden]

@brian-brazil not quite sure I follow your comment. Are you saying a user should not be able to configure cipher suites or prefered cipher suites? Given that Golang supports many insecure ones I think we should expose these options to the users, e.g. to have them prevent down-grade attacks, ... What do you think?

[brian-brazil]

Yes, that's what I'm saying. The Go defaults are sane, if someone has a more advanced use case they're free to use a reverse proxy. This was what was agreed at the last dev summit.

[ksherryBAE]

@brian-brazil That has been removed

[mxinden]

Having this as a shared library sounds great. I would like to reuse this for securing the cluster traffic of Alertmanager in the long run (see prometheus/alertmanager#1763).

I agree with @beorn7 to not put it in client_golang. Given that repositories are basically free and we can leverage versioning with go modules, how about a separate prometheus/https repository?

Reloading the certificate files on each TLS dial sounds expensive. Could we either listen on filesystem changes or trigger a reload via sighups or an http endpoint?

In regards to the current state of this patch: I am happy to help out reviewing the code. Would you @achiuBAE and @ksherryBAE mind squashing the 66 commits into a reasonable amount of commits? It seems like you included patches not related to this pull request as well.

[ksherryBAE]

@mxinden Any specific commit you want it squashed from? Also if the patching you're on about is the vendor fixes - the ci and tests wouldn't pass without it.

[brian-brazil]

Given that repositories are basically free and we can leverage versioning with go modules, how about a separate prometheus/https repository?

common is the logical place.

Reloading the certificate files on each TLS dial sounds expensive. Could we either listen on filesystem changes or trigger a reload via sighups or an http endpoint?

I think you're confusing this with the http tls client code we have.

[roidelapluie]

what happened to this PR? 304 file changes..

[ksherryBAE]

Might it be worth taking the https file and creating a PR in common now, then create a new PR in here that implements that package from common and ditch this PR?
It would probably be more transparent and easier to note what files have actually changed.

[brian-brazil]

We want to release here first, fix any big issues that arise, and then move over to common.

[roidelapluie]

Do we want to use go 1.12 and TLS 1.3 ? ( https://github.com/containous/traefik/blob/fb617044e0221b7f9f0665d4c8adaa3736335cf4/cmd/traefik/traefik.go#L46-L52 )

[brian-brazil]

We'll use whatever the Go defaults are for the version we're compiled with.

[ksherryBAE]

The Go defaults currently to TLS 1.2, that's what has been implemented.
@brian-brazil Do you want the commits squashed and if so how many?

[brian-brazil]

Squash them in whatever way you think will be easiest to do/review. I think you need to rebase from master also.

[ksherryBAE]

@brian-brazil I've merged master in and have squashed the final commits, the rebasing was giving me a bit of grief because of the earlier merging.

[brian-brazil]

The PR doesn't look right, there's other changes there from master that aren't yours.

[ksherryBAE]

Yeah, that's probably the work of earlier merges, would it be better if we put the code changes in a new PR referencing this one on a new branch with only our changes. It would have the added benefit of being far easier to review.

[brian-brazil]

Whatever is easier for you.

[brian-brazil]

Closing in favour of #1277