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

[server] Cookies should set SameSite='None' #165

Closed
gnuletik opened this issue Mar 24, 2020 · 11 comments
Closed

[server] Cookies should set SameSite='None' #165

gnuletik opened this issue Mar 24, 2020 · 11 comments
Assignees
Labels
Projects

Comments

@gnuletik
Copy link
Collaborator

gnuletik commented Mar 24, 2020

In app/settings.py, we set

SESSION_COOKIE_SAMESITE = None
CSRF_COOKIE_SAMESITE = None

The result of this configuration is to ignore the SameSite cookie attribute (as seen below).

$ curl -v -H 'cookie: csrftoken=xxx' -H 'Referer: https://sandbox.wasabi.telemeta.org/accounts/login/' -v https://sandbox.wasabi.telemeta.org/accounts/login/ -d 'username=admin' -d'password=xxx' -d 'csrfmiddlewaretoken=xxxx' 2>&1 | grep -i set-cookie
< set-cookie: csrftoken=xxx; expires=Tue, 23 Mar 2021 16:22:44 GMT; Max-Age=31449600; Path=/; Secure
< set-cookie: sessionid=xxx; expires=Tue, 07 Apr 2020 16:22:44 GMT; HttpOnly; Max-Age=1209600; Path=/; Secure

However, starting from Chrome 80, cookies without a SameSite attribute have the 'Lax' value.
This break the player as we need the authentication cookie to be available on different domains.

Note: For testing purposes, you can disable this Chrome feature with

$ google-chrome --disable-features=SameSiteByDefaultCookies

There is no way to set this cookie to 'None' in Django (string value) as of today.

The issue has been fixed 3 months ago by the Django team in this commit and will be released in Django 3.1 (release planned for August 2020).

In the meantime, we can either:

@Tointoin @yomguy What's your thoughts?

@Tointoin Tointoin added the bug label Mar 31, 2020
@Tointoin Tointoin added this to New in WASABI via automation Mar 31, 2020
@yomguy yomguy moved this from New to Ready in WASABI Mar 31, 2020
Tointoin added a commit that referenced this issue May 4, 2020
…s-samesite with the PR to manage secure flag resolving #165
@Tointoin
Copy link
Collaborator

Tointoin commented May 4, 2020

branch update/django3 deals with this solution but raises an uncompatibily with rest framework's filters in views.py. It is in any case useful to keep it for further upgrade.

branch feature/cookiesSameSite set use of django-cookies-samesite. As discuss earlier, it will be based on this PR until it has resolved this issue on secure flag.

Another way to workaround this would be to drop use of session cookie and switch to full JWT authentication. It would enable to get ride of CSRF token, wouldn't it?

@Tointoin
Copy link
Collaborator

Tointoin commented May 5, 2020

hi @yomguy and @gnuletik

switch to full JWT authentication. would enable to get ride of CSRF token, wouldn't it?

https://security.stackexchange.com/questions/166724/should-i-use-csrf-protection-on-rest-api-endpoints/166798#166798
The need of CSRF has indeed to be discussed regarding session authentication use or not.

@gnuletik, shall we drop use of token authentication, would following solution fit your actual stack on timeside-player?

https://medium.com/hackernoon/jwt-authentication-in-vue-js-and-django-rest-framework-part-2-788f0ad53ad5

We need anyhow to specify more precisely TimeSide REST API requirements regarding login workflow.

@gnuletik
Copy link
Collaborator Author

gnuletik commented May 5, 2020

Hi @Tointoin,

We should be able to drop the CSRF token if we use JWT authentication yes.
Is it already working ?

The solution mentioned on medium is based on Vuex. Most of this should already be abstracted by the SDK but the same principle remains :)
I can try switching to JWT-based authentication on the player in the coming days.

@yomguy
Copy link
Owner

yomguy commented May 5, 2020

Hi @Tointoin

Thanks for the inputs ! Regarding the limitation of the cookie usage, the CSRF stuff, the SameSite property limitations, the study you have done on the JWT technique and the fact that it is clearly the future for managing various clients, it seems really welcome to go for it. But I know it can be hard to implement.

