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

Remove HTTPS upgrade for metrics #212

Closed
kelltrick opened this issue Apr 20, 2021 · 5 comments · Fixed by #320
Closed

Remove HTTPS upgrade for metrics #212

kelltrick opened this issue Apr 20, 2021 · 5 comments · Fixed by #320
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@kelltrick
Copy link
Contributor

Proposed Feature

When a user connects to http://[addr]:52325/ and receives a custom metrics entry, an https is currently performed. The https upgrade should not be done. Metrics is an unauthenticated endpoint and the https does not protect any data from being egressed, anyone who could man-in-the-middle the http endpoint could simply call the endpoint themselves.

@kelltrick kelltrick added the enhancement New feature or request label Apr 20, 2021
@jander-msft
Copy link
Member

To add some food for thought, the https upgrade doesn't protect from exfiltration of the data, but does protect other callers from getting bogus data (preventing MITM attacks). It could conceivably be a real concern such that a rogue actor could force metrics to effectively report metrics that indicate a healthy and well-behaving process when in reality the process is being attacked. But if this is a real concern, we would enable https on the endpoint by default regardless of the standard vs custom metrics being reported.

If the only concern is who can get the data, then authentication needs to be enabled, but if Prometheus doesn't support "advanced" forms of authentication, then I don't think there's anything we can do about that.

@jander-msft
Copy link
Member

If I'm reading this correctly, Prometheus supports authentication using the Authorization header: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#scrape_config

authorization:
  type: MonitorApiKey
  credentials: <base 64 encoded key value>

cc @wiktork

@wiktork
Copy link
Member

wiktork commented Apr 21, 2021

That seems like it would work. We may want to consider having a separate key for metrics.

@jander-msft
Copy link
Member

That seems like it would work. We may want to consider having a separate key for metrics.

I think we mulled the idea of supporting multiple API keys but didn't go with it because we didn't have a differentiation of authorization per key, but this would put that back on the table. I'll create a separate issue for this.

@jander-msft
Copy link
Member

Created #216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants