server, lint: centralize and restrict HTTP cookie construction

[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

[cockroach-teamcity]

This change is 

[dhartunian]

Please only review last 2 commits. First two getting merged here: #119261

[stevendanna]

Thanks for going the extra mile here.

I left some comments but neither are blocking.

[stevendanna]

You've already written it so I wouldn't change it. But I do wonder for a linter like this whether we get most of the benefit out of a git-grep based linter like some of the others in pkg/testutils/lint_test.go, which I only mention because I imagine it would be a lot faster.

That said, this lays the groundwork for adding more checks in the future around aspects of cookie creation if we want.

[dhartunian]

I'm just learning about the linter infra so I'll give this a quick shot and see if I get anywhere.

[dhartunian]

git grep linter is way simpler so I'm keeping it. thanks for the pointer.

[abarganier]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian, @stevendanna, and @xinhaoz)

---

pkg/server/server_controller_http.go line 101 at r5 (raw file):

		// Clear session and tenant cookies since it appears they reference invalid state.
		http.SetCookie(w, authserver.CreateEmptySessionCookieWithImmediateExpiry(!c.disableTLSForHTTP))
		http.SetCookie(w, authserver.CreateEmptyTenantCookieWithImmediateExpiry(!c.disableTLSForHTTP))

Nice! This is a big win for readability. Good idea.

Code quote:

authserver.CreateEmptyTenantCookieWithImmediateExpiry

---

pkg/server/authserver/cookie.go line 214 at r5 (raw file):

// be optionally marked `Secure` if you're runinning in secure mode by
// setting the `forHttpsOnly` argument to true.
func CreateTenantCookie(value string, forHTTPSOnly bool) *http.Cookie {

nit: Here and below, would TenantSelect instead of just Tenant be a bit clearer?

Code quote:

CreateTenantCookie

[dhartunian]

TFTRs

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)