aws-amplify / amplify-js

A declarative JavaScript library for application development using cloud services.
https://docs.amplify.aws/lib/q/platform/js
Apache License 2.0
9.44k stars 2.13k forks source link

AWS Amplify Auth is not using Rotating Refresh Tokens #5214

Open frederikprijck opened 4 years ago

frederikprijck commented 4 years ago

Which Category is your question related to? Auth What AWS Services are you utilizing? Cognito Provide additional details e.g. code snippets

As discussed on twitter with @undefobj I had a question/concern about the way AWS Amplify is handling Refresh Tokens. As it was hard to explain the full story on twitter, I was told to open a GitHub issue for further explanation of my concern.

We are using a Single Page Application (Angular) that has implemented AWS Amplify Auth. I've noticed that it's using non-rotating Refresh Tokens, meaning a single refresh token is usable more then once. I'm wondering if we have something configured incorrectly as the security draft best practices for implementing OAuth2.0 in Browser Based Apps , which u can find here https://tools.ietf.org/html/draft-ietf-oauth-browser-based-apps-05?fbclid=IwAR2Rz_joGqTZIEax3C7tTWExXFWBr4F6pdIYnki1U7Z8ZwMDxP4w_49fyy8#section-8, clearly states the following in section 8 (Refresh Tokens):

Authorization servers may choose whether or not to issue refresh tokens to browser-based applications. [oauth-security-topics] describes some additional requirements around refresh tokens on top of the recommendations of [RFC6749]. Applications and authorization servers conforming to this BCP MUST also follow the recommendations in [oauth-security-topics] around refresh tokens if refresh tokens are issued to browser-based apps.

In particular, authorization servers:

  • MUST rotate refresh tokens on each use, in order to be able to detect a stolen refresh token if one is replayed (described in [oauth-security-topics] section 4.12)

Important to note here is that it mentions it should rotate on each use, and not just after expiring as mentioned on the twitter thread. There's a difference between an expiring Refresh Token and a Rotating Refresh Token, which of course also has an expiration date.

The idea of this is that browser based applications have a greater risk of leaking Refresh Tokens, which is also explained in the document I linked above. By using rotating refresh tokens, whenever a refresh token is leaked and used, the user will not be able to use it's token itself and the system is able to detect the use of a refresh token that has already been used. In this case both the user and/or the system are able to react upon this accordingly.

When using a non-rotating refresh token, it's a lot harder to detect when a leaked refresh token has been used. Of course there are ways to work around this by detecting malicious login attempts (e.g. you generally login from the US, but suddenly you are trying to log in from Europe), however this is not the the same thing as checking a token has already been used.

As the OAuth 2.0 security best practices document I linked above states that, for browser based applications, Refresh Tokens should be single use and rotate after each use, my main question is: Are we configuring it incorrectly, are you explicitly not following the best practices or is this something that has been overseen in the implementation?

Additional Resources:

r-moore commented 4 years ago

Some related discussion about the security implications of this in #1972

frederikprijck commented 4 years ago

Hey @r-moore, thanks for pointing that out. That thread definitely has some interesting comments!

It looks like most of that thread is talking about the expiration date, however I think this is probably the comment that relates the most to the issue I created: https://github.com/aws-amplify/amplify-js/issues/1972#issuecomment-544886675

As much as I agree with what is being said there about short expiration dates, I do want to emphasis that the current issue here is not about expiration but about rotating Refresh Tokens (meaning: single use). Storing a non-rotating Refresh Token in local storage is a security vulnerability (e.g. XSS attacks). Making them shorter lived, as is being discussing in that thread you linked is a good starting point, but it still does not allow for the user or system to be informed when a refresh token was leaked/stolen.

