From 08d02d4b55c1a2f5135baf055c121dd0d75c447a Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Tue, 19 Jul 2022 09:21:30 -0700
Subject: [PATCH] Hash OAuth access tokens

Create a new Doorkeeper secret storing class and configure
access tokens to use it. This is a more secure
alternative to storing secrets and tokens in plaintext.

Changelog: added
---
 app/models/oauth_access_token.rb              | 10 ++++
 .../development/hash_oauth_tokens.yml         |  8 ++++
 config/initializers/doorkeeper.rb             |  2 +
 ee/spec/requests/api/scim_spec.rb             |  2 +-
 ee/spec/requests/rack_attack_spec.rb          |  2 +-
 .../create_service_spec.rb                    |  6 +--
 .../pbkdf2_sha512.rb                          | 28 +++++++++++
 spec/lib/gitlab/auth/auth_finders_spec.rb     |  6 +--
 .../pbkdf2_sha512_spec.rb                     | 36 ++++++++++++++
 spec/models/oauth_access_token_spec.rb        | 47 +++++++++++++++++++
 spec/requests/api/doorkeeper_access_spec.rb   |  6 +--
 spec/requests/api/go_proxy_spec.rb            |  6 +--
 spec/requests/api/helpers_spec.rb             |  2 +-
 .../requests/api/npm_project_packages_spec.rb |  6 +--
 spec/requests/rack_attack_global_spec.rb      | 12 ++---
 spec/support/helpers/api_helpers.rb           |  6 ++-
 .../helpers/rack_attack_spec_helpers.rb       |  6 ++-
 .../api/npm_packages_shared_examples.rb       |  8 ++--
 18 files changed, 168 insertions(+), 31 deletions(-)
 create mode 100644 config/feature_flags/development/hash_oauth_tokens.yml
 create mode 100644 lib/gitlab/doorkeeper_secret_storing/pbkdf2_sha512.rb
 create mode 100644 spec/lib/gitlab/doorkeeper_secret_storing/pbkdf2_sha512_spec.rb

diff --git a/app/models/oauth_access_token.rb b/app/models/oauth_access_token.rb
index 20130f01d448ff..b3c09d5250deae 100644
--- a/app/models/oauth_access_token.rb
+++ b/app/models/oauth_access_token.rb
@@ -17,4 +17,14 @@ def scopes=(value)
       super
     end
   end
+
+  # this method overrides a shortcoming upstream, more context:
+  # https://gitlab.com/gitlab-org/gitlab/-/issues/367888
+  def self.find_by_fallback_token(attr, plain_secret)
+    return unless fallback_secret_strategy && fallback_secret_strategy == Doorkeeper::SecretStoring::Plain
+    # token is hashed, don't allow plaintext comparison
+    return if plain_secret.starts_with?("$")
+
+    super
+  end
 end
diff --git a/config/feature_flags/development/hash_oauth_tokens.yml b/config/feature_flags/development/hash_oauth_tokens.yml
new file mode 100644
index 00000000000000..43cd5672fc4b9e
--- /dev/null
+++ b/config/feature_flags/development/hash_oauth_tokens.yml
@@ -0,0 +1,8 @@
+---
+name: hash_oauth_tokens
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91501
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/367570
+milestone: '15.3'
+type: development
+group: group::authentication and authorization
+default_enabled: false
diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb
index 6ad8b02bfea496..7f8f32f433a790 100644
--- a/config/initializers/doorkeeper.rb
+++ b/config/initializers/doorkeeper.rb
@@ -86,6 +86,8 @@
   # Check out the wiki for more information on customization
   access_token_methods :from_access_token_param, :from_bearer_authorization, :from_bearer_param
 
+  hash_token_secrets using: '::Gitlab::DoorkeeperSecretStoring::Pbkdf2Sha512', fallback: :plain
+
   # Specify what grant flows are enabled in array of Strings. The valid
   # strings and the flows they enable are:
   #
diff --git a/ee/spec/requests/api/scim_spec.rb b/ee/spec/requests/api/scim_spec.rb
index d3fa06a9fa8f7d..0b91d85202b137 100644
--- a/ee/spec/requests/api/scim_spec.rb
+++ b/ee/spec/requests/api/scim_spec.rb
@@ -17,7 +17,7 @@
   end
 
   def scim_api(url, token: true)
-    api(url, user, version: '', oauth_access_token: token ? scim_token : nil)
+    api(url, user, version: '', access_token: token ? scim_token : nil)
   end
 
   shared_examples 'SCIM token authenticated' do
