diff options
author | Alessio Caiazza <acaiazza@gitlab.com> | 2020-02-03 18:58:19 +0300 |
---|---|---|
committer | Alessio Caiazza <acaiazza@gitlab.com> | 2020-02-03 18:58:19 +0300 |
commit | 711b882cdd7790281e431de1d77784ba53063a99 (patch) | |
tree | 3b8cd1c6a7f6b888247d913dccd3ba593f22ecdf | |
parent | 57a8b1184da1d53184aad5d4f0bedea51d330b1a (diff) | |
parent | f6bb22c68b2dfb5b5162335b6ffda813b79e8426 (diff) |
Merge branch '219-use-proxy-headers-https' into 'master'
Set the r.URL.Scheme via middleware and log mismatches with the https flag in the context
See merge request gitlab-org/gitlab-pages!225
-rw-r--r-- | README.md | 9 | ||||
-rw-r--r-- | app.go | 22 | ||||
-rw-r--r-- | app_test.go | 60 | ||||
-rw-r--r-- | go.mod | 1 | ||||
-rw-r--r-- | go.sum | 2 | ||||
-rw-r--r-- | internal/request/request.go | 25 | ||||
-rw-r--r-- | internal/request/request_test.go | 57 | ||||
-rw-r--r-- | internal/testhelpers/testhelpers.go | 16 |
8 files changed, 186 insertions, 6 deletions
@@ -177,6 +177,15 @@ $ ./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. @@ -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" @@ -271,10 +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) - handler.ServeHTTP(w, r) + handler.ServeHTTP(w, setRequestScheme(r)) }) } @@ -293,6 +292,20 @@ 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 + 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) { // Handlers should be applied in a reverse order handler := a.serveFileOrNotFoundHandler() @@ -330,7 +343,8 @@ func (a *theApp) Run() { log.WithError(err).Fatal("Unable to configure pipeline") } - proxyHandler := a.proxyInitialMiddleware(commonHandlerPipeline) + proxyHandler := a.proxyInitialMiddleware(ghandlers.ProxyHeaders(commonHandlerPipeline)) + httpHandler := a.httpInitialMiddleware(commonHandlerPipeline) // Listen for HTTP diff --git a/app_test.go b/app_test.go new file mode 100644 index 00000000..d78d6737 --- /dev/null +++ b/app_test.go @@ -0,0 +1,60 @@ +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, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := setRequestScheme(tt.r) + require.Equal(t, got.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 +} @@ -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 @@ -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= diff --git a/internal/request/request.go b/internal/request/request.go index 06a45071..ba56f45b 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -5,6 +5,8 @@ import ( "net" "net/http" + log "github.com/sirupsen/logrus" + "gitlab.com/gitlab-org/gitlab-pages/internal/domain" ) @@ -14,6 +16,11 @@ const ( ctxHTTPSKey ctxKey = "https" ctxHostKey ctxKey = "host" ctxDomainKey ctxKey = "domain" + + // SchemeHTTP name for the HTTP scheme + SchemeHTTP = "http" + // SchemeHTTPS name for the HTTPS scheme + SchemeHTTPS = "https" ) // WithHTTPSFlag saves https flag in request's context @@ -23,9 +30,23 @@ func WithHTTPSFlag(r *http.Request, https bool) *http.Request { return r.WithContext(ctx) } -// IsHTTPS restores https flag from request's context +// 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 https://gitlab.com/gitlab-org/gitlab-pages/issues/219 func IsHTTPS(r *http.Request) bool { - return r.Context().Value(ctxHTTPSKey).(bool) + https := r.Context().Value(ctxHTTPSKey).(bool) + + if https != (r.URL.Scheme == SchemeHTTPS) { + log.WithFields(log.Fields{ + "ctxHTTPSKey": https, + "scheme": r.URL.Scheme, + }).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 } // 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 642209f0..3c0f970c 100644 --- a/internal/request/request_test.go +++ b/internal/request/request_test.go @@ -5,9 +5,11 @@ import ( "net/http/httptest" "testing" + "github.com/sirupsen/logrus/hooks/test" "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) { @@ -15,10 +17,65 @@ func TestWithHTTPSFlag(t *testing.T) { require.NoError(t, err) httpsRequest := WithHTTPSFlag(r, true) + httpsRequest.URL.Scheme = SchemeHTTPS require.True(t, IsHTTPS(httpsRequest)) httpRequest := WithHTTPSFlag(r, false) + httpsRequest.URL.Scheme = SchemeHTTP require.False(t, IsHTTPS(httpRequest)) + +} + +func TestIsHTTPS(t *testing.T) { + hook := test.NewGlobal() + + tests := []struct { + name string + flag bool + scheme string + wantLogEntries string + }{ + { + name: "https", + flag: true, + scheme: "https", + }, + { + name: "http", + flag: false, + scheme: "http", + }, + { + name: "scheme true but flag is false", + flag: false, + scheme: "https", + wantLogEntries: "request: r.URL.Scheme does not match value in ctxHTTPSKey", + }, + { + name: "scheme false but flag is true", + flag: true, + scheme: "http", + 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 + + httpsRequest := WithHTTPSFlag(r, tt.flag) + + got := IsHTTPS(httpsRequest) + require.Equal(t, tt.flag, got) + + testhelpers.AssertLogContains(t, tt.wantLogEntries, hook.AllEntries()) + }) + } + } func TestPanics(t *testing.T) { diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index c0b87d93..d703769b 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,19 @@ 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) + } } |