Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server, ui: set HttpOnly: true and Secure flags on cookies #119261

Merged
merged 2 commits into from
Mar 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 13 additions & 0 deletions pkg/ccl/serverccl/server_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,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)
Expand Down Expand Up @@ -558,6 +559,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)
Expand All @@ -582,6 +587,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)
Expand All @@ -607,6 +616,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)
Expand Down
1 change: 1 addition & 0 deletions pkg/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -1229,6 +1229,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (serverctl.ServerStartupInterf
pgPreServer.SendRoutingError,
tenantCapabilitiesWatcher,
cfg.DisableSQLServer,
cfg.BaseConfig.DisableTLSForHTTP,
)
drain.serverCtl = sc

Expand Down
4 changes: 4 additions & 0 deletions pkg/server/server_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ type serverController struct {

watcher *tenantcapabilitieswatcher.Watcher

disableTLSForHTTP bool

mu struct {
syncutil.RWMutex

Expand Down Expand Up @@ -128,6 +130,7 @@ func newServerController(
sendSQLRoutingError func(ctx context.Context, conn net.Conn, tenantName roachpb.TenantName),
watcher *tenantcapabilitieswatcher.Watcher,
disableSQLServer bool,
disableTLSForHTTP bool,
) *serverController {
c := &serverController{
AmbientContext: ambientCtx,
Expand All @@ -141,6 +144,7 @@ func newServerController(
tenantWaiter: singleflight.NewGroup("tenant server poller", "poll"),
drainCh: make(chan struct{}),
disableSQLServer: disableSQLServer,
disableTLSForHTTP: disableTLSForHTTP,
}
c.orchestrator = newServerOrchestrator(parentStopper, c)
c.mu.servers = map[roachpb.TenantName]serverState{
Expand Down
10 changes: 8 additions & 2 deletions pkg/server/server_controller_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
54 changes: 54 additions & 0 deletions pkg/server/server_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ package server
import (
"context"
"crypto/tls"
"encoding/json"
"net/http"
"strings"

"github.com/NYTimes/gziphandler"
"github.com/cockroachdb/cmux"
Expand All @@ -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"
)

Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
99 changes: 99 additions & 0 deletions pkg/server/server_http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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))
}
4 changes: 4 additions & 0 deletions pkg/ui/workspaces/db-console/src/app.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
7 changes: 5 additions & 2 deletions pkg/ui/workspaces/db-console/src/redux/cachedDataReducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 (
Expand Down