diff --git a/ee/spec/requests/rack_attack_spec.rb b/ee/spec/requests/rack_attack_spec.rb
index 000feb022e43fe..81dc194505944d 100644
--- a/ee/spec/requests/rack_attack_spec.rb
+++ b/ee/spec/requests/rack_attack_spec.rb
@@ -12,7 +12,7 @@
   shared_examples_for 'incident management rate limiting' do
     let(:settings) { Gitlab::CurrentSettings.current_application_settings }
 
-    let(:token) { double(token: '123456') }
+    let(:token) { double(plaintext_token: '123456') }
     let(:period) { period_in_seconds.seconds }
 
     let(:settings_to_set) do
diff --git a/ee/spec/services/gitlab_subscriptions/create_service_spec.rb b/ee/spec/services/gitlab_subscriptions/create_service_spec.rb
index d6d11899695d83..e659bc3ab26218 100644
--- a/ee/spec/services/gitlab_subscriptions/create_service_spec.rb
+++ b/ee/spec/services/gitlab_subscriptions/create_service_spec.rb
@@ -50,7 +50,7 @@
       end
 
       it 'does not save oauth token' do
-        expect { execute }.not_to change { Doorkeeper::AccessToken.count }
+        expect { execute }.not_to change(OauthAccessToken, :count)
       end
     end
 
@@ -71,13 +71,13 @@
       end
 
       it 'saves oauth token' do
-        expect { execute }.to change { Doorkeeper::AccessToken.count }.by(1)
+        expect { execute }.to change(OauthAccessToken, :count).by(1)
       end
 
       it 'creates oauth token with correct application id' do
         execute
 
-        created_oauth_token = Doorkeeper::AccessToken.find_by_token('foo_token')
+        created_oauth_token = OauthAccessToken.by_token('foo_token')
 
         expect(created_oauth_token.application_id).to eq(oauth_app.id)
       end
diff --git a/lib/gitlab/doorkeeper_secret_storing/pbkdf2_sha512.rb b/lib/gitlab/doorkeeper_secret_storing/pbkdf2_sha512.rb
new file mode 100644
index 00000000000000..4bfb5f9e64c917
--- /dev/null
+++ b/lib/gitlab/doorkeeper_secret_storing/pbkdf2_sha512.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+module Gitlab
+  module DoorkeeperSecretStoring
+    class Pbkdf2Sha512 < ::Doorkeeper::SecretStoring::Base
+      STRETCHES = 20_000
+      # An empty salt is used because we need to look tokens up solely by
+      # their hashed value. Additionally, tokens are always cryptographically
+      # pseudo-random and unique, therefore salting provides no
+      # additional security.
+      SALT = ''
+
+      def self.transform_secret(plain_secret)
+        return plain_secret unless Feature.enabled?(:hash_oauth_tokens)
+
+        Devise::Pbkdf2Encryptable::Encryptors::Pbkdf2Sha512.digest(plain_secret, STRETCHES, SALT)
+      end
+
+      ##
+      # Determines whether this strategy supports restoring
+      # secrets from the database. This allows detecting users
+      # trying to use a non-restorable strategy with +reuse_access_tokens+.
+      def self.allows_restoring_secrets?
+        false
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/auth/auth_finders_spec.rb b/spec/lib/gitlab/auth/auth_finders_spec.rb
index e985f66bfe9449..d0b44135a2f0f9 100644
--- a/spec/lib/gitlab/auth/auth_finders_spec.rb
+++ b/spec/lib/gitlab/auth/auth_finders_spec.rb
@@ -127,7 +127,7 @@ def set_bearer_token(token)
       let(:doorkeeper_access_token) { Doorkeeper::AccessToken.create!(application_id: application.id, resource_owner_id: user.id, scopes: 'api') }
 
       before do
-        set_bearer_token(doorkeeper_access_token.token)
+        set_bearer_token(doorkeeper_access_token.plaintext_token)
       end
 
       it { is_expected.to eq user }
@@ -577,7 +577,7 @@ def set_bearer_token(token)
 
     context 'passed as header' do
       before do
-        set_bearer_token(doorkeeper_access_token.token)
+        set_bearer_token(doorkeeper_access_token.plaintext_token)
       end
 
       it 'returns token if valid oauth_access_token' do
@@ -587,7 +587,7 @@ def set_bearer_token(token)
 
     context 'passed as param' do
       it 'returns user if valid oauth_access_token' do
-        set_param(:access_token, doorkeeper_access_token.token)
+        set_param(:access_token, doorkeeper_access_token.plaintext_token)
 
         expect(find_oauth_access_token.token).to eq doorkeeper_access_token.token
       end
