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

HTTP Headers support #41

Merged
merged 1 commit into from
Oct 6, 2021
Merged

Conversation

roidelapluie
Copy link
Member

This pull request let users configure the HTTP headers as recommended by
the 2020 Cure53 security audit on Node Exporter.

It is not an arbitrary way to inject any HTTP headers as some backends
(e.g. Prometheus) already add headers (CORS), and we do not want to let
users change those.

Signed-off-by: Julien Pivotto [email protected]

@roidelapluie roidelapluie force-pushed the headers branch 2 times, most recently from 8c74a4c to dfd1881 Compare January 24, 2021 16:58
@SuperQ
Copy link
Member

SuperQ commented Jan 24, 2021

Should we set any of the defaults to non-empty?

@roidelapluie
Copy link
Member Author

X-Frame-Options: There are valid usecases for X-Frame-Options, e.g. someone could embed a prometheus console in an iframe.

X-Content-Type-Options: Future changes (default to openmetrics?) would make the /metrics page downloadable by browsers. I would not set it as a default.

X-XSS-Protection: Could be set to 1 but moderns browsers do not even implement it.

Strict-Transport-Security: No default

@brian-brazil
Copy link
Contributor

A better question to ask is, what if a backend starts specifying these headers itself? It could lead to security issues if e.g. X-Content-Type-Options ended up unset due to this code.

I'd also suggest a fat warning on Strict-Transport-Security, as blindly setting that can cause extended outages with impact beyond the process itself with no real way to recover. It's a massive footgun.

@brian-brazil
Copy link
Contributor

Future changes (default to openmetrics?) would make the /metrics page downloadable by browsers. I would not set it as a default.

It should be completely safe to set this on any /metrics page, and I don't see any relationship with OpenMetrics or downloads. Downloads are the Content-Disposition header not Content-Type, and the OM spec requires a correct content type.

The question is more what other endpoints may break due to depending on this. Offhand within this may cause issues with Prometheus console templates, as Go content type detection has had issues there before, but otherwise we should be okay. Thus it is not safe to set - this is the sort of thing that only the application itself can know if it's safe to set be default.

@SuperQ
Copy link
Member

SuperQ commented Jan 24, 2021

Yes, some strong warnings here are worthwhile.

@grobie
Copy link
Member

grobie commented Jan 25, 2021

I'd also suggest a fat warning on Strict-Transport-Security, as blindly setting that can cause extended outages with impact beyond the process itself with no real way to recover. It's a massive footgun.

I'm not following you here. Are you confusing it with HSTS preloading? Unless someone runs their exporter on a naked domain under / that is completely independent.

@roidelapluie
Copy link
Member Author

roidelapluie commented Jan 25, 2021

I'd also suggest a fat warning on Strict-Transport-Security, as blindly setting that can cause extended outages with impact beyond the process itself with no real way to recover. It's a massive footgun.

I'm not following you here. Are you confusing it with HSTS preloading? Unless someone runs their exporter on a naked domain under / that is completely independent.

if you set Strict-Transport-Security badly, browsers will no longer try to load HTTP pages, which might be unexpected. They would always try to load the HTTPS version.

@brian-brazil
Copy link
Contributor

I'm not following you here. Are you confusing it with HSTS preloading? Unless someone runs their exporter on a naked domain under / that is completely independent.

No, I'm not. Someone running it on the naked domain is one of the biggest risks here, and we have seen at least one user who was blindly attempting to do basically that based entirely on what a security scanner told them.

@grobie
Copy link
Member

grobie commented Jan 25, 2021

Without HSTS preloading only developers going to /metrics would be affected and can recover from that by resetting their browser cache. Neither end-customers nor Prometheus would be affected, or what else are you referring to?

@brian-brazil
Copy link
Contributor

Yes, anyone who hits that endpoint will end up with HSTS - whether that be a customer or anyone else.

In addition it's possible that there's things like reverse proxies in play, so that the /metrics page is ends up served on a customer facing website somewhere. It is not practical to ask all of your customers to reset their browser cache (and from a quick peek, this is not stored in the cache in Chrome - it's a separate internals thing), especially if you just effectively took down your entire website so would have difficult communicating with them. This would be tricky even for just your employees.

As I said, HSTS is a massive footgun.

This pull request let users configure the HTTP headers as recommended by
the 2020 Cure53 security audit on Node Exporter.

It is not an arbitrary way to inject any HTTP headers as some backends
(e.g. Prometheus) already add headers (CORS), and we do not want to let
users change those.

Signed-off-by: Julien Pivotto <[email protected]>
@roidelapluie
Copy link
Member Author

Updated, cc @juliusv @SuperQ

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

@roidelapluie roidelapluie merged commit 19e732c into prometheus:master Oct 6, 2021
mrueg pushed a commit to mrueg/exporter-toolkit that referenced this pull request Jan 29, 2022
This pull request let users configure the HTTP headers as recommended by
the 2020 Cure53 security audit on Node Exporter.

It is not an arbitrary way to inject any HTTP headers as some backends
(e.g. Prometheus) already add headers (CORS), and we do not want to let
users change those.

Signed-off-by: Julien Pivotto <[email protected]>
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.

4 participants