Oauth2-server: Old refresh token should not be IMMEDIATELY revoked

[halaei]

From RFC 6749 Section 6 - Refreshing an Access Token:

The authorization server MAY issue a new refresh token, ....
The authorization server MAY revoke the old refresh token after issuing a new refresh token to the client.

Here is 2 points I have in mind:

1. Based on the use cases I am familiar with, the only time the authorization server may decide not to issue a new refresh token is when it is sure the current refresh token is not going to be expired sooner than the next refresh token request - AKA NEVER.
2. It is a good idea that the authorization server revoke the old refresh token, but it may be better to think about the chance of network failure as well. Let say a client has a token granted via password, and now, it has to request for a refresh token. The request will be accepted by the server, but the response is not reached backed to the client. Now the client should ask for the user's password, only because of a temporary network error.

So I propose that the server should revoke the old refresh token only after it is sure the client has the new one. To make sure about it the server may do as followed:

1. Revoke the old refresh token the first time the new access/refresh token is used.
2. Revoke the new refresh token, if the old but un-revoked refresh token is used again, which indicates that the new refresh token is never delivered to the client.

[alexbilbie]

My concern is that if the refresh token isn't revoked immediately then there is a risk of replay attacks.

[Xerkus]

Revoking refresh token immediately is problematic for distributed consumers, central point must exist to request and distribute access tokens and store refresh token. Which makes single point of failure.
Immediately revoking access token is problematic too, it will cause unnecessary failed requests before new token can be distributed.

[alexbilbie]

@xerkus could you please detail a little more the scenario you’re experiencing?

[Xerkus]

It is not something i experience. Oauth server is not using this lib.

I have microservice that receives first refresh token when user authorizes application, then keeps it current and provides to other microservices.
Microservices use refresh token to request access tokens independently. Access token is shared between workers and is refreshed before expiry, by single worker that acquires exclusive lock, while rest keep using existing token.

Hypothetically, if there was a chance for high load for that particular external service and if both request and access tokens were revoked on refresh we would have following:
Some requests will fail every 30 minutes when access token is revoked/expired. Since refresh token is revoked as well, forcing central distribution of tokens, they will fail more often all across the place. That is something undesirable but i can live with. What is really problematic is that token microservice will experience regular rushes of requests for the new token (prefetching new access token is impossible, existing is revoked immediately, right?) which can overwhelm it and make whole thing unresponsive or even bring it down.

Something like that

[halaei]

This was a big problem for our mobile applications. Here is my fix for Laravel Passport implementation:

<?php

namespace App\OAuth;

use Carbon\Carbon;

class RefreshTokenRepository extends \Laravel\Passport\Bridge\RefreshTokenRepository
{
    public function revokeRefreshToken($tokenId)
    {
        $this->database->table('oauth_refresh_tokens')
            ->where('id', $tokenId)
            ->where('expires_at', '>=', Carbon::now()->addHour())
            ->update([
                'expires_at' => Carbon::now()->addHour(),
            ]);
    }

    public function isRefreshTokenRevoked($tokenId)
    {
        return ! $this->database->table('oauth_refresh_tokens')
            ->where('expires_at', '>', Carbon::now())
            ->where('id', $tokenId)
            ->where('revoked', false)
            ->exists();
    }

}

<?php

namespace App\OAuth;

use Laravel\Passport\Passport;
use League\OAuth2\Server\Grant\RefreshTokenGrant;

class PassportServiceProvider extends \Laravel\Passport\PassportServiceProvider
{
    protected function makeRefreshTokenGrant()
    {
        $repository = $this->app->make(RefreshTokenRepository::class);

        return tap(new RefreshTokenGrant($repository), function ($grant) {
            $grant->setRefreshTokenTTL(Passport::refreshTokensExpireIn());
        });
    }
}

Then I register App\OAuth\PassportServiceProvider instead of the original provider.

[alexbilbie]

@halaei so I can understand the problem better - what was the issue exactly when you say "This was a big problem for our mobile applications" ?

[halaei]

