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

No ability to keep refresh tokens after use #255

Closed
taylornz opened this issue Mar 31, 2018 · 3 comments
Closed

No ability to keep refresh tokens after use #255

taylornz opened this issue Mar 31, 2018 · 3 comments

Comments

@taylornz
Copy link

Hello,

I am using Hydra with ifttt.com who have a particular requirement on refresh tokens

From the IFTTT platform site: "Please note that refresh tokens cannot have a time-based expiry. The only time it is technically permissible for a refresh token to expire is after an access token has been refreshed. At that time, it is acceptable to return a new refresh token; however, we require that the previous refresh token not immediately expire.

This is something that is part of the IFTTT oauth connection test, which fails against a standard installation of hydra.

I can build a custom copy of hydra with a patched version of fosite/handler/oauth2/flow_refresh.go where the call to TokenRevocationStorage.RevokeRefreshToken is removed, however keeping a custom compile of hydra is less than ideal.

Is there a sensible way to make something like this configurable that would be acceptable for an upstream merge into fosite? perhaps a special scope name on requester.GetGrantedScopes() eg "delay_refresh_token_delete" or "keep_refresh_token" ?

https://tools.ietf.org/html/rfc6819#section-5.2.2.3 does mention regarding Refresh Token Rotation : Note: This measure may cause problems in clustered environments, since usage of the currently valid refresh token must be ensured. In such an environment, other measures might be more appropriate. This must be why IFTTT require delayed deletion ( or no deletion ) of the refresh token.

I would very much appreciate any guidance that could be provided with a view to providing a pull request for evaluation.

Thank you.

@aeneasr
Copy link
Member

aeneasr commented Apr 1, 2018

Thank you for the clear write up. The idea if revoking RT immediately is to prevent replay attacks and potential issuance of more than one new refresh tokens. That’s why this is disabled at the moment.

Does the test require a second acces token, or just no error message?

@taylornz
Copy link
Author

taylornz commented Apr 3, 2018

Thank you very much for taking the time to reply.

I understand that the way Hydra/Fosite is currently set up is ideal from a security perspective and that my issue is a relatively unique restraint of the IFTTT.com platform.

The IFTTT connection test performs the following:

1: Retrieve authorization code ( Currently from Hydra )
-Code was provided
-State matches original original state from request

2: Request access token ( Currently from Hydra )
-Status Code is 200
-Body is valid JSON
-Response is an object
-Access token is provided
-Access token is a string
-Access token is at least one character long
-Refresh token is provided
-Refresh token is a string
-Refresh token is at least one character long

3: Retrieve user information ( A custom endpoint which proxies to Hydra introspection )
Status Code is 200
Body is valid JSON
Response has one top-level "data" object
"name" is a string
"id" is a string

4: Refresh access token ( Currently from Hydra )
-Status Code is 200
-Body is valid JSON
-Response is an object
-Access token is provided
-Access token is a string
Access token is at least one character long

5: Ensure older refresh token does not expire immediately ( Currently from Hydra )
-Status Code is 200
-Body is valid JSON
-Response is an object
-Access token is provided
-Access token is at least one character long

Everything works perfectly until step 5 where they try to re-use a refresh_token that has already just been used to gain a new access_token.

So the tests actually refresh the access_token twice and expects no errors and the above data present.

Beyond the test requirements, I assume they have this specific requirement as they do not have a central repository of refresh tokens but instead propagate them over their clusters. one end of the cluster may have to refresh an access_token, then moments later another end of the cluster with the now revoked refresh_token needs to refresh and gets an error.

It seems my options are:

1: Patch Hydra for non/delayed expiring refresh tokens ( and hope to make something configurable that might be acceptable to you as a pull request.

2: Patch Hydra for non expiring access tokens ( and hope to make something configurable that might be acceptable to you as a pull request.

3: Add a Man-in-the-middle caching app between IFTTT and Hydra temporarily returning the last access code+refresh_token retrieved for a particular refresh_token.

4: Cache access tokens in our app, and hide the expiry field in the response from Hydra to IFTTT effectively making access_tokens last forever from IFTTT's point of view.

5: Use other oauth software .

I do understand neither option ( delayed-expiring/non-expiring refresh_tokens and non expiring access_tokens ) would be suitable for a high security environment, however the data involved in this particular use-case does not have the strictest security requirements.

Perhaps a particular scope or configuration variable or client policy that would be acceptable to add enabling optional ( non default) support for either non expiring access_tokens or non/delayed expiring refresh tokens.

You will know best 👍

In any case, this is a very nice bit of software, thank you. 💯

@taylornz taylornz closed this as completed Apr 5, 2018
@aeneasr
Copy link
Member

aeneasr commented Apr 6, 2018

Hm, I think you could implement your own refresh strategy and run your own hydra with it (although forking is never a nice experience). The caching idea does also sound quite nice. What I don't like about their approach is that the tokens are re-issued, so caching would make more sense IMO here.

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

No branches or pull requests

2 participants