Merge pull request #119249 from dhartunian/23.2-cookie-settings-fixes

release-23.2.1-rc: server, ui: set `HttpOnly: true` and `Secure` flags on cookies

pkg/ccl/serverccl/server_controller_test.go

@@ -336,6 +336,7 @@ func TestServerControllerDefaultHTTPTenant(t *testing.T) {
 	for _, c := range resp.Cookies() {
 		if c.Name == authserver.TenantSelectCookieName {
 			tenantCookie = c.Value
+			require.True(t, c.Secure)
 		}
 	}
 	require.Equal(t, "hello", tenantCookie)
@@ -554,6 +555,10 @@ func TestServerControllerLoginLogout(t *testing.T) {
 	for i, c := range resp.Cookies() {
 		cookieNames[i] = c.Name
 		cookieValues[i] = c.Value
+		require.True(t, c.Secure)
+		if c.Name == "session" {
+			require.True(t, c.HttpOnly)
+		}
 	}
 	require.ElementsMatch(t, []string{"session", "tenant"}, cookieNames)
 	require.ElementsMatch(t, []string{"", ""}, cookieValues)
@@ -578,6 +583,10 @@ func TestServerControllerLoginLogout(t *testing.T) {
 	for i, c := range resp.Cookies() {
 		cookieNames[i] = c.Name
 		cookieValues[i] = c.Value
+		require.True(t, c.Secure)
+		if c.Name == "session" {
+			require.True(t, c.HttpOnly)
+		}
 	}
 	require.ElementsMatch(t, []string{"session", "tenant"}, cookieNames)
 	require.ElementsMatch(t, []string{"", ""}, cookieValues)
@@ -603,6 +612,10 @@ func TestServerControllerLoginLogout(t *testing.T) {
 	for i, c := range resp.Cookies() {
 		cookieNames[i] = c.Name
 		cookieValues[i] = c.Value
+		require.True(t, c.Secure)
+		if c.Name == "session" {
+			require.True(t, c.HttpOnly)
+		}
 	}
 	require.ElementsMatch(t, []string{"session", "tenant"}, cookieNames)
 	require.ElementsMatch(t, []string{"", ""}, cookieValues)

pkg/server/server.go

@@ -1233,6 +1233,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (serverctl.ServerStartupInterf
 		systemTenantNameContainer,
 		pgPreServer.SendRoutingError,
 		tenantCapabilitiesWatcher,
+		cfg.BaseConfig.DisableTLSForHTTP,
 	)
 	drain.serverCtl = sc
 

pkg/server/server_controller.go

@@ -90,6 +90,8 @@ type serverController struct {
 
 	watcher *tenantcapabilitieswatcher.Watcher
 
+	disableTLSForHTTP bool
+
 	mu struct {
 		syncutil.RWMutex
 
@@ -124,6 +126,7 @@ func newServerController(
 	systemTenantNameContainer *roachpb.TenantNameContainer,
 	sendSQLRoutingError func(ctx context.Context, conn net.Conn, tenantName roachpb.TenantName),
 	watcher *tenantcapabilitieswatcher.Watcher,
+	disableTLSForHTTP bool,
 ) *serverController {
 	c := &serverController{
 		AmbientContext:      ambientCtx,
@@ -136,6 +139,7 @@ func newServerController(
 		watcher:             watcher,
 		tenantWaiter:        singleflight.NewGroup("tenant server poller", "poll"),
 		drainCh:             make(chan struct{}),
+		disableTLSForHTTP:   disableTLSForHTTP,
 	}
 	c.orchestrator = newServerOrchestrator(parentStopper, c)
 	c.mu.servers = map[roachpb.TenantName]serverState{

pkg/server/server_controller_http.go

@@ -103,13 +103,15 @@ func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) {
 			Value:    "",
 			Path:     "/",
 			HttpOnly: true,
+			Secure:   !c.disableTLSForHTTP,
 			Expires:  timeutil.Unix(0, 0),
 		})
 		http.SetCookie(w, &http.Cookie{
 			Name:     authserver.TenantSelectCookieName,
 			Value:    "",
 			Path:     "/",
 			HttpOnly: false,
+			Secure:   !c.disableTLSForHTTP,
 			Expires:  timeutil.Unix(0, 0),
 		})
 		// Fall back to serving requests from the default tenant. This helps us serve
@@ -235,7 +237,8 @@ func (c *serverController) attemptLoginToAllTenants() http.Handler {
 				Name:     authserver.SessionCookieName,
 				Value:    sessionsStr,
 				Path:     "/",
-				HttpOnly: false,
+				HttpOnly: true,
+				Secure:   !c.disableTLSForHTTP,
 			}
 			http.SetCookie(w, &cookie)
 			// The tenant cookie needs to be set at some point in order for
@@ -257,6 +260,7 @@ func (c *serverController) attemptLoginToAllTenants() http.Handler {
 				Value:    tenantSelection,
 				Path:     "/",
 				HttpOnly: false,
+				Secure:   !c.disableTLSForHTTP,
 			}
 			http.SetCookie(w, &cookie)
 			if r.Header.Get(AcceptHeader) == JSONContentType {
@@ -353,7 +357,8 @@ func (c *serverController) attemptLogoutFromAllTenants() http.Handler {
 			Name:     authserver.SessionCookieName,
 			Value:    "",
 			Path:     "/",
-			HttpOnly: false,
+			HttpOnly: true,
+			Secure:   !c.disableTLSForHTTP,
 			Expires:  timeutil.Unix(0, 0),
 		}
 		http.SetCookie(w, &cookie)
@@ -362,6 +367,7 @@ func (c *serverController) attemptLogoutFromAllTenants() http.Handler {
 			Value:    "",
 			Path:     "/",
 			HttpOnly: false,
+			Secure:   !c.disableTLSForHTTP,
 			Expires:  timeutil.Unix(0, 0),
 		}
 		http.SetCookie(w, &cookie)

pkg/server/server_http.go

@@ -13,7 +13,9 @@ package server
 import (
 	"context"
 	"crypto/tls"
+	"encoding/json"
 	"net/http"
+	"strings"
 
 	"github.com/NYTimes/gziphandler"
 	"github.com/cockroachdb/cmux"
@@ -33,6 +35,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/netutil"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
+	"github.com/cockroachdb/errors"
 	"google.golang.org/grpc/metadata"
 )
 
@@ -91,6 +94,56 @@ func (s *httpServer) handleHealth(healthHandler http.Handler) {
 	s.mux.Handle(healthPath, healthHandler)
 }
 
+const virtualClustersPath = "/virtual_clusters"
+
+// virtualClustersResp is the response returned by the virtual clusters
+// endpoint that contains a list of active tenant sessions in the
+// current session cookie. This allows the DB Console to populate a
+// dropdown allowing the user to select a cluster.
+type virtualClustersResp struct {
+	// VirtualClusters is a list of virtual cluster names.
+	VirtualClusters []string `json:"virtual_clusters"`
+}
+
+// virtualClustersHandler parses the session cookie from the request
+// and returns a JSON body with the list of cluster names contained
+// within the session. If the session does not contain any cluster
+// names, it returns an empty list. If the cookie is missing, it
+// returns 200 OK with an empty request body.
+var virtualClustersHandler = http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+	sessionCookie, err := req.Cookie(authserver.SessionCookieName)
+	if err != nil {
+		if errors.Is(err, http.ErrNoCookie) {
+			w.Header().Add("Content-Type", "application/json")
+			if _, err := w.Write([]byte(`{"virtual_clusters":[]}`)); err != nil {
+				log.Errorf(req.Context(), "unable to write virtual clusters response: %s", err.Error())
+			}
+			return
+		}
+		http.Error(w, "unable to decode session cookie", http.StatusInternalServerError)
+		return
+	}
+	sessionsAndClusters := strings.Split(sessionCookie.Value, ",")
+	resp := &virtualClustersResp{
+		VirtualClusters: make([]string, 0),
+	}
+	for i, c := range sessionsAndClusters {
+		if i%2 == 1 {
+			resp.VirtualClusters = append(resp.VirtualClusters, c)
+		}
+	}
+
+	respBytes, err := json.Marshal(resp)
+	if err != nil {
+		http.Error(w, "unable to marshal virtual clusterse JSON", http.StatusInternalServerError)
+		return
+	}
+	w.Header().Add("Content-Type", "application/json")
+	if _, err := w.Write(respBytes); err != nil {
+		log.Errorf(req.Context(), "unable to write virtual clusters response: %s", err.Error())
+	}
+})
+
 func (s *httpServer) setupRoutes(
 	ctx context.Context,
 	authnServer authserver.Server,
@@ -147,6 +200,7 @@ func (s *httpServer) setupRoutes(
 	// The /login endpoint is, by definition, available pre-authentication.
 	s.mux.Handle(authserver.LoginPath, handleRequestsUnauthenticated)
 	s.mux.Handle(authserver.LogoutPath, authenticatedHandler)
+	s.mux.Handle(virtualClustersPath, virtualClustersHandler)
 	// The login path for 'cockroach demo', if we're currently running
 	// that.
 	if s.cfg.EnableDemoLoginEndpoint {

pkg/server/server_http_test.go

@@ -12,10 +12,13 @@ package server
 
 import (
 	"context"
+	"io"
+	"net/http"
 	"strings"
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/server/authserver"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -84,3 +87,99 @@ func TestHSTS(t *testing.T) {
 		require.Empty(t, resp.Header.Get(hstsHeaderKey))
 	}
 }
+
+func TestVirtualClustersSingleTenant(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	s := serverutils.StartServerOnly(t, base.TestServerArgs{})
+	defer s.Stopper().Stop(ctx)
+
+	httpClient, err := s.GetUnauthenticatedHTTPClient()
+	require.NoError(t, err)
+	defer httpClient.CloseIdleConnections()
+
+	secureClient, err := s.GetAuthenticatedHTTPClient(false, serverutils.SingleTenantSession)
+	require.NoError(t, err)
+	defer secureClient.CloseIdleConnections()
+
+	adminURLHTTPS := s.AdminURL().String()
+	adminURLHTTP := strings.Replace(adminURLHTTPS, "https", "http", 1)
+
+	resp, err := httpClient.Get(adminURLHTTP + "/virtual_clusters")
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	read, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.Equal(t, "application/json", resp.Header.Get("content-type"))
+	require.Equal(t, `{"virtual_clusters":[]}`, string(read))
+
+	resp, err = secureClient.Get(adminURLHTTPS + "/virtual_clusters")
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	read, err = io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.Equal(t, "application/json", resp.Header.Get("content-type"))
+	require.Equal(t, `{"virtual_clusters":[]}`, string(read))
+}
+
+func TestVirtualClustersMultiTenant(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	s := serverutils.StartServerOnly(t, base.TestServerArgs{
+		DefaultTestTenant: base.SharedTestTenantAlwaysEnabled,
+	})
+	defer s.Stopper().Stop(ctx)
+	appServer := s.ApplicationLayer()
+
+	httpClient, err := appServer.GetUnauthenticatedHTTPClient()
+	require.NoError(t, err)
+	defer httpClient.CloseIdleConnections()
+
+	secureClient, err := appServer.GetAuthenticatedHTTPClient(false, serverutils.MultiTenantSession)
+	require.NoError(t, err)
+	defer secureClient.CloseIdleConnections()
+
+	adminURLHTTPS := appServer.AdminURL()
+	adminURLHTTPS.Scheme = "http"
+	adminURLHTTPS.Path = adminURLHTTPS.Path + "/virtual_clusters"
+
+	resp, err := httpClient.Get(adminURLHTTPS.String())
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	read, err := io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.Equal(t, "application/json", resp.Header.Get("content-type"))
+	require.Equal(t, `{"virtual_clusters":[]}`, string(read))
+
+	adminURLHTTPS.Scheme = "https"
+	resp, err = secureClient.Get(adminURLHTTPS.String())
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	read, err = io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.Equal(t, "application/json", resp.Header.Get("content-type"))
+	require.Equal(t, `{"virtual_clusters":["test-tenant"]}`, string(read))
+
+	secureClient.Jar.SetCookies(adminURLHTTPS.URL, []*http.Cookie{
+		{
+			Name: "session",
+			Value: authserver.CreateAggregatedSessionCookieValue([]authserver.SessionCookieValue{
+				authserver.MakeSessionCookieValue("system", "session=abcd1234"),
+				authserver.MakeSessionCookieValue("app", "session=efgh5678"),
+			}),
+		},
+	})
+
+	adminURLHTTPS.Scheme = "https"
+	resp, err = secureClient.Get(adminURLHTTPS.String())
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	read, err = io.ReadAll(resp.Body)
+	require.NoError(t, err)
+	require.Equal(t, "application/json", resp.Header.Get("content-type"))
+	require.Equal(t, `{"virtual_clusters":["system","app"]}`, string(read))
+}

pkg/ui/workspaces/db-console/src/app.spec.tsx

@@ -49,6 +49,10 @@ stubComponentInModule("src/views/insights/schemaInsightsPage", "default");
 stubComponentInModule("src/views/schedules/schedulesPage", "default");
 stubComponentInModule("src/views/schedules/scheduleDetails", "default");
 stubComponentInModule("src/views/tracez_v2/snapshotPage", "default");
+stubComponentInModule(
+  "src/views/app/components/tenantDropdown/tenantDropdown",
+  "default",
+);
 
 import React from "react";
 import { Action, Store } from "redux";

pkg/ui/workspaces/db-console/src/redux/cachedDataReducer.ts

@@ -28,7 +28,7 @@ import { util as clusterUiUtil } from "@cockroachlabs/cluster-ui";
 const { isForbiddenRequestError } = clusterUiUtil;
 
 import { PayloadAction, WithRequest } from "src/interfaces/action";
-import { maybeClearTenantCookie } from "./cookies";
+import { clearTenantCookie } from "./cookies";
 
 export interface WithPaginationRequest {
   page_size: number;
@@ -323,7 +323,10 @@ export class CachedDataReducer<
             // codes.  However, at the moment that's all that the underlying
             // timeoutFetch offers.  Major changes to this plumbing are warranted.
             if (error.message === "Unauthorized") {
-              maybeClearTenantCookie();
+              // Clearing the tenant cookie is necessary when we force a login
+              // because otherwise the DB routing will continue routing to that
+              // specific tenant.
+              clearTenantCookie();
               // TODO(couchand): This is an unpleasant dependency snuck in here...
               const { location } = createHashHistory();
               if (