From f054682fb7a879d7fc285d4fdeeb243e6d92f9f0 Mon Sep 17 00:00:00 2001 From: Nick Meves Date: Sat, 2 Jan 2021 13:16:45 -0800 Subject: [PATCH] Make HTTPS Redirect middleware Reverse Proxy aware --- pkg/middleware/redirect_to_https.go | 13 +++++----- pkg/middleware/redirect_to_https_test.go | 31 ++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 6 deletions(-) diff --git a/pkg/middleware/redirect_to_https.go b/pkg/middleware/redirect_to_https.go index 18b4b967..72f9dac4 100644 --- a/pkg/middleware/redirect_to_https.go +++ b/pkg/middleware/redirect_to_https.go @@ -7,7 +7,7 @@ import ( "strings" "github.com/justinas/alice" - "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/util" + requestutil "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/requests/util" ) const httpsScheme = "https" @@ -26,10 +26,11 @@ func NewRedirectToHTTPS(httpsPort string) alice.Constructor { // to the port from the httpsAddress given. func redirectToHTTPS(httpsPort string, next http.Handler) http.Handler { return http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) { - proto := req.Header.Get("X-Forwarded-Proto") - if strings.EqualFold(proto, httpsScheme) || (req.TLS != nil && proto == "") { - // Only care about the connection to us being HTTPS if the proto is empty, - // otherwise the proto is source of truth + proto := requestutil.GetRequestProto(req) + if strings.EqualFold(proto, httpsScheme) || (req.TLS != nil && proto == req.URL.Scheme) { + // Only care about the connection to us being HTTPS if the proto wasn't + // from a trusted `X-Forwarded-Proto` (proto == req.URL.Scheme). + // Otherwise the proto is source of truth next.ServeHTTP(rw, req) return } @@ -41,7 +42,7 @@ func redirectToHTTPS(httpsPort string, next http.Handler) http.Handler { // Set the Host in case the targetURL still does not have one // or it isn't X-Forwarded-Host aware - targetURL.Host = util.GetRequestHost(req) + targetURL.Host = requestutil.GetRequestHost(req) // Overwrite the port if the original request was to a non-standard port if targetURL.Port() != "" { diff --git a/pkg/middleware/redirect_to_https_test.go b/pkg/middleware/redirect_to_https_test.go index ca8bdb99..f8c2c6bb 100644 --- a/pkg/middleware/redirect_to_https_test.go +++ b/pkg/middleware/redirect_to_https_test.go @@ -5,6 +5,7 @@ import ( "fmt" "net/http/httptest" + middlewareapi "github.com/oauth2-proxy/oauth2-proxy/v7/pkg/apis/middleware" . "github.com/onsi/ginkgo" . "github.com/onsi/ginkgo/extensions/table" . "github.com/onsi/gomega" @@ -21,6 +22,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { requestString string useTLS bool headers map[string]string + reverseProxy bool expectedStatus int expectedBody string expectedLocation string @@ -35,6 +37,10 @@ var _ = Describe("RedirectToHTTPS suite", func() { if in.useTLS { req.TLS = &tls.ConnectionState{} } + scope := &middlewareapi.RequestScope{ + ReverseProxy: in.reverseProxy, + } + req = middlewareapi.AddRequestScope(req, scope) rw := httptest.NewRecorder() @@ -52,6 +58,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { requestString: "http://example.com", useTLS: false, headers: map[string]string{}, + reverseProxy: false, expectedStatus: 308, expectedBody: permanentRedirectBody("https://example.com"), expectedLocation: "https://example.com", @@ -60,6 +67,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { requestString: "https://example.com", useTLS: true, headers: map[string]string{}, + reverseProxy: false, expectedStatus: 200, expectedBody: "test", }), @@ -69,15 +77,28 @@ var _ = Describe("RedirectToHTTPS suite", func() { headers: map[string]string{ "X-Forwarded-Proto": "HTTPS", }, + reverseProxy: true, expectedStatus: 200, expectedBody: "test", }), + Entry("without TLS and X-Forwarded-Proto=HTTPS but ReverseProxy not set", &requestTableInput{ + requestString: "http://example.com", + useTLS: false, + headers: map[string]string{ + "X-Forwarded-Proto": "HTTPS", + }, + reverseProxy: false, + expectedStatus: 308, + expectedBody: permanentRedirectBody("https://example.com"), + expectedLocation: "https://example.com", + }), Entry("with TLS and X-Forwarded-Proto=HTTPS", &requestTableInput{ requestString: "https://example.com", useTLS: true, headers: map[string]string{ "X-Forwarded-Proto": "HTTPS", }, + reverseProxy: true, expectedStatus: 200, expectedBody: "test", }), @@ -87,6 +108,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { headers: map[string]string{ "X-Forwarded-Proto": "https", }, + reverseProxy: true, expectedStatus: 200, expectedBody: "test", }), @@ -96,6 +118,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { headers: map[string]string{ "X-Forwarded-Proto": "https", }, + reverseProxy: true, expectedStatus: 200, expectedBody: "test", }), @@ -105,6 +128,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { headers: map[string]string{ "X-Forwarded-Proto": "HTTP", }, + reverseProxy: true, expectedStatus: 308, expectedBody: permanentRedirectBody("https://example.com"), expectedLocation: "https://example.com", @@ -115,6 +139,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { headers: map[string]string{ "X-Forwarded-Proto": "HTTP", }, + reverseProxy: true, expectedStatus: 308, expectedBody: permanentRedirectBody("https://example.com"), expectedLocation: "https://example.com", @@ -125,6 +150,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { headers: map[string]string{ "X-Forwarded-Proto": "http", }, + reverseProxy: true, expectedStatus: 308, expectedBody: permanentRedirectBody("https://example.com"), expectedLocation: "https://example.com", @@ -135,6 +161,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { headers: map[string]string{ "X-Forwarded-Proto": "http", }, + reverseProxy: true, expectedStatus: 308, expectedBody: permanentRedirectBody("https://example.com"), expectedLocation: "https://example.com", @@ -143,6 +170,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { requestString: "http://example.com:8080", useTLS: false, headers: map[string]string{}, + reverseProxy: false, expectedStatus: 308, expectedBody: permanentRedirectBody("https://example.com:8443"), expectedLocation: "https://example.com:8443", @@ -151,6 +179,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { requestString: "https://example.com:8443", useTLS: true, headers: map[string]string{}, + reverseProxy: false, expectedStatus: 200, expectedBody: "test", }), @@ -161,6 +190,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { requestString: "/", useTLS: false, expectedStatus: 308, + reverseProxy: false, expectedBody: permanentRedirectBody("https://example.com/"), expectedLocation: "https://example.com/", }), @@ -171,6 +201,7 @@ var _ = Describe("RedirectToHTTPS suite", func() { "X-Forwarded-Proto": "HTTP", "X-Forwarded-Host": "external.example.com", }, + reverseProxy: true, expectedStatus: 308, expectedBody: permanentRedirectBody("https://external.example.com"), expectedLocation: "https://external.example.com",