Merge branch '1-6-stable-auth-cookie-fixes' into '1-6-stable'v1.6.31-6-stable

Set max-age and secure flag for auth cookies

See merge request gitlab/gitlab-pages!17

9 files changed, 199 insertions, 28 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 33535ebf..6569e325 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,3 +1,9 @@
+v 1.6.3
+
+- Fix https to http downgrade for auth process
+- Limit auth cookie max-age to 10 minutes
+- Use secure cookies for auth session
+
 v 1.6.2
 
 - Security fix for recovering gitlab access token from cookies
diff --git a/VERSION b/VERSION
index fdd3be6d..266146b8 100644
--- a/VERSION
+++ b/VERSION
@@ -1 +1 @@
-1.6.2
+1.6.3
diff --git a/acceptance_test.go b/acceptance_test.go
index eaa32318..7b323f2e 100644
--- a/acceptance_test.go
+++ b/acceptance_test.go
@@ -37,6 +37,7 @@ var listeners = []ListenSpec{
 var (
 	httpListener  = listeners[0]
 	httpsListener = listeners[2]
+	proxyListener = listeners[4]
 )
 
 func skipUnlessEnabled(t *testing.T, conditions ...string) {
@@ -931,6 +932,71 @@ func TestAccessControlUnderCustomDomain(t *testing.T) {
 	assert.Equal(t, http.StatusOK, authrsp.StatusCode)
 }
 
+func TestAccessControlUnderCustomDomainWithHTTPSProxy(t *testing.T) {
+	skipUnlessEnabled(t, "not-inplace-chroot")
+
+	testServer := makeGitLabPagesAccessStub(t)
+	testServer.Start()
+	defer testServer.Close()
+
+	teardown := RunPagesProcessWithAuthServer(t, *pagesBinary, listeners, "", testServer.URL)
+	defer teardown()
+
+	rsp, err := GetProxyRedirectPageWithCookie(t, proxyListener, "private.domain.com", "/", "", true)
+	require.NoError(t, err)
+	defer rsp.Body.Close()
+
+	cookie := rsp.Header.Get("Set-Cookie")
+
+	url, err := url.Parse(rsp.Header.Get("Location"))
+	require.NoError(t, err)
+
+	state := url.Query().Get("state")
+	require.Equal(t, url.Query().Get("domain"), "https://private.domain.com")
+	pagesrsp, err := GetProxyRedirectPageWithCookie(t, proxyListener, url.Host, url.Path+"?"+url.RawQuery, "", true)
+	require.NoError(t, err)
+	defer pagesrsp.Body.Close()
+
+	pagescookie := pagesrsp.Header.Get("Set-Cookie")
+
+	// Go to auth page with correct state will cause fetching the token
+	authrsp, err := GetProxyRedirectPageWithCookie(t, proxyListener,
+		"projects.gitlab-example.com", "/auth?code=1&state="+state,
+		pagescookie, true)
+
+	require.NoError(t, err)
+	defer authrsp.Body.Close()
+
+	url, err = url.Parse(authrsp.Header.Get("Location"))
+	require.NoError(t, err)
+
+	// Will redirect to custom domain
+	require.Equal(t, "private.domain.com", url.Host)
+	require.Equal(t, "1", url.Query().Get("code"))
+	require.Equal(t, state, url.Query().Get("state"))
+
+	// Run auth callback in custom domain
+	authrsp, err = GetProxyRedirectPageWithCookie(t, proxyListener, "private.domain.com",
+		"/auth?code=1&state="+state, cookie, true)
+
+	require.NoError(t, err)
+	defer authrsp.Body.Close()
+
+	// Will redirect to the page
+	cookie = authrsp.Header.Get("Set-Cookie")
+	require.Equal(t, http.StatusFound, authrsp.StatusCode)
+
+	url, err = url.Parse(authrsp.Header.Get("Location"))
+	require.NoError(t, err)
+
+	// Will redirect to custom domain
+	require.Equal(t, "https://private.domain.com/", url.String())
+	// Fetch page in custom domain
+	authrsp, err = GetProxyRedirectPageWithCookie(t, proxyListener, "private.domain.com", "/",
+		cookie, true)
+	require.Equal(t, http.StatusOK, authrsp.StatusCode)
+}
+
 func TestAccessControlGroupDomain404RedirectsAuth(t *testing.T) {
 	skipUnlessEnabled(t)
 	teardown := RunPagesProcessWithAuth(t, *pagesBinary, listeners, "")
diff --git a/app.go b/app.go
index 9d4f5bdb..4d050809 100644
--- a/app.go
+++ b/app.go
@@ -24,6 +24,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/netutil"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 	"gitlab.com/gitlab-org/gitlab-pages/metrics"
 )
 
@@ -222,12 +223,15 @@ func (a *theApp) serveFileOrNotFound(domain *domain.D) http.HandlerFunc {
 
 func (a *theApp) ServeHTTP(ww http.ResponseWriter, r *http.Request) {
 	https := r.TLS != nil
+	r = request.WithHTTPSFlag(r, https)
+
 	a.serveContent(ww, r, https)
 }
 
 func (a *theApp) ServeProxy(ww 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 != "" {
 		r.Host = forwardedHost
diff --git a/helpers_test.go b/helpers_test.go
index 61fa5279..ad7c65f1 100644
--- a/helpers_test.go
+++ b/helpers_test.go
@@ -314,15 +314,31 @@ func GetRedirectPage(t *testing.T, spec ListenSpec, host, urlsuffix string) (*ht
 	return GetRedirectPageWithCookie(t, spec, host, urlsuffix, "")
 }
 
+func GetProxyRedirectPageWithCookie(t *testing.T, spec ListenSpec, host string, urlsuffix string, cookie string, https bool) (*http.Response, error) {
+	schema := "http"
+	if https {
+		schema = "https"
+	}
+	header := http.Header{
+		"X-Forwarded-Proto": []string{schema},
+		"X-Forwarded-Host":  []string{host},
+		"cookie":            []string{cookie},
+	}
+
+	return GetRedirectPageWithHeaders(t, spec, host, urlsuffix, header)
+}
+
 func GetRedirectPageWithCookie(t *testing.T, spec ListenSpec, host, urlsuffix string, cookie string) (*http.Response, error) {
+	return GetRedirectPageWithHeaders(t, spec, host, urlsuffix, http.Header{"cookie": []string{cookie}})
+}
+
+func GetRedirectPageWithHeaders(t *testing.T, spec ListenSpec, host, urlsuffix string, header http.Header) (*http.Response, error) {
 	url := spec.URL(urlsuffix)
 	req, err := http.NewRequest("GET", url, nil)
 	if err != nil {
 		return nil, err
 	}
-	if cookie != "" {
-		req.Header.Set("Cookie", cookie)
-	}
+	req.Header = header
 
 	req.Host = host
 
diff --git a/internal/auth/auth.go b/internal/auth/auth.go
index b9d224ae..5aa56ad0 100644
--- a/internal/auth/auth.go
+++ b/internal/auth/auth.go
@@ -21,6 +21,7 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httperrors"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/httptransport"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 
 	"golang.org/x/crypto/hkdf"
 )
@@ -33,6 +34,7 @@ const (
 	tokenContentTemplate   = "client_id=%s&client_secret=%s&code=%s&grant_type=authorization_code&redirect_uri=%s"
 	callbackPath           = "/auth"
 	authorizeProxyTemplate = "%s?domain=%s&state=%s"
+	authSessionMaxAge      = 60 * 10 // 10 minutes
 )
 
 // Auth handles authenticating users with GitLab API
@@ -63,10 +65,10 @@ func (a *Auth) getSessionFromStore(r *http.Request) (*sessions.Session, error) {
 
 	if session != nil {
 		// Cookie just for this domain
-		session.Options = &sessions.Options{
-			Path:     "/",
-			HttpOnly: true,
-		}
+		session.Options.Path = "/"
+		session.Options.HttpOnly = true
+		session.Options.Secure = request.IsHTTPS(r)
+		session.Options.MaxAge = authSessionMaxAge
 	}
 
 	return session, err
@@ -261,14 +263,14 @@ func (a *Auth) handleProxyingAuth(session *sessions.Session, w http.ResponseWrit
 }
 
 func getRequestAddress(r *http.Request) string {
-	if r.TLS != nil {
+	if request.IsHTTPS(r) {
 		return "https://" + r.Host + r.RequestURI
 	}
 	return "http://" + r.Host + r.RequestURI
 }
 
 func getRequestDomain(r *http.Request) string {
-	if r.TLS != nil {
+	if request.IsHTTPS(r) {
 		return "https://" + r.Host
 	}
 	return "http://" + r.Host
@@ -545,7 +547,6 @@ func createCookieStore(storeSecret string) sessions.Store {
 // New when authentication supported this will be used to create authentication handler
 func New(pagesDomain string, storeSecret string, clientID string, clientSecret string,
 	redirectURI string, gitLabServer string) *Auth {
-
 	return &Auth{
 		pagesDomain:  pagesDomain,
 		clientID:     clientID,
diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go
index 2fbbb938..1aa3bfae 100644
--- a/internal/auth/auth_test.go
+++ b/internal/auth/auth_test.go
@@ -13,6 +13,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
+	"gitlab.com/gitlab-org/gitlab-pages/internal/request"
 )
 
 func createAuth(t *testing.T) *Auth {
@@ -28,13 +29,32 @@ func defaultCookieStore() sessions.Store {
 	return createCookieStore("something-very-secret")
 }
 
+// Gorilla's sessions use request context to save session
+// Which makes session sharable between test code and actually manipulating session
+// Which leads to negative side effects: we can't test encryption, and cookie params
+// like max-age and secure are not being properly set
+// To avoid that we use fake request, and set only session cookie without copying context
+func setSessionValues(r *http.Request, values map[interface{}]interface{}) {
+	tmpRequest, _ := http.NewRequest("GET", "/", nil)
+	result := httptest.NewRecorder()
+	store := defaultCookieStore()
+
+	session, _ := store.Get(tmpRequest, "gitlab-pages")
+	session.Values = values
+	session.Save(tmpRequest, result)
+
+	for _, cookie := range result.Result().Cookies() {
+		r.AddCookie(cookie)
+	}
+}
+
 func TestTryAuthenticate(t *testing.T) {
 	auth := createAuth(t)
 
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/something/else")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	assert.Equal(t, false, auth.TryAuthenticate(result, r, make(domain.Map), &sync.RWMutex{}))
 }
@@ -45,7 +65,7 @@ func TestTryAuthenticateWithError(t *testing.T) {
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/auth?error=access_denied")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	assert.Equal(t, true, auth.TryAuthenticate(result, r, make(domain.Map), &sync.RWMutex{}))
 	assert.Equal(t, 401, result.Code)
@@ -58,7 +78,7 @@ func TestTryAuthenticateWithCodeButInvalidState(t *testing.T) {
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/auth?code=1&state=invalid")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["state"] = "state"
@@ -68,7 +88,7 @@ func TestTryAuthenticateWithCodeButInvalidState(t *testing.T) {
 	assert.Equal(t, 401, result.Code)
 }
 
-func TestTryAuthenticateWithCodeAndState(t *testing.T) {
+func testTryAuthenticateWithCodeAndState(t *testing.T, https bool) {
 	apiServer := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		switch r.URL.Path {
 		case "/oauth/token":
@@ -88,7 +108,6 @@ func TestTryAuthenticateWithCodeAndState(t *testing.T) {
 	apiServer.Start()
 	defer apiServer.Close()
 
-	store := defaultCookieStore()
 	auth := New("pages.gitlab-example.com",
 		"something-very-secret",
 		"id",
@@ -96,19 +115,28 @@ func TestTryAuthenticateWithCodeAndState(t *testing.T) {
 		"http://pages.gitlab-example.com/auth",
 		apiServer.URL)
 
-	result := httptest.NewRecorder()
-	reqURL, err := url.Parse("/auth?code=1&state=state")
-	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r, _ := http.NewRequest("GET", "/auth?code=1&state=state", nil)
+	r = request.WithHTTPSFlag(r, https)
 
-	session, _ := store.Get(r, "gitlab-pages")
-	session.Values["uri"] = "http://pages.gitlab-example.com/project/"
-	session.Values["state"] = "state"
-	session.Save(r, result)
+	setSessionValues(r, map[interface{}]interface{}{
+		"uri":   "https://pages.gitlab-example.com/project/",
+		"state": "state",
+	})
 
+	result := httptest.NewRecorder()
 	assert.Equal(t, true, auth.TryAuthenticate(result, r, make(domain.Map), &sync.RWMutex{}))
 	assert.Equal(t, 302, result.Code)
-	assert.Equal(t, "http://pages.gitlab-example.com/project/", result.Header().Get("Location"))
+	assert.Equal(t, "https://pages.gitlab-example.com/project/", result.Header().Get("Location"))
+	assert.Equal(t, 600, result.Result().Cookies()[0].MaxAge)
+	assert.Equal(t, https, result.Result().Cookies()[0].Secure)
+}
+
+func TestTryAuthenticateWithCodeAndStateOverHTTP(t *testing.T) {
+	testTryAuthenticateWithCodeAndState(t, false)
+}
+
+func TestTryAuthenticateWithCodeAndStateOverHTTPS(t *testing.T) {
+	testTryAuthenticateWithCodeAndState(t, true)
 }
 
 func TestCheckAuthenticationWhenAccess(t *testing.T) {
@@ -138,7 +166,7 @@ func TestCheckAuthenticationWhenAccess(t *testing.T) {
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"
@@ -175,7 +203,7 @@ func TestCheckAuthenticationWhenNoAccess(t *testing.T) {
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"
@@ -214,6 +242,7 @@ func TestCheckAuthenticationWhenInvalidToken(t *testing.T) {
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
 	r := &http.Request{URL: reqURL}
+	r = request.WithHTTPSFlag(r, false)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"
@@ -250,7 +279,7 @@ func TestCheckAuthenticationWithoutProject(t *testing.T) {
 	result := httptest.NewRecorder()
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
-	r := &http.Request{URL: reqURL}
+	r := request.WithHTTPSFlag(&http.Request{URL: reqURL}, true)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"
@@ -289,6 +318,7 @@ func TestCheckAuthenticationWithoutProjectWhenInvalidToken(t *testing.T) {
 	reqURL, err := url.Parse("/auth?code=1&state=state")
 	require.NoError(t, err)
 	r := &http.Request{URL: reqURL}
+	r = request.WithHTTPSFlag(r, false)
 
 	session, _ := store.Get(r, "gitlab-pages")
 	session.Values["access_token"] = "abc"
diff --git a/internal/request/request.go b/internal/request/request.go
new file mode 100644
index 00000000..dad6af3d
--- /dev/null
+++ b/internal/request/request.go
@@ -0,0 +1,24 @@
+package request
+
+import (
+	"context"
+	"net/http"
+)
+
+type ctxKey string
+
+const (
+	ctxHTTPSKey ctxKey = "https"
+)
+
+// WithHTTPSFlag saves https flag in request's context
+func WithHTTPSFlag(r *http.Request, https bool) *http.Request {
+	ctx := context.WithValue(r.Context(), ctxHTTPSKey, https)
+
+	return r.WithContext(ctx)
+}
+
+// IsHTTPS restores https flag from request's context
+func IsHTTPS(r *http.Request) bool {
+	return r.Context().Value(ctxHTTPSKey).(bool)
+}
diff --git a/internal/request/request_test.go b/internal/request/request_test.go
new file mode 100644
index 00000000..1f47ee2e
--- /dev/null
+++ b/internal/request/request_test.go
@@ -0,0 +1,24 @@
+package request
+
+import (
+	"net/http"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestWithHTTPSFlag(t *testing.T) {
+	r, err := http.NewRequest("GET", "/", nil)
+	require.NoError(t, err)
+
+	assert.Panics(t, func() {
+		IsHTTPS(r)
+	})
+
+	httpsRequest := WithHTTPSFlag(r, true)
+	assert.True(t, IsHTTPS(httpsRequest))
+
+	httpRequest := WithHTTPSFlag(r, false)
+	assert.False(t, IsHTTPS(httpRequest))
+}