Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: support cookie-based auth for read-only queries for sql-over-http #84311

Closed
dhartunian opened this issue Jul 12, 2022 · 4 comments · Fixed by #84617
Closed

api: support cookie-based auth for read-only queries for sql-over-http #84311

dhartunian opened this issue Jul 12, 2022 · 4 comments · Fixed by #84617
Assignees
Labels
A-authentication Pertains to authn subsystems A-observability-inf A-webui-general Issues on the DB Console that span multiple areas or don't have another clear category. A-webui-security C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@dhartunian
Copy link
Collaborator

dhartunian commented Jul 12, 2022

Is your feature request related to a problem? Please describe.
Currently, the JS code in DB Console uses cookie-based auth for all requests to CRDB. This works smoothly because the browser handles all the cookie storage and security and the cookie stored with the httponly tag that prevents any JS access.

This model presents some struggles with attempting to use endpoints under the newer /api/v2 prefix since these endpoints weren't built with a UI as a primary consumer. They implement header-based authentication, and do not accept cookies.

Attempting to use the header-based auth in DB console in order to use the recently merged SQL-over-HTTP endpoint (#79663) has presented some challenges because we need to add the session auth header but have no programmatic access to it since the browser hides it from us in the list of cookies.

Describe the solution you'd like
Ideally, the /api/v2 should accept cookie-based authentication just like the rest of the endpoints we currently rely on since this is how UIs like the DB Console expect to authenticate with backends and it lets us rely on browser-based security models instead of storing our own auth header...somewhere else.

One proposal from @knz is to allow cookie-based auth for read-only queries only instead of the current read/write ability in the new endpoint. Observability needs are almost entirely read-only and we can find other solutions or custom gRPC endpoints for write endpoints if we have to. This can mitigate the blast radius of security CSRF attacks while allowing for smoother DB Console development.

Describe alternatives you've considered

  • The UI could login with the /api/v2 endpoint separately and store the session in redux (does not persist between browser sessions) or local storage (persists between browser sessions but is insecure)
  • /api/v2 auth code could be modified to accept cookie-based auth. (reduces security for all API endpoints)
  • /api/v2 auth code could accept cookie-based auth but only with a CSRF token which would be retrieved in a separate call (this adds complexity to the UI code).

Slack thread

Jira issue: CRDB-17583

@dhartunian dhartunian added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-observability-inf labels Jul 12, 2022
@knz knz added A-webui-security A-webui-general Issues on the DB Console that span multiple areas or don't have another clear category. A-authentication Pertains to authn subsystems labels Jul 12, 2022
@knz
Copy link
Contributor

knz commented Jul 12, 2022

Based on our convo today I think a good next step would be to define a cookie-only read-only alternative to the current endpoint. This could also leverage follower reads for better performance. We can then discuss other options after the initial prototype is out.

@bdarnell @kpatron-cockroachlabs - an extra 👀 from you would be welcome on this.

@sjbarag
Copy link
Contributor

sjbarag commented Jul 12, 2022

local storage (persists between browser sessions but is insecure)

IIRC browsers provide per-domain isolation of localStorage — can you elaborate on what's insecure about putting a session token in localStorage? Maybe I'm mis-remembering things 😅

@sjbarag
Copy link
Contributor

sjbarag commented Jul 12, 2022

local storage (persists between browser sessions but is insecure)

IIRC browsers provide per-domain isolation of localStorage — can you elaborate on what's insecure about putting a session token in localStorage? Maybe I'm mis-remembering things 😅

Ah, @xinhaoz just linked me to https://cheatsheetseries.owasp.org/cheatsheets/HTML5_Security_Cheat_Sheet.html#local-storage , so that makes sense.

@kpatron-cockroachlabs
Copy link
Contributor

I'm personally a fan of continuing with HttpOnly cookies so that even if there is an XSS vulnerability the attacker can't exfiltrate the credential, which makes attacks harder. If for some reason that is not feasible I'd personally prefer non-HttpOnly cookies to LocalStorage as I believe LocalStorage doesn't allow expiring values nor the equivalent "session" cookie functionality.

I think preventing CSRF attacks would require less effort than seems to be implied on this issue and the Slack thread. Some ways to solve the problem:

  • Set the SameSite property on the cookie. Notably the Lax value is already the default on some modern browsers which allows users to navigate via third party links to our sites but restricts other types of requests (no loading images for example). We'd still want to manually set the SameSite field as Firefox and Safari don't appear to treat Lax as the default.
  • Require a content-type header of application/json (or similar) for all cookie authed POST requests. As this doc lays out, you only can do SIMPLE cross origin requests. These requirements are very strict and include the request is only SIMPLE if the HTTP verb is GET HEAD or POST and if the Content-Type is text/plain, multipart/form-data, or application/x-www-form-urlencoded. It is important to note that non-SIMPLE requests do not run the risk of CSRF as the browser will block them unless the proper Access-control-* headers are populated by the server in response to options requests.
  • Similar to above, we could guarantee that by a request isn't simple by having the server check that an additional header has a value. This header could be Authorization or could be X-CRL-CUSTOM-STUFF. The contents of these headers wouldn't need to be any in particular. Enforcing that they have any value would be sufficient.

Some notes:

  • SameSite=Lax and setting the content type requires that we don't have GET endpoints that have side effects or are excessively expensive. This is already a best practice but this would up the stakes.
  • Setting an additional header or setting SameSite=secure would also protect the GET endpoints although the latter might overprotect by preventing even following links (this would be fine for the API but problematic for other uses of this cookie).

@dhartunian dhartunian self-assigned this Jul 14, 2022
dhartunian added a commit to dhartunian/cockroach that referenced this issue Jul 18, 2022
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 but ignoring its value. 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 cockroachdb#84311

Release note: None
dhartunian added a commit to dhartunian/cockroach that referenced this issue Aug 1, 2022
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 cockroachdb#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.
craig bot pushed a commit that referenced this issue Aug 2, 2022
83120: server: include system and meta ranges in hot ranges report  r=koorosh a=koorosh

 `status.HotRangesV2` endpoint iterates over ranges that have the highest
 QPS, and it might include meta- or system ranges that don't have
 table descriptors. Before, it didn't include such ranges that 
 didn't provide users full information on what ranges are hot even
 if it's a system range.

 The current patch includes system and meta ranges in the Hot Ranges list
 with limited information (that doesn't include table-specific data).

Release note: None

Resolves #83117

84617: apiv2: accept cookie auth when header is non-empty r=knz,kpatron-cockroachlabs,xinhaoz a=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.

85421: explain: print "FULL SCAN (SOFT LIMIT)" for full scans with soft limits r=rytaft,mgartner a=michae2

Change `EXPLAIN` output of full scans with soft limits to
"FULL SCAN (SOFT LIMIT)" instead of "FULL SCAN". This will help
distinguish them from unlimited full scans.

So now we print three different messages for three kinds of full scans:
- For full scans with hard limits we print "LIMITED SCAN".
- For full scans with soft limits we print "FULL SCAN (SOFT LIMIT)".
- For unlimited full scans we print "FULL SCAN".

Release note (sql change): Change `EXPLAIN` output of full scans with
soft limits to "FULL SCAN (SOFT LIMIT)" instead of "FULL SCAN", to
distinguish them from unlimited full scans. Unlimited full scans always
scan the entire index. Full scans with soft limits could scan the entire
index, but usually halt early once enough rows have been found to
satisfy their parent operator.

85486: roachtest: re-enable costfuzz test r=msirek,rytaft a=michae2

We've fixed a number of issues found by costfuzz's sibling test
unoptimized-query-oracle, so it feels like the right time to cry
'Havoc!', and let slip the dogs of war.

Fixes: #81717

Release note: None

Co-authored-by: Andrii Vorobiov <[email protected]>
Co-authored-by: David Hartunian <[email protected]>
Co-authored-by: Michael Erickson <[email protected]>
@craig craig bot closed this as completed in b5bad38 Aug 2, 2022
dhartunian added a commit to dhartunian/cockroach that referenced this issue Aug 8, 2022
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 cockroachdb#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-authentication Pertains to authn subsystems A-observability-inf A-webui-general Issues on the DB Console that span multiple areas or don't have another clear category. A-webui-security C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants