Use httpAuth.* flags as a fallback for endpoints protected by *AuthKey flags (#3582)

* {lib/server, app/}: use `httpAuth.*` flag as fallback for `*AuthKey` if it is not set

* lib/ingestserver/opentsdbhttp: fix opentdb HTTP handler not respecting `httpAuth.*` flags

* Apply suggestions from code review

Co-authored-by: Aliaksandr Valialkin <valyala@victoriametrics.com>

app/vmagent/main.go

@@ -347,12 +347,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request) bool {
 		}
 		return true
 	case "/prometheus/config", "/config":
-		if *configAuthKey != "" && r.FormValue("authKey") != *configAuthKey {
-			err := &httpserver.ErrorWithStatusCode{
-				Err:        fmt.Errorf("The provided authKey doesn't match -configAuthKey"),
-				StatusCode: http.StatusUnauthorized,
-			}
-			httpserver.Errorf(w, r, "%s", err)
+		if !httpserver.CheckAuthFlag(w, r, configAuthKey, "configAuthKey") {
 			return true
 		}
 		promscrapeConfigRequests.Inc()
@@ -361,12 +356,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request) bool {
 		return true
 	case "/prometheus/api/v1/status/config", "/api/v1/status/config":
 		// See https://prometheus.io/docs/prometheus/latest/querying/api/#config
-		if *configAuthKey != "" && r.FormValue("authKey") != *configAuthKey {
-			err := &httpserver.ErrorWithStatusCode{
-				Err:        fmt.Errorf("The provided authKey doesn't match -configAuthKey"),
-				StatusCode: http.StatusUnauthorized,
-			}
-			httpserver.Errorf(w, r, "%s", err)
+		if !httpserver.CheckAuthFlag(w, r, configAuthKey, "configAuthKey") {
 			return true
 		}
 		promscrapeStatusConfigRequests.Inc()

app/vmauth/main.go

@@ -60,9 +60,7 @@ func main() {
 func requestHandler(w http.ResponseWriter, r *http.Request) bool {
 	switch r.URL.Path {
 	case "/-/reload":
-		authKey := r.FormValue("authKey")
-		if authKey != *reloadAuthKey {
-			httpserver.Errorf(w, r, "invalid authKey %q. It must match the value from -reloadAuthKey command line flag", authKey)
+		if !httpserver.CheckAuthFlag(w, r, reloadAuthKey, "reloadAuthKey") {
 			return true
 		}
 		configReloadRequests.Inc()

app/vmselect/main.go

@@ -237,8 +237,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request) bool {
 
 	path := strings.Replace(r.URL.Path, "//", "/", -1)
 	if path == "/internal/resetRollupResultCache" {
-		if len(*resetCacheAuthKey) > 0 && r.FormValue("authKey") != *resetCacheAuthKey {
-			sendPrometheusError(w, r, fmt.Errorf("invalid authKey=%q for %q", r.FormValue("authKey"), path))
+		if !httpserver.CheckAuthFlag(w, r, resetCacheAuthKey, "resetCacheAuthKey") {
 			return true
 		}
 		promql.ResetRollupResultCache()

app/vmstorage/main.go

@@ -170,9 +170,7 @@ func newRequestHandler(strg *storage.Storage) httpserver.RequestHandler {
 func requestHandler(w http.ResponseWriter, r *http.Request, strg *storage.Storage) bool {
 	path := r.URL.Path
 	if path == "/internal/force_merge" {
-		authKey := r.FormValue("authKey")
-		if authKey != *forceMergeAuthKey {
-			httpserver.Errorf(w, r, "invalid authKey %q. It must match the value from -forceMergeAuthKey command line flag", authKey)
+		if !httpserver.CheckAuthFlag(w, r, forceMergeAuthKey, "forceMergeAuthKey") {
 			return true
 		}
 		// Run force merge in background
@@ -191,9 +189,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request, strg *storage.Storag
 		return true
 	}
 	if path == "/internal/force_flush" {
-		authKey := r.FormValue("authKey")
-		if authKey != *forceFlushAuthKey {
-			httpserver.Errorf(w, r, "invalid authKey %q. It must match the value from -forceFlushAuthKey command line flag", authKey)
+		if !httpserver.CheckAuthFlag(w, r, forceFlushAuthKey, "forceFlushAuthKey") {
 			return true
 		}
 		logger.Infof("flushing storage to make pending data available for reading")
@@ -203,9 +199,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request, strg *storage.Storag
 	if !strings.HasPrefix(path, "/snapshot") {
 		return false
 	}
-	authKey := r.FormValue("authKey")
-	if authKey != *snapshotAuthKey {
-		httpserver.Errorf(w, r, "invalid authKey %q. It must match the value from -snapshotAuthKey command line flag", authKey)
+	if !httpserver.CheckAuthFlag(w, r, snapshotAuthKey, "snapshotAuthKey") {
 		return true
 	}
 	path = path[len("/snapshot"):]

docs/CHANGELOG.md

@@ -30,6 +30,7 @@ The following tip changes can be tested by building VictoriaMetrics components f
 * FEATURE: [csvimport](https://docs.victoriametrics.com/#how-to-import-csv-data): support empty values for imported metrics. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3540).
 * FEATURE: [vmalert](httpпоs://docs.victoriametrics.com/vmalert.html): allow configuring the default number of stored rule's update states in memory via global `-rule.updateEntriesLimit` command-line flag or per-rule via rule's `update_entries_limit` configuration param. See [these docs](https://docs.victoriametrics.com/vmalert.html#rules) and [this pull request](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/3556).
 * FEATURE: improve the logic benhind `-maxConcurrentInserts` command-line flag. Previously this flag was limiting the number of concurrent connections from clients, which write data to VictoriaMetrics or [vmagent](https://docs.victoriametrics.com/vmagent.html). Some of these connections could be idle for some time. These connections do not need significant amounts of CPU and memory, so there is no sense in limiting their count. The updated logic behind `-maxConcurrentInserts` limits the number of **active** insert requests, not counting idle connections.
+* FEATURE: protect all the http endpoints with `-httpAuth.*` command-line flag. Previously endpoints protected by `-*AuthKey` command-line flags weren't protected by `-httpAuth.*`. This could complicate the proper security setup. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3060).
 * FEATURE: [VictoriaMetrics cluster](https://docs.victoriametrics.com/Cluster-VictoriaMetrics.html): add `-maxConcurrentInserts` and `-insert.maxQueueDuration` command-line flags to `vmstorage`, so they could be tuned if needed in the same way as at `vminsert` nodes.
 * FEATURE: [VictoriaMetrics cluster](https://docs.victoriametrics.com/Cluster-VictoriaMetrics.html): limit the number of concurrently executed requests at `vmstorage` proportionally to the number of available CPU cores, since every request can saturate a single CPU core at `vmstorage`. Previously a single `vmstorage` could accept and start processing arbitrary number of concurrent requests received from big number of `vmselect` nodes. This could result in increased RAM, CPU and disk IO usage or event to out of memory crash at `vmstorage` side under high load. The limit can be fine-tuned if needed via `-search.maxConcurrentRequests` command-line flag at `vmstorage` according to [these docs](https://docs.victoriametrics.com/Cluster-VictoriaMetrics.html#resource-usage-limits). `vmstorage` now [exposes](https://docs.victoriametrics.com/Cluster-VictoriaMetrics.html#monitoring) the following additional metrics at `http://vmstorage:8482/metrics` page:
   - `vm_vmselect_concurrent_requests_capacity` - the maximum number of requests allowed to execute concurrently
@@ -46,6 +47,7 @@ The following tip changes can be tested by building VictoriaMetrics components f
 * BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): fix possible resource leak after hot reload of the updated [consul_sd_configs](https://docs.victoriametrics.com/sd_configs.html#consul_sd_configs). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3468).
 * BUGFIX: [vmagent](https://docs.victoriametrics.com/vmagent.html): fix a panic in [gce_sd_configs](https://docs.victoriametrics.com/sd_configs.html#gce_sd_configs) when the discovered instance has zero labels. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3624). The issue has been introduced in [v1.85.0](https://docs.victoriametrics.com/CHANGELOG.html#v1850).
 * BUGFIX: properly return label names starting from uppercase such as `CamelCaseLabel` from [/api/v1/labels](https://docs.victoriametrics.com/url-examples.html#apiv1labels). See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3566).
+* BUGFIX: fix `opentsdb` HTTP endpoint not respecting `-httpAuth.*` flags. See [this issue](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3060)
 * BUGFIX: consistently select the sample with the biggest value out of samples with identical timestamps during querying when the [deduplication](https://docs.victoriametrics.com/#deduplication) is enabled according to [this feature request](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/3333). Previously random samples could be selected during querying.
 
 

lib/httpserver/httpserver.go

@@ -292,8 +292,7 @@ func handlerWrapper(s *server, w http.ResponseWriter, r *http.Request, rh Reques
 		return
 	case "/metrics":
 		metricsRequests.Inc()
-		if len(*metricsAuthKey) > 0 && r.FormValue("authKey") != *metricsAuthKey {
-			http.Error(w, "The provided authKey doesn't match -metricsAuthKey", http.StatusUnauthorized)
+		if !CheckAuthFlag(w, r, metricsAuthKey, "metricsAuthKey") {
 			return
 		}
 		startTime := time.Now()
@@ -302,8 +301,7 @@ func handlerWrapper(s *server, w http.ResponseWriter, r *http.Request, rh Reques
 		metricsHandlerDuration.UpdateDuration(startTime)
 		return
 	case "/flags":
-		if len(*flagsAuthKey) > 0 && r.FormValue("authKey") != *flagsAuthKey {
-			http.Error(w, "The provided authKey doesn't match -flagsAuthKey", http.StatusUnauthorized)
+		if !CheckAuthFlag(w, r, flagsAuthKey, "flagsAuthKey") {
 			return
 		}
 		w.Header().Set("Content-Type", "text/plain; charset=utf-8")
@@ -322,8 +320,7 @@ func handlerWrapper(s *server, w http.ResponseWriter, r *http.Request, rh Reques
 	default:
 		if strings.HasPrefix(r.URL.Path, "/debug/pprof/") {
 			pprofRequests.Inc()
-			if len(*pprofAuthKey) > 0 && r.FormValue("authKey") != *pprofAuthKey {
-				http.Error(w, "The provided authKey doesn't match -pprofAuthKey", http.StatusUnauthorized)
+			if !CheckAuthFlag(w, r, pprofAuthKey, "pprofAuthKey") {
 				return
 			}
 			DisableResponseCompression(w)
@@ -331,7 +328,7 @@ func handlerWrapper(s *server, w http.ResponseWriter, r *http.Request, rh Reques
 			return
 		}
 
-		if !checkBasicAuth(w, r) {
+		if !CheckBasicAuth(w, r) {
 			return
 		}
 		if rh(w, r) {
@@ -344,7 +341,25 @@ func handlerWrapper(s *server, w http.ResponseWriter, r *http.Request, rh Reques
 	}
 }
 
-func checkBasicAuth(w http.ResponseWriter, r *http.Request) bool {
+// CheckAuthFlag checks whether the given authKey is set and valid
+//
+// Falls back to checkBasicAuth if authKey is not set
+func CheckAuthFlag(w http.ResponseWriter, r *http.Request, flagValue *string, flagName string) bool {
+	if len(*flagValue) == 0 {
+		return CheckBasicAuth(w, r)
+	}
+
+	if r.FormValue("authKey") != *flagValue {
+		http.Error(w, fmt.Sprintf("The provided authKey doesn't match -%s", flagName), http.StatusUnauthorized)
+		return false
+	}
+
+	return true
+}
+
+// CheckBasicAuth validates credentials provided in request if httpAuth.* flags are set
+// returns true if credentials are valid or httpAuth.* flags are not set
+func CheckBasicAuth(w http.ResponseWriter, r *http.Request) bool {
 	if len(*httpAuthUsername) == 0 {
 		// HTTP Basic Auth is disabled.
 		return true

lib/ingestserver/opentsdbhttp/server.go

@@ -86,6 +86,9 @@ func (s *Server) MustStop() {
 
 func newRequestHandler(insertHandler func(r *http.Request) error) http.Handler {
 	rh := func(w http.ResponseWriter, r *http.Request) {
+		if !httpserver.CheckBasicAuth(w, r) {
+			return
+		}
 		writeRequests.Inc()
 		if err := insertHandler(r); err != nil {
 			writeErrors.Inc()