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

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

Merged
merged 1 commit into from Jul 10, 2018

Conversation

Projects
None yet
2 participants
@f3ndot
Contributor

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

nbulaj commented Jul 10, 2018

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

Use Application#confidential? to determine revocation auth eligibility
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

Add NEWS entry

Add specs for both public and confidential apps in revocation

@f3ndot f3ndot force-pushed the f3ndot:use-confidential branch from 691dc3c to 8e8b9a0 Jul 10, 2018

@f3ndot

This comment has been minimized.

Contributor

f3ndot commented Jul 10, 2018

@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

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 98.584%
Details
@nbulaj

This comment has been minimized.

Member

nbulaj commented Jul 10, 2018

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

@f3ndot f3ndot deleted the f3ndot:use-confidential branch Jul 11, 2018

camallen added a commit to camallen/Panoptes that referenced this pull request Jul 26, 2018

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

jfly added a commit to jfly/worldcubeassociation.org that referenced this pull request Jul 30, 2018

When upgrading doorkeeper from 4.4.0 to 4.4.1, I saw the following po…
…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

When upgrading doorkeeper from 4.4.0 to 4.4.1, I saw the following po…
…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

When upgrading doorkeeper from 4.4.0 to 4.4.1, I saw the following po…
…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

When upgrading doorkeeper from 4.4.0 to 4.4.1, I saw the following po…
…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 added a commit to zooniverse/Panoptes that referenced this pull request Aug 7, 2018

[Security] Bump doorkeeper from 4.2.6 to 4.4.0 (#2847)
* [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.

dalikim added a commit to parti-coop/catan-web that referenced this pull request Aug 17, 2018

@huacnlee huacnlee referenced this pull request Sep 12, 2018

Merged

Upgrade Doorkeeper 5 #1067

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment