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

@@ -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,26 +891,14 @@ 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
 		}
 	}
 	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 ""
-}

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)
 		})

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,23 +282,15 @@ 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,
 			Expires:  timeutil.Unix(0, 0),
 		}
 		http.SetCookie(w, &cookie)
 		cookie = http.Cookie{
-			Name:     SessionCookieName,
+			Name:     TenantSelectCookieName,
 			Value:    "",
 			Path:     "/",
 			HttpOnly: false,

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<string, string> => {
   const cookieMap: Map<string, string> = 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();

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(<TenantDropdown />);
     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