From 3d06600cf91a459c4b8c13043401f612b17651ca Mon Sep 17 00:00:00 2001 From: Plehanov Dmitriy Date: Thu, 21 Sep 2017 14:23:42 +0300 Subject: [PATCH 1/3] ROTP v3 --- devise-two-factor.gemspec | 2 +- .../two_factor_authenticatable_shared_examples.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/devise-two-factor.gemspec b/devise-two-factor.gemspec index a951b00..6b9943b 100644 --- a/devise-two-factor.gemspec +++ b/devise-two-factor.gemspec @@ -28,7 +28,7 @@ Gem::Specification.new do |s| s.add_runtime_dependency 'activesupport' s.add_runtime_dependency 'attr_encrypted', '>= 1.3', '< 4', '!= 2' s.add_runtime_dependency 'devise', '~> 4.0' - s.add_runtime_dependency 'rotp', '~> 2.0' + s.add_runtime_dependency 'rotp', '~> 3.0' s.add_development_dependency 'activemodel' s.add_development_dependency 'bundler', '> 1.0' diff --git a/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb b/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb index 43a907b..bf11660 100644 --- a/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb +++ b/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb @@ -102,8 +102,8 @@ end it 'should return uri with issuer option' do - expect(subject.otp_provisioning_uri(account, issuer: issuer)).to match(%r{otpauth://totp/#{account}\?.*secret=\w{#{otp_secret_length}}(&|$)}) - expect(subject.otp_provisioning_uri(account, issuer: issuer)).to match(%r{otpauth://totp/#{account}\?.*issuer=#{issuer}(&|$)}) + expect(subject.otp_provisioning_uri(account, issuer: issuer)).to match(%r{otpauth://totp/#{issuer}:#{account}\?.*secret=\w{#{otp_secret_length}}(&|$)}) + expect(subject.otp_provisioning_uri(account, issuer: issuer)).to match(%r{otpauth://totp/#{issuer}:#{account}\?.*issuer=#{issuer}(&|$)}) end end end From c0da5d02969d2122f6522f660def1f2b587a136e Mon Sep 17 00:00:00 2001 From: Plehanov Dmitriy Date: Thu, 21 Sep 2017 15:03:59 +0300 Subject: [PATCH 2/3] Preventing reuse of time based otps --- .../20170516191259_add_lastotpat_to_users.rb | 5 +++++ devise-two-factor.gemspec | 1 + .../models/two_factor_authenticatable.rb | 8 +++++-- ..._factor_authenticatable_shared_examples.rb | 21 ++++++++++++++++++- .../devise_two_factor_generator.rb | 1 + .../models/two_factor_authenticatable_spec.rb | 2 ++ 6 files changed, 35 insertions(+), 3 deletions(-) create mode 100644 demo/db/migrate/20170516191259_add_lastotpat_to_users.rb diff --git a/demo/db/migrate/20170516191259_add_lastotpat_to_users.rb b/demo/db/migrate/20170516191259_add_lastotpat_to_users.rb new file mode 100644 index 0000000..c373c8c --- /dev/null +++ b/demo/db/migrate/20170516191259_add_lastotpat_to_users.rb @@ -0,0 +1,5 @@ +class AddLastotpatToUsers < ActiveRecord::Migration + def change + add_column :users, :last_otp_at, :datetime + end +end diff --git a/devise-two-factor.gemspec b/devise-two-factor.gemspec index 6b9943b..f3562a7 100644 --- a/devise-two-factor.gemspec +++ b/devise-two-factor.gemspec @@ -36,4 +36,5 @@ Gem::Specification.new do |s| s.add_development_dependency 'simplecov' s.add_development_dependency 'faker' s.add_development_dependency 'timecop' + s.add_development_dependency 'byebug' end diff --git a/lib/devise_two_factor/models/two_factor_authenticatable.rb b/lib/devise_two_factor/models/two_factor_authenticatable.rb index 7c16024..a74f8ee 100644 --- a/lib/devise_two_factor/models/two_factor_authenticatable.rb +++ b/lib/devise_two_factor/models/two_factor_authenticatable.rb @@ -22,7 +22,7 @@ module TwoFactorAuthenticatable end def self.required_fields(klass) - [:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt, :consumed_timestep] + [:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt, :consumed_timestep, :last_otp_at] end # This defaults to the model's otp_secret @@ -32,7 +32,11 @@ def validate_and_consume_otp!(code, options = {}) return false unless code.present? && otp_secret.present? totp = self.otp(otp_secret) - return consume_otp! if totp.verify_with_drift(code, self.class.otp_allowed_drift) + verified_at_timestamp = totp.verify_with_drift_and_prior(code, self.class.otp_allowed_drift, self.last_otp_at) + if verified_at_timestamp + self.last_otp_at = verified_at_timestamp + return consume_otp! + end false end diff --git a/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb b/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb index bf11660..0177521 100644 --- a/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb +++ b/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb @@ -6,7 +6,7 @@ describe 'required_fields' do it 'should have the attr_encrypted fields for otp_secret' do - expect(Devise::Models::TwoFactorAuthenticatable.required_fields(subject.class)).to contain_exactly(:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt, :consumed_timestep) + expect(Devise::Models::TwoFactorAuthenticatable.required_fields(subject.class)).to contain_exactly(:encrypted_otp_secret, :encrypted_otp_secret_iv, :encrypted_otp_secret_salt, :consumed_timestep, :last_otp_at) end end @@ -106,4 +106,23 @@ expect(subject.otp_provisioning_uri(account, issuer: issuer)).to match(%r{otpauth://totp/#{issuer}:#{account}\?.*issuer=#{issuer}(&|$)}) end end + + context 'given a valid OTP used multiple times within the allowed drift' do + let(:consumed_otp) { ROTP::TOTP.new(subject.otp_secret).at(Time.now) } + + before do + subject.validate_and_consume_otp!(consumed_otp) + end + + context 'after the otp interval' do + before do + Timecop.travel(subject.otp.interval.seconds.from_now) + end + + # This currently fails! + it 'fails to validate' do + expect(subject.validate_and_consume_otp!(consumed_otp)).to be false + end + end + end end diff --git a/lib/generators/devise_two_factor/devise_two_factor_generator.rb b/lib/generators/devise_two_factor/devise_two_factor_generator.rb index d4898fc..655d2b7 100644 --- a/lib/generators/devise_two_factor/devise_two_factor_generator.rb +++ b/lib/generators/devise_two_factor/devise_two_factor_generator.rb @@ -23,6 +23,7 @@ def create_devise_two_factor_migration "encrypted_otp_secret_iv:string", "encrypted_otp_secret_salt:string", "consumed_timestep:integer", + "last_otp_at:datetime", "otp_required_for_login:boolean" ] diff --git a/spec/devise/models/two_factor_authenticatable_spec.rb b/spec/devise/models/two_factor_authenticatable_spec.rb index e89dad5..119e1ef 100644 --- a/spec/devise/models/two_factor_authenticatable_spec.rb +++ b/spec/devise/models/two_factor_authenticatable_spec.rb @@ -11,6 +11,7 @@ class TwoFactorAuthenticatableDouble devise :two_factor_authenticatable, :otp_secret_encryption_key => 'test-key'*4 attr_accessor :consumed_timestep + attr_accessor :last_otp_at def save(validate) # noop for testing @@ -36,6 +37,7 @@ class TwoFactorAuthenticatableWithCustomizeAttrEncryptedDouble devise :two_factor_authenticatable, :otp_secret_encryption_key => 'test-key'*4 attr_accessor :consumed_timestep + attr_accessor :last_otp_at def save(validate) # noop for testing From 3512bb5cbe05dda90989447b35728d9208ebaae3 Mon Sep 17 00:00:00 2001 From: Saranga Komanduri Date: Mon, 9 Oct 2017 08:48:53 -0500 Subject: [PATCH 3/3] Fix bug in last_otp_at definition The rotp gem sets this value to an integer, but setting a datetime field to an integer causes it to be silently set to null. Make this value an integer instead. --- demo/db/migrate/20170516191259_add_lastotpat_to_users.rb | 2 +- demo/db/schema.rb | 1 + .../spec_helpers/two_factor_authenticatable_shared_examples.rb | 2 +- lib/generators/devise_two_factor/devise_two_factor_generator.rb | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/demo/db/migrate/20170516191259_add_lastotpat_to_users.rb b/demo/db/migrate/20170516191259_add_lastotpat_to_users.rb index c373c8c..24a0042 100644 --- a/demo/db/migrate/20170516191259_add_lastotpat_to_users.rb +++ b/demo/db/migrate/20170516191259_add_lastotpat_to_users.rb @@ -1,5 +1,5 @@ class AddLastotpatToUsers < ActiveRecord::Migration def change - add_column :users, :last_otp_at, :datetime + add_column :users, :last_otp_at, :integer end end diff --git a/demo/db/schema.rb b/demo/db/schema.rb index b08c69a..da1fc6c 100644 --- a/demo/db/schema.rb +++ b/demo/db/schema.rb @@ -34,6 +34,7 @@ t.string "encrypted_otp_secret_salt" t.integer "consumed_timestep" t.boolean "otp_required_for_login" + t.integer "last_otp_at" end add_index "users", ["email"], name: "index_users_on_email", unique: true, using: :btree diff --git a/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb b/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb index 0177521..9065948 100644 --- a/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb +++ b/lib/devise_two_factor/spec_helpers/two_factor_authenticatable_shared_examples.rb @@ -119,7 +119,7 @@ Timecop.travel(subject.otp.interval.seconds.from_now) end - # This currently fails! + # This spec tests that reuse of the OTP is not allowed it 'fails to validate' do expect(subject.validate_and_consume_otp!(consumed_otp)).to be false end diff --git a/lib/generators/devise_two_factor/devise_two_factor_generator.rb b/lib/generators/devise_two_factor/devise_two_factor_generator.rb index 655d2b7..e963f73 100644 --- a/lib/generators/devise_two_factor/devise_two_factor_generator.rb +++ b/lib/generators/devise_two_factor/devise_two_factor_generator.rb @@ -23,7 +23,7 @@ def create_devise_two_factor_migration "encrypted_otp_secret_iv:string", "encrypted_otp_secret_salt:string", "consumed_timestep:integer", - "last_otp_at:datetime", + "last_otp_at:integer", "otp_required_for_login:boolean" ]