diff --git a/spec/lib/gitlab/doorkeeper_secret_storing/pbkdf2_sha512_spec.rb b/spec/lib/gitlab/doorkeeper_secret_storing/pbkdf2_sha512_spec.rb
new file mode 100644
index 00000000000000..e953733c9977d6
--- /dev/null
+++ b/spec/lib/gitlab/doorkeeper_secret_storing/pbkdf2_sha512_spec.rb
@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::DoorkeeperSecretStoring::Pbkdf2Sha512 do
+  describe '.transform_secret' do
+    let(:plaintext_token) { 'CzOBzBfU9F-HvsqfTaTXF4ivuuxYZuv3BoAK4pnvmyw' }
+
+    it 'generates a PBKDF2+SHA512 hashed value in the correct format' do
+      expect(described_class.transform_secret(plaintext_token))
+        .to eq("$pbkdf2-sha512$20000$$.c0G5XJVEew1TyeJk5TrkvB0VyOaTmDzPrsdNRED9vVeZlSyuG3G90F0ow23zUCiWKAVwmNnR/ceh.nJG3MdpQ") # rubocop:disable Layout/LineLength
+    end
+
+    context 'when hash_oauth_tokens is disabled' do
+      before do
+        stub_feature_flags(hash_oauth_tokens: false)
+      end
+
+      it 'returns a plaintext token' do
+        expect(described_class.transform_secret(plaintext_token)).to eq(plaintext_token)
+      end
+    end
+  end
+
+  describe 'STRETCHES' do
+    it 'is 20_000' do
+      expect(described_class::STRETCHES).to eq(20_000)
+    end
+  end
+
+  describe 'SALT' do
+    it 'is empty' do
+      expect(described_class::SALT).to be_empty
+    end
+  end
+end
diff --git a/spec/models/oauth_access_token_spec.rb b/spec/models/oauth_access_token_spec.rb
index 2b47da1ebe18a5..ccbb490ae7da78 100644
--- a/spec/models/oauth_access_token_spec.rb
+++ b/spec/models/oauth_access_token_spec.rb
@@ -43,4 +43,51 @@
       end
     end
   end
+
+  describe 'Doorkeeper secret storing' do
+    it 'stores the token in hashed format' do
+      expect(token.token).not_to eq(token.plaintext_token)
+    end
+
+    it 'does not allow falling back to plaintext token comparison' do
+      expect(described_class.by_token(token.token)).to be_nil
+    end
+
+    it 'finds a token by plaintext token' do
+      expect(described_class.by_token(token.plaintext_token)).to be_a(OauthAccessToken)
+    end
+
+    context 'when the token is stored in plaintext' do
+      let(:plaintext_token) { Devise.friendly_token(20) }
+
+      before do
+        token.update_column(:token, plaintext_token)
+      end
+
+      it 'falls back to plaintext token comparison' do
+        expect(described_class.by_token(plaintext_token)).to be_a(OauthAccessToken)
+      end
+    end
+
+    context 'when hash_oauth_secrets is disabled' do
+      let(:hashed_token) { create(:oauth_access_token, application_id: app_one.id) }
+
+      before do
+        hashed_token
+        stub_feature_flags(hash_oauth_tokens: false)
+      end
+
+      it 'stores the token in plaintext' do
+        expect(token.token).to eq(token.plaintext_token)
+      end
+
+      it 'finds a token by plaintext token' do
+        expect(described_class.by_token(token.plaintext_token)).to be_a(OauthAccessToken)
+      end
+
+      it 'does not find a token that was previously stored as hashed' do
+        expect(described_class.by_token(hashed_token.plaintext_token)).to be_nil
+      end
+    end
+  end
 end
diff --git a/spec/requests/api/doorkeeper_access_spec.rb b/spec/requests/api/doorkeeper_access_spec.rb
index 77f1dadff4607a..14da9a600cdf6d 100644
--- a/spec/requests/api/doorkeeper_access_spec.rb
+++ b/spec/requests/api/doorkeeper_access_spec.rb
@@ -9,13 +9,13 @@
 
   describe "unauthenticated" do
     it "returns authentication success" do
-      get api("/user"), params: { access_token: token.token }
+      get api("/user"), params: { access_token: token.plaintext_token }
       expect(response).to have_gitlab_http_status(:ok)
     end
 
     include_examples 'user login request with unique ip limit' do
       def request
