Skip to content

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

Merged
nbulaj merged 1 commit into
doorkeeper-gem:masterfrom
f3ndot:use-confidential
Jul 10, 2018
Merged

Use Application#confidential? to determine revocation auth eligibility#1119
nbulaj merged 1 commit into
doorkeeper-gem:masterfrom
f3ndot:use-confidential

Conversation

@f3ndot

@f3ndot f3ndot commented Jul 10, 2018

Copy link
Copy Markdown
Contributor

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

f3ndot commented Jul 10, 2018

Copy link
Copy Markdown
Contributor Author

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

@nbulaj

nbulaj commented Jul 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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
Justin Bull
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
f3ndot force-pushed the use-confidential branch from 691dc3c to 8e8b9a0 Compare July 10, 2018 19:04
@f3ndot

f3ndot commented Jul 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

nbulaj commented Jul 10, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

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 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
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