pkg/server: make MT session cookies backwards compatible

[abarganier]

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 (and their session) 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

Epic: CRDB-12100

Fixes: #97786

[cockroach-teamcity]

This change is 

[dhartunian]

Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @knz, and @Santamaura)

---

pkg/server/authentication.go line 816 at r1 (raw file):

	var err error
	var sessionCookies []*http.Cookie
	for _, c := range cookies {

you can probably merge the two loops because we just need to act on the first session cookie we find.

---

pkg/server/authentication.go line 823 at r1 (raw file):

	for _, sessionCookie := range sessionCookies {
		found = true
		if sessionCookie.Name == SessionCookieName {

you can assume your cookie is the session cookie in here. there's no other possibility anymore.

---

pkg/server/authentication_test.go line 596 at r1 (raw file):

		cNames[i] = c.Name
	}
	require.ElementsMatch(t, cNames, []string{SessionCookieName, SessionCookieName, TenantSelectCookieName})

I think we can remove the duplicated SessionCookieName

[abarganier]

TFTR! I addressed your comments and also did some slight refactoring of the cookie parsing logic to make it more straightforward (there were lots of repeated iterations through the cookie set that I found confusing).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @knz, and @Santamaura)

---

pkg/server/authentication.go line 816 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

you can probably merge the two loops because we just need to act on the first session cookie we find.

Done, although note that we do have logic that will continue iterating through the cookies if we fail to decode the 1st session cookie that we find, as per this comment:

// 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

---

pkg/server/authentication.go line 823 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

you can assume your cookie is the session cookie in here. there's no other possibility anymore.

Done.

---

pkg/server/authentication_test.go line 596 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

I think we can remove the duplicated SessionCookieName

Ah, missed this! Thanks for catching.

[dhartunian]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @abarganier, @knz, and @Santamaura)

---

pkg/server/authentication.go line 816 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

Done, although note that we do have logic that will continue iterating through the cookies if we fail to decode the 1st session cookie that we find, as per this comment:

// 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

Ah, true! Thx for the reminder. lol I wrote that comment.

[abarganier]

TFTR!

bors r=dhartunian

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[renatolabs]

bors r-

This PR has drifted from master, please see bors failures.

[abarganier]

Okay, fixed the drift from master and tested again. Let's try again.

bors r=dhartunian

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[abarganier]

Failure appears to be a flake from what I can tell. TestUpgradeSchemaChangerElements passes on my local branch.

bors retry

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)