-        get api('/user'), params: { access_token: token.token }
+        get api('/user'), params: { access_token: token.plaintext_token }
       end
     end
   end
@@ -42,7 +42,7 @@ def request
 
   shared_examples 'forbidden request' do
     it 'returns 403 response' do
-      get api("/user"), params: { access_token: token.token }
+      get api("/user"), params: { access_token: token.plaintext_token }
 
       expect(response).to have_gitlab_http_status(:forbidden)
     end
diff --git a/spec/requests/api/go_proxy_spec.rb b/spec/requests/api/go_proxy_spec.rb
index 0143340de11046..45ac6d8593d875 100644
--- a/spec/requests/api/go_proxy_spec.rb
+++ b/spec/requests/api/go_proxy_spec.rb
@@ -376,7 +376,7 @@
       end
 
       it 'returns ok with a job token' do
-        get_resource(oauth_access_token: job)
+        get_resource(access_token: job)
 
         expect(response).to have_gitlab_http_status(:ok)
       end
@@ -395,7 +395,7 @@
 
       it 'returns unauthorized with a failed job token' do
         job.update!(status: :failed)
-        get_resource(oauth_access_token: job)
+        get_resource(access_token: job)
 
         expect(response).to have_gitlab_http_status(:unauthorized)
       end
@@ -445,7 +445,7 @@ def get_resource(user = nil, **params)
       end
 
       it 'returns not found with a job token' do
-        get_resource(oauth_access_token: job)
+        get_resource(access_token: job)
 
         expect(response).to have_gitlab_http_status(:not_found)
       end
diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb
index 8961f3177b67ab..2862cd9ee42b59 100644
--- a/spec/requests/api/helpers_spec.rb
+++ b/spec/requests/api/helpers_spec.rb
@@ -539,7 +539,7 @@ def set_param(key, value)
       let(:token) { create(:oauth_access_token) }
 
       before do
-        env['HTTP_AUTHORIZATION'] = "Bearer #{token.token}"
+        env['HTTP_AUTHORIZATION'] = "Bearer #{token.plaintext_token}"
       end
 
       it_behaves_like 'sudo'
diff --git a/spec/requests/api/npm_project_packages_spec.rb b/spec/requests/api/npm_project_packages_spec.rb
index 62809b432af01d..57052ff21976c9 100644
--- a/spec/requests/api/npm_project_packages_spec.rb
+++ b/spec/requests/api/npm_project_packages_spec.rb
@@ -59,7 +59,7 @@
       end
 
       context 'with access token' do
-        let(:headers) { build_token_auth_header(token.token) }
+        let(:headers) { build_token_auth_header(token.plaintext_token) }
 
         it_behaves_like 'successfully downloads the file'
         it_behaves_like 'a package tracking event', 'API::NpmPackages', 'pull_package'
@@ -95,7 +95,7 @@
       it_behaves_like 'a package file that requires auth'
 
       context 'with guest' do
-        let(:headers) { build_token_auth_header(token.token) }
+        let(:headers) { build_token_auth_header(token.plaintext_token) }
 
         it 'denies download when not enough permissions' do
           project.add_guest(user)
@@ -356,7 +356,7 @@ def upload_package(package_name, params = {})
     end
 
     def upload_with_token(package_name, params = {})
-      upload_package(package_name, params.merge(access_token: token.token))
+      upload_package(package_name, params.merge(access_token: token.plaintext_token))
     end
 
     def upload_with_job_token(package_name, params = {})
diff --git a/spec/requests/rack_attack_global_spec.rb b/spec/requests/rack_attack_global_spec.rb
index 115f78a5600e62..f6b9bc527ac763 100644
--- a/spec/requests/rack_attack_global_spec.rb
+++ b/spec/requests/rack_attack_global_spec.rb
@@ -104,8 +104,8 @@
     end
 
     context 'with the token in the OAuth headers' do
-      let(:request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(token)) }
-      let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(other_user_token)) }
+      let(:request_args) { api_get_args_with_token_headers(api_partial_url, bearer_headers(token)) }
+      let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, bearer_headers(other_user_token)) }
 
       it_behaves_like 'rate-limited user based token-authenticated requests'
     end
@@ -131,8 +131,8 @@
       end
 
       context 'with the token in the OAuth headers' do
-        let(:request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(token)) }
-        let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, oauth_token_headers(other_user_token)) }
+        let(:request_args) { api_get_args_with_token_headers(api_partial_url, bearer_headers(token)) }
+        let(:other_user_request_args) { api_get_args_with_token_headers(api_partial_url, bearer_headers(other_user_token)) }
 
         it_behaves_like 'rate-limited user based token-authenticated requests'
       end
