From ab53cb6f7b11101f4acd9ad762348601bfb4f063 Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Wed, 16 Sep 2020 22:34:01 +0300 Subject: [PATCH] app/vmagent: substitute `-remoteWrite.url` with `secret-url` value in logs, since it may contain sensitive info such as passwords or auth tokens Pass `-remoteWrite.showURL` command-line flag in order to see real `-remoteWrite.url` values in logs and at `/metrics` page. Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/773 --- app/vmagent/README.md | 4 ++++ app/vmagent/remotewrite/client.go | 30 +++++++++++++------------- app/vmagent/remotewrite/remotewrite.go | 21 +++++++++--------- docs/vmagent.md | 4 ++++ lib/flagutil/secret.go | 26 ++++++++++++++++++++++ lib/httpserver/metrics.go | 23 ++------------------ lib/logger/flag.go | 3 ++- 7 files changed, 63 insertions(+), 48 deletions(-) create mode 100644 lib/flagutil/secret.go diff --git a/app/vmagent/README.md b/app/vmagent/README.md index 4df934005..abb2e1118 100644 --- a/app/vmagent/README.md +++ b/app/vmagent/README.md @@ -228,6 +228,10 @@ If you have suggestions, improvements or found a bug - feel free to open an issu The directory can grow large when remote storage is unavailable for extended periods of time and if `-remoteWrite.maxDiskUsagePerURL` isn't set. If you don't want to send all the data from the directory to remote storage, simply stop `vmagent` and delete the directory. +* By default `vmagent` masks `-remoteWrite.url` with `secret-url` values in logs and at `/metrics` page because + the url may contain sensitive information such as auth tokens or passwords. + Pass `-remoteWrite.showURL` command-line flag when starting `vmagent` in order to see all the valid urls. + * If you see `skipping duplicate scrape target with identical labels` errors when scraping Kubernetes pods, then it is likely these pods listen multiple ports or they use init container. diff --git a/app/vmagent/remotewrite/client.go b/app/vmagent/remotewrite/client.go index 59d4cf8cf..6596bd75c 100644 --- a/app/vmagent/remotewrite/client.go +++ b/app/vmagent/remotewrite/client.go @@ -44,7 +44,7 @@ var ( ) type client struct { - urlLabelValue string + sanitizedURL string remoteWriteURL string authHeader string fq *persistentqueue.FastQueue @@ -59,7 +59,7 @@ type client struct { stopCh chan struct{} } -func newClient(argIdx int, remoteWriteURL, urlLabelValue string, fq *persistentqueue.FastQueue, concurrency int) *client { +func newClient(argIdx int, remoteWriteURL, sanitizedURL string, fq *persistentqueue.FastQueue, concurrency int) *client { tlsCfg, err := getTLSConfig(argIdx) if err != nil { logger.Panicf("FATAL: cannot initialize TLS config: %s", err) @@ -101,7 +101,7 @@ func newClient(argIdx int, remoteWriteURL, urlLabelValue string, fq *persistentq authHeader = "Bearer " + token } c := &client{ - urlLabelValue: urlLabelValue, + sanitizedURL: sanitizedURL, remoteWriteURL: remoteWriteURL, authHeader: authHeader, fq: fq, @@ -111,10 +111,10 @@ func newClient(argIdx int, remoteWriteURL, urlLabelValue string, fq *persistentq }, stopCh: make(chan struct{}), } - c.requestDuration = metrics.GetOrCreateHistogram(fmt.Sprintf(`vmagent_remotewrite_duration_seconds{url=%q}`, c.urlLabelValue)) - c.requestsOKCount = metrics.GetOrCreateCounter(fmt.Sprintf(`vmagent_remotewrite_requests_total{url=%q, status_code="2XX"}`, c.urlLabelValue)) - c.errorsCount = metrics.GetOrCreateCounter(fmt.Sprintf(`vmagent_remotewrite_errors_total{url=%q}`, c.urlLabelValue)) - c.retriesCount = metrics.GetOrCreateCounter(fmt.Sprintf(`vmagent_remotewrite_retries_count_total{url=%q}`, c.urlLabelValue)) + c.requestDuration = metrics.GetOrCreateHistogram(fmt.Sprintf(`vmagent_remotewrite_duration_seconds{url=%q}`, c.sanitizedURL)) + c.requestsOKCount = metrics.GetOrCreateCounter(fmt.Sprintf(`vmagent_remotewrite_requests_total{url=%q, status_code="2XX"}`, c.sanitizedURL)) + c.errorsCount = metrics.GetOrCreateCounter(fmt.Sprintf(`vmagent_remotewrite_errors_total{url=%q}`, c.sanitizedURL)) + c.retriesCount = metrics.GetOrCreateCounter(fmt.Sprintf(`vmagent_remotewrite_retries_count_total{url=%q}`, c.sanitizedURL)) for i := 0; i < concurrency; i++ { c.wg.Add(1) go func() { @@ -122,14 +122,14 @@ func newClient(argIdx int, remoteWriteURL, urlLabelValue string, fq *persistentq c.runWorker() }() } - logger.Infof("initialized client for -remoteWrite.url=%q", c.remoteWriteURL) + logger.Infof("initialized client for -remoteWrite.url=%q", c.sanitizedURL) return c } func (c *client) MustStop() { close(c.stopCh) c.wg.Wait() - logger.Infof("stopped client for -remoteWrite.url=%q", c.remoteWriteURL) + logger.Infof("stopped client for -remoteWrite.url=%q", c.sanitizedURL) } func getTLSConfig(argIdx int) (*tls.Config, error) { @@ -176,7 +176,7 @@ func (c *client) runWorker() { // The block has been sent successfully. case <-time.After(graceDuration): logger.Errorf("couldn't sent block with size %d bytes to %q in %.3f seconds during shutdown; dropping it", - len(block), c.remoteWriteURL, graceDuration.Seconds()) + len(block), c.sanitizedURL, graceDuration.Seconds()) } return } @@ -190,7 +190,7 @@ func (c *client) sendBlock(block []byte) { again: req, err := http.NewRequest("POST", c.remoteWriteURL, bytes.NewBuffer(block)) if err != nil { - logger.Panicf("BUG: unexected error from http.NewRequest(%q): %s", c.remoteWriteURL, err) + logger.Panicf("BUG: unexected error from http.NewRequest(%q): %s", c.sanitizedURL, err) } h := req.Header h.Set("User-Agent", "vmagent") @@ -211,7 +211,7 @@ again: retryDuration = time.Minute } logger.Errorf("couldn't send a block with size %d bytes to %q: %s; re-sending the block in %.3f seconds", - len(block), c.remoteWriteURL, err, retryDuration.Seconds()) + len(block), c.sanitizedURL, err, retryDuration.Seconds()) t := time.NewTimer(retryDuration) select { case <-c.stopCh: @@ -231,7 +231,7 @@ again: // Unexpected status code returned retriesCount++ - metrics.GetOrCreateCounter(fmt.Sprintf(`vmagent_remotewrite_requests_total{url=%q, status_code="%d"}`, c.urlLabelValue, statusCode)).Inc() + metrics.GetOrCreateCounter(fmt.Sprintf(`vmagent_remotewrite_requests_total{url=%q, status_code="%d"}`, c.sanitizedURL, statusCode)).Inc() retryDuration *= 2 if retryDuration > time.Minute { retryDuration = time.Minute @@ -239,10 +239,10 @@ again: body, err := ioutil.ReadAll(resp.Body) _ = resp.Body.Close() if err != nil { - logger.Errorf("cannot read response body from %q during retry #%d: %s", c.remoteWriteURL, retriesCount, err) + logger.Errorf("cannot read response body from %q during retry #%d: %s", c.sanitizedURL, retriesCount, err) } else { logger.Errorf("unexpected status code received after sending a block with size %d bytes to %q during retry #%d: %d; response body=%q; "+ - "re-sending the block in %.3f seconds", len(block), c.remoteWriteURL, retriesCount, statusCode, body, retryDuration.Seconds()) + "re-sending the block in %.3f seconds", len(block), c.sanitizedURL, retriesCount, statusCode, body, retryDuration.Seconds()) } t := time.NewTimer(retryDuration) select { diff --git a/app/vmagent/remotewrite/remotewrite.go b/app/vmagent/remotewrite/remotewrite.go index 498093d2d..f018c34b6 100644 --- a/app/vmagent/remotewrite/remotewrite.go +++ b/app/vmagent/remotewrite/remotewrite.go @@ -9,7 +9,6 @@ import ( "github.com/VictoriaMetrics/VictoriaMetrics/lib/decimal" "github.com/VictoriaMetrics/VictoriaMetrics/lib/flagutil" - "github.com/VictoriaMetrics/VictoriaMetrics/lib/httpserver" "github.com/VictoriaMetrics/VictoriaMetrics/lib/logger" "github.com/VictoriaMetrics/VictoriaMetrics/lib/memory" "github.com/VictoriaMetrics/VictoriaMetrics/lib/persistentqueue" @@ -63,7 +62,7 @@ func Init() { } if !*showRemoteWriteURL { // remoteWrite.url can contain authentication codes, so hide it at `/metrics` output. - httpserver.RegisterSecretFlag("remoteWrite.url") + flagutil.RegisterSecretFlag("remoteWrite.url") } initLabelsGlobal() rcs, err := loadRelabelConfigs() @@ -83,11 +82,11 @@ func Init() { maxInmemoryBlocks = 2 } for i, remoteWriteURL := range *remoteWriteURLs { - urlLabelValue := fmt.Sprintf("secret-url-%d", i+1) + sanitizedURL := fmt.Sprintf("%d:secret-url", i+1) if *showRemoteWriteURL { - urlLabelValue = fmt.Sprintf("%d:%s", i+1, remoteWriteURL) + sanitizedURL = fmt.Sprintf("%d:%s", i+1, remoteWriteURL) } - rwctx := newRemoteWriteCtx(i, remoteWriteURL, maxInmemoryBlocks, urlLabelValue) + rwctx := newRemoteWriteCtx(i, remoteWriteURL, maxInmemoryBlocks, sanitizedURL) rwctxs = append(rwctxs, rwctx) } @@ -190,17 +189,17 @@ type remoteWriteCtx struct { relabelMetricsDropped *metrics.Counter } -func newRemoteWriteCtx(argIdx int, remoteWriteURL string, maxInmemoryBlocks int, urlLabelValue string) *remoteWriteCtx { +func newRemoteWriteCtx(argIdx int, remoteWriteURL string, maxInmemoryBlocks int, sanitizedURL string) *remoteWriteCtx { h := xxhash.Sum64([]byte(remoteWriteURL)) path := fmt.Sprintf("%s/persistent-queue/%d_%016X", *tmpDataPath, argIdx+1, h) - fq := persistentqueue.MustOpenFastQueue(path, remoteWriteURL, maxInmemoryBlocks, maxPendingBytesPerURL.N) - _ = metrics.GetOrCreateGauge(fmt.Sprintf(`vmagent_remotewrite_pending_data_bytes{path=%q, url=%q}`, path, urlLabelValue), func() float64 { + fq := persistentqueue.MustOpenFastQueue(path, sanitizedURL, maxInmemoryBlocks, maxPendingBytesPerURL.N) + _ = metrics.GetOrCreateGauge(fmt.Sprintf(`vmagent_remotewrite_pending_data_bytes{path=%q, url=%q}`, path, sanitizedURL), func() float64 { return float64(fq.GetPendingBytes()) }) - _ = metrics.GetOrCreateGauge(fmt.Sprintf(`vmagent_remotewrite_pending_inmemory_blocks{path=%q, url=%q}`, path, urlLabelValue), func() float64 { + _ = metrics.GetOrCreateGauge(fmt.Sprintf(`vmagent_remotewrite_pending_inmemory_blocks{path=%q, url=%q}`, path, sanitizedURL), func() float64 { return float64(fq.GetInmemoryQueueLen()) }) - c := newClient(argIdx, remoteWriteURL, urlLabelValue, fq, *queues) + c := newClient(argIdx, remoteWriteURL, sanitizedURL, fq, *queues) pss := make([]*pendingSeries, *queues) for i := range pss { pss[i] = newPendingSeries(fq.MustWriteBlock) @@ -211,7 +210,7 @@ func newRemoteWriteCtx(argIdx int, remoteWriteURL string, maxInmemoryBlocks int, c: c, pss: pss, - relabelMetricsDropped: metrics.GetOrCreateCounter(fmt.Sprintf(`vmagent_remotewrite_relabel_metrics_dropped_total{path=%q, url=%q}`, path, urlLabelValue)), + relabelMetricsDropped: metrics.GetOrCreateCounter(fmt.Sprintf(`vmagent_remotewrite_relabel_metrics_dropped_total{path=%q, url=%q}`, path, sanitizedURL)), } } diff --git a/docs/vmagent.md b/docs/vmagent.md index 4df934005..abb2e1118 100644 --- a/docs/vmagent.md +++ b/docs/vmagent.md @@ -228,6 +228,10 @@ If you have suggestions, improvements or found a bug - feel free to open an issu The directory can grow large when remote storage is unavailable for extended periods of time and if `-remoteWrite.maxDiskUsagePerURL` isn't set. If you don't want to send all the data from the directory to remote storage, simply stop `vmagent` and delete the directory. +* By default `vmagent` masks `-remoteWrite.url` with `secret-url` values in logs and at `/metrics` page because + the url may contain sensitive information such as auth tokens or passwords. + Pass `-remoteWrite.showURL` command-line flag when starting `vmagent` in order to see all the valid urls. + * If you see `skipping duplicate scrape target with identical labels` errors when scraping Kubernetes pods, then it is likely these pods listen multiple ports or they use init container. diff --git a/lib/flagutil/secret.go b/lib/flagutil/secret.go new file mode 100644 index 000000000..7a8af546b --- /dev/null +++ b/lib/flagutil/secret.go @@ -0,0 +1,26 @@ +package flagutil + +import ( + "strings" +) + +// RegisterSecretFlag registers flagName as secret. +// +// This function must be called before starting logging. +// It cannot be called from concurrent goroutines. +// +// Secret flags aren't exported at `/metrics` page. +func RegisterSecretFlag(flagName string) { + lname := strings.ToLower(flagName) + secretFlags[lname] = true +} + +var secretFlags = make(map[string]bool) + +// IsSecretFlag returns true of s contains flag name with secret value, which shouldn't be exposed. +func IsSecretFlag(s string) bool { + if strings.Contains(s, "pass") || strings.Contains(s, "key") || strings.Contains(s, "secret") || strings.Contains(s, "token") { + return true + } + return secretFlags[s] +} diff --git a/lib/httpserver/metrics.go b/lib/httpserver/metrics.go index 7fab0a134..aff8791e6 100644 --- a/lib/httpserver/metrics.go +++ b/lib/httpserver/metrics.go @@ -8,6 +8,7 @@ import ( "time" "github.com/VictoriaMetrics/VictoriaMetrics/lib/buildinfo" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/flagutil" "github.com/VictoriaMetrics/VictoriaMetrics/lib/memory" "github.com/VictoriaMetrics/metrics" ) @@ -27,7 +28,7 @@ func WritePrometheusMetrics(w io.Writer) { flag.VisitAll(func(f *flag.Flag) { lname := strings.ToLower(f.Name) value := f.Value.String() - if isSecretFlag(lname) { + if flagutil.IsSecretFlag(lname) { // Do not expose passwords and keys to prometheus. value = "secret" } @@ -36,23 +37,3 @@ func WritePrometheusMetrics(w io.Writer) { } var startTime = time.Now() - -// RegisterSecretFlag registers flagName as secret. -// -// This function must be called before starting httpserver. -// It cannot be called from concurrent goroutines. -// -// Secret flags aren't exported at `/metrics` page. -func RegisterSecretFlag(flagName string) { - lname := strings.ToLower(flagName) - secretFlags[lname] = true -} - -var secretFlags = make(map[string]bool) - -func isSecretFlag(s string) bool { - if strings.Contains(s, "pass") || strings.Contains(s, "key") || strings.Contains(s, "secret") || strings.Contains(s, "token") { - return true - } - return secretFlags[s] -} diff --git a/lib/logger/flag.go b/lib/logger/flag.go index 5766d21a1..43fcdcc4a 100644 --- a/lib/logger/flag.go +++ b/lib/logger/flag.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/VictoriaMetrics/VictoriaMetrics/lib/buildinfo" + "github.com/VictoriaMetrics/VictoriaMetrics/lib/flagutil" ) func logAllFlags() { @@ -13,7 +14,7 @@ func logAllFlags() { flag.VisitAll(func(f *flag.Flag) { lname := strings.ToLower(f.Name) value := f.Value.String() - if strings.Contains(lname, "pass") || strings.Contains(lname, "key") || strings.Contains(lname, "secret") { + if flagutil.IsSecretFlag(lname) { // Do not expose passwords and keys to prometheus. value = "secret" }