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.

Sign up for GitHub

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

Jump to bottom

Use Application#confidential? to determine revocation auth eligibility #1119

Merged
merged 1 commit into from
Jul 10, 2018
Merged

Use Application#confidential? to determine revocation auth eligibility #1119

merged 1 commit into from
Jul 10, 2018

Conversation

f3ndot
Copy link
Contributor

@f3ndot f3ndot commented Jul 10, 2018

OAuth applications that obtain an access token using the "implicit" grant flow will have their ID set on the token record. Unfortunately this causes the revocation controller code to think it's as confidential application. Because of this, Doorkeeper enforces oauth client authentication and the revocation call fails.

Fixes #891

@f3ndot
Copy link
Contributor Author

f3ndot commented Jul 10, 2018

Since this is security related, is it worth backporting to 4.x? Would require #1031 to also be backported.

@nbulaj
Copy link
Member

nbulaj commented Jul 10, 2018 •
edited
Loading

I don't sure we can backport it due to incompatibility issues. Everyone need to migrate new columns and so on.

@nbulaj nbulaj mentioned this pull request Jul 10, 2018
Closed
Use Application#confidential? to determine revocation auth eligibility
8e8b9a0 
OAuth applications that obtain an access token using the "implicit" grant flow will have their ID set on the token record. Unfortunately this causes the revocation controller code to think it's as confidential application. Because of this, Doorkeeper enforces oauth client authentication and the revocation call fails.

Fixes doorkeeper-gem#891

Add NEWS entry

Add specs for both public and confidential apps in revocation
@f3ndot
Copy link
Contributor Author

f3ndot commented Jul 10, 2018 •
edited
Loading

@nbulaj this can be merged for 5.x fix.

Semver is getting in the way here, because I agree that we cannot "backport" to 4.x without requiring an upgrade path. #1031 defaults all apps as confidential, so it maintains backwards compatibility in that sense. However developers must:

  1. Run a migration to add the column.
  2. Decide for themselves which apps are public and update them accordingly

I'm not familiar with the rules around stuff like this, but can't we autogenerate some "hey you're using 4.2.0-secfix1, please run rails generate doorkeeper:install_client_confidentiality" which would simply add the column? That's essentially a MINOR-level sort of change. Then they opt into using it by toggling which apps are public?

🤷‍♂️

@nbulaj nbulaj merged commit 66d8e18 into doorkeeper-gem:master Jul 10, 2018
@nbulaj
Copy link
Member

nbulaj commented Jul 10, 2018 •
edited
Loading

No problem for the 5.x release, yes,. What about 4.x - I'll think on it tomorow. Thanks!

@f3ndot f3ndot deleted the use-confidential branch July 11, 2018 13:49
camallen added a commit to camallen/Panoptes that referenced this pull request Jul 26, 2018
@camallen
add confidential column to oauth_applications
b7407d6 
WARNING: This is a security release that addresses token revocation not working for public apps (CVE-2018-1000211)

There is no breaking change in this release, however to take advantage of the security fix you must:

  1. Run `rails generate doorkeeper:add_client_confidentiality` for the migration
  2. Review your OAuth apps and determine which ones exclusively use public grant flows (eg implicit)
  3. Update their `confidential` column to `false` for those public apps

This is a backported security release.

  For more information:

    * doorkeeper-gem/doorkeeper#1119
    * doorkeeper-gem/doorkeeper#891
jfly added a commit to jfly/worldcubeassociation.org that referenced this pull request Jul 30, 2018
@jfly
When upgrading doorkeeper from 4.4.0 to 4.4.1, I saw the following po…
6a23195 
…st install message:

  WARNING: This is a security release that addresses token revocation not working for public apps (CVE-2018-1000211)

  There is no breaking change in this release, however to take advantage of the security fix you must:

    1. Run `rails generate doorkeeper:add_client_confidentiality` for the migration
    2. Review your OAuth apps and determine which ones exclusively use public grant flows (eg implicit)
    3. Update their `confidential` column to `false` for those public apps

  This is a backported security release.

  For more information:

    * doorkeeper-gem/doorkeeper#1119
    * doorkeeper-gem/doorkeeper#891

This adds the mentioned migration, but does not bother to set
confidential to false for public applications. As far as I can tell,
this just means that despite upgrading doorkeeper, it will still not be
possible for public applications to revoke tokens, so we're no worse off
than we were before. I believe that in doorkeeper 5, there will be a UI
that allows application developers to decide if their application is
"confidential" or not. I think it's ok for us to just wait for that day,
and leave this effort on our various 3rd party developers.
jfly added a commit to jfly/worldcubeassociation.org that referenced this pull request Jul 31, 2018
@jfly
When upgrading doorkeeper from 4.4.0 to 4.4.1, I saw the following po…
0e56743 
…st install message:

  WARNING: This is a security release that addresses token revocation not working for public apps (CVE-2018-1000211)

  There is no breaking change in this release, however to take advantage of the security fix you must:

    1. Run `rails generate doorkeeper:add_client_confidentiality` for the migration
    2. Review your OAuth apps and determine which ones exclusively use public grant flows (eg implicit)
    3. Update their `confidential` column to `false` for those public apps

  This is a backported security release.

  For more information:

    * doorkeeper-gem/doorkeeper#1119
    * doorkeeper-gem/doorkeeper#891

