diff options
author | Nick Thomas <nick@gitlab.com> | 2019-09-09 18:06:04 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2019-09-09 18:06:04 +0300 |
commit | a254ab8233a08cd6c615352c77cabc5e06a6a16c (patch) | |
tree | e12028206ef1fab96fa3c4eec8a17fdffd55e547 | |
parent | d623618c95e5a96e6c7be7e173f7188f682994c1 (diff) | |
parent | 08c3c4c574592ba0cd8903b405fd500b14be5a29 (diff) |
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
-rw-r--r-- | CHANGELOG | 6 | ||||
-rw-r--r-- | VERSION | 2 | ||||
-rw-r--r-- | acceptance_test.go | 66 | ||||
-rw-r--r-- | app.go | 4 | ||||
-rw-r--r-- | helpers_test.go | 22 | ||||
-rw-r--r-- | internal/auth/auth.go | 15 | ||||
-rw-r--r-- | internal/auth/auth_test.go | 64 | ||||
-rw-r--r-- | internal/request/request.go | 24 | ||||
-rw-r--r-- | internal/request/request_test.go | 24 |
9 files changed, 199 insertions, 28 deletions
@@ -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 @@ -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, "") @@ -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)) +} |