Doorkeeper: Allow for token refresh without secret when token obtained with PKCE

[mazikwyry]

Steps to reproduce

• Add enable_pkce_without_secret in configuration
• Create confidential application
• Obtain access token using authorization_code with PKCE code_challage (without client_secret)
• Try to refresh token (also without secret, as client don't have it)

Expected behavior

Because there is a assumption that when using PKCE we don't need a client_secret then when access token has been obtained without secret, the same client should be also able to refresh it without secret.

See https://github.com/openid/AppAuth-Android/issues/178#issuecomment-283074250 for further explanation.

I don't want to set a application not to be confidentatial, because it will allow to authenticate the client without client_secret and PKCE which is unsecure.

Actual behavior

When trying to refresh I get 401, because of lack of the client_secret.

System configuration

You can help us to understand your problem if you will share some very
useful information about your project environment (don't forget to
remove any confidential data if it exists).

Doorkeeper initializer:

Doorkeeper.configure do
  # Change the ORM that doorkeeper will use (needs plugins)
  orm :active_record

  # This block will be called to check whether the resource owner is authenticated or not.
  resource_owner_authenticator do
    # Put your resource owner authentication logic here.
    # Example implementation:
    current_user ||
      redirect_to(users_oauth_sign_in_path(oauth_url: request.fullpath))
  end

  # If you want to restrict access to the web interface for adding oauth authorized applications,
  # you need to declare the block below.
  admin_authenticator do
    # Put your admin authentication logic here.
    # Example implementation:
    current_admin_user ||
      (session[:return_to] = request.fullpath && redirect_to(new_admin_user_session_url))
  end

  use_refresh_token
  default_scopes :mobile_wallet
  enable_pkce_without_secret
end

Ruby version:
2.3.6

Gemfile.lock:
I use master of Doorkeeper.

[nbulaj]

ping @bpieck , could you please look at this issue?

[nbulaj]

Also maybe @rsharrott would be interested in

[bpieck]

Sorry, I did not get the first ping. I will have a look

[bpieck]

So, I agree with the discussion. If you do so, too:

I can mark the token as generated by pkce flow - and remove need of secret in refresh grant, if token is a pkce token. Would that be ok, @nbulaj?

[nbulaj]

@bpieck I don't really like the approach with the additional data in the token.. But currently I don't see any other solutions..

[bpieck]

I understand your dislike. And I share it. I still investigate a little further, to figure another way.

[mazikwyry]

@nbulaj @bpieck Maybe another solution could be to decide if client uses PCKE, as it now can select confidential option. PKCE option will force code challange for authorization flow and won't require secret for refresh token. That way we can leave token authorization-flow agnostic.

[nbulaj]

@mazikwyry sounds reasonable, because confidential option will be available by default in Doorkeeper 5.0, but can we assume that if app is non confidential, then we can skip verification of the secret ?

[bpieck]

I think the pkce option sounds totally reasonable. So if pkce option is enabled, this app can only do pkce and nothing else.
I will put some time after Ruby Unconf today into it.

[mazikwyry]

@nbulaj I was thinking about that. When app will be non-confidential it will allow for refreshing-token without secret, but will also enable client authentication without secret which I for example don't want to enable for app for security reasons. This is how Confidential and PKCE options can work together:

1. If app is Confidential then PKCE should be disabled (as when secret is provided there is no real sense in using PKCE)
2. When app is non-confidential and PKCE is disabled then it allows for authentication and refreshing token without secret (skipping PKCE even if provided).
3. When app is non-confidential and PKCE is enabled then authentication can be done ONLY with PKCE and refreshing/revoking can by done without secret. (I think we should not generate/store/show secret in that case).

[nbulaj]

@mazikwyry sounds reasonable to me

[mazikwyry]

@bpieck would you like to do a PR for that or can/should I work on it?

[bpieck]

Sorry, last four days I looked deeper into the matter. But I just encountered one question: What do you think you gain by this feature? There is nothing more confidential in a pkce app at all. You cannot trust, that a pkce app is really the one, you permitted in the beginning.
So there is a very small gain: You force your own mobile app developers to do pkce (what they should of course). If other apps, that disguise as your mobile app - do pkce or not - who cares? There is no gain to force them, to do so - since implementing PKCE is done by library, so it's not more complex. PKCE ensures no app in the middle overregistering to your redirect url gains anything by getting the authorization code. Nothing more. So there is nothing making it more confidential.
On the other hand configuration off applications for others get a little harder. And I get to think, that we even would give people the impression, that pkce-Apps are more confidential then public apps. But they aren't. They are totally public.

[bpieck]

I could do the PR - but I am not convinced anymore, that there is really anything good in doing so. I am very sorry, I needed so much time, just to change my mind in the end. If there are good arguments in implementing this, I still would do so. I just don't get the gain anymore. There is no big difference between a public (non-confidential) app that is allowed to do pkce - and a pkce app, that is only alllowed to do pkce at all. Both should clearly be marked as public (non-confidential). If this small gain to force mobile app developers to use the pkce library over asking them to do so and trusting that they really did - is really an improvement: I will think about how to really communicate this in app registration and configuration without confusing and misdirecting. That was the real point I struggled with, when preparing to implement.

[nbulaj]

So, @bpieck , once again: is there are a problemand we need to work on it or how? I lost the thread of conversation, sorry :(

[bpieck]

Hi Nikita,
I totally understand you loosing the thread.
So: The original request was to enable confidential PKCE-Apps to ensure, app is using a secret or pkce flow. I currently don't get this request. Since there is nothing confidential about such an app. Any app that uses PKCE cannot be confidential by definition - else it would not need to use PKCE.

So: The only gain by this feature request would be to ensure, the app developers are really using pkce (and securing itself from overwriting url-registration app in the middle attacks). After now put some time into rethinking, I wouldn't estimate this gain really valuable.

So in my opinion only one thing should be done: the enable_pkce_without_secret configuration should be removed (since non-confidential apps should be totally enough).

[nbulaj]

@bpieck sounds reasonable to me.

@mazikwyry any thoughts?

[bpieck]

If I should create a pull request for removing this configuration option - please tell me about it.
If in the end you like pkce option as third option next to confidential/non-confidential on apps (maybe for some case I did not consider or really just to be sure, no app developer forgets to implement it) - I'd create a PR about that, too. Just would need to put some thoughts, how to present that option. I would like to ensure noone gets the thought a pkce app is more confidential then a non-confidential app.

[nbulaj]

Hi @bpieck . Thanks for your interest in this! I think we can drop this option (enable_pkce_without_secret) now from the 5.0 release, but if somebody else would need it and create an issue for example - we can return to this topic and think what do we need to do on that.

[nbulaj]

Hi @bpieck . Any updates here?

[mazikwyry]

Sorry guys for no response for a long time. @bpieck I totally get your point of view and understand it and also agree with:

So in my opinion only one thing should be done: the enable_pkce_without_secret configuration should be removed (since non-confidential apps should be totally enough).

It looks like my view of security that PKCE brings was overrated. You are right. Still, anyone else having clinet_id from my mobile app code and using the same library that I use with PKCE support can create a clone of the app and send it to one of my app users. In that clone app, he can track the access token that user will get after login and that way he can gain access to user's data. There is no real way to protect the user from that kind of scenario today. 😢

Sorry @bpieck that you lost a lot of time thinking about that. You're a very clever guy!

[bpieck]

Hm, I would not totally agree, that there is no way to prevent that.
We have some apps, where every app gets it's own client credentials. So we created an API to create a Doorkeeper::Application. On those apps we did not need PKCE, too. Since there was no common one secret. To access this additional API we created some validation.pem - and we change it with every release (and invalidate the old one). So as long as we still do version updates often enough, we are quite secure, that there is no other App working - and at least, if there is for short time some other app working with our credentials, it won't work for new users with next version release.
But of course those processes are quite some effort. So we still switched to PKCE flow for some apps, that do not need private user data.

@nbulaj Sorry for inactivity, I will open a PR later this day :)

[bpieck]

Ah, and please don't be sorry, to have opened this issue. It's great. I got the opportunity to think this through. And I learnt much in thinking this through :)

[nbulaj]

Thanks @mazikwyry @bpieck for your work!