This adds the mentioned migration, but does not bother to set
confidential to false for public applications. As far as I can tell,
this just means that despite upgrading doorkeeper, it will still not be
possible for public applications to revoke tokens, so we're no worse off
than we were before. I believe that in doorkeeper 5, there will be a UI
that allows application developers to decide if their application is
"confidential" or not. I think it's ok for us to just wait for that day,
and leave this effort on our various 3rd party developers.
jfly added a commit to jfly/worldcubeassociation.org that referenced this pull request Jul 31, 2018
@jfly
When upgrading doorkeeper from 4.4.0 to 4.4.1, I saw the following po…
966d6f4 
…st install message:

  WARNING: This is a security release that addresses token revocation not working for public apps (CVE-2018-1000211)

  There is no breaking change in this release, however to take advantage of the security fix you must:

    1. Run `rails generate doorkeeper:add_client_confidentiality` for the migration
    2. Review your OAuth apps and determine which ones exclusively use public grant flows (eg implicit)
    3. Update their `confidential` column to `false` for those public apps

  This is a backported security release.

  For more information:

    * doorkeeper-gem/doorkeeper#1119
    * doorkeeper-gem/doorkeeper#891

This adds the mentioned migration, but does not bother to set
confidential to false for public applications. As far as I can tell,
this just means that despite upgrading doorkeeper, it will still not be
possible for public applications to revoke tokens, so we're no worse off
than we were before. I believe that in doorkeeper 5, there will be a UI
that allows application developers to decide if their application is
"confidential" or not. I think it's ok for us to just wait for that day,
and leave this effort on our various 3rd party developers.
jfly added a commit to thewca/worldcubeassociation.org that referenced this pull request Jul 31, 2018
@jfly
When upgrading doorkeeper from 4.4.0 to 4.4.1, I saw the following po…
69a84a7 
…st install message:

  WARNING: This is a security release that addresses token revocation not working for public apps (CVE-2018-1000211)

  There is no breaking change in this release, however to take advantage of the security fix you must:

    1. Run `rails generate doorkeeper:add_client_confidentiality` for the migration
    2. Review your OAuth apps and determine which ones exclusively use public grant flows (eg implicit)
    3. Update their `confidential` column to `false` for those public apps

  This is a backported security release.

  For more information:

    * doorkeeper-gem/doorkeeper#1119
    * doorkeeper-gem/doorkeeper#891

This adds the mentioned migration, but does not bother to set
confidential to false for public applications. As far as I can tell,
this just means that despite upgrading doorkeeper, it will still not be
possible for public applications to revoke tokens, so we're no worse off
than we were before. I believe that in doorkeeper 5, there will be a UI
that allows application developers to decide if their application is
"confidential" or not. I think it's ok for us to just wait for that day,
and leave this effort on our various 3rd party developers.
camallen pushed a commit to zooniverse/panoptes that referenced this pull request Aug 7, 2018
@dependabot @camallen
[Security] Bump doorkeeper from 4.2.6 to 4.4.0 (#2847)
f633f95 
* [Security] Bump doorkeeper from 4.2.6 to 4.4.0

Bumps [doorkeeper](https://github.com/doorkeeper-gem/doorkeeper) from 4.2.6 to 4.4.0. **This update includes security fixes.**
- [Release notes](https://github.com/doorkeeper-gem/doorkeeper/releases)
- [Changelog](https://github.com/doorkeeper-gem/doorkeeper/blob/master/NEWS.md)
- [Commits](doorkeeper-gem/doorkeeper@v4.2.6...v4.4.0)

Signed-off-by: dependabot[bot] <support@dependabot.com>

* add confidential column to oauth_applications

WARNING: This is a security release that addresses token revocation not working for public apps (CVE-2018-1000211)

There is no breaking change in this release, however to take advantage of the security fix you must:

  1. Run `rails generate doorkeeper:add_client_confidentiality` for the migration
  2. Review your OAuth apps and determine which ones exclusively use public grant flows (eg implicit)
  3. Update their `confidential` column to `false` for those public apps

This is a backported security release.

  For more information:

    * doorkeeper-gem/doorkeeper#1119
    * doorkeeper-gem/doorkeeper#891

* add note on what this migration default value means

some of our apps (first_party password / session grants and implicit) will require the non-default value of confidential = false

* switch to non-confidential oauth application

ensure the spec testing devise session to token via password grant (#101) uses a non-confidential application

* add data migration for oauth confidential attribute

set confidential to false for implicit, native apps as well as any first party apps that use the password grant without supplying the client secret (PFE / python client).

* move comment to be more informative

* only change apps we know use non-confidential password flow

avoid changing other apps that may be using client_id & client_secrets to authenticate, only update the first party apps we know about.
@huacnlee huacnlee mentioned this pull request Sep 12, 2018
Merged
@arianvp arianvp mentioned this pull request Oct 17, 2018
Closed
kaznum added a commit to kaznum/sanataro that referenced this pull request Nov 14, 2019
@kaznum
Migration for doorkeeper udpate
48b5cc3 
Post-install message from doorkeeper:

  WARNING: This is a security release that addresses token revocation not working for public apps (CVE-2018-1000211)

  There is no breaking change in this release, however to take advantage of the security fix you must:

    1. Run `rails generate doorkeeper:add_client_confidentiality` for the migration
    2. Review your OAuth apps and determine which ones exclusively use public grant flows (eg implicit)
    3. Update their `confidential` column to `false` for those public apps

  This is a backported security release.

  For more information:

    * doorkeeper-gem/doorkeeper#1119
    * doorkeeper-gem/doorkeeper#891
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@f3ndot @nbulaj