From 23bf6344632e96de15aacc617b5859e806bb4bec Mon Sep 17 00:00:00 2001 From: ngala Date: Wed, 15 Nov 2023 19:16:20 +0530 Subject: [PATCH 1/4] Add path in session cookie when namespace is provided in path Related to: https://gitlab.com/gitlab-org/gitlab/-/issues/17584 This will prevent cookie sharing when the namespace is provided in the path. We are setting the path variable at the cookie level so that the browser only sends the specific cookie assigned to that namespace. More context: https://gitlab.com/gitlab-org/gitlab/-/issues/211677#note_1646965167 Changelog: added --- internal/auth/auth.go | 23 ++++++++++++++------ internal/auth/session.go | 19 +++++++++++++++- internal/request/request.go | 14 ++++++++++++ internal/request/request_test.go | 37 ++++++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 8 deletions(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index dcc81eee2..b10cee5bb 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -140,7 +140,8 @@ func (a *Auth) checkAuthenticationResponse(session *hostSession, w http.Response return } - decryptedCode, err := a.DecryptCode(r.URL.Query().Get("code"), getRequestDomain(r)) + decryptedCode, err := a.DecryptCode(r.URL.Query().Get("code"), + getRequestDomain(r, session.getNamespaceInPathFromSession())) if err != nil { logRequest(r).WithError(err).Error("failed to decrypt secure code") errortracking.CaptureErrWithReqAndStackTrace(err, r) @@ -317,11 +318,16 @@ func getRequestAddress(r *http.Request) string { return "http://" + r.Host + r.RequestURI } -func getRequestDomain(r *http.Request) string { +func getRequestDomain(r *http.Request, namespace string) string { + requestDomain := r.Host + if len(namespace) > 0 && strings.HasPrefix(r.Host, namespace) { + requestDomain = strings.TrimPrefix(r.Host, namespace+".") + "/" + namespace + } + if request.IsHTTPS(r) { - return "https://" + r.Host + return "https://" + requestDomain } - return "http://" + r.Host + return "http://" + requestDomain } func shouldProxyAuthToGitlab(r *http.Request) bool { @@ -440,15 +446,18 @@ func (a *Auth) checkTokenExists(session *hostSession, w http.ResponseWriter, r * // Because the pages domain might be in public suffix list, we have to // redirect to pages domain to trigger authorization flow - http.Redirect(w, r, a.getProxyAddress(r, session.Values["state"].(string)), http.StatusFound) + http.Redirect(w, + r, + a.getProxyAddress(r, session.Values["state"].(string), session.getNamespaceInPathFromSession()), + http.StatusFound) return true } return false } -func (a *Auth) getProxyAddress(r *http.Request, state string) string { - return fmt.Sprintf(authorizeProxyTemplate, a.redirectURI, getRequestDomain(r), state) +func (a *Auth) getProxyAddress(r *http.Request, state string, namespace string) string { + return fmt.Sprintf(authorizeProxyTemplate, a.redirectURI, getRequestDomain(r, namespace), state) } func destroySession(session *hostSession, w http.ResponseWriter, r *http.Request) { diff --git a/internal/auth/session.go b/internal/auth/session.go index 5bfc8e034..6697ae308 100644 --- a/internal/auth/session.go +++ b/internal/auth/session.go @@ -4,6 +4,7 @@ import ( "net/http" "github.com/gorilla/sessions" + "gitlab.com/gitlab-org/labkit/log" "gitlab.com/gitlab-org/gitlab-pages/internal/errortracking" "gitlab.com/gitlab-org/gitlab-pages/internal/httperrors" @@ -22,17 +23,33 @@ func (s *hostSession) Save(r *http.Request, w http.ResponseWriter) error { return s.Session.Save(r, w) } +func (s *hostSession) getNamespaceInPathFromSession() string { + namespaceInPath := "" + if len(s.Options.Path) > 1 && s.Options.Path[0] == '/' { + namespaceInPath = s.Options.Path[1:] + } + return namespaceInPath +} + func (a *Auth) getSessionFromStore(r *http.Request) (*hostSession, error) { session, err := a.store.Get(r, "gitlab-pages") if session != nil { + namespaceInPath := request.GetNamespaceInPathFromRequest(r, a.pagesDomain) + // Cookie just for this domain - session.Options.Path = "/" + session.Options.Path = "/" + namespaceInPath session.Options.HttpOnly = true session.Options.Secure = request.IsHTTPS(r) session.Options.MaxAge = int(a.cookieSessionTimeout.Seconds()) if session.Values[sessionHostKey] == nil || session.Values[sessionHostKey] != r.Host { + logRequest(r).WithFields(log.Fields{ + "Session host": session.Values[sessionHostKey], + "Request host": r.Host, + "Namespace in path": namespaceInPath, + }).Info("Resetting session values") + session.Values = make(map[interface{}]interface{}) } } diff --git a/internal/request/request.go b/internal/request/request.go index f98b08195..066afd560 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -3,6 +3,7 @@ package request import ( "net" "net/http" + "strings" ) const ( @@ -38,3 +39,16 @@ func GetRemoteAddrWithoutPort(r *http.Request) string { return remoteAddr } + +// GetNamespaceInPathFromRequest fetches X-Gitlab-Namespace-In-Path from r.Header +// and validates against pagesDomain before returning +func GetNamespaceInPathFromRequest(r *http.Request, pagesDomain string) string { + namespaceInPath := "" + if pagesDomain != "" { + namespaceInPath = r.Header.Get("X-Gitlab-Namespace-In-Path") + if namespaceInPath != "" && !strings.HasPrefix(r.Host, namespaceInPath+"."+pagesDomain) { + namespaceInPath = "" + } + } + return namespaceInPath +} diff --git a/internal/request/request_test.go b/internal/request/request_test.go index 9e71db37e..90f7d79e2 100644 --- a/internal/request/request_test.go +++ b/internal/request/request_test.go @@ -87,3 +87,40 @@ func TestGetRemoteAddrWithoutPort(t *testing.T) { }) } } + +func TestGetNamespaceInPathFromRequest(t *testing.T) { + tests := map[string]struct { + pagesDomain string + u string + namespace string + expected string + }{ + "when valid X-Gitlab-Namespace-In-Path is provided in request header": { + pagesDomain: "example.com", + u: "https://namespace.example.com/myProject", + namespace: "namespace", + expected: "namespace", + }, + "when valid X-Gitlab-Namespace-In-Path with '.' in between is provided in request header": { + pagesDomain: "example.com", + u: "https://namespace.test.example.com/myProject", + namespace: "namespace.test", + expected: "namespace.test", + }, + "when forged X-Gitlab-Namespace-In-Path is provided in request header": { + pagesDomain: "example.com", + u: "https://namespace.example.com/myProject", + namespace: "namespace-forged", + expected: "", + }, + } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, test.u, nil) + req.Header.Set("X-Gitlab-Namespace-In-Path", test.namespace) + + namespace := GetNamespaceInPathFromRequest(req, test.pagesDomain) + require.Equal(t, test.expected, namespace) + }) + } +} -- GitLab From 7d0d9308ca49a9565c6e64ffe117555bc9f51044 Mon Sep 17 00:00:00 2001 From: ngala Date: Thu, 16 Nov 2023 14:11:01 +0530 Subject: [PATCH 2/4] Add namespace-in-path flag --- internal/config/config.go | 4 ++++ internal/config/flags.go | 2 ++ 2 files changed, 6 insertions(+) diff --git a/internal/config/config.go b/internal/config/config.go index d2016550a..ba2270168 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -61,6 +61,8 @@ type General struct { ShowVersion bool CustomHeaders http.Header + + NamespaceInPath bool } // RateLimit config struct @@ -317,6 +319,7 @@ func loadConfig() (*Config, error) { InsecureCiphers: *insecureCiphers, PropagateCorrelationID: *propagateCorrelationID, ShowVersion: *showVersion, + NamespaceInPath: *namespaceInPath, }, RateLimit: RateLimit{ SourceIPLimitPerSecond: *rateLimitSourceIP, @@ -518,6 +521,7 @@ func logFields(config *Config) map[string]any { "sentry-dsn": config.Sentry.DSN, "sentry-environment": config.Sentry.Environment, "version": config.General.ShowVersion, + "namespace-in-path": *namespaceInPath, } } diff --git a/internal/config/flags.go b/internal/config/flags.go index bce3b07ea..069173c3b 100644 --- a/internal/config/flags.go +++ b/internal/config/flags.go @@ -107,6 +107,8 @@ var ( header = MultiStringFlag{separator: ";;"} + namespaceInPath = flag.Bool("namespace-in-path", false, "Enable Namespace in path") + // flags that won't be logged to the output on Pages boot nonLoggableFlags = map[string]bool{ "auth-client-id": true, -- GitLab From 4717d5795b6642e71dbd156d20ba4509b364869f Mon Sep 17 00:00:00 2001 From: ngala Date: Mon, 20 Nov 2023 22:01:06 +0530 Subject: [PATCH 3/4] Add a check before setting path in cookie --- app.go | 1 + internal/auth/auth.go | 3 +++ internal/auth/auth_test.go | 1 + internal/auth/session.go | 5 ++++- 4 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app.go b/app.go index b022d7c21..0bfbff43f 100644 --- a/app.go +++ b/app.go @@ -422,6 +422,7 @@ func (a *theApp) setAuth(config *cfg.Config) error { AuthScope: config.Authentication.Scope, AuthTimeout: config.Authentication.Timeout, CookieSessionTimeout: config.Authentication.CookieSessionTimeout, + NamespaceInPath: config.General.NamespaceInPath, }) if err != nil { return fmt.Errorf("could not initialize auth package: %w", err) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index b10cee5bb..c5a78f1fe 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -67,6 +67,7 @@ type Auth struct { store sessions.Store now func() time.Time // allows to stub time.Now() easily in tests cookieSessionTimeout time.Duration + namespaceInPath bool } type tokenResponse struct { @@ -673,6 +674,7 @@ type Options struct { AuthScope string AuthTimeout time.Duration CookieSessionTimeout time.Duration + NamespaceInPath bool } // New when authentication supported this will be used to create authentication handler @@ -701,5 +703,6 @@ func New(options *Options) (*Auth, error) { jwtExpiry: time.Minute, now: time.Now, cookieSessionTimeout: options.CookieSessionTimeout, + namespaceInPath: options.NamespaceInPath, }, nil } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index 40c6db15c..fc0b2efa1 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -33,6 +33,7 @@ func createTestAuth(t *testing.T, internalServer string, publicServer string) *A AuthScope: "scope", AuthTimeout: 5 * time.Second, CookieSessionTimeout: 10 * time.Minute, + NamespaceInPath: false, }) require.NoError(t, err) diff --git a/internal/auth/session.go b/internal/auth/session.go index 6697ae308..a28bbbf04 100644 --- a/internal/auth/session.go +++ b/internal/auth/session.go @@ -35,7 +35,10 @@ func (a *Auth) getSessionFromStore(r *http.Request) (*hostSession, error) { session, err := a.store.Get(r, "gitlab-pages") if session != nil { - namespaceInPath := request.GetNamespaceInPathFromRequest(r, a.pagesDomain) + namespaceInPath := "" + if a.namespaceInPath { + namespaceInPath = request.GetNamespaceInPathFromRequest(r, a.pagesDomain) + } // Cookie just for this domain session.Options.Path = "/" + namespaceInPath -- GitLab From eca7192ebe6358fa173761985ddfb81a640e4262 Mon Sep 17 00:00:00 2001 From: ngala Date: Wed, 22 Nov 2023 21:32:15 +0530 Subject: [PATCH 4/4] Address review comment --- app.go | 2 +- internal/auth/auth.go | 19 ++++++++++-- internal/auth/auth_test.go | 52 +++++++++++++++++++++++++++++++- internal/auth/session.go | 5 +-- internal/request/request.go | 14 --------- internal/request/request_test.go | 37 ----------------------- 6 files changed, 69 insertions(+), 60 deletions(-) diff --git a/app.go b/app.go index 0bfbff43f..086e44931 100644 --- a/app.go +++ b/app.go @@ -422,7 +422,7 @@ func (a *theApp) setAuth(config *cfg.Config) error { AuthScope: config.Authentication.Scope, AuthTimeout: config.Authentication.Timeout, CookieSessionTimeout: config.Authentication.CookieSessionTimeout, - NamespaceInPath: config.General.NamespaceInPath, + AllowNamespaceInPath: config.General.NamespaceInPath, }) if err != nil { return fmt.Errorf("could not initialize auth package: %w", err) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index c5a78f1fe..fe092ae9c 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -67,7 +67,7 @@ type Auth struct { store sessions.Store now func() time.Time // allows to stub time.Now() easily in tests cookieSessionTimeout time.Duration - namespaceInPath bool + allowNamespaceInPath bool } type tokenResponse struct { @@ -612,6 +612,19 @@ func (a *Auth) CheckResponseForInvalidToken(w http.ResponseWriter, r *http.Reque return false } +func (a *Auth) getNamespaceInPath(r *http.Request) string { + if !a.allowNamespaceInPath { + return "" + } + + namespaceInPath := r.Header.Get("X-Gitlab-Namespace-In-Path") + if namespaceInPath == "" || !strings.HasPrefix(r.Host, namespaceInPath+"."+a.pagesDomain) { + return "" + } + + return namespaceInPath +} + func checkResponseForInvalidToken(resp *http.Response, session *hostSession, w http.ResponseWriter, r *http.Request) bool { if resp.StatusCode == http.StatusUnauthorized { errResp := errorResponse{} @@ -674,7 +687,7 @@ type Options struct { AuthScope string AuthTimeout time.Duration CookieSessionTimeout time.Duration - NamespaceInPath bool + AllowNamespaceInPath bool } // New when authentication supported this will be used to create authentication handler @@ -703,6 +716,6 @@ func New(options *Options) (*Auth, error) { jwtExpiry: time.Minute, now: time.Now, cookieSessionTimeout: options.CookieSessionTimeout, - namespaceInPath: options.NamespaceInPath, + allowNamespaceInPath: options.AllowNamespaceInPath, }, nil } diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index fc0b2efa1..8582a973e 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -33,7 +33,7 @@ func createTestAuth(t *testing.T, internalServer string, publicServer string) *A AuthScope: "scope", AuthTimeout: 5 * time.Second, CookieSessionTimeout: 10 * time.Minute, - NamespaceInPath: false, + AllowNamespaceInPath: false, }) require.NoError(t, err) @@ -64,6 +64,9 @@ func setSessionValues(t *testing.T, r *http.Request, auth *Auth, values map[inte t.Helper() tmpRequest, err := http.NewRequest("GET", "http://"+r.Host, nil) + if len(r.Header.Get("X-Gitlab-Namespace-In-Path")) > 0 { + tmpRequest.Header.Set("X-Gitlab-Namespace-In-Path", r.Header.Get("X-Gitlab-Namespace-In-Path")) + } require.NoError(t, err) result := httptest.NewRecorder() @@ -630,3 +633,50 @@ func TestDomainAllowed(t *testing.T) { }) } } + +func testNamespaceInPath(t *testing.T, allowNamespaceInPath bool, namespace string, expectedPath string, msg string) { + auth := createTestAuth(t, "", "") + auth.allowNamespaceInPath = allowNamespaceInPath + + result := httptest.NewRecorder() + + r, err := http.NewRequest("Get", "https://namespace.pages.gitlab-example.com", nil) + r.Header.Set("X-Gitlab-Namespace-In-Path", namespace) + require.NoError(t, err) + + // pre-set an state + setSessionValues(t, r, auth, map[interface{}]interface{}{ + "state": "given_state", + }) + + contentServed := auth.CheckAuthentication(result, r, &domainMock{projectID: 1000}) + require.True(t, contentServed) + + // check if the state was re-used instead of re-created + session, _ := auth.getSessionFromStore(r) + require.Equal(t, expectedPath, session.Options.Path, msg) +} + +func TestValidSessionPathWhenNamespaceInPathIsDisabled(t *testing.T) { + testNamespaceInPath(t, + false, + "namespace", + "/", + "did not set path as '/' in session options") +} + +func TestValidSessionPathWhenNamespaceInPathIsEnabled(t *testing.T) { + testNamespaceInPath(t, + true, + "namespace", + "/namespace", + "did not set namespace path in session options") +} + +func TestForgedNamespaceWhenNamespaceInPathIsEnabled(t *testing.T) { + testNamespaceInPath(t, + true, + "namespace-forged", + "/", + "did not set path as '/' in session options") +} diff --git a/internal/auth/session.go b/internal/auth/session.go index a28bbbf04..c9b4ee8da 100644 --- a/internal/auth/session.go +++ b/internal/auth/session.go @@ -35,10 +35,7 @@ func (a *Auth) getSessionFromStore(r *http.Request) (*hostSession, error) { session, err := a.store.Get(r, "gitlab-pages") if session != nil { - namespaceInPath := "" - if a.namespaceInPath { - namespaceInPath = request.GetNamespaceInPathFromRequest(r, a.pagesDomain) - } + namespaceInPath := a.getNamespaceInPath(r) // Cookie just for this domain session.Options.Path = "/" + namespaceInPath diff --git a/internal/request/request.go b/internal/request/request.go index 066afd560..f98b08195 100644 --- a/internal/request/request.go +++ b/internal/request/request.go @@ -3,7 +3,6 @@ package request import ( "net" "net/http" - "strings" ) const ( @@ -39,16 +38,3 @@ func GetRemoteAddrWithoutPort(r *http.Request) string { return remoteAddr } - -// GetNamespaceInPathFromRequest fetches X-Gitlab-Namespace-In-Path from r.Header -// and validates against pagesDomain before returning -func GetNamespaceInPathFromRequest(r *http.Request, pagesDomain string) string { - namespaceInPath := "" - if pagesDomain != "" { - namespaceInPath = r.Header.Get("X-Gitlab-Namespace-In-Path") - if namespaceInPath != "" && !strings.HasPrefix(r.Host, namespaceInPath+"."+pagesDomain) { - namespaceInPath = "" - } - } - return namespaceInPath -} diff --git a/internal/request/request_test.go b/internal/request/request_test.go index 90f7d79e2..9e71db37e 100644 --- a/internal/request/request_test.go +++ b/internal/request/request_test.go @@ -87,40 +87,3 @@ func TestGetRemoteAddrWithoutPort(t *testing.T) { }) } } - -func TestGetNamespaceInPathFromRequest(t *testing.T) { - tests := map[string]struct { - pagesDomain string - u string - namespace string - expected string - }{ - "when valid X-Gitlab-Namespace-In-Path is provided in request header": { - pagesDomain: "example.com", - u: "https://namespace.example.com/myProject", - namespace: "namespace", - expected: "namespace", - }, - "when valid X-Gitlab-Namespace-In-Path with '.' in between is provided in request header": { - pagesDomain: "example.com", - u: "https://namespace.test.example.com/myProject", - namespace: "namespace.test", - expected: "namespace.test", - }, - "when forged X-Gitlab-Namespace-In-Path is provided in request header": { - pagesDomain: "example.com", - u: "https://namespace.example.com/myProject", - namespace: "namespace-forged", - expected: "", - }, - } - for name, test := range tests { - t.Run(name, func(t *testing.T) { - req := httptest.NewRequest(http.MethodGet, test.u, nil) - req.Header.Set("X-Gitlab-Namespace-In-Path", test.namespace) - - namespace := GetNamespaceInPathFromRequest(req, test.pagesDomain) - require.Equal(t, test.expected, namespace) - }) - } -} -- GitLab