prometheus-community / PushProx

Proxy to allow Prometheus to scrape through NAT etc.
Apache License 2.0
719 stars 133 forks source link

Added basic auth for proxy and /clients endpoint #94

Closed Szpadel closed 2 years ago

Szpadel commented 3 years ago

This PR adds possibility to enable basic auth for proxy requests and /clients endpoint. Also it adds possibility to disable /clients endpoint completely.

I do not see any benefit from securing other endpoints as those should not have any possibility to penetrate network. I know that it was mentioned already that any security features should be added by other means for prometheus projects, but as the whole purpose of this project is to overcome firewall limitations this addition would be very welcome.

hansmi commented 3 years ago

LGTM on adding a flag to disable /clients. Regarding basic auth I think we should add TLS support at the same time. prometheus/exporter-toolkit is used by the node exporter and provides both. This is how I added it in a private exporter:

import (
  "flag"
  "github.com/prometheus/exporter-toolkit/https"
  kitlog "github.com/go-kit/kit/log"
)
…
var configFile = flag.String("web.config", "", "Path to config yaml file that can enable TLS or authentication")
…
http.Handle(*metricsPath, promhttp.HandlerFor(reg, promhttp.HandlerOpts{}))
…
logger := kitlog.NewLogfmtLogger(kitlog.StdlibWriter{})
server := &http.Server{Addr: *listenAddress}
if err := https.Listen(server, *configFile, logger); err != nil {
  log.Fatal(err)
}

@SuperQ ^ FYI on the LGTM

ashish1099 commented 3 years ago

@hansmi isn't the client already support tls https://github.com/prometheus-community/PushProx/pull/27 ?

hansmi commented 3 years ago

@hansmi isn't the client already support tls #27 ?

The client does, yes. This PR deals with the server/proxy component.

ashish1099 commented 3 years ago

@hansmi so this is waiting on the serverside tls setup ?

hansmi commented 3 years ago

@ashish1099 It's essentially waiting for a PushProx maintainer to review the code. My January 5, 2021 comment pointed out that merely adding basic auth support isn't enough and that reusing existing functionality would be preferrable.

SuperQ commented 2 years ago

Yes, this implementation should use the exporter toolkit. Passing passwords to the command line is unsafe.