app/vmauth: properly proxy HTTP requests without body

The Request.Body for requests without body can be nil. This could break readTrackingBody.Read() logic,
which could incorrectly return "cannot read data after closing the reader" error in this case.
Fix this by initializing the readTrackingBody.r with zeroReader.

While at it, properly set Host header if it is specified in 'headers' section.
It must be set net/http.Request.Host instead of net/http.Request.Header.Set(),
since the net/http.Client overwrites the Host header with the value from req.Host
before sending the request.

While at it, add tests for requestHandler(). Additional tests for various requestHandler() cases
will be added in future commits.

Updates VictoriaMetrics#6445
Updates VictoriaMetrics#5707
Updates VictoriaMetrics#5240
Updates VictoriaMetrics#6525

Author: valyala

app/vmauth/auth_config.go

@@ -84,9 +84,6 @@ type UserInfo struct {
 	concurrencyLimitCh      chan struct{}
 	concurrencyLimitReached *metrics.Counter
 
-	// Whether to use backend host header in requests to backend.
-	useBackendHostHeader bool
-
 	rt http.RoundTripper
 
 	requests         *metrics.Counter
@@ -96,8 +93,9 @@ type UserInfo struct {
 
 // HeadersConf represents config for request and response headers.
 type HeadersConf struct {
-	RequestHeaders  []*Header `yaml:"headers,omitempty"`
-	ResponseHeaders []*Header `yaml:"response_headers,omitempty"`
+	RequestHeaders   []*Header `yaml:"headers,omitempty"`
+	ResponseHeaders  []*Header `yaml:"response_headers,omitempty"`
+	KeepOriginalHost *bool     `yaml:"keep_original_host,omitempty"`
 }
 
 func (ui *UserInfo) beginConcurrencyLimit() error {
@@ -152,15 +150,6 @@ func (h *Header) MarshalYAML() (any, error) {
 	return h.sOriginal, nil
 }
 
-func hasEmptyHostHeader(headers []*Header) bool {
-	for _, h := range headers {
-		if h.Name == "Host" && h.Value == "" {
-			return true
-		}
-	}
-	return false
-}
-
 // URLMap is a mapping from source paths to target urls.
 type URLMap struct {
 	// SrcPaths is an optional list of regular expressions, which must match the request path.
@@ -608,7 +597,7 @@ func initAuthConfig() {
 	// See https://github.com/VictoriaMetrics/VictoriaMetrics/issues/1240
 	sighupCh := procutil.NewSighupChan()
 
-	_, err := loadAuthConfig()
+	_, err := reloadAuthConfig()
 	if err != nil {
 		logger.Fatalf("cannot load auth config: %s", err)
 	}
@@ -640,7 +629,7 @@ func authConfigReloader(sighupCh <-chan os.Signal) {
 
 	updateFn := func() {
 		configReloads.Inc()
-		updated, err := loadAuthConfig()
+		updated, err := reloadAuthConfig()
 		if err != nil {
 			logger.Errorf("failed to load auth config; using the last successfully loaded config; error: %s", err)
 			configSuccess.Set(0)
@@ -666,27 +655,45 @@ func authConfigReloader(sighupCh <-chan os.Signal) {
 	}
 }
 
-// authConfigData stores the yaml definition for this config.
-// authConfigData needs to be updated each time authConfig is updated.
-var authConfigData atomic.Pointer[[]byte]
-
 var (
-	authConfig   atomic.Pointer[AuthConfig]
-	authUsers    atomic.Pointer[map[string]*UserInfo]
+	// authConfigData stores the yaml definition for this config.
+	// authConfigData needs to be updated each time authConfig is updated.
+	authConfigData atomic.Pointer[[]byte]
+
+	// authConfig contains the currently loaded auth config
+	authConfig atomic.Pointer[AuthConfig]
+
+	// authUsers contains the currently loaded auth users
+	authUsers atomic.Pointer[map[string]*UserInfo]
+
 	authConfigWG sync.WaitGroup
 	stopCh       chan struct{}
 )
 
-// loadAuthConfig loads and applies the config from *authConfigPath.
+// reloadAuthConfig loads and applies the config from *authConfigPath.
 // It returns bool value to identify if new config was applied.
 // The config can be not applied if there is a parsing error
 // or if there are no changes to the current authConfig.
-func loadAuthConfig() (bool, error) {
+func reloadAuthConfig() (bool, error) {
 	data, err := fscore.ReadFileOrHTTP(*authConfigPath)
 	if err != nil {
 		return false, fmt.Errorf("failed to read -auth.config=%q: %w", *authConfigPath, err)
 	}
 
+	ok, err := reloadAuthConfigData(data)
+	if err != nil {
+		return false, fmt.Errorf("failed to pars -auth.config=%q: %w", *authConfigPath, err)
+	}
+	if !ok {
+		return false, nil
+	}
+
+	mp := authUsers.Load()
+	logger.Infof("loaded information about %d users from -auth.config=%q", len(*mp), *authConfigPath)
+	return true, nil
+}
+
+func reloadAuthConfigData(data []byte) (bool, error) {
 	oldData := authConfigData.Load()
 	if oldData != nil && bytes.Equal(data, *oldData) {
 		// there are no updates in the config - skip reloading.
@@ -695,14 +702,13 @@ func loadAuthConfig() (bool, error) {
 
 	ac, err := parseAuthConfig(data)
 	if err != nil {
-		return false, fmt.Errorf("failed to parse -auth.config=%q: %w", *authConfigPath, err)
+		return false, fmt.Errorf("failed to parse auth config: %w", err)
 	}
 
 	m, err := parseAuthConfigUsers(ac)
 	if err != nil {
-		return false, fmt.Errorf("failed to parse users from -auth.config=%q: %w", *authConfigPath, err)
+		return false, fmt.Errorf("failed to parse users from auth config: %w", err)
 	}
-	logger.Infof("loaded information about %d users from -auth.config=%q", len(m), *authConfigPath)
 
 	acPrev := authConfig.Load()
 	if acPrev != nil {
@@ -749,7 +755,6 @@ func parseAuthConfig(data []byte) (*AuthConfig, error) {
 		if err := ui.initURLs(); err != nil {
 			return nil, err
 		}
-		ui.useBackendHostHeader = hasEmptyHostHeader(ui.HeadersConf.RequestHeaders)
 
 		metricLabels, err := ui.getMetricLabels()
 		if err != nil {
@@ -814,7 +819,6 @@ func parseAuthConfigUsers(ac *AuthConfig) (map[string]*UserInfo, error) {
 		_ = ac.ms.GetOrCreateGauge(`vmauth_user_concurrent_requests_current`+metricLabels, func() float64 {
 			return float64(len(ui.concurrencyLimitCh))
 		})
-		ui.useBackendHostHeader = hasEmptyHostHeader(ui.HeadersConf.RequestHeaders)
 
 		rt, err := newRoundTripper(ui.TLSCAFile, ui.TLSCertFile, ui.TLSKeyFile, ui.TLSServerName, ui.TLSInsecureSkipVerify)
 		if err != nil {

app/vmauth/auth_config_test.go

@@ -82,6 +82,14 @@ users:
   headers: foobar
 `)
 
+	// Invalid keep_original_host value
+	f(`
+users:
+- username: foo
+  url_prefix: http://foo.bar
+  keep_original_host: foobar
+`)
+
 	// empty url_prefix
 	f(`
 users:
@@ -458,6 +466,7 @@ users:
 	})
 
 	// with default url
+	keepOriginalHost := true
 	f(`
 users:
 - bearer_token: foo
@@ -469,6 +478,7 @@ users:
     headers:
     - "foo: bar"
     - "xxx: y"
+    keep_original_host: true
   default_url:
   - http://default1/select/0/prometheus
   - http://default2/select/0/prometheus
@@ -491,6 +501,7 @@ users:
 							mustNewHeader("'foo: bar'"),
 							mustNewHeader("'xxx: y'"),
 						},
+						KeepOriginalHost: &keepOriginalHost,
 					},
 				},
 			},
@@ -517,6 +528,7 @@ users:
 							mustNewHeader("'foo: bar'"),
 							mustNewHeader("'xxx: y'"),
 						},
+						KeepOriginalHost: &keepOriginalHost,
 					},
 				},
 			},
@@ -536,12 +548,17 @@ users:
   metric_labels:
     dc: eu
     team: dev
+  keep_original_host: true
 - username: foo-same
   password: bar
   url_prefix: https://bar/x
   metric_labels:
     backend_env: test
     team: accounting
+  headers:
+  - "foo: bar"
+  response_headers:
+  - "Abc: def"
 `, map[string]*UserInfo{
 		getHTTPAuthBasicToken("foo-same", "baz"): {
 			Username:  "foo-same",
@@ -551,6 +568,9 @@ users:
 				"dc":   "eu",
 				"team": "dev",
 			},
+			HeadersConf: HeadersConf{
+				KeepOriginalHost: &keepOriginalHost,
+			},
 		},
 		getHTTPAuthBasicToken("foo-same", "bar"): {
 			Username:  "foo-same",
@@ -560,6 +580,14 @@ users:
 				"backend_env": "test",
 				"team":        "accounting",
 			},
+			HeadersConf: HeadersConf{
+				RequestHeaders: []*Header{
+					mustNewHeader("'foo: bar'"),
+				},
+				ResponseHeaders: []*Header{
+					mustNewHeader("'Abc: def'"),
+				},
+			},
 		},
 	})
 }

app/vmauth/main.go

@@ -236,18 +236,18 @@ func processRequest(w http.ResponseWriter, r *http.Request, ui *UserInfo) {
 }
 
 func tryProcessingRequest(w http.ResponseWriter, r *http.Request, targetURL *url.URL, hc HeadersConf, retryStatusCodes []int, ui *UserInfo) bool {
-	// This code has been copied from net/http/httputil/reverseproxy.go
 	req := sanitizeRequestHeaders(r)
-	req.URL = targetURL
 
-	if req.URL.Scheme == "https" || ui.useBackendHostHeader {
-		// Override req.Host only for https requests, since https server verifies hostnames during TLS handshake,
-		// so it expects the targetURL.Host in the request.
-		// There is no need in overriding the req.Host for http requests, since it is expected that backend server
-		// may properly process queries with the original req.Host.
-		req.Host = targetURL.Host
-	}
+	req.URL = targetURL
 	updateHeadersByConfig(req.Header, hc.RequestHeaders)
+	if hc.KeepOriginalHost == nil || !*hc.KeepOriginalHost {
+		if host := getHostHeader(hc.RequestHeaders); host != "" {
+			req.Host = host
+		} else {
+			req.Host = targetURL.Host
+		}
+	}
+
 	var trivialRetries int
 	rtb, rtbOK := req.Body.(*readTrackingBody)
 again:
@@ -338,12 +338,21 @@ func copyHeader(dst, src http.Header) {
 	}
 }
 
-func updateHeadersByConfig(headers http.Header, config []*Header) {
-	for _, h := range config {
+func getHostHeader(headers []*Header) string {
+	for _, h := range headers {
+		if h.Name == "Host" {
+			return h.Value
+		}
+	}
+	return ""
+}
+
+func updateHeadersByConfig(dst http.Header, src []*Header) {
+	for _, h := range src {
 		if h.Value == "" {
-			headers.Del(h.Name)
+			dst.Del(h.Name)
 		} else {
-			headers.Set(h.Name, h.Value)
+			dst.Set(h.Name, h.Value)
 		}
 	}
 }
@@ -537,10 +546,24 @@ func getReadTrackingBody(r io.ReadCloser, maxBodySize int) *readTrackingBody {
 		maxBodySize = 0
 	}
 	rtb.maxBodySize = maxBodySize
+
+	if r == nil {
+		// This is GET request without request body
+		r = (*zeroReader)(nil)
+	}
 	rtb.r = r
 	return rtb
 }
 
+type zeroReader struct{}
+
+func (r *zeroReader) Read(_ []byte) (int, error) {
+	return 0, io.EOF
+}
+func (r *zeroReader) Close() error {
+	return nil
+}
+
 func putReadTrackingBody(rtb *readTrackingBody) {
 	rtb.reset()
 	readTrackingBodyPool.Put(rtb)
@@ -560,7 +583,7 @@ func (rtb *readTrackingBody) Read(p []byte) (int, error) {
 		if rtb.bufComplete {
 			return 0, io.EOF
 		}
-		return 0, fmt.Errorf("cannot read data after closing the reader")
+		return 0, fmt.Errorf("cannot read client request body after closing client reader")
 	}
 
 	n, err := rtb.r.Read(p)