From 9e0c37be2d8c6eeab231c788f3a1a5be1df9acdc Mon Sep 17 00:00:00 2001 From: Aliaksandr Valialkin Date: Fri, 19 Jul 2024 17:26:51 +0200 Subject: [PATCH] app/vmauth: properly proxy requests to backend paths ending with / Previously the traling / was incorrectly removed when proxying requests from http://vmauth/ While at it, add more tests for requestHandler() --- app/vmauth/auth_config.go | 12 ++- app/vmauth/main.go | 2 +- app/vmauth/main_test.go | 178 +++++++++++++++++++++++++++++++++++--- docs/CHANGELOG.md | 1 + 4 files changed, 176 insertions(+), 17 deletions(-) diff --git a/app/vmauth/auth_config.go b/app/vmauth/auth_config.go index b10bcc083..747259ccc 100644 --- a/app/vmauth/auth_config.go +++ b/app/vmauth/auth_config.go @@ -1009,6 +1009,14 @@ func getAuthTokensFromRequest(r *http.Request) []string { } } + // Authorization via http://user:pass@hosname/path + if u := r.URL.User; u != nil && u.Username() != "" { + username := u.Username() + password, _ := u.Password() + at := getHTTPAuthBasicToken(username, password) + ats = append(ats, at) + } + return ats } @@ -1034,10 +1042,6 @@ func (up *URLPrefix) sanitizeAndInitialize() error { } func sanitizeURLPrefix(urlPrefix *url.URL) (*url.URL, error) { - // Remove trailing '/' from urlPrefix - for strings.HasSuffix(urlPrefix.Path, "/") { - urlPrefix.Path = urlPrefix.Path[:len(urlPrefix.Path)-1] - } // Validate urlPrefix if urlPrefix.Scheme != "http" && urlPrefix.Scheme != "https" { return nil, fmt.Errorf("unsupported scheme for `url_prefix: %q`: %q; must be `http` or `https`", urlPrefix, urlPrefix.Scheme) diff --git a/app/vmauth/main.go b/app/vmauth/main.go index d82f36663..542016f9e 100644 --- a/app/vmauth/main.go +++ b/app/vmauth/main.go @@ -118,7 +118,7 @@ func requestHandler(w http.ResponseWriter, r *http.Request) bool { } w.Header().Set("WWW-Authenticate", `Basic realm="Restricted"`) - http.Error(w, "missing `Authorization` request header", http.StatusUnauthorized) + http.Error(w, "missing 'Authorization' request header", http.StatusUnauthorized) return true } diff --git a/app/vmauth/main_test.go b/app/vmauth/main_test.go index 7e63a20b1..7dd443257 100644 --- a/app/vmauth/main_test.go +++ b/app/vmauth/main_test.go @@ -46,10 +46,11 @@ func TestRequestHandler(t *testing.T) { } response := w.getResponse() + response = strings.ReplaceAll(response, "\r\n", "\n") response = strings.TrimSpace(response) responseExpected = strings.TrimSpace(responseExpected) if response != responseExpected { - t.Fatalf("unexpected response\ngot\n%v\nwant\n%v", response, responseExpected) + t.Fatalf("unexpected response\ngot\n%s\nwant\n%s", response, responseExpected) } } @@ -62,7 +63,8 @@ unauthorized_user: backendHandler := func(w http.ResponseWriter, r *http.Request) { fmt.Fprintf(w, "requested_url=http://%s%s", r.Host, r.URL) } - responseExpected := `statusCode=200 + responseExpected := ` +statusCode=200 requested_url={BACKEND}/foo/abc/def?bar=baz&some_arg=some_value ` f(cfgStr, requestURL, backendHandler, responseExpected) @@ -73,12 +75,13 @@ unauthorized_user: url_prefix: "{BACKEND}/foo?bar=baz" keep_original_host: true ` - requestURL = "http://some-host.com/abc/def?some_arg=some_value" + requestURL = "http://some-host.com/abc/def" 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 + responseExpected = ` +statusCode=200 +requested_url=http://some-host.com/foo/abc/def?bar=baz ` f(cfgStr, requestURL, backendHandler, responseExpected) @@ -89,15 +92,165 @@ unauthorized_user: headers: - "Host: other-host:12345" ` - requestURL = "http://some-host.com/abc/def?some_arg=some_value" + requestURL = "http://some-host.com/abc/def" 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 + responseExpected = ` +statusCode=200 +requested_url=http://other-host:12345/foo/abc/def?bar=baz ` f(cfgStr, requestURL, backendHandler, responseExpected) + // /-/reload handler failure + origAuthKey := reloadAuthKey.Get() + if err := reloadAuthKey.Set("secret"); err != nil { + t.Fatalf("unexpected error: %s", err) + } + cfgStr = ` +unauthorized_user: + url_prefix: "{BACKEND}/foo" +` + requestURL = "http://some-host.com/-/reload" + backendHandler = func(_ http.ResponseWriter, _ *http.Request) { + panic(fmt.Errorf("backend handler shouldn't be called")) + } + responseExpected = ` +statusCode=401 +The provided authKey doesn't match -reloadAuthKey +` + f(cfgStr, requestURL, backendHandler, responseExpected) + if err := reloadAuthKey.Set(origAuthKey); err != nil { + t.Fatalf("unexpected error: %s", err) + } + + // missing authorization + cfgStr = ` +users: +- username: foo + url_prefix: "{BACKEND}/bar" +` + requestURL = "http://some-host.com/a/b" + backendHandler = func(_ http.ResponseWriter, _ *http.Request) { + panic(fmt.Errorf("backend handler shouldn't be called")) + } + responseExpected = ` +statusCode=401 +Www-Authenticate: Basic realm="Restricted" +missing 'Authorization' request header +` + f(cfgStr, requestURL, backendHandler, responseExpected) + + // incorrect authorization + cfgStr = ` +users: +- username: foo + password: secret + url_prefix: "{BACKEND}/bar" +` + requestURL = "http://foo:invalid-secret@some-host.com/a/b" + backendHandler = func(_ http.ResponseWriter, _ *http.Request) { + panic(fmt.Errorf("backend handler shouldn't be called")) + } + responseExpected = ` +statusCode=401 +Unauthorized +` + f(cfgStr, requestURL, backendHandler, responseExpected) + + // correct authorization + cfgStr = ` +users: +- username: foo + password: secret + url_prefix: "{BACKEND}/bar" +` + requestURL = "http://foo:secret@some-host.com/a/b" + 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}/bar/a/b +` + f(cfgStr, requestURL, backendHandler, responseExpected) + + // verify how path cleanup works + cfgStr = ` +unauthorized_user: + url_prefix: {BACKEND}/foo?bar=baz +` + requestURL = "http://some-host.com/../../a//.///bar/" + 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/a/bar/?bar=baz +` + f(cfgStr, requestURL, backendHandler, responseExpected) + + // verify how path cleanup works for url without path + cfgStr = ` +unauthorized_user: + url_prefix: {BACKEND}/foo?bar=baz +` + requestURL = "http://some-host.com/" + 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?bar=baz +` + f(cfgStr, requestURL, backendHandler, responseExpected) + + // verify how path cleanup works for url without path if url_prefix path ends with / + cfgStr = ` +unauthorized_user: + url_prefix: {BACKEND}/foo/?bar=baz +` + requestURL = "http://some-host.com/" + 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/?bar=baz +` + f(cfgStr, requestURL, backendHandler, responseExpected) + // verify how path cleanup works for url without path and the url_prefix without path prefix + cfgStr = ` +unauthorized_user: + url_prefix: {BACKEND}/?bar=baz +` + requestURL = "http://some-host.com/" + 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}/?bar=baz +` + f(cfgStr, requestURL, backendHandler, responseExpected) + + // verify routing to default_url + cfgStr = ` +unauthorized_user: + url_map: + - src_paths: ["/foo/.+"] + url_prefix: {BACKEND}/x-foo/ + default_url: {BACKEND}/404.html +` + requestURL = "http://some-host.com/abc?de=fg" + 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}/404.html?request_path=http%3A%2F%2Fsome-host.com%2Fabc%3Fde%3Dfg +` + f(cfgStr, requestURL, backendHandler, responseExpected) } type fakeResponseWriter struct { @@ -127,12 +280,13 @@ func (w *fakeResponseWriter) WriteHeader(statusCode int) { return } err := w.h.WriteSubset(&w.bb, map[string]bool{ - "Content-Length": true, - "Content-Type": true, - "Date": true, + "Content-Length": true, + "Content-Type": true, + "Date": true, + "X-Content-Type-Options": true, }) if err != nil { - panic(fmt.Errorf("BUG: cannot marshal headers: %s", err)) + panic(fmt.Errorf("cannot marshal headers: %s", err)) } } diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index f882fafdd..5f6a5dd2f 100644 --- a/docs/CHANGELOG.md +++ b/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/): properly proxy requests to backend urls ending with `/` if the original request path equals to `/`. Previously the trailing `/` at the backend path was incorrectly removed. For example, if the request to `http://vmauth/` is configured to be proxied to `url_prefix=http://backend/foo/`, then it was proxied to `http://backend/foo`, while it should go to `http://backend/foo/`. * 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)