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 https://github.com/VictoriaMetrics/VictoriaMetrics/issues/6445
Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5707
Updates https://github.com/VictoriaMetrics/VictoriaMetrics/issues/5240
Updates https://github.com/VictoriaMetrics/VictoriaMetrics/pull/6525

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)

app/vmauth/main_test.go

@@ -2,10 +2,140 @@ package main
 
 import (
 	"bytes"
+	"fmt"
 	"io"
+	"net/http"
+	"net/http/httptest"
+	"strings"
 	"testing"
 )
 
+func TestRequestHandler(t *testing.T) {
+	f := func(cfgStr, requestURL string, backendHandler http.HandlerFunc, responseExpected string) {
+		t.Helper()
+
+		ts := httptest.NewServer(backendHandler)
+		defer ts.Close()
+
+		cfgStr = strings.ReplaceAll(cfgStr, "{BACKEND}", ts.URL)
+		responseExpected = strings.ReplaceAll(responseExpected, "{BACKEND}", ts.URL)
+
+		cfgOrigP := authConfigData.Load()
+		if _, err := reloadAuthConfigData([]byte(cfgStr)); err != nil {
+			t.Fatalf("cannot load config data: %s", err)
+		}
+		defer func() {
+			cfgOrig := []byte("unauthorized_user:\n  url_prefix: http://foo/bar")
+			if cfgOrigP != nil {
+				cfgOrig = *cfgOrigP
+			}
+			_, err := reloadAuthConfigData(cfgOrig)
+			if err != nil {
+				t.Fatalf("cannot load the original config: %s", err)
+			}
+		}()
+
+		r, err := http.NewRequest(http.MethodGet, requestURL, nil)
+		if err != nil {
+			t.Fatalf("cannot initialize http request: %s", err)
+		}
+
+		w := &fakeResponseWriter{}
+		if !requestHandler(w, r) {
+			t.Fatalf("unexpected false is returned from requestHandler")
+		}
+
+		response := w.getResponse()
+		response = strings.TrimSpace(response)
+		responseExpected = strings.TrimSpace(responseExpected)
+		if response != responseExpected {
+			t.Fatalf("unexpected response\ngot\n%v\nwant\n%v", response, responseExpected)
+		}
+	}
+
+	// regular url_prefix
+	cfgStr := `
+unauthorized_user:
+  url_prefix: {BACKEND}/foo?bar=baz
+`
+	requestURL := "http://some-host.com/abc/def?some_arg=some_value"
+	backendHandler := func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprintf(w, "requested_url=http://%s%s", r.Host, r.URL)
+	}
+	responseExpected := `statusCode=200
+requested_url={BACKEND}/foo/abc/def?bar=baz&some_arg=some_value
+`
+	f(cfgStr, requestURL, backendHandler, responseExpected)
+
+	// keep_original_host
+	cfgStr = `
+unauthorized_user:
+  url_prefix: "{BACKEND}/foo?bar=baz"
+  keep_original_host: true
+`
+	requestURL = "http://some-host.com/abc/def?some_arg=some_value"
+	backendHandler = func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprintf(w, "requested_url=http://%s%s", r.Host, r.URL)
+	}
+	responseExpected = `statusCode=200
+requested_url=http://some-host.com/foo/abc/def?bar=baz&some_arg=some_value
+`
+	f(cfgStr, requestURL, backendHandler, responseExpected)
+
+	// override request host
+	cfgStr = `
+unauthorized_user:
+  url_prefix: "{BACKEND}/foo?bar=baz"
+  headers:
+  - "Host: other-host:12345"
+`
+	requestURL = "http://some-host.com/abc/def?some_arg=some_value"
+	backendHandler = func(w http.ResponseWriter, r *http.Request) {
+		fmt.Fprintf(w, "requested_url=http://%s%s", r.Host, r.URL)
+	}
+	responseExpected = `statusCode=200
+requested_url=http://other-host:12345/foo/abc/def?bar=baz&some_arg=some_value
+`
+	f(cfgStr, requestURL, backendHandler, responseExpected)
+
+}
+
+type fakeResponseWriter struct {
+	h http.Header
+
+	bb bytes.Buffer
+}
+
+func (w *fakeResponseWriter) getResponse() string {
+	return w.bb.String()
+}
+
+func (w *fakeResponseWriter) Header() http.Header {
+	if w.h == nil {
+		w.h = http.Header{}
+	}
+	return w.h
+}
+
+func (w *fakeResponseWriter) Write(p []byte) (int, error) {
+	return w.bb.Write(p)
+}
+
+func (w *fakeResponseWriter) WriteHeader(statusCode int) {
+	fmt.Fprintf(&w.bb, "statusCode=%d\n", statusCode)
+	if w.h == nil {
+		return
+	}
+	err := w.h.WriteSubset(&w.bb, map[string]bool{
+		"Content-Length": true,
+		"Content-Type":   true,
+		"Date":           true,
+	})
+	if err != nil {
+		panic(fmt.Errorf("BUG: cannot marshal headers: %s", err))
+	}
+}
+
 func TestReadTrackingBody_RetrySuccess(t *testing.T) {
 	f := func(s string, maxBodySize int) {
 		t.Helper()

docs/CHANGELOG.md

@@ -31,6 +31,7 @@ See also [LTS releases](https://docs.victoriametrics.com/lts-releases/).
 
 ## tip
 
+* BUGFIX: [vmauth](https://docs.victoriametrics.com/vmauth/): fix `cannot read data after closing the reader` error when proxying HTTP requests without body (aka `GET` requests). The issue has been introduced in [v1.102.0](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.102.0) in [this commit](https://github.com/VictoriaMetrics/VictoriaMetrics/commit/7ee57974935a662896f2de40fdf613156630617d).
 
 ## [v1.102.0](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.102.0)
 

docs/vmauth.md

@@ -647,15 +647,6 @@ unauthorized_user:
   - "X-Forwarded-For:"
 ```
 
-It is also possible to update `Host` request header to the backend host specified in `url_prefix` by setting an empty value for `Host` header:
-
-```yaml
-unauthorized_user:
-  url_prefix: "http://backend:1234/"
-  headers:
-  - "Host:"    # Update host header to backend:1234
-```
-
 `vmauth` also supports the ability to set and remove HTTP response headers before returning the response from the backend to client.
 This is done via `response_headers` option. For example, the following [`-auth.config`](#auth-config) sets `Foo: bar` response header
 and removes `Server` response header before returning the response to client:
@@ -668,6 +659,30 @@ unauthorized_user:
   - "Server:"
 ```
 
+See also [`Host` header docs](#host-http-header).
+
+## Host HTTP header
+
+By default `vmauth` sets the `Host` HTTP header to the backend hostname when proxying requests to the corresponding backend.
+Sometimes it is needed to keep the original `Host` header from the client request sent to `vmauth`. For example, if backends use host-based routing.
+In this case set `keep_original_host: true`. For example, the following config instructs to use the original `Host` header from client requests
+when proxying requests to the `backend:1234`:
+
+```yaml
+unauthorized_user:
+  url_prefix: "http://backend:1234/"
+  keep_iriginal_host: true
+```
+
+It is also possible to set the `Host` header to arbitrary value when proxying the request to the configured backend, via [`headers` option](#modifying-http-headers):
+
+```yaml
+unauthorized_user:
+  url_prefix: "http://backend:1234/"
+  headers:
+  - "Host: foobar"
+```
+
 ## Config reload
 
 `vmauth` supports dynamic reload of [`-auth.config`](#auth-config) via the following ways: