diff options
author | Nick Thomas <nick@gitlab.com> | 2019-09-09 18:05:57 +0300 |
---|---|---|
committer | Nick Thomas <nick@gitlab.com> | 2019-09-09 18:05:57 +0300 |
commit | 520398c7154a50f7768ee52da71afd52efff85b1 (patch) | |
tree | c0f53022068268000e107b0fd61026f931a28fb2 /internal | |
parent | 1fa5c7b079831a73b55bb874b84a0b53fd4c0f23 (diff) | |
parent | 6f578e102e4aa504045164173ff7292327f561e6 (diff) |
Merge branch '1-7-auth-cookie-fixes' into '1-7-stable'v1.7.21-7-stable
Set max-age and secure flag for auth cookies
See merge request gitlab/gitlab-pages!16
Diffstat (limited to 'internal')
-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 |
4 files changed, 103 insertions, 24 deletions
diff --git a/internal/auth/auth.go b/internal/auth/auth.go index d6cbdff1..77bc7d8e 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -22,6 +22,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" ) @@ -34,6 +35,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 ) var ( @@ -74,10 +76,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 @@ -285,14 +287,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 @@ -581,7 +583,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)) +} |