Skip to content

backup codes #10434

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

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from
Draft
7 changes: 7 additions & 0 deletions app/controllers/logins_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,12 @@ def complete
else
return redirect_to totp_login_path(@login), flash: { error: "Invalid TOTP code, please try again." }
end
when "backup_code"
if @user.redeem_backup_code(params[:backup_code])
@login.update(authenticated_with_backup_code: true)
else
return redirect_to backup_code_login_path(@login), flash: { error: "Invalid backup code, please try again." }
end
end

# Clear the flash - this prevents the error message showing up after an unsuccessful -> successful login
Expand Down Expand Up @@ -204,6 +210,7 @@ def set_available_methods
@sms_available = @user&.phone_number_verified && [email protected]_with_sms
@webauthn_available = @user&.webauthn_credentials&.any? && [email protected]_with_webauthn
@totp_available = @user&.totp.present? && [email protected]_with_totp
@backup_code_available = @user&.unused_backup_codes&.any? && [email protected]_with_backup_code
end

def set_return_to
Expand Down
43 changes: 43 additions & 0 deletions app/mailers/user/backup_code_mailer.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
# frozen_string_literal: true

class User
class BackupCodeMailer < ApplicationMailer
before_action :set_user

default to: -> {
emails = @user.email_address_with_name
}
Comment on lines +7 to +9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
default to: -> {
emails = @user.email_address_with_name
}
default to: -> { @user.email_address_with_name }


def new_codes_generated
mail subject: "You've generated new backup codes for HCB"
end

def code_used
mail subject: "You've used a backup code to login to HCB"
end

def backup_codes_enabled
mail subject: "HCB backup codes are enabled"
end

def backup_codes_disabled
mail subject: "HCB backup codes are disabled"
end

def two_or_fewer_codes_left
mail subject: "[Action Requested] You've almost used all your backup codes for HCB"
end

def no_codes_remaining
mail subject: "[Action Required] You've used all your backup codes for HCB"
end

private

def set_user
@user = User.find(params[:user_id])
end

end

end
4 changes: 2 additions & 2 deletions app/models/login.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class Login < ApplicationRecord
self.ignored_columns += ["browser_token"]
before_validation :ensure_browser_token

store_accessor :authentication_factors, :sms, :email, :webauthn, :totp, prefix: :authenticated_with
store_accessor :authentication_factors, :sms, :email, :webauthn, :totp, :backup_code, prefix: :authenticated_with

EXPIRATION = 15.minutes

