Skip to content

Comments

Add TLS and basic auth policy#1611

Merged
roidelapluie merged 33 commits intoprometheus:masterfrom
roidelapluie:devsummit
May 15, 2020
Merged

Add TLS and basic auth policy#1611
roidelapluie merged 33 commits intoprometheus:masterfrom
roidelapluie:devsummit

Conversation

@roidelapluie
Copy link
Member

Julien Pivotto added 2 commits April 29, 2020 14:53
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Copy link
Member

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

LGTM!

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Copy link
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

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

Thanks for this.

Julien Pivotto added 4 commits April 29, 2020 15:32
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie
Copy link
Member Author

Added a note about security of the config file, too

@roidelapluie roidelapluie changed the title Add TLS Policy Add TLS and basic auth policy Apr 29, 2020
Julien Pivotto added 7 commits April 29, 2020 16:15
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
tunnel between the servers and reverse proxies with special settings.

HTTP Basic Authentication is also supported. In such a case, usernames
and passwords are provided in the same configuration file as the TLS
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is the first mention of a configuration file which makes it seem as though we forgot that - though I don't think we need this level of detail in this doc (that's more for the usage docs).

Copy link
Member Author

Choose a reason for hiding this comment

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

that config file has security implications -- if people give access to anyone it is possible to change certs, disable client side auth, add users..

Copy link
Contributor

Choose a reason for hiding this comment

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

What goes in what config files is too detailed to go here I think, plus other config files also have security implications depending on your setup.

