From d6364c55ab51708b3ec92cde07ffa80fa9b84765 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 21 Jan 2020 17:46:47 +1100 Subject: [PATCH 01/22] update struct field names for TestAccessControl to follow coding style --- acceptance_test.go | 130 ++++++++++++++++++++++----------------------- 1 file changed, 65 insertions(+), 65 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index 49d391b18..380fe31cf 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -1245,92 +1245,92 @@ func TestAccessControl(t *testing.T) { testServer.StartTLS() defer testServer.Close() - cases := []struct { - Host string - Path string - Status int - RedirectBack bool - Description string + tests := []struct { + host string + path string + status int + redirectBack bool + name string }{ { - "group.auth.gitlab-example.com", - "/private.project/", - http.StatusOK, - false, - "project with access", + name: "project with access", + host: "group.auth.gitlab-example.com", + path: "/private.project/", + status: http.StatusOK, + redirectBack: false, }, { - "group.auth.gitlab-example.com", - "/private.project.1/", - http.StatusNotFound, // Do not expose project existed - false, - "project without access", + name: "project without access", + host: "group.auth.gitlab-example.com", + path: "/private.project.1/", + status: http.StatusNotFound, // Do not expose project existed + redirectBack: false, }, { - "group.auth.gitlab-example.com", - "/private.project.2/", - http.StatusFound, - true, - "invalid token test should redirect back", + name: "invalid token test should redirect back", + host: "group.auth.gitlab-example.com", + path: "/private.project.2/", + status: http.StatusFound, + redirectBack: true, }, { - "group.auth.gitlab-example.com", - "/nonexistent/", - http.StatusNotFound, - false, - "no project should redirect to login and then return 404", + name: "no project should redirect to login and then return 404", + host: "group.auth.gitlab-example.com", + path: "/nonexistent/", + status: http.StatusNotFound, + redirectBack: false, }, { - "nonexistent.gitlab-example.com", - "/nonexistent/", - http.StatusNotFound, - false, - "no project should redirect to login and then return 404", + name: "no project should redirect to login and then return 404", + host: "nonexistent.gitlab-example.com", + path: "/nonexistent/", + status: http.StatusNotFound, + redirectBack: false, }, // subgroups { - "group.auth.gitlab-example.com", - "/subgroup/private.project/", - http.StatusOK, - false, - "[subgroup] project with access", + name: "[subgroup] project with access", + host: "group.auth.gitlab-example.com", + path: "/subgroup/private.project/", + status: http.StatusOK, + redirectBack: false, }, { - "group.auth.gitlab-example.com", - "/subgroup/private.project.1/", - http.StatusNotFound, // Do not expose project existed - false, - "[subgroup] project without access", + name: "[subgroup] project without access", + host: "group.auth.gitlab-example.com", + path: "/subgroup/private.project.1/", + status: http.StatusNotFound, // Do not expose project existed + redirectBack: false, }, { - "group.auth.gitlab-example.com", - "/subgroup/private.project.2/", - http.StatusFound, - true, - "[subgroup] invalid token test should redirect back", + name: "[subgroup] invalid token test should redirect back", + host: "group.auth.gitlab-example.com", + path: "/subgroup/private.project.2/", + status: http.StatusFound, + redirectBack: true, }, { - "group.auth.gitlab-example.com", - "/subgroup/nonexistent/", - http.StatusNotFound, - false, - "[subgroup] no project should redirect to login and then return 404", + name: "[subgroup] no project should redirect to login and then return 404", + host: "group.auth.gitlab-example.com", + path: "/subgroup/nonexistent/", + status: http.StatusNotFound, + redirectBack: false, }, { - "nonexistent.gitlab-example.com", - "/subgroup/nonexistent/", - http.StatusNotFound, - false, - "[subgroup] no project should redirect to login and then return 404", + name: "[subgroup] no project should redirect to login and then return 404", + host: "nonexistent.gitlab-example.com", + path: "/subgroup/nonexistent/", + status: http.StatusNotFound, + redirectBack: false, }, } - for _, c := range cases { + for _, tt := range tests { - t.Run(fmt.Sprintf("Access Control Test: %s", c.Description), func(t *testing.T) { + t.Run(tt.name, func(t *testing.T) { teardown := RunPagesProcessWithAuthServerWithSSL(t, *pagesBinary, listeners, "", certFile, testServer.URL) defer teardown() - rsp, err := GetRedirectPage(t, httpsListener, c.Host, c.Path) + rsp, err := GetRedirectPage(t, httpsListener, tt.host, tt.path) require.NoError(t, err) defer rsp.Body.Close() @@ -1363,7 +1363,7 @@ func TestAccessControl(t *testing.T) { // Will redirect auth callback to correct host url, err = url.Parse(authrsp.Header.Get("Location")) require.NoError(t, err) - require.Equal(t, c.Host, url.Host) + require.Equal(t, tt.host, url.Host) require.Equal(t, "/auth", url.Path) // Request auth callback in project domain @@ -1372,21 +1372,21 @@ func TestAccessControl(t *testing.T) { // server returns the ticket, user will be redirected to the project page require.Equal(t, http.StatusFound, authrsp.StatusCode) cookie = authrsp.Header.Get("Set-Cookie") - rsp, err = GetRedirectPageWithCookie(t, httpsListener, c.Host, c.Path, cookie) + rsp, err = GetRedirectPageWithCookie(t, httpsListener, tt.host, tt.path, cookie) require.NoError(t, err) defer rsp.Body.Close() - require.Equal(t, c.Status, rsp.StatusCode) + require.Equal(t, tt.status, rsp.StatusCode) require.Equal(t, "", rsp.Header.Get("Cache-Control")) - if c.RedirectBack { + if tt.redirectBack { url, err = url.Parse(rsp.Header.Get("Location")) require.NoError(t, err) require.Equal(t, "https", url.Scheme) - require.Equal(t, c.Host, url.Host) - require.Equal(t, c.Path, url.Path) + require.Equal(t, tt.host, url.Host) + require.Equal(t, tt.path, url.Path) } }) } -- GitLab From 16c3b26d17024be48b481d7f41e0a4c66fd3d6e5 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 21 Jan 2020 17:55:25 +1100 Subject: [PATCH 02/22] use gorilla/handlers.ProxyHeaders to get the X-Forwarded-* headers and set them in the appropriate http.Request fields --- app.go | 4 +++- go.mod | 1 + go.sum | 2 ++ 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app.go b/app.go index 9d275d294..d86f5c787 100644 --- a/app.go +++ b/app.go @@ -6,6 +6,7 @@ import ( "net/http" "sync" + ghandlers "github.com/gorilla/handlers" "github.com/prometheus/client_golang/prometheus/promhttp" "github.com/rs/cors" log "github.com/sirupsen/logrus" @@ -330,7 +331,8 @@ func (a *theApp) Run() { log.WithError(err).Fatal("Unable to configure pipeline") } - proxyHandler := a.proxyInitialMiddleware(commonHandlerPipeline) + proxyHandler := ghandlers.ProxyHeaders(commonHandlerPipeline) + httpHandler := a.httpInitialMiddleware(commonHandlerPipeline) // Listen for HTTP diff --git a/go.mod b/go.mod index 2ae4178c2..bd66f4b4b 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/getsentry/raven-go v0.1.2 // indirect github.com/golang/mock v1.3.1 github.com/gorilla/context v1.1.1 + github.com/gorilla/handlers v1.4.2 github.com/gorilla/securecookie v1.1.1 github.com/gorilla/sessions v1.2.0 github.com/kardianos/osext v0.0.0-20190222173326-2bc1f35cddc0 diff --git a/go.sum b/go.sum index a4d327397..ba642c223 100644 --- a/go.sum +++ b/go.sum @@ -47,6 +47,8 @@ github.com/google/go-cmp v0.3.0/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMyw github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/gorilla/context v1.1.1 h1:AWwleXJkX/nhcU9bZSnZoi3h/qGYqQAGhq6zZe/aQW8= github.com/gorilla/context v1.1.1/go.mod h1:kBGZzfjB9CEq2AlWe17Uuf7NDRt0dE0s8S51q0aT7Yg= +github.com/gorilla/handlers v1.4.2 h1:0QniY0USkHQ1RGCLfKxeNHK9bkDHGRYGNDFBCS+YARg= +github.com/gorilla/handlers v1.4.2/go.mod h1:Qkdc/uu4tH4g6mTK6auzZ766c4CA0Ng8+o/OAirnOIQ= github.com/gorilla/securecookie v1.1.1 h1:miw7JPhV+b/lAHSXz4qd/nN9jRiAFV5FwjeKyCS8BvQ= github.com/gorilla/securecookie v1.1.1/go.mod h1:ra0sb63/xPlUeL+yeDciTfxMRAA+MP+HVt/4epWDjd4= github.com/gorilla/sessions v1.2.0 h1:S7P+1Hm5V/AT9cjEcUD5uDaQSX0OE577aCXgoaKpYbQ= -- GitLab From c4da6ef61d338c19832b99574eaf2dce383be70e Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 21 Jan 2020 18:00:29 +1100 Subject: [PATCH 03/22] add t.Helper to a test func --- helpers_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/helpers_test.go b/helpers_test.go index b1f5e1b27..06a268b7f 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -208,6 +208,7 @@ func RunPagesProcessWithAuthServerWithSSL(t *testing.T, pagesPath string, listen } func runPagesProcess(t *testing.T, wait bool, pagesPath string, listeners []ListenSpec, promPort string, extraEnv []string, extraArgs ...string) (teardown func()) { + t.Helper() _, err := os.Stat(pagesPath) require.NoError(t, err) -- GitLab From 99a587562f48d5f73ffddb88e20390836098ec75 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 21 Jan 2020 18:01:57 +1100 Subject: [PATCH 04/22] Read https flag from context, fallback to read the r.URL.Scheme. IsHTTPS will not panic anymore --- internal/request/request.go | 13 +++++++++++-- internal/request/request_test.go | 4 ---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/internal/request/request.go b/internal/request/request.go index 06a45071a..189ad5dc8 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -23,9 +23,18 @@ func WithHTTPSFlag(r *http.Request, https bool) *http.Request { return r.WithContext(ctx) } -// IsHTTPS restores https flag from request's context +// IsHTTPS checks wheather the request originated from HTTP or HTTPS. +// It first tries to read the ctxHTTPSKey from the context (for non-proxy requests) +// It fallbacks to check r.URL.Scheme if the request was forwarded by a proxy relying +// on the github.com/gorilla/handlers.ProxyHeaders middleware func IsHTTPS(r *http.Request) bool { - return r.Context().Value(ctxHTTPSKey).(bool) + + https, ok := r.Context().Value(ctxHTTPSKey).(bool) + if !ok { + return r.URL.Scheme == "https" + } + + return https } // WithHostAndDomain saves host name and domain in the request's context diff --git a/internal/request/request_test.go b/internal/request/request_test.go index 642209f0f..f33f0874a 100644 --- a/internal/request/request_test.go +++ b/internal/request/request_test.go @@ -25,10 +25,6 @@ func TestPanics(t *testing.T) { r, err := http.NewRequest("GET", "/", nil) require.NoError(t, err) - require.Panics(t, func() { - IsHTTPS(r) - }) - require.Panics(t, func() { GetHost(r) }) -- GitLab From 0a363cb9191006e4494e27e6ad6e94f7fee528ee Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 21 Jan 2020 18:11:27 +1100 Subject: [PATCH 05/22] Add extra space in helper line --- helpers_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/helpers_test.go b/helpers_test.go index 06a268b7f..29ed87de8 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -209,6 +209,7 @@ func RunPagesProcessWithAuthServerWithSSL(t *testing.T, pagesPath string, listen func runPagesProcess(t *testing.T, wait bool, pagesPath string, listeners []ListenSpec, promPort string, extraEnv []string, extraArgs ...string) (teardown func()) { t.Helper() + _, err := os.Stat(pagesPath) require.NoError(t, err) -- GitLab From 8e3153164caed72b6d064fd26e841ca239b56908 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 22 Jan 2020 10:36:34 +1100 Subject: [PATCH 06/22] Revert "add t.Helper to a test func" This reverts commit c4da6ef61d338c19832b99574eaf2dce383be70e. --- helpers_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/helpers_test.go b/helpers_test.go index 29ed87de8..b1f5e1b27 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -208,8 +208,6 @@ func RunPagesProcessWithAuthServerWithSSL(t *testing.T, pagesPath string, listen } func runPagesProcess(t *testing.T, wait bool, pagesPath string, listeners []ListenSpec, promPort string, extraEnv []string, extraArgs ...string) (teardown func()) { - t.Helper() - _, err := os.Stat(pagesPath) require.NoError(t, err) -- GitLab From 59757dd0d8b08043fe50199fcd2341d07dc0308c Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 22 Jan 2020 10:37:14 +1100 Subject: [PATCH 07/22] Revert "update struct field names for TestAccessControl to follow coding style" This reverts commit d6364c55ab51708b3ec92cde07ffa80fa9b84765. --- acceptance_test.go | 130 ++++++++++++++++++++++----------------------- 1 file changed, 65 insertions(+), 65 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index 380fe31cf..49d391b18 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -1245,92 +1245,92 @@ func TestAccessControl(t *testing.T) { testServer.StartTLS() defer testServer.Close() - tests := []struct { - host string - path string - status int - redirectBack bool - name string + cases := []struct { + Host string + Path string + Status int + RedirectBack bool + Description string }{ { - name: "project with access", - host: "group.auth.gitlab-example.com", - path: "/private.project/", - status: http.StatusOK, - redirectBack: false, + "group.auth.gitlab-example.com", + "/private.project/", + http.StatusOK, + false, + "project with access", }, { - name: "project without access", - host: "group.auth.gitlab-example.com", - path: "/private.project.1/", - status: http.StatusNotFound, // Do not expose project existed - redirectBack: false, + "group.auth.gitlab-example.com", + "/private.project.1/", + http.StatusNotFound, // Do not expose project existed + false, + "project without access", }, { - name: "invalid token test should redirect back", - host: "group.auth.gitlab-example.com", - path: "/private.project.2/", - status: http.StatusFound, - redirectBack: true, + "group.auth.gitlab-example.com", + "/private.project.2/", + http.StatusFound, + true, + "invalid token test should redirect back", }, { - name: "no project should redirect to login and then return 404", - host: "group.auth.gitlab-example.com", - path: "/nonexistent/", - status: http.StatusNotFound, - redirectBack: false, + "group.auth.gitlab-example.com", + "/nonexistent/", + http.StatusNotFound, + false, + "no project should redirect to login and then return 404", }, { - name: "no project should redirect to login and then return 404", - host: "nonexistent.gitlab-example.com", - path: "/nonexistent/", - status: http.StatusNotFound, - redirectBack: false, + "nonexistent.gitlab-example.com", + "/nonexistent/", + http.StatusNotFound, + false, + "no project should redirect to login and then return 404", }, // subgroups { - name: "[subgroup] project with access", - host: "group.auth.gitlab-example.com", - path: "/subgroup/private.project/", - status: http.StatusOK, - redirectBack: false, + "group.auth.gitlab-example.com", + "/subgroup/private.project/", + http.StatusOK, + false, + "[subgroup] project with access", }, { - name: "[subgroup] project without access", - host: "group.auth.gitlab-example.com", - path: "/subgroup/private.project.1/", - status: http.StatusNotFound, // Do not expose project existed - redirectBack: false, + "group.auth.gitlab-example.com", + "/subgroup/private.project.1/", + http.StatusNotFound, // Do not expose project existed + false, + "[subgroup] project without access", }, { - name: "[subgroup] invalid token test should redirect back", - host: "group.auth.gitlab-example.com", - path: "/subgroup/private.project.2/", - status: http.StatusFound, - redirectBack: true, + "group.auth.gitlab-example.com", + "/subgroup/private.project.2/", + http.StatusFound, + true, + "[subgroup] invalid token test should redirect back", }, { - name: "[subgroup] no project should redirect to login and then return 404", - host: "group.auth.gitlab-example.com", - path: "/subgroup/nonexistent/", - status: http.StatusNotFound, - redirectBack: false, + "group.auth.gitlab-example.com", + "/subgroup/nonexistent/", + http.StatusNotFound, + false, + "[subgroup] no project should redirect to login and then return 404", }, { - name: "[subgroup] no project should redirect to login and then return 404", - host: "nonexistent.gitlab-example.com", - path: "/subgroup/nonexistent/", - status: http.StatusNotFound, - redirectBack: false, + "nonexistent.gitlab-example.com", + "/subgroup/nonexistent/", + http.StatusNotFound, + false, + "[subgroup] no project should redirect to login and then return 404", }, } - for _, tt := range tests { + for _, c := range cases { - t.Run(tt.name, func(t *testing.T) { + t.Run(fmt.Sprintf("Access Control Test: %s", c.Description), func(t *testing.T) { teardown := RunPagesProcessWithAuthServerWithSSL(t, *pagesBinary, listeners, "", certFile, testServer.URL) defer teardown() - rsp, err := GetRedirectPage(t, httpsListener, tt.host, tt.path) + rsp, err := GetRedirectPage(t, httpsListener, c.Host, c.Path) require.NoError(t, err) defer rsp.Body.Close() @@ -1363,7 +1363,7 @@ func TestAccessControl(t *testing.T) { // Will redirect auth callback to correct host url, err = url.Parse(authrsp.Header.Get("Location")) require.NoError(t, err) - require.Equal(t, tt.host, url.Host) + require.Equal(t, c.Host, url.Host) require.Equal(t, "/auth", url.Path) // Request auth callback in project domain @@ -1372,21 +1372,21 @@ func TestAccessControl(t *testing.T) { // server returns the ticket, user will be redirected to the project page require.Equal(t, http.StatusFound, authrsp.StatusCode) cookie = authrsp.Header.Get("Set-Cookie") - rsp, err = GetRedirectPageWithCookie(t, httpsListener, tt.host, tt.path, cookie) + rsp, err = GetRedirectPageWithCookie(t, httpsListener, c.Host, c.Path, cookie) require.NoError(t, err) defer rsp.Body.Close() - require.Equal(t, tt.status, rsp.StatusCode) + require.Equal(t, c.Status, rsp.StatusCode) require.Equal(t, "", rsp.Header.Get("Cache-Control")) - if tt.redirectBack { + if c.RedirectBack { url, err = url.Parse(rsp.Header.Get("Location")) require.NoError(t, err) require.Equal(t, "https", url.Scheme) - require.Equal(t, tt.host, url.Host) - require.Equal(t, tt.path, url.Path) + require.Equal(t, c.Host, url.Host) + require.Equal(t, c.Path, url.Path) } }) } -- GitLab From 56eea86f26028e6142efb15d5b0050e174e5b017 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 22 Jan 2020 18:04:05 +1100 Subject: [PATCH 08/22] use both middlewares for proxy requests; log when the scheme cannot be determined --- app.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/app.go b/app.go index d86f5c787..50b3c6fc6 100644 --- a/app.go +++ b/app.go @@ -216,7 +216,10 @@ func (a *theApp) auxiliaryMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { host := request.GetHost(r) domain := request.GetDomain(r) - https := request.IsHTTPS(r) + https, err := request.IsHTTPS(r) + if err != nil { + log.WithError(err).Warn("failed to determine scheme of the request") + } if a.tryAuxiliaryHandlers(w, r, https, host, domain) { return @@ -331,7 +334,7 @@ func (a *theApp) Run() { log.WithError(err).Fatal("Unable to configure pipeline") } - proxyHandler := ghandlers.ProxyHeaders(commonHandlerPipeline) + proxyHandler := a.proxyInitialMiddleware(ghandlers.ProxyHeaders(commonHandlerPipeline)) httpHandler := a.httpInitialMiddleware(commonHandlerPipeline) -- GitLab From 5f9c6a8c3fa4b2af9d5cc7047bdef889e662f3e8 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 22 Jan 2020 18:05:04 +1100 Subject: [PATCH 09/22] check if HTTPS flag exists in the context and that it matches the scheme or the TLS config of a request --- internal/auth/auth.go | 20 +++++++-- internal/logging/logging.go | 6 ++- internal/request/request.go | 45 +++++++++++++++---- internal/request/request_test.go | 75 +++++++++++++++++++++++++++++++- 4 files changed, 131 insertions(+), 15 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index f30c74070..5a9443434 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -77,7 +77,11 @@ func (a *Auth) getSessionFromStore(r *http.Request) (*sessions.Session, error) { // Cookie just for this domain session.Options.Path = "/" session.Options.HttpOnly = true - session.Options.Secure = request.IsHTTPS(r) + secure, err := request.IsHTTPS(r) + if err != nil { + return nil, err + } + session.Options.Secure = secure session.Options.MaxAge = authSessionMaxAge } @@ -289,14 +293,24 @@ func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWrit } func getRequestAddress(r *http.Request) string { - if request.IsHTTPS(r) { + https, err := request.IsHTTPS(r) + if err != nil { + log.WithError(err).Warn("failed to determine if request is HTTPS or not") + } + + if https { return "https://" + r.Host + r.RequestURI } return "http://" + r.Host + r.RequestURI } func getRequestDomain(r *http.Request) string { - if request.IsHTTPS(r) { + https, err := request.IsHTTPS(r) + if err != nil { + log.WithError(err).Warn("failed to determine if request is HTTPS or not") + } + + if https { return "https://" + r.Host } return "http://" + r.Host diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 28c43c2ed..53dcc33a2 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -57,9 +57,13 @@ func getExtraLogFields(r *http.Request) log.Fields { if d := request.GetDomain(r); d != nil { projectID = d.GetProjectID(r) } + https, err := request.IsHTTPS(r) + if err != nil { + log.WithError(err).Warn("failed to determine if request is HTTPS or not") + } return log.Fields{ - "pages_https": request.IsHTTPS(r), + "pages_https": https, "pages_host": request.GetHost(r), "pages_project_id": projectID, } diff --git a/internal/request/request.go b/internal/request/request.go index 189ad5dc8..60829e1d5 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -2,6 +2,7 @@ package request import ( "context" + "errors" "net" "net/http" @@ -11,30 +12,56 @@ import ( type ctxKey string const ( - ctxHTTPSKey ctxKey = "https" + ctxSchemeKey ctxKey = "scheme" ctxHostKey ctxKey = "host" ctxDomainKey ctxKey = "domain" + + schemeHTTP = "http" + schemeHTTPS = "https" ) +// ErrHTTPSKeyNotInCtx error is returned when the `ctxHTTPSKey` is not found in the context +var ErrHTTPSKeyNotInCtx = errors.New("request: https key was not found in the context") + +// ErrSchemeDoesNotMatchCtx error is returned when the value of `ctxHTTPSKey` does not match the value of r.URL.Scheme +var ErrSchemeDoesNotMatchCtx = errors.New("request: scheme in context does not match the r.URL.Scheme") + // WithHTTPSFlag saves https flag in request's context func WithHTTPSFlag(r *http.Request, https bool) *http.Request { - ctx := context.WithValue(r.Context(), ctxHTTPSKey, https) + scheme := schemeHTTP + if https { + scheme = schemeHTTPS + } + ctx := context.WithValue(r.Context(), ctxSchemeKey, scheme) return r.WithContext(ctx) } // IsHTTPS checks wheather the request originated from HTTP or HTTPS. -// It first tries to read the ctxHTTPSKey from the context (for non-proxy requests) -// It fallbacks to check r.URL.Scheme if the request was forwarded by a proxy relying -// on the github.com/gorilla/handlers.ProxyHeaders middleware -func IsHTTPS(r *http.Request) bool { +// It first tries to read the ctxHTTPSKey from the context (for non-proxy requests), +// returns ErrHTTPSKeyNotInCtx if not found +// It also checks that r.URL.Scheme matches the ctxHTTPSKey if it was forwarded by a proxy +// relying on the github.com/gorilla/handlers.ProxyHeaders middleware, +// returns ErrSchemeDoesNotMatchCtx otherwise +func IsHTTPS(r *http.Request) (bool, error) { - https, ok := r.Context().Value(ctxHTTPSKey).(bool) + scheme, ok := r.Context().Value(ctxSchemeKey).(string) if !ok { - return r.URL.Scheme == "https" + return false, ErrHTTPSKeyNotInCtx + } + // r.URL.Scheme is empty if not passed through proxy + if r.URL.Scheme != "" { + if scheme != r.URL.Scheme { + return false, ErrSchemeDoesNotMatchCtx + } + } else { + // the only way to determine if HTTPS is to check TLS https://github.com/golang/go/issues/28940 + if r.TLS != nil && scheme == schemeHTTP { + return false, ErrSchemeDoesNotMatchCtx + } } - return https + return scheme == schemeHTTPS, nil } // WithHostAndDomain saves host name and domain in the request's context diff --git a/internal/request/request_test.go b/internal/request/request_test.go index f33f0874a..e6cf9993b 100644 --- a/internal/request/request_test.go +++ b/internal/request/request_test.go @@ -15,10 +15,70 @@ func TestWithHTTPSFlag(t *testing.T) { require.NoError(t, err) httpsRequest := WithHTTPSFlag(r, true) - require.True(t, IsHTTPS(httpsRequest)) + httpsRequest.URL.Scheme = "https" + + https, err := IsHTTPS(httpsRequest) + require.NoError(t, err) + require.True(t, https) httpRequest := WithHTTPSFlag(r, false) - require.False(t, IsHTTPS(httpRequest)) + httpsRequest.URL.Scheme = "http" + + http, err := IsHTTPS(httpRequest) + require.False(t, http) + +} + +func TestIsHTTPS(t *testing.T) { + tests := []struct { + name string + https bool + scheme string + witHTTPSFlag func(*http.Request, bool) *http.Request + wantErrMsg string + }{ + { + name: "https", + https: true, + scheme: "https", + witHTTPSFlag: WithHTTPSFlag, + }, + { + name: "http", + https: false, + scheme: "http", + witHTTPSFlag: WithHTTPSFlag, + }, + { + name: "not in ctx", + https: false, + scheme: "https", + witHTTPSFlag: func(r *http.Request, b bool) *http.Request { return r }, // do not attach the flag to the context + wantErrMsg: ErrHTTPSKeyNotInCtx.Error(), + }, + { + name: "scheme does not match", + https: false, + scheme: "https", + witHTTPSFlag: WithHTTPSFlag, + wantErrMsg: ErrSchemeDoesNotMatchCtx.Error(), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r, err := http.NewRequest("GET", "/", nil) + require.NoError(t, err) + r.URL.Scheme = tt.scheme + + httpsRequest := tt.witHTTPSFlag(r, tt.https) + + got, err := IsHTTPS(httpsRequest) + requireWantErrMsg(t, err, tt.wantErrMsg) + require.Equal(t, tt.https, got) + }) + } + } func TestPanics(t *testing.T) { @@ -82,3 +142,14 @@ func TestGetHostWithoutPort(t *testing.T) { require.Equal(t, "my.example.com", host) }) } + +// if wantErrMsg is empty an error is NOT required. Otherwise it asserts that the err matches the wantErrMsg +func requireWantErrMsg(t *testing.T, err error, wantErrMsg string) { + t.Helper() + + if wantErrMsg != "" { + require.EqualError(t, err, wantErrMsg) + } else { + require.NoError(t, err) + } +} -- GitLab From a8ee96422431fd07c02ac1f25586e61f1c7bf690 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Thu, 23 Jan 2020 15:52:27 +1100 Subject: [PATCH 10/22] set scheme to r.URL.Scheme inside middleware revert returning error from IsHTTPS --- app.go | 28 +++++++++++++++-------- app_test.go | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 10 deletions(-) create mode 100644 app_test.go diff --git a/app.go b/app.go index 50b3c6fc6..1caa704cd 100644 --- a/app.go +++ b/app.go @@ -216,10 +216,7 @@ func (a *theApp) auxiliaryMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { host := request.GetHost(r) domain := request.GetDomain(r) - https, err := request.IsHTTPS(r) - if err != nil { - log.WithError(err).Warn("failed to determine scheme of the request") - } + https := request.IsHTTPS(r) if a.tryAuxiliaryHandlers(w, r, https, host, domain) { return @@ -275,9 +272,8 @@ func (a *theApp) serveFileOrNotFoundHandler() http.Handler { // httpInitialMiddleware sets up HTTP requests func (a *theApp) httpInitialMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - https := r.TLS != nil - r = request.WithHTTPSFlag(r, https) + setRequestScheme(r) handler.ServeHTTP(w, r) }) } @@ -285,11 +281,10 @@ func (a *theApp) httpInitialMiddleware(handler http.Handler) http.Handler { // proxyInitialMiddleware sets up proxy requests func (a *theApp) proxyInitialMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - forwardedProto := r.Header.Get(xForwardedProto) - https := forwardedProto == xForwardedProtoHTTPS - r = request.WithHTTPSFlag(r, https) - if forwardedHost := r.Header.Get(xForwardedHost); forwardedHost != "" { + setRequestScheme(r) + + if forwardedHost := r.Header.Get(xForwardedHost); forwardedHost != "" && r.Host == "" { r.Host = forwardedHost } @@ -297,6 +292,19 @@ func (a *theApp) proxyInitialMiddleware(handler http.Handler) http.Handler { }) } +// setRequestScheme will update r.URL.Scheme if empty based on r.TLS +func setRequestScheme(r *http.Request) { + + // this might have been set by the ProxyHeaders middleware + if r.URL.Scheme == "" { + if r.TLS != nil { + r.URL.Scheme = request.SchemeHTTPS + } else { + r.URL.Scheme = request.SchemeHTTP + } + } +} + func (a *theApp) buildHandlerPipeline() (http.Handler, error) { // Handlers should be applied in a reverse order handler := a.serveFileOrNotFoundHandler() diff --git a/app_test.go b/app_test.go new file mode 100644 index 000000000..5c976734b --- /dev/null +++ b/app_test.go @@ -0,0 +1,66 @@ +package main + +import ( + "crypto/tls" + "fmt" + "net/http" + "testing" + + "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/request" +) + +func Test_setRequestScheme(t *testing.T) { + + tests := []struct { + name string + r *http.Request + expectedScheme string + }{ + { + name: "http", + r: newGetRequestWithScheme(t, request.SchemeHTTP, false), + expectedScheme: request.SchemeHTTP, + }, + { + name: "https", + r: newGetRequestWithScheme(t, request.SchemeHTTPS, true), + expectedScheme: request.SchemeHTTPS, + }, + { + name: "empty_scheme_no_tls", + r: newGetRequestWithScheme(t, "", false), + expectedScheme: request.SchemeHTTP, + }, + { + name: "empty_scheme_with_tls", + r: newGetRequestWithScheme(t, "", true), + expectedScheme: request.SchemeHTTPS, + }, + { + name: "something_else", + r: newGetRequestWithScheme(t, "random", true), + expectedScheme: "random", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + setRequestScheme(tt.r) + + require.Equal(t, tt.r.URL.Scheme, tt.expectedScheme) + }) + } +} + +func newGetRequestWithScheme(t *testing.T, scheme string, withTLS bool) *http.Request { + t.Helper() + + req, err := http.NewRequest("GET", fmt.Sprintf("%s//localost/", scheme), nil) + require.NoError(t, err) + req.URL.Scheme = scheme + if withTLS { + req.TLS = &tls.ConnectionState{} + } + + return req +} -- GitLab From e9aa3f8d70f4a7b3982cfdd06bab527a30bacf7b Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Thu, 23 Jan 2020 15:53:42 +1100 Subject: [PATCH 11/22] remove error checking from IsHTTPS --- internal/auth/auth.go | 18 +++--------------- internal/logging/logging.go | 6 +----- 2 files changed, 4 insertions(+), 20 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index 5a9443434..fb0b78f48 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -77,11 +77,7 @@ func (a *Auth) getSessionFromStore(r *http.Request) (*sessions.Session, error) { // Cookie just for this domain session.Options.Path = "/" session.Options.HttpOnly = true - secure, err := request.IsHTTPS(r) - if err != nil { - return nil, err - } - session.Options.Secure = secure + session.Options.Secure = request.IsHTTPS(r) session.Options.MaxAge = authSessionMaxAge } @@ -293,24 +289,16 @@ func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWrit } func getRequestAddress(r *http.Request) string { - https, err := request.IsHTTPS(r) - if err != nil { - log.WithError(err).Warn("failed to determine if request is HTTPS or not") - } - if https { + if request.IsHTTPS(r) { return "https://" + r.Host + r.RequestURI } return "http://" + r.Host + r.RequestURI } func getRequestDomain(r *http.Request) string { - https, err := request.IsHTTPS(r) - if err != nil { - log.WithError(err).Warn("failed to determine if request is HTTPS or not") - } - if https { + if request.IsHTTPS(r) { return "https://" + r.Host } return "http://" + r.Host diff --git a/internal/logging/logging.go b/internal/logging/logging.go index 53dcc33a2..28c43c2ed 100644 --- a/internal/logging/logging.go +++ b/internal/logging/logging.go @@ -57,13 +57,9 @@ func getExtraLogFields(r *http.Request) log.Fields { if d := request.GetDomain(r); d != nil { projectID = d.GetProjectID(r) } - https, err := request.IsHTTPS(r) - if err != nil { - log.WithError(err).Warn("failed to determine if request is HTTPS or not") - } return log.Fields{ - "pages_https": https, + "pages_https": request.IsHTTPS(r), "pages_host": request.GetHost(r), "pages_project_id": projectID, } -- GitLab From 46f62daa08f9769fa8fcb471d34d1a113e31c5d6 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Thu, 23 Jan 2020 15:54:42 +1100 Subject: [PATCH 12/22] Add ctxHTTPSKey back to the context Do not return error from IsHTTPS Check both context and r.URL.Scheme when checking if r IsHTTPS Log entries as warnings and update unit tests --- internal/request/request.go | 67 +++++++++--------- internal/request/request_test.go | 116 +++++++++++++++++++++++-------- 2 files changed, 119 insertions(+), 64 deletions(-) diff --git a/internal/request/request.go b/internal/request/request.go index 60829e1d5..3ff50c726 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -2,66 +2,65 @@ package request import ( "context" - "errors" "net" "net/http" + log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" ) type ctxKey string const ( - ctxSchemeKey ctxKey = "scheme" + ctxHTTPSKey ctxKey = "https" ctxHostKey ctxKey = "host" ctxDomainKey ctxKey = "domain" - schemeHTTP = "http" - schemeHTTPS = "https" + // SchemeHTTP name for the HTTP scheme + SchemeHTTP = "http" + // SchemeHTTPS name for the HTTPS scheme + SchemeHTTPS = "https" ) -// ErrHTTPSKeyNotInCtx error is returned when the `ctxHTTPSKey` is not found in the context -var ErrHTTPSKeyNotInCtx = errors.New("request: https key was not found in the context") - -// ErrSchemeDoesNotMatchCtx error is returned when the value of `ctxHTTPSKey` does not match the value of r.URL.Scheme -var ErrSchemeDoesNotMatchCtx = errors.New("request: scheme in context does not match the r.URL.Scheme") - // WithHTTPSFlag saves https flag in request's context func WithHTTPSFlag(r *http.Request, https bool) *http.Request { - scheme := schemeHTTP - if https { - scheme = schemeHTTPS - } - ctx := context.WithValue(r.Context(), ctxSchemeKey, scheme) + ctx := context.WithValue(r.Context(), ctxHTTPSKey, https) return r.WithContext(ctx) } // IsHTTPS checks wheather the request originated from HTTP or HTTPS. -// It first tries to read the ctxHTTPSKey from the context (for non-proxy requests), -// returns ErrHTTPSKeyNotInCtx if not found -// It also checks that r.URL.Scheme matches the ctxHTTPSKey if it was forwarded by a proxy -// relying on the github.com/gorilla/handlers.ProxyHeaders middleware, -// returns ErrSchemeDoesNotMatchCtx otherwise -func IsHTTPS(r *http.Request) (bool, error) { - - scheme, ok := r.Context().Value(ctxSchemeKey).(string) - if !ok { - return false, ErrHTTPSKeyNotInCtx +// It tries to read the ctxHTTPSKey from the context (for non-proxy requests), +// It also checks that r.URL.Scheme matches the value in ctxHTTPSKey +// returns value based on either the ctxHTTPSKey if r.URL.Scheme is empty +// or whether r.URL.Scheme matches http/https +// TODO: remove the ctxHTTPSKey from the context +func IsHTTPS(r *http.Request) bool { + + https, ctxKeyExists := r.Context().Value(ctxHTTPSKey).(bool) + if !ctxKeyExists { + log.Warn("request: ctxSchemeKey is empty") } - // r.URL.Scheme is empty if not passed through proxy - if r.URL.Scheme != "" { - if scheme != r.URL.Scheme { - return false, ErrSchemeDoesNotMatchCtx + if r.URL.Scheme == SchemeHTTPS { + if !https && ctxKeyExists { + log.Warn("request: r.URL.Scheme does not match value in ctxHTTPSKey") } - } else { - // the only way to determine if HTTPS is to check TLS https://github.com/golang/go/issues/28940 - if r.TLS != nil && scheme == schemeHTTP { - return false, ErrSchemeDoesNotMatchCtx + + return true + + } else if r.URL.Scheme == SchemeHTTP { + if https && ctxKeyExists { + log.Warn("request: r.URL.Scheme does not match value in ctxHTTPSKey") } + + return false + + } else if r.URL.Scheme == "" { + log.Warn("request: r.URL.Scheme is empty") } - return scheme == schemeHTTPS, nil + return https + } // WithHostAndDomain saves host name and domain in the request's context diff --git a/internal/request/request_test.go b/internal/request/request_test.go index e6cf9993b..1cf9c905b 100644 --- a/internal/request/request_test.go +++ b/internal/request/request_test.go @@ -5,6 +5,10 @@ import ( "net/http/httptest" "testing" + "github.com/sirupsen/logrus" + "github.com/sirupsen/logrus/hooks/test" + + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" @@ -15,67 +19,94 @@ func TestWithHTTPSFlag(t *testing.T) { require.NoError(t, err) httpsRequest := WithHTTPSFlag(r, true) - httpsRequest.URL.Scheme = "https" - - https, err := IsHTTPS(httpsRequest) - require.NoError(t, err) - require.True(t, https) + httpsRequest.URL.Scheme = SchemeHTTPS + require.True(t, IsHTTPS(httpsRequest)) httpRequest := WithHTTPSFlag(r, false) - httpsRequest.URL.Scheme = "http" - - http, err := IsHTTPS(httpRequest) - require.False(t, http) + httpsRequest.URL.Scheme = SchemeHTTP + require.False(t, IsHTTPS(httpRequest)) } func TestIsHTTPS(t *testing.T) { + hook := test.NewGlobal() + tests := []struct { - name string - https bool - scheme string - witHTTPSFlag func(*http.Request, bool) *http.Request - wantErrMsg string + name string + flag bool + scheme string + witHTTPSFlag func(*http.Request, bool) *http.Request + wantHTTPS bool + wantLogEntries []string }{ { name: "https", - https: true, + flag: true, scheme: "https", witHTTPSFlag: WithHTTPSFlag, + wantHTTPS: true, }, { name: "http", - https: false, + flag: false, scheme: "http", witHTTPSFlag: WithHTTPSFlag, + wantHTTPS: false, }, { - name: "not in ctx", - https: false, - scheme: "https", - witHTTPSFlag: func(r *http.Request, b bool) *http.Request { return r }, // do not attach the flag to the context - wantErrMsg: ErrHTTPSKeyNotInCtx.Error(), + name: "not in ctx", + flag: true, + scheme: "https", + witHTTPSFlag: func(r *http.Request, b bool) *http.Request { return r }, // do not attach the flag to the context + wantHTTPS: true, + wantLogEntries: []string{"request: ctxSchemeKey is empty"}, }, { - name: "scheme does not match", - https: false, - scheme: "https", - witHTTPSFlag: WithHTTPSFlag, - wantErrMsg: ErrSchemeDoesNotMatchCtx.Error(), + name: "scheme true but flag is false", + flag: false, + scheme: "https", + witHTTPSFlag: WithHTTPSFlag, + wantHTTPS: true, + wantLogEntries: []string{"request: r.URL.Scheme does not match value in ctxHTTPSKey"}, + }, + { + name: "scheme false but flag is true", + flag: true, + scheme: "http", + witHTTPSFlag: WithHTTPSFlag, + wantHTTPS: false, + wantLogEntries: []string{"request: r.URL.Scheme does not match value in ctxHTTPSKey"}, + }, + { + name: "scheme is empty", + flag: true, + scheme: "", + witHTTPSFlag: WithHTTPSFlag, + wantHTTPS: true, + wantLogEntries: []string{"request: r.URL.Scheme is empty"}, + }, + { + name: "scheme is empty and not in ctx", + scheme: "", + witHTTPSFlag: func(r *http.Request, b bool) *http.Request { return r }, // do not attach the flag to the context + wantHTTPS: false, + wantLogEntries: []string{"request: ctxSchemeKey is empty", "request: ctxSchemeKey is empty"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + hook.Reset() r, err := http.NewRequest("GET", "/", nil) require.NoError(t, err) r.URL.Scheme = tt.scheme - httpsRequest := tt.witHTTPSFlag(r, tt.https) + httpsRequest := tt.witHTTPSFlag(r, tt.flag) + + got := IsHTTPS(httpsRequest) + require.Equal(t, tt.wantHTTPS, got) - got, err := IsHTTPS(httpsRequest) - requireWantErrMsg(t, err, tt.wantErrMsg) - require.Equal(t, tt.https, got) + assertLogEntries(t, tt.wantLogEntries, hook.AllEntries()) }) } @@ -153,3 +184,28 @@ func requireWantErrMsg(t *testing.T, err error, wantErrMsg string) { require.NoError(t, err) } } + +// assertLogEntries checks that wantLogEntries exist in a slice of entries +func assertLogEntries(t *testing.T, wantLogEntries []string, entries []*logrus.Entry) { + if len(wantLogEntries) > 0 { + messages := make([]string, len(entries)) + + for k, entry := range entries { + messages[k] = entry.Message + } + require.True(t, assertContainsElement(t, wantLogEntries, messages)) + } +} + +// assertContainsElement checks that a slice contains at least one element of another slice +func assertContainsElement(t *testing.T, a, b []string) bool { + t.Helper() + + for _, x := range a { + if assert.Contains(t, b, x) { + return true + } + } + + return false +} -- GitLab From cb86bf072d31783390a825a4aa009a7e404665e4 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Thu, 23 Jan 2020 16:06:30 +1100 Subject: [PATCH 13/22] run goimports --- app_test.go | 1 + internal/request/request.go | 1 + internal/request/request_test.go | 1 - 3 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app_test.go b/app_test.go index 5c976734b..aa8a30df9 100644 --- a/app_test.go +++ b/app_test.go @@ -7,6 +7,7 @@ import ( "testing" "github.com/stretchr/testify/require" + "gitlab.com/gitlab-org/gitlab-pages/internal/request" ) diff --git a/internal/request/request.go b/internal/request/request.go index 3ff50c726..7bcb7fd31 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -6,6 +6,7 @@ import ( "net/http" log "github.com/sirupsen/logrus" + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" ) diff --git a/internal/request/request_test.go b/internal/request/request_test.go index 1cf9c905b..c8aee4b63 100644 --- a/internal/request/request_test.go +++ b/internal/request/request_test.go @@ -7,7 +7,6 @@ import ( "github.com/sirupsen/logrus" "github.com/sirupsen/logrus/hooks/test" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" -- GitLab From 075778c0c77cc4b773f37389b2b2dc9b029d06f6 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Fri, 24 Jan 2020 11:18:36 +1100 Subject: [PATCH 14/22] rollback flag setting in proxy middleware set the scheme for non-proxy requests update the setRequestScheme test --- app.go | 30 ++++++++++++++++-------------- app_test.go | 10 ++-------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/app.go b/app.go index 1caa704cd..414e3749e 100644 --- a/app.go +++ b/app.go @@ -273,18 +273,18 @@ func (a *theApp) serveFileOrNotFoundHandler() http.Handler { func (a *theApp) httpInitialMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - setRequestScheme(r) - handler.ServeHTTP(w, r) + handler.ServeHTTP(w, setRequestScheme(r)) }) } // proxyInitialMiddleware sets up proxy requests func (a *theApp) proxyInitialMiddleware(handler http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + forwardedProto := r.Header.Get(xForwardedProto) + https := forwardedProto == xForwardedProtoHTTPS - setRequestScheme(r) - - if forwardedHost := r.Header.Get(xForwardedHost); forwardedHost != "" && r.Host == "" { + r = request.WithHTTPSFlag(r, https) + if forwardedHost := r.Header.Get(xForwardedHost); forwardedHost != "" { r.Host = forwardedHost } @@ -293,16 +293,18 @@ func (a *theApp) proxyInitialMiddleware(handler http.Handler) http.Handler { } // setRequestScheme will update r.URL.Scheme if empty based on r.TLS -func setRequestScheme(r *http.Request) { - - // this might have been set by the ProxyHeaders middleware - if r.URL.Scheme == "" { - if r.TLS != nil { - r.URL.Scheme = request.SchemeHTTPS - } else { - r.URL.Scheme = request.SchemeHTTP - } +func setRequestScheme(r *http.Request) *http.Request { + + https := false + if r.URL.Scheme == request.SchemeHTTPS || r.TLS != nil { + // make sure is set for non-proxy requests + r.URL.Scheme = request.SchemeHTTPS + https = true + } else { + r.URL.Scheme = request.SchemeHTTP } + + return request.WithHTTPSFlag(r, https) } func (a *theApp) buildHandlerPipeline() (http.Handler, error) { diff --git a/app_test.go b/app_test.go index aa8a30df9..0e778eeda 100644 --- a/app_test.go +++ b/app_test.go @@ -38,17 +38,11 @@ func Test_setRequestScheme(t *testing.T) { r: newGetRequestWithScheme(t, "", true), expectedScheme: request.SchemeHTTPS, }, - { - name: "something_else", - r: newGetRequestWithScheme(t, "random", true), - expectedScheme: "random", - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - setRequestScheme(tt.r) - - require.Equal(t, tt.r.URL.Scheme, tt.expectedScheme) + got := setRequestScheme(tt.r) + require.Equal(t, got.URL.Scheme, tt.expectedScheme) }) } } -- GitLab From ead682fb26f3afd4123197617095a21d6131af2e Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Fri, 24 Jan 2020 11:25:05 +1100 Subject: [PATCH 15/22] simplify IsHTTPS function and update tests accordingly --- internal/auth/auth.go | 2 -- internal/request/request.go | 30 +++++------------- internal/request/request_test.go | 53 ++++++++------------------------ 3 files changed, 20 insertions(+), 65 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index fb0b78f48..f30c74070 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -289,7 +289,6 @@ func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWrit } func getRequestAddress(r *http.Request) string { - if request.IsHTTPS(r) { return "https://" + r.Host + r.RequestURI } @@ -297,7 +296,6 @@ func getRequestAddress(r *http.Request) string { } func getRequestDomain(r *http.Request) string { - if request.IsHTTPS(r) { return "https://" + r.Host } diff --git a/internal/request/request.go b/internal/request/request.go index 7bcb7fd31..b95ccc50e 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -37,31 +37,17 @@ func WithHTTPSFlag(r *http.Request, https bool) *http.Request { // or whether r.URL.Scheme matches http/https // TODO: remove the ctxHTTPSKey from the context func IsHTTPS(r *http.Request) bool { + https := r.Context().Value(ctxHTTPSKey).(bool) - https, ctxKeyExists := r.Context().Value(ctxHTTPSKey).(bool) - if !ctxKeyExists { - log.Warn("request: ctxSchemeKey is empty") + if https != (r.URL.Scheme == SchemeHTTPS) { + log.WithFields(log.Fields{ + "ctxHTTPSKey": https, + "scheme": r.URL.Scheme, + }).Error("request: r.URL.Scheme does not match value in ctxHTTPSKey") } - if r.URL.Scheme == SchemeHTTPS { - if !https && ctxKeyExists { - log.Warn("request: r.URL.Scheme does not match value in ctxHTTPSKey") - } - - return true - - } else if r.URL.Scheme == SchemeHTTP { - if https && ctxKeyExists { - log.Warn("request: r.URL.Scheme does not match value in ctxHTTPSKey") - } - - return false - - } else if r.URL.Scheme == "" { - log.Warn("request: r.URL.Scheme is empty") - } - + // Returning the value of ctxHTTPSKey for now, can just switch to r.URL.Scheme == SchemeHTTPS later + // and later can remove IsHTTPS method altogether return https - } // WithHostAndDomain saves host name and domain in the request's context diff --git a/internal/request/request_test.go b/internal/request/request_test.go index c8aee4b63..fa9e291cb 100644 --- a/internal/request/request_test.go +++ b/internal/request/request_test.go @@ -34,63 +34,30 @@ func TestIsHTTPS(t *testing.T) { name string flag bool scheme string - witHTTPSFlag func(*http.Request, bool) *http.Request - wantHTTPS bool wantLogEntries []string }{ { - name: "https", - flag: true, - scheme: "https", - witHTTPSFlag: WithHTTPSFlag, - wantHTTPS: true, + name: "https", + flag: true, + scheme: "https", }, { - name: "http", - flag: false, - scheme: "http", - witHTTPSFlag: WithHTTPSFlag, - wantHTTPS: false, - }, - { - name: "not in ctx", - flag: true, - scheme: "https", - witHTTPSFlag: func(r *http.Request, b bool) *http.Request { return r }, // do not attach the flag to the context - wantHTTPS: true, - wantLogEntries: []string{"request: ctxSchemeKey is empty"}, + name: "http", + flag: false, + scheme: "http", }, { name: "scheme true but flag is false", flag: false, scheme: "https", - witHTTPSFlag: WithHTTPSFlag, - wantHTTPS: true, wantLogEntries: []string{"request: r.URL.Scheme does not match value in ctxHTTPSKey"}, }, { name: "scheme false but flag is true", flag: true, scheme: "http", - witHTTPSFlag: WithHTTPSFlag, - wantHTTPS: false, wantLogEntries: []string{"request: r.URL.Scheme does not match value in ctxHTTPSKey"}, }, - { - name: "scheme is empty", - flag: true, - scheme: "", - witHTTPSFlag: WithHTTPSFlag, - wantHTTPS: true, - wantLogEntries: []string{"request: r.URL.Scheme is empty"}, - }, - { - name: "scheme is empty and not in ctx", - scheme: "", - witHTTPSFlag: func(r *http.Request, b bool) *http.Request { return r }, // do not attach the flag to the context - wantHTTPS: false, - wantLogEntries: []string{"request: ctxSchemeKey is empty", "request: ctxSchemeKey is empty"}, - }, } for _, tt := range tests { @@ -100,10 +67,10 @@ func TestIsHTTPS(t *testing.T) { require.NoError(t, err) r.URL.Scheme = tt.scheme - httpsRequest := tt.witHTTPSFlag(r, tt.flag) + httpsRequest := WithHTTPSFlag(r, tt.flag) got := IsHTTPS(httpsRequest) - require.Equal(t, tt.wantHTTPS, got) + require.Equal(t, tt.flag, got) assertLogEntries(t, tt.wantLogEntries, hook.AllEntries()) }) @@ -115,6 +82,10 @@ func TestPanics(t *testing.T) { r, err := http.NewRequest("GET", "/", nil) require.NoError(t, err) + require.Panics(t, func() { + IsHTTPS(r) + }) + require.Panics(t, func() { GetHost(r) }) -- GitLab From cfe3623316dc4efd9e1b7fdfc96dcc0e232ffe92 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Fri, 24 Jan 2020 13:40:12 +1100 Subject: [PATCH 16/22] make comment on IsHTTPS more accurate --- internal/request/request.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/request/request.go b/internal/request/request.go index b95ccc50e..71f73dfd4 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -31,10 +31,8 @@ func WithHTTPSFlag(r *http.Request, https bool) *http.Request { } // IsHTTPS checks wheather the request originated from HTTP or HTTPS. -// It tries to read the ctxHTTPSKey from the context (for non-proxy requests), -// It also checks that r.URL.Scheme matches the value in ctxHTTPSKey -// returns value based on either the ctxHTTPSKey if r.URL.Scheme is empty -// or whether r.URL.Scheme matches http/https +// It reads the ctxHTTPSKey from the context and returns its value +// It also checks that r.URL.Scheme matches the value in ctxHTTPSKey for HTTPS requests // TODO: remove the ctxHTTPSKey from the context func IsHTTPS(r *http.Request) bool { https := r.Context().Value(ctxHTTPSKey).(bool) -- GitLab From f04e021b1101beb38f6e7822277a70846685ea03 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 28 Jan 2020 14:54:47 +1100 Subject: [PATCH 17/22] Add AssertLogContains to testhelpers Fix somse styling issues Make IsHTTPS test simpler --- app.go | 1 - app_test.go | 1 - internal/request/request_test.go | 48 ++++------------------------- internal/testhelpers/testhelpers.go | 15 +++++++++ 4 files changed, 21 insertions(+), 44 deletions(-) diff --git a/app.go b/app.go index 414e3749e..312d171fe 100644 --- a/app.go +++ b/app.go @@ -294,7 +294,6 @@ func (a *theApp) proxyInitialMiddleware(handler http.Handler) http.Handler { // setRequestScheme will update r.URL.Scheme if empty based on r.TLS func setRequestScheme(r *http.Request) *http.Request { - https := false if r.URL.Scheme == request.SchemeHTTPS || r.TLS != nil { // make sure is set for non-proxy requests diff --git a/app_test.go b/app_test.go index 0e778eeda..d78d67370 100644 --- a/app_test.go +++ b/app_test.go @@ -12,7 +12,6 @@ import ( ) func Test_setRequestScheme(t *testing.T) { - tests := []struct { name string r *http.Request diff --git a/internal/request/request_test.go b/internal/request/request_test.go index fa9e291cb..3c0f970c0 100644 --- a/internal/request/request_test.go +++ b/internal/request/request_test.go @@ -5,12 +5,11 @@ import ( "net/http/httptest" "testing" - "github.com/sirupsen/logrus" "github.com/sirupsen/logrus/hooks/test" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitlab-pages/internal/domain" + "gitlab.com/gitlab-org/gitlab-pages/internal/testhelpers" ) func TestWithHTTPSFlag(t *testing.T) { @@ -34,7 +33,7 @@ func TestIsHTTPS(t *testing.T) { name string flag bool scheme string - wantLogEntries []string + wantLogEntries string }{ { name: "https", @@ -50,19 +49,20 @@ func TestIsHTTPS(t *testing.T) { name: "scheme true but flag is false", flag: false, scheme: "https", - wantLogEntries: []string{"request: r.URL.Scheme does not match value in ctxHTTPSKey"}, + wantLogEntries: "request: r.URL.Scheme does not match value in ctxHTTPSKey", }, { name: "scheme false but flag is true", flag: true, scheme: "http", - wantLogEntries: []string{"request: r.URL.Scheme does not match value in ctxHTTPSKey"}, + wantLogEntries: "request: r.URL.Scheme does not match value in ctxHTTPSKey", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { hook.Reset() + r, err := http.NewRequest("GET", "/", nil) require.NoError(t, err) r.URL.Scheme = tt.scheme @@ -72,7 +72,7 @@ func TestIsHTTPS(t *testing.T) { got := IsHTTPS(httpsRequest) require.Equal(t, tt.flag, got) - assertLogEntries(t, tt.wantLogEntries, hook.AllEntries()) + testhelpers.AssertLogContains(t, tt.wantLogEntries, hook.AllEntries()) }) } @@ -143,39 +143,3 @@ func TestGetHostWithoutPort(t *testing.T) { require.Equal(t, "my.example.com", host) }) } - -// if wantErrMsg is empty an error is NOT required. Otherwise it asserts that the err matches the wantErrMsg -func requireWantErrMsg(t *testing.T, err error, wantErrMsg string) { - t.Helper() - - if wantErrMsg != "" { - require.EqualError(t, err, wantErrMsg) - } else { - require.NoError(t, err) - } -} - -// assertLogEntries checks that wantLogEntries exist in a slice of entries -func assertLogEntries(t *testing.T, wantLogEntries []string, entries []*logrus.Entry) { - if len(wantLogEntries) > 0 { - messages := make([]string, len(entries)) - - for k, entry := range entries { - messages[k] = entry.Message - } - require.True(t, assertContainsElement(t, wantLogEntries, messages)) - } -} - -// assertContainsElement checks that a slice contains at least one element of another slice -func assertContainsElement(t *testing.T, a, b []string) bool { - t.Helper() - - for _, x := range a { - if assert.Contains(t, b, x) { - return true - } - } - - return false -} diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index c0b87d93b..d2c08d425 100644 --- a/internal/testhelpers/testhelpers.go +++ b/internal/testhelpers/testhelpers.go @@ -7,6 +7,7 @@ import ( "net/url" "testing" + "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" ) @@ -40,4 +41,18 @@ func AssertRedirectTo(t *testing.T, handler http.HandlerFunc, method string, handler(recorder, req) require.Equal(t, expectedURL, recorder.Header().Get("Location")) + +} + +// AssertLogContains checks that wantLogEntry is contained in at least one of the log entries +func AssertLogContains(t *testing.T, wantLogEntry string, entries []*logrus.Entry) { + t.Helper() + + if wantLogEntry != "" { + messages := make([]string, len(entries)) + for k, entry := range entries { + messages[k] = entry.Message + } + require.Contains(t, messages, wantLogEntry) + } } -- GitLab From 00b80103dc06be98f6b41a4ad7c0284d92168e45 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 28 Jan 2020 15:08:15 +1100 Subject: [PATCH 18/22] fix typo in function comment --- internal/request/request.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/request/request.go b/internal/request/request.go index 71f73dfd4..e4f9777a9 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -30,7 +30,7 @@ func WithHTTPSFlag(r *http.Request, https bool) *http.Request { return r.WithContext(ctx) } -// IsHTTPS checks wheather the request originated from HTTP or HTTPS. +// IsHTTPS checks whether the request originated from HTTP or HTTPS. // It reads the ctxHTTPSKey from the context and returns its value // It also checks that r.URL.Scheme matches the value in ctxHTTPSKey for HTTPS requests // TODO: remove the ctxHTTPSKey from the context -- GitLab From a55d418509667c1cfa6ee277312e1ba64766dd65 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 29 Jan 2020 09:27:05 +1100 Subject: [PATCH 19/22] Add link to issue to remove the ctxHTTPSKey from the context --- internal/request/request.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/request/request.go b/internal/request/request.go index e4f9777a9..2bcb7a319 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -33,7 +33,7 @@ func WithHTTPSFlag(r *http.Request, https bool) *http.Request { // IsHTTPS checks whether the request originated from HTTP or HTTPS. // It reads the ctxHTTPSKey from the context and returns its value // It also checks that r.URL.Scheme matches the value in ctxHTTPSKey for HTTPS requests -// TODO: remove the ctxHTTPSKey from the context +// TODO: remove the ctxHTTPSKey from the context https://gitlab.com/gitlab-org/gitlab-pages/issues/219 func IsHTTPS(r *http.Request) bool { https := r.Context().Value(ctxHTTPSKey).(bool) -- GitLab From 01cbdf3dbcc9ee54b6c11bf765067f90042cde06 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Thu, 30 Jan 2020 11:33:08 +1100 Subject: [PATCH 20/22] improve readability --- internal/request/request.go | 3 ++- internal/testhelpers/testhelpers.go | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/request/request.go b/internal/request/request.go index 2bcb7a319..ba56f45b8 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -41,8 +41,9 @@ func IsHTTPS(r *http.Request) bool { log.WithFields(log.Fields{ "ctxHTTPSKey": https, "scheme": r.URL.Scheme, - }).Error("request: r.URL.Scheme does not match value in ctxHTTPSKey") + }).Warn("request: r.URL.Scheme does not match value in ctxHTTPSKey") } + // Returning the value of ctxHTTPSKey for now, can just switch to r.URL.Scheme == SchemeHTTPS later // and later can remove IsHTTPS method altogether return https diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index d2c08d425..d703769bc 100644 --- a/internal/testhelpers/testhelpers.go +++ b/internal/testhelpers/testhelpers.go @@ -53,6 +53,7 @@ func AssertLogContains(t *testing.T, wantLogEntry string, entries []*logrus.Entr for k, entry := range entries { messages[k] = entry.Message } + require.Contains(t, messages, wantLogEntry) } } -- GitLab From 08f468a420b4a3286f22adffd5e3031bfccea79d Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Thu, 30 Jan 2020 11:49:26 +1100 Subject: [PATCH 21/22] add note about ProxyHeaders middleware to documentation --- README.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/README.md b/README.md index 0c26c835d..73fcd2af0 100644 --- a/README.md +++ b/README.md @@ -177,6 +177,16 @@ $ ./gitlab-pages -listen-http "10.0.0.1:8080" -listen-https "[fd00::1]:8080" -pa This is most useful in dual-stack environments (IPv4+IPv6) where both Gitlab Pages and another HTTP server have to co-exist on the same server. + +#### Listening behind a reverse proxy + +When `listen-proxy` is used please make sure that your reverse proxy solution is configured to strip the [RFC7239 Forwarded headers](https://tools.ietf.org/html/rfc7239). + +The `gorilla/handlers.ProxyHeaders` middleware is used when listening behind a proxy via `listen-proxy` configuration option. For more information please review the [gorilla/handlers#ProxyHeaders](https://godoc.org/github.com/gorilla/handlers#ProxyHeaders) documentation. + +> NOTE: This middleware should only be used when behind a reverse proxy like nginx, HAProxy or Apache. Reverse proxies that don't (or are configured not to) strip these headers from client requests, or where these headers are accepted "as is" from a remote client (e.g. when Go is not behind a proxy), can manifest as a vulnerability if your application uses these headers for validating the 'trustworthiness' of a request. + + ### GitLab access control GitLab access control is configured with properties `auth-client-id`, `auth-client-secret`, `auth-redirect-uri`, `auth-server` and `auth-secret`. Client ID, secret and redirect uri are configured in the GitLab and should match. `auth-server` points to a GitLab instance used for authentication. `auth-redirect-uri` should be `http(s)://pages-domain/auth`. Note that if the pages-domain is not handled by GitLab pages, then the `auth-redirect-uri` should use some reserved namespace prefix (such as `http(s)://projects.pages-domain/auth`). Using HTTPS is _strongly_ encouraged. `auth-secret` is used to encrypt the session cookie, and it should be strong enough. -- GitLab From f3eb044f274edaca2787cb6e2af43ade7a0e20e5 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Thu, 30 Jan 2020 11:50:48 +1100 Subject: [PATCH 22/22] remove extra new line --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index 73fcd2af0..dd350265a 100644 --- a/README.md +++ b/README.md @@ -186,7 +186,6 @@ The `gorilla/handlers.ProxyHeaders` middleware is used when listening behind a p > NOTE: This middleware should only be used when behind a reverse proxy like nginx, HAProxy or Apache. Reverse proxies that don't (or are configured not to) strip these headers from client requests, or where these headers are accepted "as is" from a remote client (e.g. when Go is not behind a proxy), can manifest as a vulnerability if your application uses these headers for validating the 'trustworthiness' of a request. - ### GitLab access control GitLab access control is configured with properties `auth-client-id`, `auth-client-secret`, `auth-redirect-uri`, `auth-server` and `auth-secret`. Client ID, secret and redirect uri are configured in the GitLab and should match. `auth-server` points to a GitLab instance used for authentication. `auth-redirect-uri` should be `http(s)://pages-domain/auth`. Note that if the pages-domain is not handled by GitLab pages, then the `auth-redirect-uri` should use some reserved namespace prefix (such as `http(s)://projects.pages-domain/auth`). Using HTTPS is _strongly_ encouraged. `auth-secret` is used to encrypt the session cookie, and it should be strong enough. -- GitLab