diff --git a/README.md b/README.md index 0c26c835dca974dfa66eb3cf71ab226cf41dd77d..dd350265a2c36bd0c19a789e6a2aca3174a47854 100644 --- a/README.md +++ b/README.md @@ -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. diff --git a/app.go b/app.go index 9d275d294af5786d1d24098648cb9fbcdb5e8ba9..312d171fe6d988a8a52e1d90f92453d932b26793 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" @@ -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 0000000000000000000000000000000000000000..d78d673700cc960181051e6b3e9e698ad7c71367 --- /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 +} diff --git a/go.mod b/go.mod index 2ae4178c232f951d2811083e0e56ef092a2e251c..bd66f4b4be60bdd83baeeebf9d39023a721ebe87 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 a4d32739756938d86b1b92755f01327bb2fec605..ba642c2238d0678688e816995725c0e3a3f5ba23 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= diff --git a/internal/request/request.go b/internal/request/request.go index 06a45071a3c3e538173d74d87e9203d61bf8919a..ba56f45b8ed4aae1065c454680e34c9340ad4344 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 642209f0f0630eab3f60bf5f7ad3c0b0aa3790e0..3c0f970c0e3f61b90a521ab8aa0bfa20aed155c9 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 c0b87d93b31c69e08115de2654e5621f0f0aa237..d703769bcc6f784331c0b104e404f6a7f42c0d80 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) + } }