Skip to content

Upgrade Doorkeeper to 4.4.3

Josianne Hyson requested to merge jhyson/doorkeeper_upgrade into master

What does this MR do?

Updates the doorkeeper gem to 4.4.3 so that we can address security issue CVE-2018-1000211.

This issue involves token revocation not working for public apps.

The version migration notes are available here: https://github.com/doorkeeper-gem/doorkeeper/wiki/Migration-from-old-versions#api-changes-4

Completing this upgrade will also enable us to complete the upgrade of the doorkeeper-openid_connect gem #27383 (closed)

Steps

  • Run rails generate doorkeeper:add_client_confidentiality for the migration
  • Review your OAuth apps and determine which ones exclusively use public grant flows (eg implicit)
  • Update their confidential column to false for those public apps
    • This has been addressed by first setting all the existing applications to confidential: false, and then setting the column default to true, as suggested in this comment on the original MR: gitlab-foss!21576 (comment 100133446)

Output of bundle update

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:

  * https://github.com/doorkeeper-gem/doorkeeper/pull/1119
  * https://github.com/doorkeeper-gem/doorkeeper/issues/891

Migration generated by running rails generate doorkeeper:add_client_confidentiality

class AddConfidentialToDoorkeeperApplication < ActiveRecord::Migration[5.2]
  def change
    add_column(
      :oauth_applications,
      :confidential,
      :boolean,
      null: false,
      default: true # maintaining backwards compatibility: require secrets
    )
  end
end

Running rails db:migrate:up VERSION=20191127163053

== 20191127163053 AddConfidentialToDoorkeeperApplication: migrating ===========
-- transaction_open?()
   -> 0.0000s
-- execute("SET statement_timeout TO 0")
   -> 0.0005s
-- transaction()
-- add_column(:oauth_applications, :confidential, :boolean, {:default=>nil})
   -> 0.0018s
-- change_column_default(:oauth_applications, :confidential, false)
   -> 0.0036s
   -> 0.0064s
-- columns(:oauth_applications)
   -> 0.0014s
-- transaction_open?()
   -> 0.0000s
-- exec_query("SELECT COUNT(*) AS count FROM \"oauth_applications\"")
   -> 0.0021s
-- exec_query("SELECT  \"oauth_applications\".\"id\" FROM \"oauth_applications\" ORDER BY \"oauth_applications\".\"id\" ASC LIMIT 1")
   -> 0.0004s
-- exec_query("SELECT  \"oauth_applications\".\"id\" FROM \"oauth_applications\" WHERE \"oauth_applications\".\"id\" >= 1 ORDER BY \"oauth_applications\".\"id\" ASC LIMIT 1 OFFSET 1")
   -> 0.0003s
-- execute("UPDATE \"oauth_applications\" SET \"confidential\" = FALSE WHERE \"oauth_applications\".\"id\" >= 1 AND \"oauth_applications\".\"id\" < 2")
   -> 0.0005s
-- exec_query("SELECT  \"oauth_applications\".\"id\" FROM \"oauth_applications\" WHERE \"oauth_applications\".\"id\" >= 2 ORDER BY \"oauth_applications\".\"id\" ASC LIMIT 1 OFFSET 1")
   -> 0.0003s
-- execute("UPDATE \"oauth_applications\" SET \"confidential\" = FALSE WHERE \"oauth_applications\".\"id\" >= 2 AND \"oauth_applications\".\"id\" < 3")
   -> 0.0005s
-- exec_query("SELECT  \"oauth_applications\".\"id\" FROM \"oauth_applications\" WHERE \"oauth_applications\".\"id\" >= 3 ORDER BY \"oauth_applications\".\"id\" ASC LIMIT 1 OFFSET 1")
   -> 0.0003s
-- execute("UPDATE \"oauth_applications\" SET \"confidential\" = FALSE WHERE \"oauth_applications\".\"id\" >= 3 AND \"oauth_applications\".\"id\" < 4")
   -> 0.0004s
-- exec_query("SELECT  \"oauth_applications\".\"id\" FROM \"oauth_applications\" WHERE \"oauth_applications\".\"id\" >= 4 ORDER BY \"oauth_applications\".\"id\" ASC LIMIT 1 OFFSET 1")
   -> 0.0003s
-- execute("UPDATE \"oauth_applications\" SET \"confidential\" = FALSE WHERE \"oauth_applications\".\"id\" >= 4 AND \"oauth_applications\".\"id\" < 5")
   -> 0.0004s
-- exec_query("SELECT  \"oauth_applications\".\"id\" FROM \"oauth_applications\" WHERE \"oauth_applications\".\"id\" >= 5 ORDER BY \"oauth_applications\".\"id\" ASC LIMIT 1 OFFSET 1")
   -> 0.0003s
-- execute("UPDATE \"oauth_applications\" SET \"confidential\" = FALSE WHERE \"oauth_applications\".\"id\" >= 5")
   -> 0.0004s
-- change_column_null(:oauth_applications, :confidential, false)
   -> 0.0005s
-- execute("RESET ALL")
   -> 0.0002s
-- change_column_default(:oauth_applications, :confidential, true)
   -> 0.0025s
== 20191127163053 AddConfidentialToDoorkeeperApplication: migrated (0.0192s) ==

Running rails db:migrate:down VERSION=20191127163053

== 20191127163053 AddConfidentialToDoorkeeperApplication: reverting ===========
-- remove_column(:oauth_applications, :confidential)
   -> 0.0015s
== 20191127163053 AddConfidentialToDoorkeeperApplication: reverted (0.0016s) ==

Updates

The following areas have been updated so that users can manage the confidentiality of the applications

UI Updates

Admin Area

Screen_Shot_2019-12-12_at_4.51.26_PM

User Settings

Screen_Shot_2019-12-12_at_4.52.10_PM

API Updates

Add confidential optionally to the create query params - will default to true if not supplied:

POST /api/v4/applications?name=test&redirect_uri=http://localhost:3000/redirect&scopes=api&confidential=false

Add confidential to the API response:

{
    "id": 7,
    "application_id": "cfa8b5d03509016bbca0...",
    "application_name": "test",
    "callback_url": "http://localhost:3000/redirect",
    "confidential": true,
    "secret": "b471b8502f339dde9a0fabe..."
}

Database checklist

When adding migrations:

  • Updated db/schema.rb
  • Added a down method so the migration can be reverted
  • Added the output of the migration(s) to the MR body
  • Added tests for the migration in spec/migrations if necessary (e.g. when migrating data)
  • Added rollback procedure. Include either a rollback procedure or description how to rollback changes

When adding or modifying queries to improve performance:

  • Included data that shows the performance improvement, preferably in the form of a benchmark
  • Included the output of EXPLAIN (ANALYZE, BUFFERS) of the relevant queries

When adding foreign keys to existing tables:

  • Included a migration to remove orphaned rows in the source table before adding the foreign key
  • Removed any instances of dependent: ... that may no longer be necessary

When adding tables:

  • Ordered columns based on the Ordering Table Columns guidelines
  • Added foreign keys to any columns pointing to data in other tables
  • Added indexes for fields that are used in statements such as WHERE, ORDER BY, GROUP BY, and JOINs

When removing columns, tables, indexes or other structures:

  • Removed these in a post-deployment migration
  • Made sure the application no longer uses (or ignores) these structures

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by 🤖 GitLab Bot 🤖

Merge request reports