Merge #119748

119748: server, lint: centralize and restrict HTTP cookie construction r=dhartunian a=dhartunian

### server: centralize cookie construction

All cookie construction for session and tenant cookies is now moved to
explicit constructors instead of initializing `http.Cookie{}` structs
directly. This reduces the chance of error and explicitly requires
setting the `secure` flag via the constructor.

The constructs were created to exactly mimic all existing cookie
settings. No change in flags should happen as a result of this commit.

A separate commit will introduce a linter to prevent raw cookie
creation.

Epic: None
Release note: None

### lint: add linter to reject raw http.Cookie structs

We previously introduced bugs when creating HTTP cookies in new code
that did not have the proper fields set. This linter aims to enforce
use of cookies only through pre-generated constructors in `pkg/server/
authserver/cookie.go`. This will reduce the chance of errors in the
future and keep things consistent.

Epic: None
Release note: None

Co-authored-by: David Hartunian <[email protected]>

pkg/ccl/oidcccl/state.go

@@ -15,6 +15,7 @@ import (
 	"encoding/base64"
 	"net/http"
 
+	"github.com/cockroachdb/cockroach/pkg/server/authserver"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/errors"
@@ -61,16 +62,13 @@ func newKeyAndSignedToken(
 		return nil, err
 	}
 
-	secretKeyCookie := http.Cookie{
-		Name:     secretCookieName,
-		Value:    base64.URLEncoding.EncodeToString(secretKey),
-		Secure:   true,
-		HttpOnly: true,
-		SameSite: http.SameSiteLaxMode,
-	}
+	secretKeyCookie := authserver.CreateOIDCCookie(
+		secretCookieName,
+		base64.URLEncoding.EncodeToString(secretKey),
+	)
 
 	return &keyAndSignedToken{
-		&secretKeyCookie,
+		secretKeyCookie,
 		signedTokenEncoded,
 	}, nil
 }

pkg/server/authserver/BUILD.bazel

@@ -35,6 +35,7 @@ go_library(
         "//pkg/util/grpcutil",
         "//pkg/util/log",
         "//pkg/util/protoutil",
+        "//pkg/util/timeutil",
         "//pkg/util/uuid",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_logtags//:logtags",

pkg/server/authserver/authentication.go

@@ -328,7 +328,7 @@ func (s *authenticationServer) UserLogout(
 
 	// Send back a header which will cause the browser to destroy the cookie.
 	// See https://tools.ietf.org/search/rfc6265, page 7.
-	cookie := makeCookieWithValue("", false /* forHTTPSOnly */)
+	cookie := CreateSessionCookie("", false /* forHTTPSOnly */)
 	cookie.MaxAge = -1
 
 	// Set the cookie header on the outgoing response.
@@ -550,17 +550,7 @@ func EncodeSessionCookie(
 		return nil, errors.Wrap(err, "session cookie could not be encoded")
 	}
 	value := base64.StdEncoding.EncodeToString(cookieValueBytes)
-	return makeCookieWithValue(value, forHTTPSOnly), nil
-}
-
-func makeCookieWithValue(value string, forHTTPSOnly bool) *http.Cookie {
-	return &http.Cookie{
-		Name:     SessionCookieName,
-		Value:    value,
-		Path:     "/",
-		HttpOnly: true,
-		Secure:   forHTTPSOnly,
-	}
+	return CreateSessionCookie(value, forHTTPSOnly), nil
 }
 
 // getSession decodes the cookie from the request, looks up the corresponding session, and

pkg/server/authserver/cookie.go

@@ -20,6 +20,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server/srverrors"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 )
 
@@ -174,3 +175,81 @@ func findTenantSelectCookieValue(cookies []*http.Cookie) string {
 	}
 	return ""
 }
+
+// CreateSessionCookie constructs an HTTP cookie that holds a DB
+// Console session. This cookie is always marked `HttpOnly` and can be
+// optionally marked `Secure` if the cluster is running in secure mode,
+// based on the `forHTTPSOnly` arg.
+func CreateSessionCookie(value string, forHTTPSOnly bool) *http.Cookie {
+	return &http.Cookie{
+		Name:     SessionCookieName,
+		Value:    value,
+		Path:     "/",
+		HttpOnly: true,
+		Secure:   forHTTPSOnly,
+	}
+}
+
+// CreateEmptySessionCookieWithImmediateExpiry constructs an HTTP
+// cookie that clears the session cookie by setting an empty cookie
+// with the same name and an expiry at the Unix epoch. This will cause
+// the browser to clear the cookie since it expires immediately.
+func CreateEmptySessionCookieWithImmediateExpiry(forHTTPSOnly bool) *http.Cookie {
+	return &http.Cookie{
+		Name:     SessionCookieName,
+		Value:    "",
+		Path:     "/",
+		HttpOnly: true,
+		Secure:   forHTTPSOnly,
+		Expires:  timeutil.Unix(0, 0),
+	}
+}
+
+// CreateTenantSelectCookie constructs an HTTP cookie that holds a DB Console
+// tenant selection. This cookis is **not** marked `HttpOnly` because
+// its value can be inspected and modified by the client-side
+// Javascript code if the user changes their tenant selection. It can
+// be optionally marked `Secure` if you're runinning in secure mode by
+// setting the `forHttpsOnly` argument to true.
+func CreateTenantSelectCookie(value string, forHTTPSOnly bool) *http.Cookie {
+	return &http.Cookie{
+		Name:     TenantSelectCookieName,
+		Value:    value,
+		Path:     "/",
+		HttpOnly: false,
+		Secure:   forHTTPSOnly,
+	}
+}
+
+// CreateEmptyTenantSelectCookieWithImmediateExpiry constructs an HTTP
+// cookie that clears the tenant cookie by setting an empty cookie
+// with the same name and an expiry at the Unix epoch. This will cause
+// the browser to clear the cookie since it expires immediately.
+func CreateEmptyTenantSelectCookieWithImmediateExpiry(forHTTPSOnly bool) *http.Cookie {
+	return &http.Cookie{
+		Name:     TenantSelectCookieName,
+		Value:    "",
+		Path:     "/",
+		HttpOnly: false,
+		Secure:   forHTTPSOnly,
+		Expires:  timeutil.Unix(0, 0),
+	}
+}
+
+// CreateOIDCCookie constructs a cookie to hold the OIDC secret that's
+// used to validate requests between `/login` and `/callback` requests.
+// This cookie contains a hash that's shared by the browser between the
+// two requests and is used to validate that the `/callback` was
+// triggered in response to a valid login attempt from this cluster.
+// This cookie is **always** `Secure` and `HttpOnly` since OIDC doesn't
+// work on an insecure cluster. Its name is an argument to avoid a CCL
+// dependency.
+func CreateOIDCCookie(name string, value string) *http.Cookie {
+	return &http.Cookie{
+		Name:     name,
+		Value:    value,
+		HttpOnly: true,
+		Secure:   true,
+		SameSite: http.SameSiteLaxMode,
+	}
+}

pkg/server/server_controller_http.go

@@ -23,7 +23,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 )
 
@@ -98,22 +97,8 @@ 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:     authserver.SessionCookieName,
-			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),
-		})
+		http.SetCookie(w, authserver.CreateEmptySessionCookieWithImmediateExpiry(!c.disableTLSForHTTP))
+		http.SetCookie(w, authserver.CreateEmptyTenantSelectCookieWithImmediateExpiry(!c.disableTLSForHTTP))
 		// Fall back to serving requests from the default tenant. This helps us serve
 		// the root path along with static assets even when the browser contains invalid
 		// tenant names or sessions (common during development). Otherwise the user can
@@ -233,14 +218,7 @@ func (c *serverController) attemptLoginToAllTenants() http.Handler {
 		// for any of the tenants.
 		if len(tenantNameToSetCookieSlice) > 0 {
 			sessionsStr := authserver.CreateAggregatedSessionCookieValue(tenantNameToSetCookieSlice)
-			cookie := http.Cookie{
-				Name:     authserver.SessionCookieName,
-				Value:    sessionsStr,
-				Path:     "/",
-				HttpOnly: true,
-				Secure:   !c.disableTLSForHTTP,
-			}
-			http.SetCookie(w, &cookie)
+			http.SetCookie(w, authserver.CreateSessionCookie(sessionsStr, !c.disableTLSForHTTP /* forHTTPSOnly */))
 			// The tenant cookie needs to be set at some point in order for
 			// the dropdown to have a current selection on first load.
 
@@ -255,14 +233,7 @@ func (c *serverController) attemptLoginToAllTenants() http.Handler {
 					break
 				}
 			}
-			cookie = http.Cookie{
-				Name:     authserver.TenantSelectCookieName,
-				Value:    tenantSelection,
-				Path:     "/",
-				HttpOnly: false,
-				Secure:   !c.disableTLSForHTTP,
-			}
-			http.SetCookie(w, &cookie)
+			http.SetCookie(w, authserver.CreateTenantSelectCookie(tenantSelection, !c.disableTLSForHTTP /* forHTTPSOnly */))
 			if r.Header.Get(AcceptHeader) == JSONContentType {
 				w.Header().Add(ContentTypeHeader, JSONContentType)
 				_, err = w.Write([]byte("{}"))
@@ -353,24 +324,9 @@ func (c *serverController) attemptLogoutFromAllTenants() http.Handler {
 			}
 		}
 		// Clear session and tenant cookies after all logouts have completed.
-		cookie := http.Cookie{
-			Name:     authserver.SessionCookieName,
-			Value:    "",
-			Path:     "/",
-			HttpOnly: true,
-			Secure:   !c.disableTLSForHTTP,
-			Expires:  timeutil.Unix(0, 0),
-		}
-		http.SetCookie(w, &cookie)
-		cookie = http.Cookie{
-			Name:     authserver.TenantSelectCookieName,
-			Value:    "",
-			Path:     "/",
-			HttpOnly: false,
-			Secure:   !c.disableTLSForHTTP,
-			Expires:  timeutil.Unix(0, 0),
-		}
-		http.SetCookie(w, &cookie)
+		http.SetCookie(w, authserver.CreateEmptySessionCookieWithImmediateExpiry(!c.disableTLSForHTTP /* forHTTPSOnly */))
+		http.SetCookie(w, authserver.CreateEmptyTenantSelectCookieWithImmediateExpiry(!c.disableTLSForHTTP /* forHTTPSOnly */))
+
 		if r.Header.Get(AcceptHeader) == JSONContentType {
 			w.Header().Add(ContentTypeHeader, JSONContentType)
 			_, err = w.Write([]byte("{}"))

pkg/testutils/lint/lint_test.go

@@ -2508,4 +2508,39 @@ func TestLint(t *testing.T) {
 		repoRoot := filepath.Join("../../../")
 		codeowners.LintEverythingIsOwned(t, verbose, co, repoRoot, "pkg")
 	})
+
+	t.Run("cookie construction is forbidden", func(t *testing.T) {
+		t.Parallel()
+		cmd, stderr, filter, err := dirCmd(
+			pkgDir,
+			"git",
+			"grep",
+			"-nE",
+			`\.Cookie\{`,
+			"--",
+			":!*_test.go",
+			":!*authserver/cookie.go",
+			":!*roachtest*",
+			":!*testserver*",
+		)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if err := cmd.Start(); err != nil {
+			t.Fatal(err)
+		}
+
+		if err := stream.ForEach(filter, func(s string) {
+			t.Errorf("\n%s <- forbidden; use constructors in `authserver/cookie.go` instead", s)
+		}); err != nil {
+			t.Error(err)
+		}
+
+		if err := cmd.Wait(); err != nil {
+			if out := stderr.String(); len(out) > 0 {
+				t.Fatalf("err=%s, stderr=%s", err, out)
+			}
+		}
+	})
 }