search

dotnet / dotnet-monitor

This repository contains the source code for .NET Monitor - a tool that allows you to gather diagnostic data from running applications using HTTP endpoints
MIT License
639 stars 113 forks source link

Remove HTTPS upgrade for metrics #212

Closed kelltrick closed 3 years ago

kelltrick commented 3 years ago

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.

jander-msft commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

jander-msft commented 3 years ago

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 commented 3 years ago

Created #216