Merge branch '232670-webauthn-background-migration' into 'master'

Migrate existing U2F registrations to WebAuthn

See merge request gitlab-org/gitlab!42159

app/models/u2f_registration.rb

@@ -4,6 +4,19 @@
 
 class U2fRegistration < ApplicationRecord
   belongs_to :user
+  after_commit :schedule_webauthn_migration, on: :create
+  after_commit :update_webauthn_registration, on: :update, if: :counter_changed?
+
+  def schedule_webauthn_migration
+    BackgroundMigrationWorker.perform_async('MigrateU2fWebauthn', [id, id])
+  end
+
+  def update_webauthn_registration
+    # When we update the sign count of this registration
+    # we need to update the sign count of the corresponding webauthn registration
+    # as well if it exists already
+    WebauthnRegistration.find_by_credential_xid(webauthn_credential_xid)&.update_attribute(:counter, counter)
+  end
 
   def self.register(user, app_id, params, challenges)
     u2f = U2F::U2F.new(app_id)
@@ -40,4 +53,13 @@ class U2fRegistration < ApplicationRecord
   rescue JSON::ParserError, NoMethodError, ArgumentError, U2F::Error
     false
   end
+
+  private
+
+  def webauthn_credential_xid
+    # To find the corresponding webauthn registration, we use that
+    # the key handle of the u2f reg corresponds to the credential xid of the webauthn reg
+    # (with some base64 back and forth)
+    Base64.strict_encode64(Base64.urlsafe_decode64(key_handle))
+  end
 end

app/services/webauthn/authenticate_service.rb

@@ -11,12 +11,6 @@ module Webauthn
     def execute
       parsed_device_response = Gitlab::Json.parse(@device_response)
 
-      # appid is set for legacy U2F devices, will be used in a future iteration
-      # rp_id = @app_id
-      # unless parsed_device_response['clientExtensionResults'] && parsed_device_response['clientExtensionResults']['appid']
-      # rp_id = URI(@app_id).host
-      # end
-
       webauthn_credential = WebAuthn::Credential.from_get(parsed_device_response)
       encoded_raw_id = Base64.strict_encode64(webauthn_credential.raw_id)
       stored_webauthn_credential = @user.webauthn_registrations.find_by_credential_xid(encoded_raw_id)
@@ -52,10 +46,14 @@ module Webauthn
     # Verifies that webauthn_credential matches stored_credential with the given challenge
     #
     def verify_webauthn_credential(webauthn_credential, stored_credential, challenge, encoder)
+      # We need to adjust the relaying party id (RP id) we verify against if the registration in question
+      # is a migrated U2F registration. This is beacuse the appid of U2F and the rp id of WebAuthn differ.
+      rp_id = webauthn_credential.client_extension_outputs['appid'] ? WebAuthn.configuration.origin : URI(WebAuthn.configuration.origin).host
       webauthn_credential.response.verify(
         encoder.decode(challenge),
           public_key: encoder.decode(stored_credential.public_key),
-          sign_count: stored_credential.counter)
+          sign_count: stored_credential.counter,
+          rp_id: rp_id)
     end
   end
 end

changelogs/unreleased/232670-webauthn-background-migration.yml

+---
+title: Migrate u2f registrations to webauthn registrations
+merge_request: 42159
+author: Jan Beckmann
+type: added

config/initializers/webauthn.rb

+# frozen_string_literal: true
+
 WebAuthn.configure do |config|
   # This value needs to match `window.location.origin` evaluated by
   # the User Agent during registration and authentication ceremonies.

db/migrate/20200925125321_add_u2f_id_to_webauthn_registration.rb

+# frozen_string_literal: true
+
+class AddU2fIdToWebauthnRegistration < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :webauthn_registrations, :u2f_registration_id, :integer
+  end
+end

db/migrate/20200928125258_add_foreign_key_to_u2f_reg_id_in_webauthn_regs.rb

+# frozen_string_literal: true
+
+class AddForeignKeyToU2fRegIdInWebauthnRegs < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+  INDEX_NAME = 'index_webauthn_registrations_on_u2f_registration_id'
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :webauthn_registrations, :u2f_registration_id, where: 'u2f_registration_id IS NOT NULL', name: INDEX_NAME
+    add_concurrent_foreign_key :webauthn_registrations, :u2f_registrations, column: :u2f_registration_id, on_delete: :cascade
+  end
+
+  def down
+    remove_foreign_key_if_exists :webauthn_registrations, column: :u2f_registration_id
+    remove_concurrent_index_by_name(:webauthn_registrations, INDEX_NAME)
+  end
+end

db/post_migrate/20200929114107_schedule_migrate_u2f_webauthn.rb

