use gorilla/handlers.ProxyHeaders to get the X-Forwarded-* headers and set them in the appropriate http.Request fields

8 files changed, 186 insertions, 6 deletions

diff --git a/README.md b/README.md
index 0c26c835..dd350265 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 9d275d29..312d171f 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 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
+}
diff --git a/go.mod b/go.mod
index 2ae4178c..bd66f4b4 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 a4d32739..ba642c22 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 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)
+	}
 }