@@ -1189,7 +1189,7 @@ def expect_authenticated_request
       it 'request is authenticated by token in the OAuth headers' do
         expect_authenticated_request
 
-        get url, headers: oauth_token_headers(personal_access_token)
+        get url, headers: bearer_headers(personal_access_token)
       end
 
       it 'request is authenticated by token in basic auth' do
@@ -1206,7 +1206,7 @@ def expect_authenticated_request
       it 'request is authenticated by token in query string' do
         expect_authenticated_request
 
-        get url, params: { access_token: oauth_token.token }
+        get url, params: { access_token: oauth_token.plaintext_token }
       end
 
       it 'request is authenticated by token in the headers' do
diff --git a/spec/support/helpers/api_helpers.rb b/spec/support/helpers/api_helpers.rb
index fd85071cca3600..398588b5987a7a 100644
--- a/spec/support/helpers/api_helpers.rb
+++ b/spec/support/helpers/api_helpers.rb
@@ -19,15 +19,17 @@ module ApiHelpers
   #   => "/api/v2/issues?foo=bar&private_token=..."
   #
   # Returns the relative path to the requested API resource
-  def api(path, user = nil, version: API::API.version, personal_access_token: nil, oauth_access_token: nil, job_token: nil)
+  def api(path, user = nil, version: API::API.version, personal_access_token: nil, oauth_access_token: nil, job_token: nil, access_token: nil)
     full_path = "/api/#{version}#{path}"
 
     if oauth_access_token
-      query_string = "access_token=#{oauth_access_token.token}"
+      query_string = "access_token=#{oauth_access_token.plaintext_token}"
     elsif personal_access_token
       query_string = "private_token=#{personal_access_token.token}"
     elsif job_token
       query_string = "job_token=#{job_token}"
+    elsif access_token
+      query_string = "access_token=#{access_token.token}"
     elsif user
       personal_access_token = create(:personal_access_token, user: user)
       query_string = "private_token=#{personal_access_token.token}"
diff --git a/spec/support/helpers/rack_attack_spec_helpers.rb b/spec/support/helpers/rack_attack_spec_helpers.rb
index 6c06781df0349c..2502889e17c269 100644
--- a/spec/support/helpers/rack_attack_spec_helpers.rb
+++ b/spec/support/helpers/rack_attack_spec_helpers.rb
@@ -17,8 +17,12 @@ def personal_access_token_headers(personal_access_token)
     { Gitlab::Auth::AuthFinders::PRIVATE_TOKEN_HEADER => personal_access_token.token }
   end
 
+  def bearer_headers(token)
+    { 'AUTHORIZATION' => "Bearer #{token.token}" }
+  end
+
   def oauth_token_headers(oauth_access_token)
-    { 'AUTHORIZATION' => "Bearer #{oauth_access_token.token}" }
+    { 'AUTHORIZATION' => "Bearer #{oauth_access_token.plaintext_token}" }
   end
 
   def basic_auth_headers(user, personal_access_token)
diff --git a/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb b/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb
index 8d6d85732be92f..b651ffc8996309 100644
--- a/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb
+++ b/spec/support/shared_examples/requests/api/npm_packages_shared_examples.rb
@@ -244,7 +244,7 @@
       let(:headers) do
         case auth
         when :oauth
-          build_token_auth_header(token.token)
+          build_token_auth_header(token.plaintext_token)
         when :personal_access_token
           build_token_auth_header(personal_access_token.token)
         when :job_token
@@ -404,7 +404,7 @@
 
   shared_examples 'handling all conditions' do
     context 'with oauth token' do
-      let(:headers) { build_token_auth_header(token.token) }
+      let(:headers) { build_token_auth_header(token.plaintext_token) }
 
       it_behaves_like 'handling different package names, visibilities and user roles'
     end
@@ -514,7 +514,7 @@
 
   shared_examples 'handling all conditions' do
     context 'with oauth token' do
-      let(:headers) { build_token_auth_header(token.token) }
+      let(:headers) { build_token_auth_header(token.plaintext_token) }
 
       it_behaves_like 'handling different package names, visibilities and user roles'
     end
@@ -622,7 +622,7 @@
 
   shared_examples 'handling all conditions' do
     context 'with oauth token' do
-      let(:headers) { build_token_auth_header(token.token) }
+      let(:headers) { build_token_auth_header(token.plaintext_token) }
 
       it_behaves_like 'handling different package names, visibilities and user roles'
     end
-- 
GitLab