+# frozen_string_literal: true
+
+class ScheduleMigrateU2fWebauthn < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  INTERVAL = 2.minutes.to_i
+  DOWNTIME = false
+  MIGRATION = 'MigrateU2fWebauthn'
+  BATCH_SIZE = 1_000
+
+  disable_ddl_transaction!
+
+  class U2fRegistration < ActiveRecord::Base
+    include EachBatch
+
+    self.table_name = 'u2f_registrations'
+  end
+
+  def up
+    say "Scheduling #{MIGRATION} background migration jobs"
+
+    queue_background_migration_jobs_by_range_at_intervals(U2fRegistration, MIGRATION, INTERVAL, batch_size: BATCH_SIZE)
+  end
+
+  def down
+    # no-op
+    # There is no real way back here, because
+    # a) The U2fMigrator of webauthn_ruby gem only works in one way
+    # b) This migration only pushes jobs to Sidekiq
+  end
+end

db/schema_migrations/20200925125321

+d1d2f7d5f70e912b1d0a77417a96b9e16ffc620eb8c941ed4aa9a8c166ba26e0
\ No newline at end of file

db/schema_migrations/20200928125258

+46579fd0313068f3c9c1631f1da4a0b20513759a54dad4841bcea7d6c727646a
\ No newline at end of file

db/schema_migrations/20200929114107

+f5d4b534c230f9ac7f285bccd096a7d51bf5c9e7a73f293fafaff89bb1ee12e1
\ No newline at end of file

db/structure.sql

@@ -17168,6 +17168,7 @@ CREATE TABLE webauthn_registrations (
     credential_xid text NOT NULL,
     name text NOT NULL,
     public_key text NOT NULL,
+    u2f_registration_id integer,
     CONSTRAINT check_242f0cc65c CHECK ((char_length(credential_xid) <= 255)),
     CONSTRAINT check_2f02e74321 CHECK ((char_length(name) <= 255))
 );
@@ -21841,6 +21842,8 @@ CREATE INDEX index_web_hooks_on_type ON web_hooks USING btree (type);
 
 CREATE UNIQUE INDEX index_webauthn_registrations_on_credential_xid ON webauthn_registrations USING btree (credential_xid);
 
+CREATE INDEX index_webauthn_registrations_on_u2f_registration_id ON webauthn_registrations USING btree (u2f_registration_id) WHERE (u2f_registration_id IS NOT NULL);
+
 CREATE INDEX index_webauthn_registrations_on_user_id ON webauthn_registrations USING btree (user_id);
 
 CREATE INDEX index_wiki_page_meta_on_project_id ON wiki_page_meta USING btree (project_id);
@@ -22242,6 +22245,9 @@ ALTER TABLE ONLY vulnerabilities
 ALTER TABLE ONLY vulnerabilities
     ADD CONSTRAINT fk_131d289c65 FOREIGN KEY (milestone_id) REFERENCES milestones(id) ON DELETE SET NULL;
 
