apiv2: accept cookie auth when header is non-empty

[dhartunian]

In order to make use of HTTP endpoints under /api/v2 in the DB Console
it is necessary to support cookie-based authentication for ergonomic
Javascript use.

Previously, header-based auth was not possible to use in the DB Console
because the login endpoint we use returns the session in a Cookie.
Moving this cookie into a header would require us to read into a
less-secure storage method (local storage, redux, etc.) instead of
keeping it secure in the browser's cookie storage.

We implement a suggestion to rely on Cookie auth by requiring the
presence of the auth header with a magic value of "cookie" that tells the
server to look for the session in the session cookie. This forces the caller
to modify the request via JS, which protects us from CSRF since
cross-origin requests can only be "simple". See the issue for further
discussion.

Resolves #84311

Release note (security update): The HTTP endpoints under the /api/v2 prefix
will now accept cookie-based authentication similar to other HTTP endpoints
used by the DB Console. The encoded session must be in a cookie named
"session", and the "X-Cockroach-API-Session" header is required to be set
to "cookie" for the session to be read from the cookie header. A cookie
provided without the custom header present will be ignored.

[cockroach-teamcity]

This change is 

[knz]

May I recommend a release note in category "security update" that explains the change, e.g. "The /api/v2 HTTP endpoints now accept authentication information using a cookie (FIXME: insert required cookie name here), as long as the X-Cockroach-API-Session header is also present in the request and has a non-empty value."

I would also like to suggest that you fix the expected value of the header to a specific string, e.g. yes in your test, and make it part of your check. This way, we could use different values in the header in the future to support multiple authn protocols side-by-side.

Reviewed 3 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker, @kpatron-cockroachlabs, and @xinhaoz)

[kpatron-cockroachlabs]

For my own curiosity, is there a reason we use a custom header for the API token instead of the standard Authorization: Bearer <value> pattern?

From a person only looking at the client interactions it may make things slightly easier to understand if we use a separate header for CSRF protection vs API token.

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

[dhartunian]

@knz do you know of a historical reason why we don't use the Authorization: Bearer pattern?

@kpatron-cockroachlabs thanks for the suggestion. Perhaps I'll use X-Cockroach-API-Auth with a value of cookie to handle this particular case.

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

[knz]

do you know of a historical reason why we don't use the Authorization: Bearer pattern?

When this was first implemented I have a vague recollection of us (@bdarnell , @itsbilal and I) discussing CSRF protection and one of us determined a custom header was an existing established practice. Maybe Ben and/or Bilal can comment further?

[kpatron-cockroachlabs]

My understanding is the Authorization header should also satisfy those CSRF protections as well.

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

[xinhaoz]

I tested my changes on top of this and everything works as expected. Thanks for this, David!

Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian)

[ericharmeling]

I tested my changes on top of this and everything works as expected. Thanks for this, David!

Same (using @xinhaoz's work)! #84612

[dhartunian]

I've decided that this PR is not the forum to modify the auth session header name and, am conservatively keeping the one custom header we have for now.

In the case of cookie-based auth, the session header will require a specific value of "cookie" in order to read the session from the cookie.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @knz and @xinhaoz)

[dhartunian]

@knz PTAL

[knz]

modulo nits

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @dhartunian)

---

pkg/server/api_v2_auth.go line 290 at r2 (raw file):

// session can be looked up either from a session cookie as used in the
// non-v2 API server, or via the session header. In order for us to pick
// up a session, the header must still be non-empty in the case of

nit: update comment to mention 'cookie' keyword.

---

pkg/server/api_v2_auth.go line 322 at r2 (raw file):

	for i := range possibleSessions {
		decoded, err = base64.StdEncoding.DecodeString(possibleSessions[i])
		if err != nil {

here and below. log.Warningf for the error.

---

pkg/server/api_v2_auth.go line 329 at r2 (raw file):

			continue
		}
		// We've successfully decoded a session from cookie or header

nit: period at end of sentnece.

[dhartunian]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @knz)

---

pkg/server/api_v2_auth.go line 290 at r2 (raw file):

Previously, knz (kena) wrote…

nit: update comment to mention 'cookie' keyword.

Done.

---

pkg/server/api_v2_auth.go line 322 at r2 (raw file):

Previously, knz (kena) wrote…

here and below. log.Warningf for the error.

Done.

---

pkg/server/api_v2_auth.go line 329 at r2 (raw file):

Previously, knz (kena) wrote…

nit: period at end of sentnece.

Done.

[dhartunian]

TFTRs

bors r=knz,kpatron-cockroachlabs,xinhaoz

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from b5bad38 to blathers/backport-release-22.1-84617: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}