-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Recent Change in Token Revocation Endpoint Behavior Breaks Revocation of Tokens for Public Client #1412
Comments
Hi @edmondchui . Did you do this from your mobile app?
You need to call POST /token/revoke with More details: section 2.1. Revocation Request of the RFC7009 and then section 2.3. Client Authentication of RFC6749 |
I've got a similar problem. I'm trying to upgrade from Doorkeeper 5.1 to latest. Token revocation stopped working for me. I'm getting 403 with Previously Current upgrade PR: AndKiel/tournaments-api#8 PS |
@AndKiel does your token issued to public or private client? Do you use the same client for authorization that the token you wanna to revoke issued for? This all covered by specs, would be great if you could provide a reproducable spec to fix the bug (if it's a bug) |
@nbulaj, my project is public repository so I think it can be treated as reproducible? (you can see Travis running spec on the PR I linked) I'm using |
Even password grant flow requires auth to execute a request. And if token was issued to some client (has application_id filled) - it could be revoked only by the same client. If I have the time - I'll take a look at your repo, but it could take long time Also did you try to perform a request using |
No, I haven't tried |
Please take a look at section 2.1. Revocation Request of the RFC7009 and then section 2.3. Client Authentication of RFC6749. Before the changes Doorkeeper had a bug (as well as a security issue) and allowed to revoke token without any authentication. That was fixed according to OAuth RFC. You can find specs for changes here https://github.com/doorkeeper-gem/doorkeeper/pull/1370/files to find use cases for public/private clients and authentication. |
@AndKiel if you didn't use (aka create) any client (Doorkeeper Application) instances before - you may need to create a new one (let it be non-confidential) and use it's credentials to authorize the request. Your mobile app or whatever could store this pair (client ID and client secret) to revoke the tokens |
Okay, I've taken a look. RFC7009#2.1 says that RFC7009#2.1 also provides an example that is considered valid:
So Basic Authorization header would be the client_id + client_secret if I understand correctly. But there's RFC6749#2.3. First paragraph - only for confidential clients. Second paragraph - continuation of that. Third paragraph:
May. Doesn't have to. So shouldn't this have worked when testing:
It has I feel like I'm still getting this wrong or missing some crucial part here. |
Public client means it doesn't have s secret, just ID. Public client means that client exists and it could be identified just by ID. Please check also section 5. Security Considerations of RFC7009:
Sorry, but I wouldn't support this "May. Doesn't have to" for the security reasons. Nothing actually stops you from creating a public client and using it's credentials to authorize the request.
Nope. Lack of authentication just open a security hole for brute-forcing (throtling) any tokens you have. |
Hi @nbulaj,
Yes. My mobile app issues a logout request to my app server (the confidential client). My app server then calls the authorization server Before version 5.4.0, it works successfully as the authorization server One more observation: The token granted by the authorization server to the mobile app via the Resource Owner Password Credentials Flow does not have the
Do you mean to use the Thanks, |
Nope @edmondchui, it's just because Doorkeeper < 5.4 had a bug and security issue and didn't conform the RFC. Authorization server didn't perform any check except if token belongs to a client that requests the revocation.
Exactly. You need to add a new header Here - d194bc4 - you could find specs at the end of the PR which have all the test cases for authorizing the request |
I found out that the token granted by the authorization server to the mobile app via the Resource Owner Password Credentials Flow does not have the |
There are no issue with Password flow, it just allows to authenticate without passing client credentials. If the access token doesn't have But to revoke the token you need to authorize. Authorization server requires authentication in order to protect your from bruteforcing your tokens. Without protection I could just revoke all the tokens you have on the server. As I already answered above for AndKiel, you could just create a new Doorkeeper Application instance and use it's credentials in the revocation request from your mobile/web application. All you need is:
|
Also it's a question for me how both of you could obtain an access token using password grant flow without requiring I remember is was here: #1347 UPD: ok the culprit is ea66661 from 2013 😨 I'll check if it conforms with https://tools.ietf.org/html/rfc6749#section-4.3 |
Oh guys, I think I have bad news for you (and everyone else). I think we must fix this too and break a lot of clients that creates tokens without any client authorization :( I foresee much more such issues in a near future...
We have |
If client credentials are provided during the Resource Owner Password Grant flow, they will be validated. If invalid credentials are provided, the invalid client error will be presented. If no client credentials are provided, the flow will continue as usual.
You're saying that all those years I was using doorkeeper with password grant flow and without creating an application... was a bug. That bug is probably the reason a lot of people love doorkeeper. Simplicity and literally zero setup to have email+password authentication in any project. :D |
Yep, it's sad, but true :( I don't sure why it's so hard to create one record and use it's credentials in requests... Anyway I need to conform the RFC. So I must fix this. I'll try to do it in a backward compatible way with first just a deprecation message and then with a totally removed credentials-free behavior. More examples:
|
It's not exactly about it being hard. It's pointless when it's for a front-end app that's all in a browser. Right now I can have two dockerized projects that require running |
You could just use seeds for this: Doorkeeper::Application.find_or_create!(name: "freontend", uid "set your long own value", secret: "the same", ...) And use this credentials in the FE application. All you need is to also to run seeds ( |
I know that seeding kinda solves the problem but IMHO enforcing application makes little sense here. It provides no additional security in this particular use-case because front-end code is fully accessible in the browser so neither client_id nor client_secret would actually be secret. |
Yep, and this is one of the reasons (except sharing user credentials) why this grant flow doesn't recommended for use by the RFC (and even more - forbidden in latest Security considerations document), and could be used only by highly trusted applications. Btw whether we want it or not - we have a document that specifies the behavior and we need to follow it. Everybody who don't wanna to do it could path Doorkeeper internals and use any desired behavior :) |
Personally, I'd rather have an option to disable this client/application requirement in the configuration so Doorkeeper still would be usable as is with such front-end apps. With it being required by default, of course, and documenting how bad disabling it is. Such front-end apps for public-facing APIs are valid real-life use-case. |
Yep, this is one of the options I think about. I just don't like to bloat the configuration with a lot of options, especially when there is no need in them. So I think I'll add such option, but with deprecation and it will be removed in some major update. |
@edmondchui can we close this issue now? Do you need more help with it? |
Since there is nothing much I can do except to downgrade to version 5.3.3, I guess you can make this a known issue on the release notes before closing this. That could save other's time to research on this. Thanks. |
It's not an issue actually, and it's a known breaking change:
Known (at least now) issue is that you guys could use ROPC grant without using a client which would be fixed soon, yep. I think I can close it for now, just need to remember issue number for the same threads in the near future. |
So if I am interpreting the current status of things correctly. I can request tokens without client credentials. These tokens will have no Application client association. But when I revoke a token it will fail without a client id. I can't image that a revoke request with a client id is able to revoke a token with no associated client. So how would anyone ever revoke a clientless token? |
FYI for anyone interested in this - #1458 |
Steps to reproduce
I have a mobile app using a Rails server, which in turn depends on Doorkeeper to generate access tokens for API calls. I used to be able to logout successfully from the client using a call to
Doorkeeper::TokensController.revoke
. However, after upgrading to version Doorkeeper 5.4.0, I got HTTP response 401 during logout.Expected behavior
Token revocation using a valid access token from Resource Owner Password Credentials Flow should work.
Actual behavior
Token revocation fails with a valid access token from Resource Owner Password Credentials Flow.
System configuration
In version 5.3.3, before this change d194bc4, a token granted to a public client (https://tools.ietf.org/html/rfc6749#section-2.1) via Resource Owner Password Credentials Flow works fine as the method
Doorkeeper::TokensController.authorized?
is used for authentication.In version 5.4.0, the method
Doorkeeper::TokensController.revoke
is modified to require aserver.client
, which is a confidential client (https://tools.ietf.org/html/rfc6749#section-2.1).Ruby version:
2.6.5
The text was updated successfully, but these errors were encountered: