From 9250e1cc60aa0a3d60a6c3b84e449b7ffd45d645 Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Tue, 13 Mar 2018 16:33:18 -0500 Subject: [PATCH 1/4] Check upstream X-Forwarded-Proto prior to redirect The AWS load balancer terminates SSL and passes plain HTTP to fabio but does not offer the ability to do a schema redirect. Therefore, fabio running behind an AWS ELB with a schema redirect rule results in a redirect loop. This PR fixes #448. --- route/table.go | 4 ++++ route/table_test.go | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/route/table.go b/route/table.go index 8e74bded1..ab87840d4 100644 --- a/route/table.go +++ b/route/table.go @@ -326,6 +326,10 @@ func (t Table) Lookup(req *http.Request, trace string, pick picker, match matche hosts = append(hosts, "") for _, h := range hosts { if target = t.lookup(h, path, trace, pick, match); target != nil { + if target.RedirectCode != 0 && target.URL.Scheme == "https" && req.Header.Get("X-Forwarded-Proto") == "https" { + log.Print("[TRACE] Skipping https redirect from https upstream") + continue + } break } } diff --git a/route/table_test.go b/route/table_test.go index eb0fdea71..175737d77 100644 --- a/route/table_test.go +++ b/route/table_test.go @@ -483,6 +483,50 @@ func TestNormalizeHost(t *testing.T) { } } +// see https://github.com/fabiolb/fabio/issues/448 +// for more information on the issue and purpose of this test +func TestTableLookupIssue448(t *testing.T) { + s := ` + route add http-redirect foo.com:80/ https://foo.com/ opts "redirect=301" + route add mock-service / http://local.svc/ + ` + + tbl, err := NewTable(s) + if err != nil { + t.Fatal(err) + } + + var tests = []struct { + req *http.Request + dst string + }{ + // upstream http request should get https redirect + { + &http.Request{ + Host: "foo.com", + URL: mustParse("/"), + Header: http.Header{"X-Forwarded-Proto": {"http"}}, + }, + "https://foo.com/", + }, + // upstream https request should skip https redirect + { + &http.Request{ + Host: "foo.com", + URL: mustParse("/"), + Header: http.Header{"X-Forwarded-Proto": {"https"}}, + }, + "http://local.svc/", + }, + } + + for i, tt := range tests { + if got, want := tbl.Lookup(tt.req, "", rndPicker, prefixMatcher).URL.String(), tt.dst; got != want { + t.Errorf("%d: got %v want %v", i, got, want) + } + } +} + func TestTableLookup(t *testing.T) { s := ` route add svc / http://foo.com:800 From 5b8f04a72abfa713330a78d06932e04a1c580b82 Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Tue, 13 Mar 2018 21:03:23 -0500 Subject: [PATCH 2/4] make redirect protection more generic --- route/table.go | 10 +++++----- route/table_test.go | 38 +++++++++++++++++++++++++++++--------- 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/route/table.go b/route/table.go index ab87840d4..5429c2406 100644 --- a/route/table.go +++ b/route/table.go @@ -312,12 +312,11 @@ func (t Table) matchingHosts(req *http.Request) (hosts []string) { // and if none matches then it falls back to generic routes without // a host. This is useful for a catch-all '/' rule. func (t Table) Lookup(req *http.Request, trace string, pick picker, match matcher) (target *Target) { - path := req.URL.Path if trace != "" { if len(trace) > 16 { trace = trace[:15] } - log.Printf("[TRACE] %s Tracing %s%s", trace, req.Host, path) + log.Printf("[TRACE] %s Tracing %s%s", trace, req.Host, req.URL.Path) } // find matching hosts for the request @@ -325,9 +324,10 @@ func (t Table) Lookup(req *http.Request, trace string, pick picker, match matche hosts := t.matchingHosts(req) hosts = append(hosts, "") for _, h := range hosts { - if target = t.lookup(h, path, trace, pick, match); target != nil { - if target.RedirectCode != 0 && target.URL.Scheme == "https" && req.Header.Get("X-Forwarded-Proto") == "https" { - log.Print("[TRACE] Skipping https redirect from https upstream") + if target = t.lookup(h, req.URL.Path, trace, pick, match); target != nil { + if target.RedirectCode != 0 && target.URL.Scheme == req.Header.Get("X-Forwarded-Proto") && + target.URL.Hostname() == req.URL.Hostname() && target.URL.Path == req.URL.Path { + log.Print("[TRACE] Skipping redirect with same upstream scheme, host and path as target") continue } break diff --git a/route/table_test.go b/route/table_test.go index 175737d77..deea8c249 100644 --- a/route/table_test.go +++ b/route/table_test.go @@ -487,8 +487,10 @@ func TestNormalizeHost(t *testing.T) { // for more information on the issue and purpose of this test func TestTableLookupIssue448(t *testing.T) { s := ` - route add http-redirect foo.com:80/ https://foo.com/ opts "redirect=301" - route add mock-service / http://local.svc/ + route add mock-0 foo.com:80/ https://foo.com/ opts "redirect=301" + route add mock-2 aaa.com:80/ http://bbb.com/ opts "redirect=301" + route add mock-3 ccc.com:443/bar https://ccc.com/baz opts "redirect=301" + route add mock-4 / http://foo.com/ ` tbl, err := NewTable(s) @@ -500,23 +502,41 @@ func TestTableLookupIssue448(t *testing.T) { req *http.Request dst string }{ - // upstream http request should get https redirect { - &http.Request{ + req: &http.Request{ + Host: "foo.com", + URL: mustParse("/"), + }, + dst: "https://foo.com/", + // empty upstream header should follow redirect - standard behavior + }, + { + req: &http.Request{ Host: "foo.com", URL: mustParse("/"), Header: http.Header{"X-Forwarded-Proto": {"http"}}, }, - "https://foo.com/", + dst: "https://foo.com/", + // upstream http request to same https host and path should follow redirect }, - // upstream https request should skip https redirect { - &http.Request{ - Host: "foo.com", + req: &http.Request{ + Host: "aaa.com", URL: mustParse("/"), + Header: http.Header{"X-Forwarded-Proto": {"http"}}, + }, + dst: "http://bbb.com/", + // upstream http request to different http host should follow redirect + }, + { + req: &http.Request{ + Host: "ccc.com", + URL: mustParse("/bar"), Header: http.Header{"X-Forwarded-Proto": {"https"}}, + TLS: &tls.ConnectionState{}, }, - "http://local.svc/", + dst: "https://ccc.com/baz", + // upstream https request to same https host with different path should follow redirect" }, } From 71d29728adc8300866fe68fd7da6447f30ffe1f6 Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Thu, 15 Mar 2018 19:26:22 -0500 Subject: [PATCH 3/4] Further improvements to redirect loop detection. We now generate the redirectURL in the lookup function and use it for loop detection. We also cache the redirectURL in the target for later use to prevent re-generation. --- proxy/http_proxy.go | 3 +-- route/table.go | 12 ++++++++---- route/target.go | 31 +++++++++++++++++-------------- route/target_test.go | 6 +++--- 4 files changed, 29 insertions(+), 23 deletions(-) diff --git a/proxy/http_proxy.go b/proxy/http_proxy.go index 0898d7afe..95758bbe1 100644 --- a/proxy/http_proxy.go +++ b/proxy/http_proxy.go @@ -99,8 +99,7 @@ func (p *HTTPProxy) ServeHTTP(w http.ResponseWriter, r *http.Request) { } if t.RedirectCode != 0 { - redirectURL := t.GetRedirectURL(requestURL) - http.Redirect(w, r, redirectURL.String(), t.RedirectCode) + http.Redirect(w, r, t.RedirectURL.String(), t.RedirectCode) if t.Timer != nil { t.Timer.Update(0) } diff --git a/route/table.go b/route/table.go index 5429c2406..c417e2c34 100644 --- a/route/table.go +++ b/route/table.go @@ -325,10 +325,14 @@ func (t Table) Lookup(req *http.Request, trace string, pick picker, match matche hosts = append(hosts, "") for _, h := range hosts { if target = t.lookup(h, req.URL.Path, trace, pick, match); target != nil { - if target.RedirectCode != 0 && target.URL.Scheme == req.Header.Get("X-Forwarded-Proto") && - target.URL.Hostname() == req.URL.Hostname() && target.URL.Path == req.URL.Path { - log.Print("[TRACE] Skipping redirect with same upstream scheme, host and path as target") - continue + if target.RedirectCode != 0 { + target.BuildRedirectURL(req.URL) // build redirect url and cache in target + if target.RedirectURL.Scheme == req.Header.Get("X-Forwarded-Proto") && + target.RedirectURL.Host == req.Host && + target.RedirectURL.Path == req.URL.Path { + log.Print("[INFO] Skipping redirect with same scheme, host and path") + continue + } } break } diff --git a/route/target.go b/route/target.go index b5b63452a..e91fdc0a9 100644 --- a/route/target.go +++ b/route/target.go @@ -38,6 +38,10 @@ type Target struct { // When set to a value > 0 the client is redirected to the target url. RedirectCode int + // RedirectURL is the redirect target based on the request. + // This is cached here to prevent multiple generations per request. + RedirectURL *url.URL + // FixedWeight is the weight assigned to this target. // If the value is 0 the targets weight is dynamic. FixedWeight float64 @@ -55,28 +59,27 @@ type Target struct { accessRules map[string][]interface{} } -func (t *Target) GetRedirectURL(requestURL *url.URL) *url.URL { - redirectURL := &url.URL{ +func (t *Target) BuildRedirectURL(requestURL *url.URL) { + t.RedirectURL = &url.URL{ Scheme: t.URL.Scheme, Host: t.URL.Host, Path: t.URL.Path, RawQuery: t.URL.RawQuery, } - if strings.HasSuffix(redirectURL.Host, "$path") { - redirectURL.Host = redirectURL.Host[:len(redirectURL.Host)-len("$path")] - redirectURL.Path = "$path" + if strings.HasSuffix(t.RedirectURL.Host, "$path") { + t.RedirectURL.Host = t.RedirectURL.Host[:len(t.RedirectURL.Host)-len("$path")] + t.RedirectURL.Path = "$path" } - if strings.Contains(redirectURL.Path, "/$path") { - redirectURL.Path = strings.Replace(redirectURL.Path, "/$path", "$path", 1) + if strings.Contains(t.RedirectURL.Path, "/$path") { + t.RedirectURL.Path = strings.Replace(t.RedirectURL.Path, "/$path", "$path", 1) } - if strings.Contains(redirectURL.Path, "$path") { - redirectURL.Path = strings.Replace(redirectURL.Path, "$path", requestURL.Path, 1) - if t.StripPath != "" && strings.HasPrefix(redirectURL.Path, t.StripPath) { - redirectURL.Path = redirectURL.Path[len(t.StripPath):] + if strings.Contains(t.RedirectURL.Path, "$path") { + t.RedirectURL.Path = strings.Replace(t.RedirectURL.Path, "$path", requestURL.Path, 1) + if t.StripPath != "" && strings.HasPrefix(t.RedirectURL.Path, t.StripPath) { + t.RedirectURL.Path = t.RedirectURL.Path[len(t.StripPath):] } - if redirectURL.RawQuery == "" && requestURL.RawQuery != "" { - redirectURL.RawQuery = requestURL.RawQuery + if t.RedirectURL.RawQuery == "" && requestURL.RawQuery != "" { + t.RedirectURL.RawQuery = requestURL.RawQuery } } - return redirectURL } diff --git a/route/target_test.go b/route/target_test.go index 7d9c7794b..21ad2bc42 100644 --- a/route/target_test.go +++ b/route/target_test.go @@ -5,7 +5,7 @@ import ( "testing" ) -func TestTarget_GetRedirectURL(t *testing.T) { +func TestTarget_BuildRedirectURL(t *testing.T) { type routeTest struct { req string want string @@ -95,8 +95,8 @@ func TestTarget_GetRedirectURL(t *testing.T) { target := route.Targets[0] for _, rt := range tt.tests { reqURL, _ := url.Parse("http://foo.com" + rt.req) - got := target.GetRedirectURL(reqURL) - if got.String() != rt.want { + target.BuildRedirectURL(reqURL) + if got := target.RedirectURL.String(); got != rt.want { t.Errorf("Got %s, wanted %s", got, rt.want) } } From 9b2339042ea556ca42f6307d3f0ec5b009c7eab4 Mon Sep 17 00:00:00 2001 From: Aaron Hurt Date: Fri, 16 Mar 2018 07:52:23 -0500 Subject: [PATCH 4/4] Ensure BuildRedirectURL enforces non-empty path --- proxy/http_integration_test.go | 2 +- route/table_test.go | 20 +++++++++++++++----- route/target.go | 3 +++ 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/proxy/http_integration_test.go b/proxy/http_integration_test.go index 1a0870306..4631cdb7d 100644 --- a/proxy/http_integration_test.go +++ b/proxy/http_integration_test.go @@ -284,7 +284,7 @@ func TestRedirect(t *testing.T) { {req: "/", wantCode: 301, wantLoc: "http://a.com/"}, {req: "/aaa/bbb", wantCode: 301, wantLoc: "http://a.com/aaa/bbb"}, {req: "/foo", wantCode: 301, wantLoc: "http://a.com/abc"}, - {req: "/bar", wantCode: 302, wantLoc: "http://b.com"}, + {req: "/bar", wantCode: 302, wantLoc: "http://b.com/"}, {req: "/bar/aaa", wantCode: 302, wantLoc: "http://b.com/aaa"}, } diff --git a/route/table_test.go b/route/table_test.go index deea8c249..ce8a0dca2 100644 --- a/route/table_test.go +++ b/route/table_test.go @@ -487,10 +487,10 @@ func TestNormalizeHost(t *testing.T) { // for more information on the issue and purpose of this test func TestTableLookupIssue448(t *testing.T) { s := ` - route add mock-0 foo.com:80/ https://foo.com/ opts "redirect=301" - route add mock-2 aaa.com:80/ http://bbb.com/ opts "redirect=301" - route add mock-3 ccc.com:443/bar https://ccc.com/baz opts "redirect=301" - route add mock-4 / http://foo.com/ + route add mock foo.com:80/ https://foo.com/ opts "redirect=301" + route add mock aaa.com:80/ http://bbb.com/ opts "redirect=301" + route add mock ccc.com:443/bar https://ccc.com/baz opts "redirect=301" + route add mock / http://foo.com/ ` tbl, err := NewTable(s) @@ -517,7 +517,17 @@ func TestTableLookupIssue448(t *testing.T) { Header: http.Header{"X-Forwarded-Proto": {"http"}}, }, dst: "https://foo.com/", - // upstream http request to same https host and path should follow redirect + // upstream http request to same host and path should follow redirect + }, + { + req: &http.Request{ + Host: "foo.com", + URL: mustParse("/"), + Header: http.Header{"X-Forwarded-Proto": {"https"}}, + TLS: &tls.ConnectionState{}, + }, + dst: "http://foo.com/", + // upstream https request to same host and path should NOT follow redirect" }, { req: &http.Request{ diff --git a/route/target.go b/route/target.go index e91fdc0a9..db9e5244b 100644 --- a/route/target.go +++ b/route/target.go @@ -82,4 +82,7 @@ func (t *Target) BuildRedirectURL(requestURL *url.URL) { t.RedirectURL.RawQuery = requestURL.RawQuery } } + if t.RedirectURL.Path == "" { + t.RedirectURL.Path = "/" + } }