+ALTER TABLE ONLY webauthn_registrations
+    ADD CONSTRAINT fk_13e04d719a FOREIGN KEY (u2f_registration_id) REFERENCES u2f_registrations(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY protected_branch_push_access_levels
     ADD CONSTRAINT fk_15d2a7a4ae FOREIGN KEY (deploy_key_id) REFERENCES keys(id) ON DELETE CASCADE;
 

lib/gitlab/background_migration/migrate_u2f_webauthn.rb

+# frozen_string_literal: true
+# rubocop:disable Style/Documentation
+require "webauthn/u2f_migrator"
+
+module Gitlab
+  module BackgroundMigration
+    class MigrateU2fWebauthn
+      class U2fRegistration < ActiveRecord::Base
+        self.table_name = 'u2f_registrations'
+      end
+
+      class WebauthnRegistration < ActiveRecord::Base
+        self.table_name = 'webauthn_registrations'
+      end
+
+      def perform(start_id, end_id)
+        old_registrations = U2fRegistration.where(id: start_id..end_id)
+        old_registrations.each_slice(100) do |slice|
+          now = Time.now
+          values = slice.map do |u2f_registration|
+            converted_credential = WebAuthn::U2fMigrator.new(
+              app_id: Gitlab.config.gitlab.url,
+              certificate: u2f_registration.certificate,
+              key_handle: u2f_registration.key_handle,
+              public_key: u2f_registration.public_key,
+              counter: u2f_registration.counter
+            ).credential
+
+            {
+                credential_xid: Base64.strict_encode64(converted_credential.id),
+                public_key: Base64.strict_encode64(converted_credential.public_key),
+                counter: u2f_registration.counter || 0,
+                name: u2f_registration.name || '',
+                user_id: u2f_registration.user_id,
+                u2f_registration_id: u2f_registration.id,
+                created_at: now,
+                updated_at: now
+            }
+          end
+
+          WebauthnRegistration.insert_all(values, unique_by: :credential_xid, returning: false)
+        end
+      end
+    end
+  end
+end

spec/lib/gitlab/background_migration/migrate_u2f_webauthn_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::BackgroundMigration::MigrateU2fWebauthn, :migration, schema: 20200925125321 do
+  let(:users) { table(:users) }
+
+  let(:user) { users.create!(email: 'email@email.com', name: 'foo', username: 'foo', projects_limit: 0) }
+
+  let(:u2f_registrations) { table(:u2f_registrations) }
+  let(:webauthn_registrations) { table(:webauthn_registrations) }
+
+  let!(:u2f_registration_not_migrated) { create_u2f_registration(1, 'reg1') }
+  let!(:u2f_registration_not_migrated_no_name) { create_u2f_registration(2, nil, 2) }
+  let!(:u2f_registration_migrated) { create_u2f_registration(3, 'reg3') }
+
+  subject { described_class.new.perform(1, 3) }
+
+  before do
+    converted_credential = convert_credential_for(u2f_registration_migrated)
+    webauthn_registrations.create!(converted_credential)
+  end
+
+  it 'migrates all records' do
+    expect { subject }.to change { webauthn_registrations.count }.from(1).to(3)
+
+    all_webauthn_registrations = webauthn_registrations.all.map(&:attributes)
+
+    [u2f_registration_not_migrated, u2f_registration_not_migrated_no_name].each do |u2f_registration|
+      expected_credential = convert_credential_for(u2f_registration).except(:created_at).stringify_keys
+      expect(all_webauthn_registrations).to include(a_hash_including(expected_credential))
+    end
+  end
+
+  def create_u2f_registration(id, name, counter = 5)
+    device = U2F::FakeU2F.new(FFaker::BaconIpsum.characters(5))
+    u2f_registrations.create!({ id: id,
+                                certificate: Base64.strict_encode64(device.cert_raw),
+                                key_handle: U2F.urlsafe_encode64(device.key_handle_raw),
+                                public_key: Base64.strict_encode64(device.origin_public_key_raw),
+                                counter: counter,
+                                name: name,
+                                user_id: user.id })
+  end
+
+  def convert_credential_for(u2f_registration)
+    converted_credential = WebAuthn::U2fMigrator.new(
+      app_id: Gitlab.config.gitlab.url,
+      certificate: u2f_registration.certificate,
+      key_handle: u2f_registration.key_handle,
+      public_key: u2f_registration.public_key,
+      counter: u2f_registration.counter
+    ).credential
+
+    {
+        credential_xid: Base64.strict_encode64(converted_credential.id),
+        public_key: Base64.strict_encode64(converted_credential.public_key),
+        counter: u2f_registration.counter,
+        name: u2f_registration.name || '',
+        user_id: u2f_registration.user_id,
+        u2f_registration_id: u2f_registration.id,
+        created_at: u2f_registration.created_at
+    }
+  end
+end

spec/migrations/schedule_migrate_u2f_webauthn_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require Rails.root.join('db', 'post_migrate', '20200929114107_schedule_migrate_u2f_webauthn.rb')
+
+RSpec.describe ScheduleMigrateU2fWebauthn do
+  let(:migration_name) { described_class::MIGRATION }
+  let(:u2f_registrations) { table(:u2f_registrations) }
+  let(:webauthn_registrations) { table(:webauthn_registrations) }
+
+  let(:users) { table(:users) }
+
+  let(:user) { users.create!(email: 'email@email.com', name: 'foo', username: 'foo', projects_limit: 0) }
+
+  before do
+    stub_const("#{described_class.name}::BATCH_SIZE", 1)
+  end
+
+  context 'when there are u2f registrations' do
+    let!(:u2f_reg_1) { create_u2f_registration(1, 'reg1') }
+    let!(:u2f_reg_2) { create_u2f_registration(2, 'reg2') }
+
+    it 'schedules a background migration' do
+      Sidekiq::Testing.fake! do
+        freeze_time do
+          migrate!
+
+          expect(migration_name).to be_scheduled_delayed_migration(2.minutes, 1, 1)
+          expect(migration_name).to be_scheduled_delayed_migration(4.minutes, 2, 2)
+          expect(BackgroundMigrationWorker.jobs.size).to eq(2)
+        end
+      end
+    end
+  end
+
+  context 'when there are no u2f registrations' do
+    it 'does not schedule background migrations' do
+      Sidekiq::Testing.fake! do
+        freeze_time do
+          migrate!
+
+          expect(BackgroundMigrationWorker.jobs.size).to eq(0)
+        end
+      end
+    end
+  end
+
+  def create_u2f_registration(id, name)
+    device = U2F::FakeU2F.new(FFaker::BaconIpsum.characters(5))
+    u2f_registrations.create!({ id: id,
+                                certificate: Base64.strict_encode64(device.cert_raw),
+                                key_handle: U2F.urlsafe_encode64(device.key_handle_raw),
+                                public_key: Base64.strict_encode64(device.origin_public_key_raw),
+                                counter: 5,
+                                name: name,
+                                user_id: user.id })
+  end
+end