Expand Down Expand Up @@ -59,7 +59,7 @@ class Login < ApplicationRecord
event :mark_complete do
transitions from: :incomplete, to: :complete do
guard do
authentication_factors_count == (user.use_two_factor_authentication? ? 2 : 1)
authentication_factors_count >= (user.use_two_factor_authentication? && !authenticated_with_backup_code ? 2 : 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
authentication_factors_count >= (user.use_two_factor_authentication? && !authenticated_with_backup_code ? 2 : 1)
authentication_factors_count == (user.use_two_factor_authentication? ? 2 : 1) || authenticated_with_backup_code

end
end
end
Expand Down
57 changes: 57 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ class User < ApplicationRecord

has_many :logins
has_many :login_codes
has_many :backup_codes, class_name: "User::BackupCode", inverse_of: :user, dependent: :destroy
has_many :unused_backup_codes, -> { where(aasm_state: :unused) }, class_name: "User::BackupCode", inverse_of: :user
has_many :user_sessions, dependent: :destroy
has_many :organizer_position_invites, dependent: :destroy
has_many :organizer_position_contracts, through: :organizer_position_invites, class_name: "OrganizerPosition::Contract"
Expand Down Expand Up @@ -370,6 +372,61 @@ def only_card_grant_user?
card_grants.size >= 1 && events.size == 0
end

def backup_codes_enabled?
unused_backup_codes.any?
end

def generate_backup_codes
# this is different from unused_backup_codes - it includes unsaved codes
backup_codes.where.not(aasm_state: [:used, :invalidated]).find_each &:mark_invalidated!

codes = []
pepper = Credentials.fetch(:BACKUP_CODE_PEPPER)
while codes.size < 10
code = SecureRandom.alphanumeric(10)
next if codes.include?(code)

salt = SecureRandom.random_bytes(64)
begin
# backup code pepper must be at least 32 bytes
backup_codes.create!(hash: OpenSSL::KDF.pbkdf2_hmac(code + pepper, hash: "sha512", salt:, iterations: 20_000, length: 64).unpack1("H*"), salt: Base64.strict_encode64(salt))
rescue ActiveRecord::RecordInvalid
# if the code is already in use, skip it
next
end
codes << code
end

codes
end

def redeem_backup_code(code)
found = nil
pepper = Credentials.fetch(:BACKUP_CODE_PEPPER)
# make sure we do not short circuit
unused_backup_codes.each do |backup_code|
hash = OpenSSL::KDF.pbkdf2_hmac(code + pepper, hash: "sha512", salt: Base64.decode64(backup_code.salt), iterations: 20_000, length: 64).unpack1("H*")
Comment on lines +392 to +408
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a class method on BackupCode for calculating a hash.

if ActiveSupport::SecurityUtils.secure_compare(hash, backup_code.hash)
found = backup_code
end
end
Comment on lines +404 to +412
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone is brute forcing, the loop will already run its entirety. And is someone puts in a valid backup code, that means they've already gained access and we don't need to keep running the loop to prevent leaking timing information.

Let's instantly return when found.


if found.present?
found.transaction do
found.mark_used!
end
Comment on lines +415 to +417
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're only doing one write here, we don't need a transaction

Suggested change
found.transaction do
found.mark_used!
end
found.mark_used!

BackupCodeMailer.with(user_id: id).code_used.deliver_now
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend making this a callback on mark_used


return true
end
false
end

def disable_backup_codes
backup_codes.where.not(aasm_state: [:used, :invalidated]).find_each &:mark_invalidated!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest

def invalidate_backup_codes
  backup_codes.where(aasm_state: :unsaved).destroy_all
  backup_codes.where(aasm_state: :unused).find_each &:mark_invalidated!
end

BackupCodeMailer.with(user_id: id).backup_codes_disabled.deliver_now
end

private

def update_stripe_cardholder
Expand Down
64 changes: 64 additions & 0 deletions app/models/user/backup_code.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
# frozen_string_literal: true

# == Schema Information
#
# Table name: user_backup_codes
#
# id :bigint not null, primary key
# aasm_state :string
# deleted_at :datetime
# hash :text not null
# salt :text not null
# created_at :datetime not null
# updated_at :datetime not null
# user_id :bigint not null
#
# Indexes
#
# index_user_backup_codes_on_user_id (user_id)
#
# Foreign Keys
#
# fk_rails_... (user_id => users.id)
#
class User
class BackupCode < ApplicationRecord
has_paper_trail
acts_as_paranoid

include AASM

belongs_to :user

validates :hash, presence: true, uniqueness: true

aasm do
state :unsaved, initial: true
state :unused
state :used
state :invalidated

event :mark_unused do
transitions from: :unsaved, to: :unused
end
event :mark_used do
transitions from: :unused, to: :used

after do
case user.backup_codes.where(aasm_state: :unused).count
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
case user.backup_codes.where(aasm_state: :unused).count
case user.backup_codes.unused.size

when 0
User::BackupCodeMailer.with(user_id: user.id).no_codes_remaining.deliver_now
when 1..2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
when 1..2
when 1..3

User::BackupCodeMailer.with(user_id: user.id).two_or_fewer_codes_left.deliver_now
end
User::BackupCodeMailer.with(user_id: user.id).code_used.deliver_now
end
end
event :mark_invalidated do
transitions from: [:unused, :unsaved], to: :invalidated
end
end

end

end
34 changes: 34 additions & 0 deletions app/views/logins/backup_code.html.erb
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
<% title "Enter backup code" %>
<% content_for(:page_class) { "bg-snow" } %>

<div class="flex flex-col flex-1 justify-center max-w-md w-full">
<%= render "header", label: "Sign in to HCB" do %>
Backup code
<% end %>
<%= render "badge", user: @login.user %>
<p>
Please enter one of the backup codes you generated previously.
</p>
<%= form_tag complete_login_path(@login) do %>
<%= text_field :backup_code, "", placeholder: "Enter your backup code", name: "backup_code", class: "!max-w-full w-max", required: true, autofocus: true %>
<%= hidden_field_tag :method, :backup_code %>
<%= hidden_field_tag :fingerprint %>
<%= hidden_field_tag :device_info %>
<%= hidden_field_tag :os_info %>
<%= hidden_field_tag :timezone %>
<%= hidden_field_tag :return_to, @return_to if @return_to %>
<% end %>
<div class="flex flex-row justify-between items-center mt-4 gap-2">
<% if @webauthn_available || @totp_available || @email_available || @sms_available %>
<%= link_to "Sign in another way", choose_login_preference_login_path(@login, return_to: @return_to), class: "block mt-0 no-underline" %>
<% end %>
<button data-webauthn-auth-target="continueButton" type="submit" class="gap-2 btn">
Continue
</button>
</div>
<% end %>
<%= javascript_include_tag "https://cdn.jsdelivr.net/npm/ua-parser-js/dist/ua-parser.min.js" %>
<%= javascript_include_tag "fingerprint.js" %>
</div>
<%= render partial: "environment_banner" %>
<%= render partial: "footer" %>
6 changes: 5 additions & 1 deletion app/views/logins/choose_login_preference.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
@email_available.presence,
@sms_available.presence,
@totp_available.presence,
@webauthn_available.presence
@webauthn_available.presence,
@backup_code_available.presence
].compact %>

<div class="field field--options max-w-full w-full mt-3 <%= "trio" if methods.length == 3 %>">
Expand Down Expand Up @@ -61,6 +62,9 @@
<%= link_to "Cancel", logout_users_path, method: :delete, class: "no-underline block" %>
<%= form.submit "Continue", data: { "webauthn-auth-target" => "continueButton", "form-disable-target" => "submitButton" } %>
</div>
<% if @backup_code_available %>
<%= link_to "Don't have access to any of these? Use a Backup Code", backup_code_login_path(@login) %>
<% end %>
<% end %>
</div>
<%= render partial: "environment_banner" %>
Expand Down
13 changes: 13 additions & 0 deletions db/migrate/20250513051747_create_user_backup_codes.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
class CreateUserBackupCodes < ActiveRecord::Migration[7.2]
def change
create_table :user_backup_codes do |t|
t.references :user, null: false, foreign_key: true
t.string :aasm_state
t.text :hash, null: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a unique index

t.text :salt, null: false
t.datetime :deleted_at
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop acts as parnoid and this column


t.timestamps
end
end
end
11 changes: 11 additions & 0 deletions db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2054,6 +2054,17 @@
t.datetime "updated_at", null: false
end

create_table "user_backup_codes", force: :cascade do |t|
t.bigint "user_id", null: false
t.string "aasm_state"
t.text "hash", null: false
t.text "salt", null: false
t.datetime "deleted_at"
t.datetime "created_at", null: false
t.datetime "updated_at", null: false
t.index ["user_id"], name: "index_user_backup_codes_on_user_id"
end

create_table "user_email_updates", force: :cascade do |t|
t.bigint "user_id", null: false
t.string "aasm_state", null: false
Expand Down
Loading