Julien Pivotto added 3 commits April 29, 2020 18:17
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
The Go projects will share the same TLS library, which will be based on the
Go vanilla TLS library. We keep [Go default TLS
parameters](https://golang.org/pkg/crypto/tls/#Config), with one exception: we
support only TLSv1.2 and higher.
Copy link
Contributor

Choose a reason for hiding this comment

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

This still needs a justification of some form, this appears completely arbitrary currently which will make it more challenging to deal with requests to change it good/bad reasons.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added links.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good start, we need something to cover why we're not changing anything else - otherwise we could have fun when any of those 4 organisations propose e.g. going to TLS 1.3 only or disabling specific ciphers. For example the current standing policy is to stick to the Go defaults, so that the decision is completely out of our hands and we thus can keep it simple and not worry about any of this.

What I'm looking for here (and have been asking for in relation to TLS&friends for many years) is essentially a plan that doesn't boil down to making it up as we go along. There's various hurdles we will inevitably have to deal with as things naturally evolve over time, and I don't want us to have a half-baked approach to TLS that considers the feature done and dusted merely because some code has been written. The code is the easy bit, and a justification that only covers why we have these particular settings right now is not enough. Users requesting changes to the hard coding, vulnerabilities, increases in computing power, geopolitics, changes in dependencies, and new standards are all going to happen.
The policy needs to be able to help answer questions like when we would disable 1.2, reenable 1.0 (and yes, it has happened in the past that an older version was suddenly more secure than a newer version), switch to/from whatever the cipher flavour of the month is, and what happens when Go changes their defaults. So things to be considered might include how much breakage of existing users is okay, is it okay to create a situation where the latest releases of official Prometheus components don't work with each other, how far back are we willing to have such breakage, how perfect should our ssllabs rating be, how good the actual security is for typical users, how often we need to reevaluate/update this configuration, how much effort we're realistically able/willing to put into this, and how we try to balance all these concerns.

Now all of that doesn't belong in this document as this document is more a what than why, but if you want to have a consensus to change our policy to one that allows for TLS 1.0/1.1 to be disabled then this needs to exist in some form.

Copy link
Member Author

Choose a reason for hiding this comment

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

"We strive to keep TLS
compatibility with official client libraries
runtimes that were released less than two years ago."

Copy link
Contributor

Choose a reason for hiding this comment

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

Doing some research Go was the last to release TLS 1.3 support in February 2019, so 2 years means we would drop 1.2 support in February 2021.

Julien Pivotto added 4 commits April 29, 2020 23:37
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Julien Pivotto added 2 commits May 1, 2020 17:01
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie
Copy link
Member Author

Added:

"We strive to keep TLS
compatibility with official client libraries
runtimes that were released less than two years ago."

Copy link
Member

@SuperQ SuperQ left a comment

Choose a reason for hiding this comment

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

LGTM

support only TLSv1.2 and higher. This decision is based upon current industry
standards (notes from [Google][tlsgoogle], [Apple][tlsapple],
[Mozilla][tlsmozilla], and [Microsoft][tlsmicrosoft]). We strive to keep TLS
compatibility with official [client libraries](../instrumenting/clientlibs)
Copy link
Contributor

Choose a reason for hiding this comment

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

client library runtimes

This still isn't covering all the other settings

should not be placed in other configuration fields, as it is common for
components to expose their configuration over their HTTP endpoint.
components to expose their configuration over their HTTP endpoint. It is the
responsibility of the user to protect files on disks from unwanted reads and
Copy link
Contributor

Choose a reason for hiding this comment

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

on disk

@roidelapluie
Copy link
Member Author

roidelapluie commented May 2, 2020

To lift some of the concerns about what we can and can't accept as contributions from users, and I think that whatever we write here we will get those questions, I proposed the following pull request in the node_export https package, covering a wider range of TLS settings that can be set:
prometheus/node_exporter#1695

That way we enable users to pick their TLS settings.

@brian-brazil
Copy link
Contributor

I think that level of change would require a dev summit discussion, as any configurability was seen as off the table at the last one - particularly as that PR has made some interesting choices in what it does and doesn't allow to be configured.

@SuperQ
Copy link
Member

SuperQ commented May 3, 2020

@brian-brazil We do not require dev summits for making decisions. A dev summit is simply a way to get the same lazy consensus and/or vote done in real time.

@brian-brazil
Copy link
Contributor

You're correct, however given all the back and forth on this issue a dev summit would seem the most expedient way of moving from the standing plan of pure defaults to anything else.

Julien Pivotto added 5 commits May 4, 2020 21:19
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
Copy link
Contributor

@brian-brazil brian-brazil left a comment

Choose a reason for hiding this comment

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

👍

We default to TLS 1.2 as minimum version. Our policy regarding this is based on
[Qualys SSL Labs](https://www.ssllabs.com/) recommendations, where we strive to
achieve a grade 'A' with a default configuration and correctly provided
certificates, while sticking as closely as possible to the upstream go defaults.
Copy link
Contributor

Choose a reason for hiding this comment

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

Go

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>

Prometheus is maintained by volunteers, not by a company. Therefore, fixing
security issues is done on a best-effort basis. We strive to release security
fixes within 7 days for: Prometheus, Alertmanager, Node_exporter,
Copy link
Member

Choose a reason for hiding this comment

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

I'd either say "node_exporter" or "Node Exporter" (probably the latter in a normal English sentence), but mixing snake_case with CamelCase seems odd (same for Blackbox Exporter).

Copy link
Member Author

Choose a reason for hiding this comment

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

ack


If you have special TLS needs, like a different cipher suite or older TLS
version, you can tune the minimum TLS version and the ciphers, as long as the
cipher are not [marked as insecure](https://golang.org/pkg/crypto/tls/#InsecureCipherSuites)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cipher are not [marked as insecure](https://golang.org/pkg/crypto/tls/#InsecureCipherSuites)
cipher is not [marked as insecure](https://golang.org/pkg/crypto/tls/#InsecureCipherSuites)

Copy link
Member Author

Choose a reason for hiding this comment

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

ack

@juliusv
Copy link
Member

juliusv commented May 11, 2020

👍 besides editorial nits

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
@roidelapluie roidelapluie merged commit 9ab828f into prometheus:master May 15, 2020
@roidelapluie
Copy link
Member Author

Thanks all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants