From cc4b8b1e386901644b0526fac5023f12e6dbc79d Mon Sep 17 00:00:00 2001 From: David Hartunian Date: Wed, 28 Feb 2024 17:02:10 -0500 Subject: [PATCH 1/2] 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 --- pkg/ccl/oidcccl/state.go | 14 ++--- pkg/server/authserver/BUILD.bazel | 1 + pkg/server/authserver/authentication.go | 14 +---- pkg/server/authserver/cookie.go | 79 +++++++++++++++++++++++++ pkg/server/server_controller_http.go | 58 +++--------------- 5 files changed, 95 insertions(+), 71 deletions(-) diff --git a/pkg/ccl/oidcccl/state.go b/pkg/ccl/oidcccl/state.go index dc94044a7cb7..096220acdbe0 100644 --- a/pkg/ccl/oidcccl/state.go +++ b/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 } diff --git a/pkg/server/authserver/BUILD.bazel b/pkg/server/authserver/BUILD.bazel index 53cc66af0c48..54fe525f2728 100644 --- a/pkg/server/authserver/BUILD.bazel +++ b/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", diff --git a/pkg/server/authserver/authentication.go b/pkg/server/authserver/authentication.go index 87e7a856791f..55297d3b809b 100644 --- a/pkg/server/authserver/authentication.go +++ b/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 diff --git a/pkg/server/authserver/cookie.go b/pkg/server/authserver/cookie.go index d8145a5fea8c..58cecff94cf7 100644 --- a/pkg/server/authserver/cookie.go +++ b/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, + } +} diff --git a/pkg/server/server_controller_http.go b/pkg/server/server_controller_http.go index fee09cf7fdf5..c82ced16d534 100644 --- a/pkg/server/server_controller_http.go +++ b/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("{}")) From d63936b94eec8f1c1c35cd90e75b5ae887116835 Mon Sep 17 00:00:00 2001 From: David Hartunian Date: Mon, 4 Mar 2024 17:42:13 -0500 Subject: [PATCH 2/2] 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 --- pkg/testutils/lint/lint_test.go | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/pkg/testutils/lint/lint_test.go b/pkg/testutils/lint/lint_test.go index 2e6a2f4ce0c1..f369c8a466a8 100644 --- a/pkg/testutils/lint/lint_test.go +++ b/pkg/testutils/lint/lint_test.go @@ -2511,4 +2511,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) + } + } + }) }