However, making them rotating and single use is what's being recommended in the resources linked and what makes it even better from a security perspective. It's probably even fine to have a long lived Refresh Token (big expiration date), as long as it's single use and rotates on every use. I say this because the Access Token should be short lived, so whenever the access token is expired and we are requesting a new one, the Refresh Token is consumed and, even tho it has a long expiration date, it'll not be usable a second time. In general this makes the refresh token valid only for as long as the access token is valid, even if the expiration date on the refresh token is bigger (it's single use, so invalidated as soon as it's used to refresh the Access Token). The use-case where the Refresh Token is valid for longer than the expiration date on the Access Token is when the user closes the application and comes back after a few hours or days (or any time that's bigger than the access token expiration but smaller than the refresh tokens expiration). In that case, the Refresh Token has been around for a bit longer, but will be invalidated after the user retrieves it's first Access Token upon reopening the application.

Storing a rotating Refresh Token in local storage is also vulnerable to XSS attacks, however, as I mentioned, the system and/or the user is able to detect when a single-use refresh token was (unexpectedly) used by someone else as that would make it invalid for the current user (as explained in the resources I linked). This makes it less troublesome to keep them in the local storage (I still prefer to keep them out of the local storage, but we do need to store it somewhere on the client), even for tokens that have a longer expiration date.

rachitdhall commented 4 years ago

Thanks for raising this question. We are monitoring the draft specification and will evaluate supporting it.

Rachit Dhall Amazon Cognito

richanna commented 4 years ago

To elaborate on @rachitdhall's reply, part of that evaluation involves looking at how refresh token rotation would contribute to our overall threat mitigation strategy. As @frederikprijck rightly noted, refresh token rotation can provide some reduction in the impact of token theft via XSS in some circumstances. However, since it does not prevent theft, it cannot be seen as a replacement for other measures.

Annabelle Backman AWS Identity

baleksandr48 commented 4 years ago

Hello, @frederikprijck, thank you for explanation, but one question is still opened for me. I use nextjs with amlify {ssr: true}. I extract token from req in getServerSideProps in this way:

async function getServerSideProps (context) {
  const SSR = withSSRContext(context);
  const session = await SSR.Auth.currentSession();
  // ... extract token from session
  // return some props
}

It works if user logged in recently. But if user closes the tab and opens it a few hours (or you can just change the date on your machine), this line will throw an error: const session = await SSR.Auth.currentSession();

The reason is clear: it happened because user's tokens are not valid anymore. BUT int he documentation https://docs.amplify.aws/lib/auth/overview/q/platform/js#sign-up-and-sign-in I found that:

The Amplify client will refresh the tokens calling Auth.currentSession if they are no longer valid.

Seems that it doesn't work on server side. It just throws an error without possibility to refresh tokens. So, could you please tell me how should I handle this case according best practices?

frederikprijck commented 4 years ago

@baleksandr48 I think your issue is not related to the subject discussed in this thread, I advise you opening another issue for that.

The problem discussed in this thread related to Single Page Applications, Mobile or Native Apps as those are not able to store non-rotating tokens in a secure way on the client side. Incase you are trying to use a refresh token from the server side, it should be fine to use non-rotating refresh tokens. But I guess there might be something else going on that is not allowing u to refresh your tokens by calling Auth.currentSession().

m4rcosazevedo commented 4 years ago

Hello, @frederikprijck, thank you for explanation, but one question is still opened for me. I use nextjs with amlify {ssr: true}. I extract token from req in getServerSideProps in this way:

async function getServerSideProps (context) {
  const SSR = withSSRContext(context);
  const session = await SSR.Auth.currentSession();
  // ... extract token from session
  // return some props
}

It works if user logged in recently. But if user closes the tab and opens it a few hours (or you can just change the date on your machine), this line will throw an error: const session = await SSR.Auth.currentSession();

The reason is clear: it happened because user's tokens are not valid anymore. BUT int he documentation https://docs.amplify.aws/lib/auth/overview/q/platform/js#sign-up-and-sign-in I found that:

The Amplify client will refresh the tokens calling Auth.currentSession if they are no longer valid.

Seems that it doesn't work on server side. It just throws an error without possibility to refresh tokens. So, could you please tell me how should I handle this case according best practices?

@frederikprijck Managed to solve? if so, can you show the solution?

frederikprijck commented 4 years ago

I am probably not the person to ping for that. I reported an, unrelated, issue in this thread.

baleksandr48 commented 4 years ago

@m4rcosazevedo still no news. I will share with you when I find a solution. And will be glad if you do the same

kennand commented 3 years ago

+1, using ssr amplify + checking authentication prior to refresh is not great UX.

baleksandr48 commented 3 years ago

@kennandavison do you mean that you know how to refresh this token programmatically?

harrysolovay commented 3 years ago

There is no a way to achieve refresh token rotation without a fresh InitiateAuth flow. And there isn't a good workaround. Closing this issue (as it's a service limitation) and delegating to the service team.

frederikprijck commented 3 years ago

@harrysolovay I am a little bit confused about your reply and why this issue is getting closed. My initial report did not involve InitiateAuth. Are you saying Refresh Token Rotation is supported and I was basically using things wrongly?

I do want to add that, based on all the different comments, this issue has derailed from the initial concern I have with the way Refresh Tokens are used considering the mentioned Security Specifications.

sammartinez commented 3 years ago

Thanks for this feedback @frederikprijck, as discussed on Discord, I am going to reopen this issue.

frickjack commented 2 years ago

this should really be filed as a feature-request for Cognito rather than on amplify-js for example - Auth0 supports refresh token rotation: https://auth0.com/docs/security/tokens/refresh-tokens/refresh-token-rotation

BBanoth commented 2 years ago

@frederikprijck : Any update on this ? and did you find yourself any alternate solution for rotating the refresh tokens

frederikprijck commented 2 years ago

I dont think I am the one to ping regarding an update here. I reported this, but not the person to address this.

Also unsure if anything changed on that subject.

BBanoth commented 2 years ago

@frederikprijck yes, obviously you are not the person to give update, I saw that you reported this issue. That's why thought that you would share any workaround you might have found.

IgorPietraszko commented 2 years ago

This issue was opened more than 2 years ago and despite a good explanation and background provided by @frederikprijck, there seems to be no follow up or action. I do agree that this should have been turned into a Cognito feature request but it's 2 years in the making. From a product roadmap perspective, are there plans to modify/enhance Cognito to support OAuth2.0 recommendations and support rotating refresh tokens?

gornostal commented 1 year ago

Hi Amplify team What do you think about prioritizing this issue higher, since it impacts application security? It would be good to see Security label on this as well.

frederikprijck commented 1 year ago

In the event of going through my stale open issues and PRs, I was wondering if there would be any update or if I should just go and close this issue given the lack of communication and actions taken?

abdallahshaban557 commented 1 year ago

Hi @frederikprijck - let us please keep this open. We have discussed this request with the Cognito team, and unfortunately we still do not have a clear timeline that we can communicate. We are using this Github issue to provide feedback to the Cognito team on the urgency of this request.