Mobiles are held by motorcyclists to help them with routing and navigation. Due to the low network quality, it happened a lot that a refresh-token request is sent to the server but the response is lost, which makes the current refresh-token invalid without any new one provided to the driver. Now the driver is logged out of the application and has to re-enter his username & password before being able to use the application again. This is a really inconvenient situation for the driver :(

[alexbilbie]

@halaei would a longer TTL for access tokens work better for you so that access tokens don't expire so frequently?

[halaei]

Your recommendation means not to use refresh tokens. It will certainly work, but if I choose so, what is the point of refresh tokens in OAuth after all? Don't they exist to increase security?

[paresy]

We encountered this problem in a real life application for Amazon Alexa. When revoking the tokens immediately there were edge cases where the Alexa service stopped working. As Xerkus pointed our there might be concurrency issues on distributed systems. Therefore our approach is to invalidate the refresh token after a timeout as halaei already pointed out.

[alexbilbie]

@halaei I'm not suggesting don't use refresh tokens, just increase the lifetime of access tokens so they aren't required to be refreshed as frequently

[paresy]

@alexbilbie This does not fix the underlying issue. It just minimizes the probability of happening. And i would argue that a higher AccessToken TTL it is worse in terms of security than not revoking the refresh token immediately.

[jturmel]

@alexbilbie @paresy @halaei I'll jump in here although I'm not having a specific issue with your library I have been doing research for this issue on another project. I'm using an OAuth2 Python library with an API we run and some of our clients are experiencing this very issue. Increasing access token expiry time just pushes the chance of what they're describing further out, it doesn't eliminate it... conversely in our testing, we lowered the expiry time for access tokens to a very short time frame and saw the problem much more and we're not even on bad networks, it can happen when network switching (WiFi <-> cellular).

The main problem in our experience is client determines that an access token is expired, either by checking the expiration before making the request or from receiving a 401 response by trying to make a request with the expired token. The application then tries to refresh the access token, request is successful, but before the response can complete with the client because of network issues or any other host of issues (ie iOS only allowing so much time to complete a task in the background for example), the new access token and new refresh token are never received and stored by the application (which is also what triggers clearing out the previous tokens on the application side). When the application makes a network request again it's still using the old access token, so it tries to refresh again but this time the refresh token is invalid. The application is now forced to ask the user for authentication credentials again. For applications that perform API calls in the background often without much user interaction, a user may be logged out for some time before they ever realize it, depending on the application's importance this could be a minor inconvenience or a fairly serious issue.

I found a post on how Fitbit started handling this very issue in their own API here: https://community.fitbit.com/t5/Web-API-Development/Refresh-token-amp-network-timeout/td-p/1102761 -- tl;dr: they built in a 2 minute grace period window where the older refresh token could still be used. Others also suggested not actually invalidating the old refresh token until the first use of the new access token... although the load around this was the reason Fitbit didn't do it, that could probably be worked around with more time/thought/resources.

The Python library I'm using added an option to disable rotating refresh tokens since it is an optional part of the spec.

[simonhamp]

Interesting issue!

First off, I don't think it should be up to this library to handle the timing of refresh token revocation anywhere... and as far as I can tell it isn't.

I'm pretty sure that revoking refresh tokens only happens in your individual implementations of RefreshTokenRepositoryInterface::revokeRefreshToken() and checking for a revoked token by your implementation of RefreshTokenRepositoryInterface::isRefreshTokenRevoked().

If so, then these are the places where you can implement your grace period, if you choose to. For example, in systems with a job queue, you could register a job to carry out the revocation. (But what if your job queue fails?)

If you don't want your revocation to happen until the new access token has been successfully used, your logic will need to be a lot more complicated. (Will you/the next person remember where and why your revocation happens at a distance?)

I concur with @alexbilbie and wouldn't recommend either of these approaches as they could leave a big window of opportunity for an attacker to get in.

But if your use-case makes it impossible to approach this any other way, do what you can to make sure that the requesting client is the same as the one that has apparently previously failed.

I'm not sure if FitBit are doing anything like this, but given that they've basically announced publicly that you can make replay attacks using their refresh tokens, I'd really hope that they take extra steps to make sure their clients don't fall foul of MitM attacks.

For example, consider including an extra unique header challenge with each refresh token request that you can validate server-side and possibly even decode to a specific ID that relates to a client instance. This could in theory be used to limit re-use of a refresh token more than once to a single client instance (IP address probably won't work for this, especially if network switching is the root cause of your problem).

I think this discussion is extremely valuable and pertinent, especially around the security of consumer access to their own data whilst their mobile. However, I don't think this is an issue for OAuth2-Server to solve, as it depends largely on your own app's use-case, context and circumstances.

I recommend we close this issue as there likely won't be any development implications off the back of this.

[Sephster]

I agree with @simonhamp here. It would be negligible of us to mandate an expiry timeout for the server for everyone. The issues here seem to be largely dependent on the host server/client connection which is not the responsibility of this package.

The package itself does not provide implementation for the revokeRefreshToken method so it is up to you how you implement this. As this is an implementation concern, I think it is best this issue is closed.

[halaei]

To me it means everyone implementing RefreshTokenRepository::revokeRefreshToken() by actually revoking the token as the name of method suggests, introduces a bug. Implementations of this package might be unaware.

[tylik1]

I'm having the same issue. Imagine the case where client app calls refreshToken api endpoint, to retrieve new access and refresh tokens, but then user for some reason, refreshes browser in the middle of process. The refresh token cookie is not attached, meaning, all following requests will fail, and the user will have to login again.

[rishabhpoddar]

I believe this is a very real problem! And to solve it (plus, not have the possibility of a replay attack), here is what I thought of:

• Let's say the current refresh token is R1 and current access token is A1.
• Now, A1 has expired, so we use R1 to issue a new R2 and A2. We keep R1 in the DB, and send R2 and A2 to the client.
• If the client get's R2 and A2, they will use A2, at which point we can remove R1 from the DB.
• However, if the client does not get R2, A2, they will use R1 again, at which point, your backend should generate R3, A3 and send those over to your client. At this point, we need to keep track of R1, R2 and R3 since we do not know which one the client TRULY has.
• If the client gets R3, and they use A3, then we can remove R1 and R2.

This method will guarantee that this issue doesn't happen and that you can still use rotating refresh tokens to detect token theft!

However, this method has a problem of having to keep track of these additional tokens. To solve this, I thought that you can create R2 in a way that has R1 in it. Let me elaborate:

• Let's say R1 is some random string.
• When we create R2, it is initially treated as a child of R1. So we create it like R2 = R1.. Then we do not store R2 in the db.
• When we create R3, it is initially also treated as a child of R1. So we create it like R3 = R1.. Then we do not store R3 in the db.
• Now when the client uses either R2, or R3, then we extract the R1 out of it and check that R1 is still in the db. If it is, we "promote" the child to being the parent and get rid of R1. This also means we will now store R2 in the db. If R1 is not in the db, it means R2 cannot be used.

Above is somewhat a rough idea of the tree structure.. In fact, I have implemented this for Laravel and NodeJS here: https://supertokens.io

[Sephster]

I'm locking this thread as there is a new active discussion in #1025 - I will keep that issue open and keep discussions there. Thanks