Skip to content

Commit

Permalink
Merge pull request #119248 from dhartunian/23.1-cookie-settings-fixes
Browse files Browse the repository at this point in the history
release-23.1.15-rc: server, ui: set `HttpOnly: true` and `Secure` flags on cookies
dhartunian authored Feb 15, 2024
2 parents ada9a9a + 266f3cb commit 801d369
Showing 15 changed files with 395 additions and 93 deletions.
13 changes: 13 additions & 0 deletions pkg/ccl/serverccl/server_controller_test.go
Original file line number Diff line number Diff line change
@@ -362,6 +362,7 @@ func TestServerControllerDefaultHTTPTenant(t *testing.T) {
for _, c := range resp.Cookies() {
if c.Name == server.TenantSelectCookieName {
tenantCookie = c.Value
require.True(t, c.Secure)
}
}
require.Equal(t, "hello", tenantCookie)
@@ -579,6 +580,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)
@@ -600,6 +605,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)
@@ -627,6 +636,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)
10 changes: 7 additions & 3 deletions pkg/ccl/serverccl/tenant_vars_test.go
Original file line number Diff line number Diff line change
@@ -12,6 +12,7 @@ import (
"context"
"crypto/tls"
"net/http"
"net/url"
"os"
"testing"

@@ -62,13 +63,16 @@ func TestTenantVars(t *testing.T) {
}

startNowNanos := timeutil.Now().UnixNano()
url := tenant.AdminURL() + "/_status/load"
urlParsed, err := url.Parse(tenant.AdminURL())
require.NoError(t, err)
urlParsed.Path = urlParsed.Path + "/_status/load"
urlString := urlParsed.String()
client := http.Client{
Transport: &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
},
}
resp, err := client.Get(url)
resp, err := client.Get(urlString)
require.NoError(t, err)
defer resp.Body.Close()
require.Equal(t, 200, resp.StatusCode,
@@ -115,7 +119,7 @@ func TestTenantVars(t *testing.T) {

require.LessOrEqual(t, 0., uptimeSeconds)

resp, err = client.Get(url)
resp, err = client.Get(urlString)
require.NoError(t, err)
defer resp.Body.Close()
require.Equal(t, 200, resp.StatusCode,
1 change: 1 addition & 0 deletions pkg/server/server.go
Original file line number Diff line number Diff line change
@@ -1128,6 +1128,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
&systemServerWrapper{server: lateBoundServer},
systemTenantNameContainer,
pgPreServer.SendRoutingError,
cfg.BaseConfig.DisableTLSForHTTP,
)
drain.serverCtl = sc

4 changes: 4 additions & 0 deletions pkg/server/server_controller.go
Original file line number Diff line number Diff line change
@@ -85,6 +85,8 @@ type serverController struct {
// orchestrator is the orchestration method to use.
orchestrator serverOrchestrator

disableTLSForHTTP bool

mu struct {
syncutil.Mutex

@@ -111,6 +113,7 @@ func newServerController(
systemServer onDemandServer,
systemTenantNameContainer *roachpb.TenantNameContainer,
sendSQLRoutingError func(ctx context.Context, conn net.Conn, tenantName roachpb.TenantName),
disableTLSForHTTP bool,
) *serverController {
c := &serverController{
AmbientContext: ambientCtx,
@@ -121,6 +124,7 @@ func newServerController(
stopper: parentStopper,
tenantServerCreator: tenantServerCreator,
sendSQLRoutingError: sendSQLRoutingError,
disableTLSForHTTP: disableTLSForHTTP,
}
c.orchestrator = newServerOrchestrator(parentStopper, c)
c.mu.servers = map[roachpb.TenantName]serverState{
10 changes: 8 additions & 2 deletions pkg/server/server_controller_http.go
Original file line number Diff line number Diff line change
@@ -81,13 +81,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: 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
@@ -213,7 +215,8 @@ func (c *serverController) attemptLoginToAllTenants() http.Handler {
Name: 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
@@ -235,6 +238,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 {
@@ -331,7 +335,8 @@ func (c *serverController) attemptLogoutFromAllTenants() http.Handler {
Name: SessionCookieName,
Value: "",
Path: "/",
HttpOnly: false,
HttpOnly: true,
Secure: !c.disableTLSForHTTP,
Expires: timeutil.Unix(0, 0),
}
http.SetCookie(w, &cookie)
@@ -340,6 +345,7 @@ func (c *serverController) attemptLogoutFromAllTenants() http.Handler {
Value: "",
Path: "/",
HttpOnly: false,
Secure: !c.disableTLSForHTTP,
Expires: timeutil.Unix(0, 0),
}
http.SetCookie(w, &cookie)
54 changes: 54 additions & 0 deletions pkg/server/server_http.go
Original file line number Diff line number Diff line change
@@ -13,7 +13,9 @@ package server
import (
"context"
"crypto/tls"
"encoding/json"
"net/http"
"strings"

"github.com/NYTimes/gziphandler"
"github.com/cockroachdb/cmux"
@@ -28,6 +30,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"
)

@@ -109,6 +112,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(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 *authenticationServer,
@@ -167,6 +220,7 @@ func (s *httpServer) setupRoutes(
// The /login endpoint is, by definition, available pre-authentication.
s.mux.Handle(loginPath, handleRequestsUnauthenticated)
s.mux.Handle(logoutPath, authenticatedHandler)
s.mux.Handle(virtualClustersPath, virtualClustersHandler)
// The login path for 'cockroach demo', if we're currently running
// that.
if s.cfg.EnableDemoLoginEndpoint {
103 changes: 103 additions & 0 deletions pkg/server/server_http_test.go
Original file line number Diff line number Diff line change
@@ -12,10 +12,14 @@ package server

import (
"context"
"io"
"net/http"
"net/url"
"strings"
"testing"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -84,3 +88,102 @@ 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.StartServer(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()
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.StartServer(t, base.TestServerArgs{})
defer s.Stopper().Stop(ctx)
appServer, _, err := s.StartSharedProcessTenant(ctx,
base.TestSharedProcessTenantArgs{
TenantName: roachpb.TenantName("test-tenant"),
})
require.NoError(t, err)

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, err := url.Parse(appServer.AdminURL())
require.NoError(t, err)
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, []*http.Cookie{
{
Name: "session",
Value: createAggregatedSessionCookieValue([]sessionCookieValue{
{"system", "session=abcd1234"},
{"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))
}
1 change: 1 addition & 0 deletions pkg/server/testserver.go
Original file line number Diff line number Diff line change
@@ -938,6 +938,7 @@ func (ts *TestServer) StartSharedProcessTenant(
hts := &httpTestServer{}
hts.t.authentication = sqlServerWrapper.authentication
hts.t.sqlServer = sqlServer
hts.t.tenantName = args.TenantName
testTenant := &TestTenant{
sql: sqlServer,
Cfg: sqlServer.cfg,
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
@@ -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";
Loading

0 comments on commit 801d369

Please sign in to comment.