So, @gnuletik could you please evaluate the time needed for the switch on the frontend side before we decide to go for it ?

@yomguy
Copy link
Owner

yomguy commented May 5, 2020

We need anyhow to specify more precisely TimeSide REST API requirements regarding login workflow.

  1. the User creates an account and request for an access to the API
  2. the Admin or a method validate the access
  3. the User receive an access token and a refresh token by email
  4. the User use the tokens to setup its own client application consuming the API

At this stage, we have only to resolve the points 3. and 4 so don't bother with the signup and token sharing processes.

@gnuletik
Copy link
Collaborator Author

gnuletik commented May 5, 2020

Hi @yomguy,

I was able to login and make a request using the JWT token.

curl -v 'https://sandbox.wasabi.telemeta.org/timeside/api/token/' -X POST -H 'Content-Type: application/json' -d '{ "username": "XXX", "password": "XXX" }'

Them, I'm able to use the token to make a request

curl 'https://sandbox.wasabi.telemeta.org/timeside/api/items/f819b56c-e4c4-4762-83ac-77eb81084268/' -H 'Authorization: Bearer INSERT_YOUR_ACCESS_TOKEN_HERE'

However, we may face unexpected issues because :

If we cannot rely on the schema / SDK to handle the securitySchemes

  • I need to wrap the SDK to handle expired tokens: they should be refreshed (if possible)

I'll also have to:

  • Implement a login form which saves the accessKey to the localStorage
  • Implement a logout button
  • Check the token when the app starts
  • User should be redirected to the login form if the token is not valid anymore
  • I need to update the static authentication utils in the e2e and unit tests

So, hopefully, switching the authentication mechanism in the player/sdk can be done in 3-4 days.

@gnuletik
Copy link
Collaborator Author

gnuletik commented May 5, 2020

  1. the User receive an access token and a refresh token by email

We cannot send an access/refresh token by email because they both expires.
Also, email are not a safe place to send tokens.

Instead, the user should get his token using the /timeside/api/token route by providing his credentials (user / password).

  1. the User use the tokens to setup its own client application consuming the API

No, the JWT tokens are not meant for this usage because tokens expires.

At this stage, we have only to resolve the points 3. and 4 so don't bother with the signup and token sharing processes.

The user onboarding workflow is an interesting issue to be solved yes.
Can we follow this discussion into a separate issue ?
We first have to define the usecases.

@yomguy
Copy link
Owner

yomguy commented May 6, 2020

Hi @gnuletik

Thanks for this quick and detailed study. Regarding the issue you mention and the development we need to include all the login methods in the app, I think we have to postpone this feature. We could give it more effort in a industrial context we don't have yet. But to me, in the context of the WASABI research project, I think it not a big problem to act and say that our player works on Firefox only for good reasons. So let's focus on consuming all the API features we have already.

Sorry I was wrong with the workflow with the token exchange protocol and my point of view was effectively on the user side which finally should not have to bother any token manually. OK for discussing about the usecases in a separate issue.

@gnuletik
Copy link
Collaborator Author

Hi @Tointoin,

branch feature/cookiesSameSite set use of django-cookies-samesite. As discuss earlier, it will be based on this PR until it has resolved this issue on secure flag.

The issue you mentionned before on django-cookies-samesite has been merged :)

@Tointoin
Copy link
Collaborator

Tointoin commented May 12, 2020

Yep, I've seen it, let's wait for release.
If Django 3.1 comes before it, it is anyway better to upgrade to it if rest_framework.filters are fixed.

@gnuletik
Copy link
Collaborator Author

Hi @Tointoin,

As we can use the JWT authentication for API calls on different domains, I think that we don't need to implement the SameSite attribute anymore.

I'm closing it for now. Feel free to re-open if needed!

WASABI automation moved this from Ready to Done May 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
WASABI
  
Done
Development

No branches or pull requests

3 participants