feat(security): set SameSite=strict on session cookie (#23723)

* feat(security): set SameSite=strict on session cookie

Use SameSite=Strict as a hardening measure against cross-origin attacks.
While browsers have been moving to default to SameSite=Lax, explicitly
setting SameSite ensures that all browsers enforce it consistently.
While 'lax' is a reasonable hardening choice, the cookie is only
required for requests to '/api/...' and we don't expect 3rd party links
into '/api/...', so this stricter setting should be safe in terms of
usability. Furthermore, while our GET APIs are not state-changing, using
'strict' future-proofs us in case we add a state-changing GET API ('lax'
allows cross-origin 'GET' requests for increased usability for read-only
requests).

Also add a comment to SetCORS() lack of Access-Control-Allow-Credentials
as a reminder that its omission is intentional for defense in depth on
when to attach the cookie to a request.

* chore: mention that Lax sends the cookie with other safe HTTP methods

kit/transport/http/middleware.go

@@ -20,6 +20,18 @@ import (
 // Middleware constructor.
 type Middleware func(http.Handler) http.Handler
 
+// SetCORS configures various headers to relax CORS browser checks
+//
+// Browsers implement Cross-Origin Resource Sharing (CORS) checks to limit
+// cross-origin accesses in HTTP requests. Various CORS headers can be used to
+// relax the standard, strict browser behavior. For details on CORS, see:
+// https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS
+//
+// IMPORTANT: default CORS checks disallows attaching credentials (eg, session
+// cookie) with cross-origin requests (even when Access-Control-Allow-Origin is
+// set to the Origin) and we omit the 'Access-Control-Allow-Credentials' header
+// here to preserve this behavior. Omitting this header provides security
+// defense-in-depth.
 func SetCORS(next http.Handler) http.Handler {
 	fn := func(w http.ResponseWriter, r *http.Request) {
 		if origin := r.Header.Get("Origin"); origin != "" {

session/http_server.go

@@ -164,11 +164,50 @@ func decodeSignoutRequest(ctx context.Context, r *http.Request) (*signoutRequest
 const cookieSessionName = "influxdb-oss-session"
 
 func encodeCookieSession(w http.ResponseWriter, s *influxdb.Session) {
+	// We only need the session cookie for accesses to "/api/...", so limit
+	// it to that using "Path".
+	//
+	// Since the cookie is limited to "/api/..." and we don't expect any
+	// links directly into /api/..., use SameSite=Strict as a hardening
+	// measure. This works because external links into the UI have the form
+	// https://<url>/orgs/<origid>/..., https://<url>/signin, etc and don't
+	// need the cookie sent. By the time the UI itself calls out to
+	// /api/..., the location bar matches the cookie's domain and
+	// everything is 1st party and Strict's restriction work fine.
+	//
+	// SameSite=Lax would also be safe to use (modern browser's default if
+	// unset) since it only sends the cookie with GET (and other safe HTTP
+	// methods like HEAD and OPTIONS as defined in RFC6264) requests when
+	// the location bar matches the domain of the cookie and we know that
+	// our APIs do not perform state-changing actions with GET and other
+	// safe methods. Using SameSite=Strict helps future-proof us against
+	// that changing (ie, we add a state-changing GET API).
+	//
+	// Note: it's generally recommended that SameSite should not be relied
+	// upon (particularly Lax) because:
+	// a) SameSite doesn't work with (cookie-less) Basic Auth. We don't
+	//    share browser session BasicAuth with accesses to to /api/... so
+	//    this isn't a problem
+	// b) SameSite=lax allows GET (and other safe HTTP methods) and some
+	//    services might allow state-changing requests via GET. Our API
+	//    doesn't support state-changing GETs and SameSite=strict doesn't
+	//    allow GETs from 3rd party sites at all, so this isn't a problem
+	// c) similar to 'b', some frameworks will accept HTTP methods for
+	//    other handlers. Eg, the application is designed for POST but it
+	//    will accept requests converted to the GET method. Golang does not
+	//    do this itself and our route mounts explicitly map the HTTP
+	//    method to the specific handler and thus we are not susceptible to
+	//    this
+	// d) SameSite could be bypassed if the attacker were able to
+	//    manipulate the location bar in the browser (a serious browser
+	//    bug; it is reasonable for us to expect browsers to enforce their
+	//    SameSite restrictions)
 	c := &http.Cookie{
-		Name:    cookieSessionName,
-		Value:   s.Key,
-		Path:    "/api/",
-		Expires: s.ExpiresAt,
+		Name:     cookieSessionName,
+		Value:    s.Key,
+		Path:     "/api/", // since UI doesn't need it, limit cookie usage to API requests
+		Expires:  s.ExpiresAt,
+		SameSite: http.SameSiteStrictMode,
 	}
 
 	http.SetCookie(w, c)

session/http_server_test.go

@@ -58,7 +58,7 @@ func TestSessionHandler_handleSignin(t *testing.T) {
 				password: "supersecret",
 			},
 			wants: wants{
-				cookie: "influxdb-oss-session=abc123xyz; Path=/api/; Expires=Thu, 26 Sep 2030 00:00:00 GMT",
+				cookie: "influxdb-oss-session=abc123xyz; Path=/api/; Expires=Thu, 26 Sep 2030 00:00:00 GMT; SameSite=Strict",
 				code:   http.StatusNoContent,
 			},
 		},