From 271f8cef139c17a7a697c9796a9040cca8a3dbad Mon Sep 17 00:00:00 2001 From: Alex Barganier Date: Thu, 2 Mar 2023 16:33:44 -0400 Subject: [PATCH] pkg/server: make MT session cookies backwards compatible Previous work has been done to update the `session` cookie to instead be named `multitenant-session`, which encoded not only the session ID of the currently logged in tenant, but also the name of that tenant, as well as any *other* tenant who was able to be logged in to using the same credentials. This created backwards compatibility issues, since the cookie name changed, and the auth layer began requiring the tenant name to be encoded in the cookie value. This led to scenarios where upgrading from v22.2 to v23.1 effectively invalidated any existing `session` cookie. To fix this, this patch reverts the session cookie name back to `session` (instead of `multitenant-session`). Furthermore, if the authorization layer doesn't find any tenant name to be encoded in the cookie's value, it defaults to the system tenant. This enables older sessions to still be usable in the DB Console and apiV2. Note that this does *not* mean that existing legacy session cookie will be multitenant capable, which is to say that the cookie will not be updated to encode the system tenant's name, or sessions for other tenants. To gain this multitenant session cookie, clients will need to go through the login flow anew. Release note: none --- pkg/server/authentication.go | 116 +++++++++--------- pkg/server/authentication_test.go | 96 ++++++++------- pkg/server/server_controller_http.go | 16 +-- .../db-console/src/redux/cookies.ts | 4 +- .../tenantDropdown/tenantDropdown.spec.tsx | 4 +- 5 files changed, 119 insertions(+), 117 deletions(-) diff --git a/pkg/server/authentication.go b/pkg/server/authentication.go index 1cfc4f149fc9..6e4748062cfa 100644 --- a/pkg/server/authentication.go +++ b/pkg/server/authentication.go @@ -58,9 +58,6 @@ const ( secretLength = 16 // SessionCookieName is the name of the cookie used for HTTP auth. SessionCookieName = "session" - // MultiTenantSessionCookieName is the name of the cookie used for HTTP auth - // when the cluster is multitenant. - MultitenantSessionCookieName = "multitenant-session" // DemoLoginPath is the demo shell auto-login URL. DemoLoginPath = "/demologin" ) @@ -804,69 +801,82 @@ func createAggregatedSessionCookieValue(sessionCookieValue []sessionCookieValue) // findAndDecodeSessionCookie looks for multitenant-session and session cookies // in the cookies slice. If they are found the value will need to be processed if -// it is a multitenant-session cookie (see findSessionCookieValue for details) -// and then decoded. Multitenant-session cookies are prioritized over session cookies. -// If neither cookie types are found or there is an error in decoding or processing, -// the function will return an error. +// it is a multitenant-session cookie (see findSessionCookieValueForTenant for details) +// and then decoded. If there is an error in decoding or processing, the function +// will return an error. func findAndDecodeSessionCookie( ctx context.Context, st *cluster.Settings, cookies []*http.Cookie, ) (*serverpb.SessionCookie, error) { - // Validate the returned cookie. found := false - var cookie *serverpb.SessionCookie - var err error - var sessionCookies []*http.Cookie - // Prioritize multitenant session cookies. - for _, c := range cookies { - if c.Name == MultitenantSessionCookieName { - sessionCookies = append([]*http.Cookie{c}, sessionCookies...) - } else if c.Name == SessionCookieName { - sessionCookies = append(sessionCookies, c) + var sessionCookie *serverpb.SessionCookie + tenantSelectCookieVal := findTenantSelectCookieValue(cookies) + for _, cookie := range cookies { + if cookie.Name != SessionCookieName { + continue } - } - for _, sessionCookie := range sessionCookies { found = true - // This case is if the multitenant session cookie is set. - if sessionCookie.Name == MultitenantSessionCookieName { - mtSessionVal, err := findSessionCookieValue(st, cookies) - if err != nil { - return cookie, apiInternalError(ctx, err) - } - if mtSessionVal != "" { - sessionCookie.Value = mtSessionVal - } + mtSessionVal, err := findSessionCookieValueForTenant( + st, + cookie, + tenantSelectCookieVal) + if err != nil { + return sessionCookie, apiInternalError(ctx, err) + } + if mtSessionVal != "" { + cookie.Value = mtSessionVal } - cookie, err = decodeSessionCookie(sessionCookie) + sessionCookie, err = decodeSessionCookie(cookie) if err != nil { // Multiple cookies with the same name may be included in the // header. We continue searching even if we find a matching - // name with an invalid value + // name with an invalid value. log.Infof(ctx, "found a matching cookie that failed decoding: %v", err) found = false continue } break } - if err != nil || !found { + if !found { return nil, http.ErrNoCookie } - return cookie, nil + return sessionCookie, nil } -// findSessionCookieValue finds the encoded session in an aggregated -// session cookie value established in multi-tenant clusters. If the -// method cannot find a match between the tenant name and session -// or cannot find the tenant cookie value, it will return an empty -// string to indicate this. -// e.g. tenant cookie value is "system" and multitenant-session cookie -// value is "abcd1234,system,efgh5678,app" the output will be "abcd1234". -func findSessionCookieValue(st *cluster.Settings, cookies []*http.Cookie) (string, error) { - if mtSessionStr := findMultitenantSessionCookieValue(cookies); mtSessionStr != "" { - tenantName := findTenantCookieValue(cookies) +// findSessionCookieValueForTenant finds the encoded session in the provided +// aggregated session cookie value established in multi-tenant clusters that's +// associated with the provided tenant name. If an empty tenant name is provided, +// we default to the defaultTenantSelect cluster setting value. +// +// If the method cannot find a match between the tenant name and session, or +// if the provided session cookie is nil, it will return an empty string. +// +// e.g. tenant name is "system" and session cookie's value is +// "abcd1234,system,efgh5678,app" the output will be "abcd1234". +// +// In the case of legacy session cookies, where tenant names are not encoded +// into the cookie value, we assume that the session belongs to defaultTenantSelect. +// Note that these legacy session cookies only contained a single session string +// as the cookie's value. +func findSessionCookieValueForTenant( + st *cluster.Settings, sessionCookie *http.Cookie, tenantName string, +) (string, error) { + if sessionCookie == nil { + return "", nil + } + if mtSessionStr := sessionCookie.Value; sessionCookie.Value != "" { + sessionSlice := strings.Split(mtSessionStr, ",") + if len(sessionSlice) == 1 { + // If no separator was found in the cookie value, this is likely + // a cookie from a previous CRDB version where the cookie value + // contained a single session string without any tenant names encoded. + // To maintain backwards compatibility, assume this session belongs + // to the default tenant. In this case, the entire cookie value is + // the session string. + return mtSessionStr, nil + } if tenantName == "" { tenantName = defaultTenantSelect.Get(&st.SV) } - sessionSlice := strings.Split(mtSessionStr, ",") var encodedSession string for idx, val := range sessionSlice { if val == tenantName && idx > 0 { @@ -881,10 +891,10 @@ func findSessionCookieValue(st *cluster.Settings, cookies []*http.Cookie) (strin return "", nil } -// findTenantCookieValue iterates through all request cookies in order -// to find the value of the tenant cookie. If the tenant cookie is not -// found, it returns the empty string. -func findTenantCookieValue(cookies []*http.Cookie) string { +// findTenantSelectCookieValue iterates through all request cookies in order +// to find the value of the tenant select cookie. If the tenant select cookie +// is not found, it returns the empty string. +func findTenantSelectCookieValue(cookies []*http.Cookie) string { for _, c := range cookies { if c.Name == TenantSelectCookieName { return c.Value @@ -892,15 +902,3 @@ func findTenantCookieValue(cookies []*http.Cookie) string { } return "" } - -// findMultitenantSessionCookieValue iterates through all request -// cookies to find the value of the multitenant-session cookie. -// If not found, it return an empty string. -func findMultitenantSessionCookieValue(cookies []*http.Cookie) string { - for _, c := range cookies { - if c.Name == MultitenantSessionCookieName { - return c.Value - } - } - return "" -} diff --git a/pkg/server/authentication_test.go b/pkg/server/authentication_test.go index 08bf918d0ff5..06752be5c091 100644 --- a/pkg/server/authentication_test.go +++ b/pkg/server/authentication_test.go @@ -593,7 +593,7 @@ func TestLogoutClearsCookies(t *testing.T) { require.Equal(t, "", c.Value) cNames[i] = c.Name } - require.ElementsMatch(t, cNames, []string{SessionCookieName, MultitenantSessionCookieName, TenantSelectCookieName}) + require.ElementsMatch(t, cNames, []string{SessionCookieName, TenantSelectCookieName}) } func TestLogout(t *testing.T) { @@ -906,66 +906,78 @@ func TestFindSessionCookieValue(t *testing.T) { defer log.Scope(t).Close(t) normalSessionStr := "abcd1234,system,efgh5678,app" tests := []struct { - name string - cookieArg []*http.Cookie - resExpected string - errorExpected bool + name string + sessionCookie *http.Cookie + tenantSelectValue string + resExpected string + errorExpected bool }{ - {"standard args", []*http.Cookie{ - { - Name: MultitenantSessionCookieName, + { + name: "standard args", + sessionCookie: &http.Cookie{ + Name: SessionCookieName, Value: normalSessionStr, Path: "/", }, - { - Name: TenantSelectCookieName, - Value: "system", - Path: "/", - }, - }, "abcd1234", false}, - {"no multitenant session cookie", []*http.Cookie{ - { - Name: TenantSelectCookieName, - Value: "system", - Path: "/", - }, - }, "", false}, - {"no tenant cookie", []*http.Cookie{ - { - Name: MultitenantSessionCookieName, + tenantSelectValue: "system", + resExpected: "abcd1234", + errorExpected: false, + }, + { + name: "no multitenant session cookie", + sessionCookie: nil, + tenantSelectValue: "system", + resExpected: "", + errorExpected: false, + }, + { + name: "no tenant cookie", + sessionCookie: &http.Cookie{ + Name: SessionCookieName, Value: normalSessionStr, Path: "/", }, - }, "abcd1234", false}, - {"empty string tenant cookie", []*http.Cookie{ - { - Name: MultitenantSessionCookieName, + resExpected: "abcd1234", + errorExpected: false, + }, + { + name: "empty string tenant cookie", + sessionCookie: &http.Cookie{ + Name: SessionCookieName, Value: normalSessionStr, Path: "/", }, - { - Name: TenantSelectCookieName, - Value: "", - Path: "/", - }, - }, "abcd1234", false}, - {"no tenant name match", []*http.Cookie{ - { - Name: MultitenantSessionCookieName, + tenantSelectValue: "", + resExpected: "abcd1234", + errorExpected: false, + }, + { + name: "no tenant name match", + sessionCookie: &http.Cookie{ + Name: SessionCookieName, Value: normalSessionStr, Path: "/", }, - { - Name: TenantSelectCookieName, - Value: "app2", + tenantSelectValue: "app2", + resExpected: "", + errorExpected: true, + }, + { + name: "legacy session cookie", + sessionCookie: &http.Cookie{ + Name: SessionCookieName, + Value: "aaskjhf218==", Path: "/", }, - }, "", true}, + tenantSelectValue: "", + resExpected: "aaskjhf218==", + errorExpected: false, + }, } for _, test := range tests { t.Run(fmt.Sprintf("find-session-cookie/%s", test.name), func(t *testing.T) { st := cluster.MakeClusterSettings() - res, err := findSessionCookieValue(st, test.cookieArg) + res, err := findSessionCookieValueForTenant(st, test.sessionCookie, test.tenantSelectValue) require.Equal(t, test.resExpected, res) require.Equal(t, test.errorExpected, err != nil) }) diff --git a/pkg/server/server_controller_http.go b/pkg/server/server_controller_http.go index 8933c6326c00..751a2c0b0eaa 100644 --- a/pkg/server/server_controller_http.go +++ b/pkg/server/server_controller_http.go @@ -72,7 +72,7 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) { log.Warningf(ctx, "unable to find server for tenant %q: %v", tenantName, err) // Clear session and tenant cookies since it appears they reference invalid state. http.SetCookie(w, &http.Cookie{ - Name: MultitenantSessionCookieName, + Name: SessionCookieName, Value: "", Path: "/", HttpOnly: true, @@ -193,7 +193,7 @@ func (c *serverController) attemptLoginToAllTenants() http.Handler { if len(tenantNameToSetCookieSlice) > 0 { sessionsStr := createAggregatedSessionCookieValue(tenantNameToSetCookieSlice) cookie := http.Cookie{ - Name: MultitenantSessionCookieName, + Name: SessionCookieName, Value: sessionsStr, Path: "/", HttpOnly: false, @@ -282,15 +282,7 @@ func (c *serverController) attemptLogoutFromAllTenants() http.Handler { } // Clear session and tenant cookies after all logouts have completed. cookie := http.Cookie{ - Name: MultitenantSessionCookieName, - Value: "", - Path: "/", - HttpOnly: true, - Expires: timeutil.Unix(0, 0), - } - http.SetCookie(w, &cookie) - cookie = http.Cookie{ - Name: TenantSelectCookieName, + Name: SessionCookieName, Value: "", Path: "/", HttpOnly: false, @@ -298,7 +290,7 @@ func (c *serverController) attemptLogoutFromAllTenants() http.Handler { } http.SetCookie(w, &cookie) cookie = http.Cookie{ - Name: SessionCookieName, + Name: TenantSelectCookieName, Value: "", Path: "/", HttpOnly: false, diff --git a/pkg/ui/workspaces/db-console/src/redux/cookies.ts b/pkg/ui/workspaces/db-console/src/redux/cookies.ts index d29a12f5dd0e..972dbef21824 100644 --- a/pkg/ui/workspaces/db-console/src/redux/cookies.ts +++ b/pkg/ui/workspaces/db-console/src/redux/cookies.ts @@ -8,7 +8,7 @@ // by the Apache License, Version 2.0, included in the file // licenses/APL.txt. -export const MULTITENANT_SESSION_COOKIE_NAME = "multitenant-session"; +export const MULTITENANT_SESSION_COOKIE_NAME = "session"; export const getAllCookies = (): Map => { const cookieMap: Map = new Map(); @@ -26,7 +26,7 @@ export const getCookieValue = (cookieName: string): string => { return cookies.get(cookieName) || null; }; -// selectTenantsFromMultitenantSessionCookie formats the multitenant-session +// selectTenantsFromMultitenantSessionCookie formats the session // cookie value and returns only the tenant names. export const selectTenantsFromMultitenantSessionCookie = (): string[] => { const cookies = getAllCookies(); diff --git a/pkg/ui/workspaces/db-console/src/views/app/components/tenantDropdown/tenantDropdown.spec.tsx b/pkg/ui/workspaces/db-console/src/views/app/components/tenantDropdown/tenantDropdown.spec.tsx index c55b07b9865a..53a9ed880be7 100644 --- a/pkg/ui/workspaces/db-console/src/views/app/components/tenantDropdown/tenantDropdown.spec.tsx +++ b/pkg/ui/workspaces/db-console/src/views/app/components/tenantDropdown/tenantDropdown.spec.tsx @@ -21,7 +21,7 @@ jest.mock("src/redux/cookies", () => ({ })); describe("TenantDropdown", () => { - it("returns null if there are no tenants in the multitenant-session cookie", () => { + it("returns null if there are no tenants in the session cookie", () => { ( selectTenantsFromMultitenantSessionCookie as jest.MockedFn< typeof selectTenantsFromMultitenantSessionCookie @@ -30,7 +30,7 @@ describe("TenantDropdown", () => { const wrapper = shallow(); expect(wrapper.isEmptyRender()); }); - it("returns a dropdown list of tenant options if there are tenant in the multitenant-session cookie", () => { + it("returns a dropdown list of tenant options if there are tenant in the session cookie", () => { ( selectTenantsFromMultitenantSessionCookie as jest.MockedFn< typeof selectTenantsFromMultitenantSessionCookie