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

Add CORS middleware #1349

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Add CORS middleware #1349

merged 3 commits into from
Sep 4, 2024

Conversation

kian99
Copy link
Contributor

@kian99 kian99 commented Sep 4, 2024

Description

This PR does a few things around CORS to fix issues raised by the Juju dashboard.

  1. Added CORS middleware to return valid CORS headers - I've opted to use the https://github.com/rs/cors module which was already a dependency in our go.sum because it is pulled in by a Juju dependency.
  2. Fixed bug with cookies differing when calling login and whoami endpoints. The options set on a cookie when calling /login were wiped when calling /whoami because the options are not saved to the database. So we set them before we return a cookie to the user.
  3. Changed the cookie's "sameSite" policy from lax to none to allow the Juju dashboard to send the cookie in cross-origin requests.

There are some things worth mentioning here:

  1. Setting the cors allowed origin option to the wildcard "*" to accept all origins is not valid when also setting the Access-Control-Allow-Credentials header to true. See here.

If a request includes a credential (most commonly a Cookie header) and the response includes an Access-Control-Allow-Origin: * header (that is, with the wildcard), the browser will block access to the response, and report a CORS error in the devtools console.

  1. A "sameSite" policy of none is necessary because strict and lax do not permit sending a cookies cross-origin via AJAX. See here for more info.

Fixes CSS-10514

Engineering checklist

Check only items that apply

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

I tested the changes with a react app and made a request from the app, running at localhost:3006 to JIMM's /auth/whoami endpoint. After configuring CORS there was a 403 error because the browser did not send JIMM's session cookie in the request until the cookie's sameSite policy was changed to none.

- fix bug with cookies differing when calling login and whoami endpoints
@kian99 kian99 requested a review from a team as a code owner September 4, 2024 11:45
// Note browsers require cookies with the same-site policy as 'none' to additionally have the secure flag set.
func sessionCrossOriginSafe(session *sessions.Session, secure bool) *sessions.Session {
session.Options.Secure = secure // Ensures only sent with HTTPS
session.Options.HttpOnly = false // Allow Javascript to read it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we allowing javascript to read it? Do they manipulate the cookies in some way?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No they call whoami to get details

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @SimoneDutto I went and tried my test again, setting HttpOnly = true and the browser still sends the cookie. Had a google and I see from here

A cookie with the HttpOnly attribute can't be modified by JavaScript, for example using Document.cookie; it can only be modified when it reaches the server. Cookies that persist user sessions for example should have the HttpOnly attribute set — it would be really insecure to make them available to JavaScript. This precaution helps mitigate cross-site scripting (XSS) attacks.

So I'll change this to true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is more secure! thank you for this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you can add some requests forged to trigger cors policies in service_test.go

Copy link
Contributor

@ale8k ale8k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just so I'm understanding correctly,

now we same site none of on our session cookies allowing them to go anywhere? And the server has cors enabled with GET only?

@kian99
Copy link
Contributor Author

kian99 commented Sep 4, 2024

@ale8k

now we same site none of on our session cookies allowing them to go anywhere? And the server has cors enabled with GET only?

I'm not sure I understand the first part of your message. But with "samesite" set to none, the browser will include the cookie the cookie in all requests to the domain that set the cookie, including JS requests and via top-level navigation (i.e. a user clicking a link). And the server now has CORS only for GET requests, correct.

func sessionCrossOriginSafe(session *sessions.Session, secure bool) *sessions.Session {
session.Options.Secure = secure // Ensures only sent with HTTPS
session.Options.HttpOnly = true // Don't allow Javascript to modify cookie
session.Options.SameSite = http.SameSiteNoneMode // Allow cross-origin requests via Javascript
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it's possible to set a domain for cookies, like .canonical. I can imagine in a production environment having the dashboard and jimm under the same subdomain.
In this way we are more secure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The domain automatically gets set to the domain that set the cookie. Doc source
image

@SimoneDutto
Copy link
Contributor

@ale8k

now we same site none of on our session cookies allowing them to go anywhere? And the server has cors enabled with GET only?

I'm not sure I understand the first part of your message. But with "samesite" set to none, the browser will include the cookie the cookie in all requests to the domain that set the cookie, including JS requests and via top-level navigation (i.e. a user clicking a link). And the server now has CORS only for GET requests, correct.

I think the attack vector @ale8k is worried about is having a malicious js snippet in the victim browser using the dashboard able to send out (to malicious.server.com for example) our cookies

@kian99 kian99 requested review from SimoneDutto and ale8k September 4, 2024 14:00
@kian99 kian99 merged commit 17bc480 into canonical:v3 Sep 4, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants