Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Documentation on PKCE refresh token usage needed #1285

Closed
mwmeyer opened this issue Jul 8, 2019 · 10 comments
Closed

Documentation on PKCE refresh token usage needed #1285

mwmeyer opened this issue Jul 8, 2019 · 10 comments
Labels

Comments

@mwmeyer
Copy link
Contributor

mwmeyer commented Jul 8, 2019

I've implemented doorkeeper PKCE per the wiki:
https://github.com/doorkeeper-gem/doorkeeper/wiki/Using-PKCE-flow

But now I am needing information surrounding whether refresh token handling should be updated at all with the flow.

According to this article, it may be safer to not use refresh tokens at all, for public clients with PKCE:
https://auth0.com/blog/oauth2-implicit-grant-and-spa/#Using-the-Authorization-Code-Grant-from-JavaScript

...
one of the reasons why identity experts pushed back when customers proposed use of authorization code in SPA — the risk of leaking refresh tokens. Refresh tokens are powerful artifacts, and in the case of public clients you don't even need to steal a secret to use them: once you get your hands on a refresh token, you can use it right away.

...
What to do? Considering that a browser is a far more dangerous environment than, say, a mobile platform app sandbox, I would recommend that unless (or until) your scenario does offer one of those refresh token protection features, that you do NOT use refresh tokens in your SPAs.

But a ( now abandoned PR ) contains some comments contradicting that:
https://github.com/doorkeeper-gem/doorkeeper/pull/1113/files#diff-1282d211312f992c2a614d1f4a6302feR105

So it isn't clear what should be done with the refresh token flow for PKCE and, if there are security concerns, whether doorkeeper could be configured in some way to disable refresh token usage for public client tokens.

It looks like others are seeking this information related to this as well:
https://stackoverflow.com/questions/50049511/how-doorkeeper-implements-refreshing-a-token-for-pkce-client

@nbulaj
Copy link
Member

nbulaj commented Jul 10, 2019

I think we can try to ask @bpieck as he is an expert in PKCE :)

@nbulaj nbulaj added the docs label Jul 13, 2019
@mwmeyer
Copy link
Contributor Author

mwmeyer commented Jul 15, 2019

@bpieck
Copy link

bpieck commented Jul 22, 2019

Sorry, I was in vacation for some time :)

You are quite right about the refresh token. Yes: It is a very powerful instrument, that should not get into wrong hands. I once had to monkey patch doorkeeper for some other project, to ensure to require a secret on refresh token flow for confidential apps (but I assume, that is not anymore the case - in current projects, I do not have any confidential apps, so I am not sure).

I don't agree about PKCE should not return a refresh token. All PKCE does, is: Ensuring, the client which requested the auth code can use the auth code - and noone in the middle. If the server cannot trust its clients to secure its credentials - in my opinion it should disable refresh tokens at all. But that means, every user has to login again every expiration time (so in default 2 hours - I normally recommend to decrease that even). And this is in many cases an anti-usability feature, I could not recommend. I would not recommend to increase the expiration time by far instead, too. Although that maybe a valid solution in some special cases.

The RCF sais:

If the values are equal, the token endpoint MUST continue processing
   as normal (as defined by OAuth 2.0 [RFC6749](https://tools.ietf.org/html/rfc6749#section-4.1.4)).

And since there the optional refresh token is part of it - I think, it's correct, to respond with a refresh token, as long as refresh token is enabled on the doorkeeper server.

I normally ensure, that a refresh token has some expiration time itself, since I totally agree, a forever valid token, even if it is a one time token, is dangerous (And I normally set it to 2 weeks or one month). But that is no standard ability of doorkeeper (and my request for this was rejected some time ago. But that is no oauth standard, too - you can easily reach this, by using some kind of cleanup cron deleting tokens that are created 2 weeks ago from database).

So: We use PKCE and we use refresh tokens, to give a user the ability to be logged into our mobile apps without always to re-identify themselves. I see a point in feeling less secure about browsers and frontend should give a user the possibility to not remember refresh tokens on public Browsers. I would always recommend to give a user an overview about open sessions - so they are able to kill anything (revoke tokens with refresh tokens) for sessions, they cannot identify.

@bpieck
Copy link

bpieck commented Jul 22, 2019

Ah I just followed your last link. And there is quite a good response to the claim:

Surely the same concerns that led the standard to invent PKCE are at play here? 

The answer is: No it is not. Since there is no redirection involved in the second request - and the refresh token is not hard coded into the code (to be decompiled) - it's not comparable with the original attack vector PKCE protects from at all. The real issues about storing the refresh token - are about how safely they are stored. That is something the mobile developer or the frontend developer have to be really sensitive about. They need to store the refresh token in a way, no other app on the mobile could get access it. There are some quite different attack vectors they need to protect the refresh tokens from. But those are quite different from "the authorization code interception attack".

@mwmeyer
Copy link
Contributor Author

mwmeyer commented Jul 22, 2019

@bpieck thanks so much for clarifying

I believe my confusion ( and that of the SO post, maybe others? ) is coming from the fact that both oauth and refresh tokens are sensitive in the authorization flow.

However, as you've said, they have different attack vectors. ( redirect vs storage )

For reference, here are some links relevant to refresh token storage security:
https://stackoverflow.com/questions/14163632/how-refresh-token-should-be-saved/44149678
https://stackoverflow.com/questions/49290819/why-are-refresh-tokens-considered-insecure-for-an-spa

Instead of worrying about excluding the refresh token in the oauth response, it sounds like developers implementing PKCE should just ensure standard storage security precautions are in place.

This helped, thanks again!

@mwmeyer mwmeyer closed this as completed Jul 22, 2019
@bpieck
Copy link

bpieck commented Jul 23, 2019

thanks for the links - interesting discussions :)

@nbulaj
Copy link
Member

nbulaj commented Jul 23, 2019

Added link to this discussion in the Wiki page, thanks @bpieck for the clarification and such a detailed answer and thanks @mwmeyer for the question :)

@justinjdickow
Copy link

This might be one of the most under-appreciated internet security threads of all time 🙏

@saurabhlime
Copy link

@bpieck: Could you shed some light on why the refresh token rotation request was rejected? Will it be a good time to reevaluate this decision?

"But that is no standard ability of doorkeeper (and my request for this was rejected some time ago. But that is no oauth standard, too - you can easily reach this, by using some kind of cleanup cron deleting tokens that were created 2 weeks ago from the database)."

@bpieck
Copy link

bpieck commented Dec 5, 2023

Hi @saurabhlime:
So my request was about letting refresh tokens expire. So I don't think that was about the refresh token rotation. Refresh token rotation is already part of current refresh token approach, right?
Sorry, I just have a quick look into doorkeeper code

          refresh_token.revoke unless refresh_token_revoked_on_use?

from lib/doorkeeper/oauth/refresh_token_request.rb
So as far as I see it, as long as you have configured Doorkeeper.config.access_token_model.refresh_token_revoked_on_use? you should have rotation active.

In my opinion forever living tokens just are no good approach. And if I remember correctly the reason not to accept the proposal was, that it's not in the RFC that refresh tokens expire. But since I just recognized, I can easily get around that by deleting old tokens in a cron (and I had no need for a history) - I never looked back into it. So I am sorry, I think I cannot really shed any light on it.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants