Skip to content
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

2FA should be re-enabled. #507

Closed
andrewculver opened this issue Nov 10, 2022 · 11 comments · Fixed by #519
Closed

2FA should be re-enabled. #507

andrewculver opened this issue Nov 10, 2022 · 11 comments · Fixed by #519
Assignees

Comments

@andrewculver
Copy link
Contributor

andrewculver commented Nov 10, 2022

We had this enabled previously, but we disabled it when upgrading to Rails 7. You can see where we were including it at https://github.com/bullet-train-co/bullet_train-base/blob/main/bullet_train.gemspec#L46 . Would love to see this working again.

@andrewculver andrewculver changed the title 2FA should be enabled. 2FA should be re-enabled. Nov 10, 2022
@gazayas
Copy link
Contributor

gazayas commented Nov 11, 2022

I added those two gems to the Gemfile in the starter repository which got rid of the uninitialized constant error for Devise::Models::TwoFactorAuthenticatable.

I had to edit the credentials for Rails 7 to actually show the QR code, but once I added it to my authenticator app, the login page wasn't letting me sign in with the 2FA code, so I feel close to solving it but not quite there yet. I'd like to see 2FA enabled again too.

@bhumi1102
Copy link
Contributor

bhumi1102 commented Nov 11, 2022

Hi @gazayas 👋 I'm Bhumi, I'm new around here, to Bullet Train development. Nice to meet you!

So I started taking a look at this as well...and starting at the beginning by reading devise-two-factor, as I have not used this gem before.

When you say 'the login page wasn't letting me sign in with the 2FA code' curious what the error is. I working on how to reproduce this issue locally atm.

@gazayas
Copy link
Contributor

gazayas commented Nov 11, 2022

Hello @bhumi1102, welcome! 🎉 Nice to meet you too.

Setup

Basically, I did this.

  1. Added the two gems to the starter repository's gemfile
  2. Added credentials as the docs say on the devise-two-factor page (this part appears to be new with Rails 7)
# generate suitable encryption secrets to stdout
$ ./bin/rails db:encryption:init

# Add the output from the command above to your encrypted credentials file via
# Setting the EDITOR environment variable is optional, without it, your default editor will open
$ EDITOR=code ./bin/rails credentials:edit
  1. Enable 2FA via the "Account Details" page and scanned the QR code with my google authenticator app
  2. Logged out, and tried to sign in with 2FA

Error

As you can see here, I'm just getting a 401 when trying to sign in.

07:14:09 web.1              | Processing by SessionsController#create as TURBO_STREAM
07:14:09 web.1              |   Parameters: {"authenticity_token"=>"[FILTERED]", "user"=>{"email"=>"g-zayas@hotmail.com", "password"=>"[FILTERED]", "otp_attempt"=>"[FILTERED]", "remember_me"=>"0"}, "commit"=>"Next"}
07:14:09 web.1              |   User Load (1.6ms)  SELECT "users".* FROM "users" WHERE "users"."email" = $1 ORDER BY "users"."id" ASC LIMIT $2  [["email", "g-zayas@hotmail.com"], ["LIMIT", 1]]
07:14:10 web.1              |   CACHE User Load (0.1ms)  SELECT "users".* FROM "users" WHERE "users"."email" = $1 ORDER BY "users"."id" ASC LIMIT $2  [["email", "g-zayas@hotmail.com"], ["LIMIT", 1]]
07:14:10 web.1              | Completed 401 Unauthorized in 756ms (ActiveRecord: 16.0ms | Allocations: 26937)

Potentially an issue with TWO_FACTOR_ENCRYPTION_KEY?

Since the credentials setup is new, maybe we have to configure this environment variable differently to work?

Glad to have you working on this, thanks for helping out!

@bhumi1102
Copy link
Contributor

@gazayas yeah what you say makes sense about the devise-two-factor readme involving db:encryption stuff. I was wondering about those instructions as their demo app is Rails 4 and since Active Record encryption was new in Rails 7 they must have updated that part.

Regarding TWO_FACTOR_ENCRYPTION_KEY yeah I ran into that when I was trying to figure out how to even get the 'enable' button to show up in account settings. I set the value to something random and I got the setting to show up. But not clear what the value is suppose to be for that...I gather that 2FA was working before the Rails 7 upgrade? If so how was the value of TWO_FACTOR_ENCRYPTION_KEY set in the past if you recall?

Thanks for the message! I'm going to continue on this tomorrow/Monday.

@gazayas
Copy link
Contributor

gazayas commented Nov 14, 2022

@bhumi1102

I gather that 2FA was working before the Rails 7 upgrade?

Yes, and as far as I know TWO_FACTOR_ENCRYPTION_KEY doesn't have to be a specific value, as long as its something that can't be easily guessed. For example, you could use bundle exec rake secret to generate a value.

Here's the original PR:

I would take a look through this issue, it might have some leads for fixing our issue: devise-two-factor/devise-two-factor#192

@bhumi1102
Copy link
Contributor

https://github.com/bullet-train-co/bullet-train-tailwind-css/pull/125

That 404 for me. Changing it to this also doesn't find anything #125

@bhumi1102
Copy link
Contributor

bhumi1102 commented Nov 14, 2022

I've investigated this and noting my findings here of what I think needs to be done:
Note: this is a bit more involved since latest version of the devise-two-factor gem introduced breaking changes in how encryption is done.

user model

  • Add a new column otp_secret. This one uses the new rails7 Active Record Encryption. The 3 existing columns we have encrypted_otp_secret, encrypted_otp_secret_iv, and encrypted_otp_secret_salt are replaced by this single new column (this implies that data in this column would need to be migrated for existing applications and later removed I think).
  • Add a method legacy_otp_secret to Users::Base. This is a method needed by the devise-two-factor gem to decode OTP secret from the legacy database columns during migration (sample implementation here may or may not work for us).

keys

  • Setup Rails 7 encryption secrets
./bin/rails db:encryption:init
# capture the output and put in encrypted credentials via
./bin/rails credentials:edit
  • Change devise :two_factor_authenticatable, :two_factor_backupable, otp_secret_encryption_key: ENV["TWO_FACTOR_ENCRYPTION_KEY"] in Users::Base to devise :two_factor_authenticatable, :two_factor_backupable, :otp_secret_encryption_key => Rails.application.credentials.active_record_encryption.primary_key
  • ?? Add encryption key to devise.rb config config.otp_encrypted_attribute_options = { key: 'some_secret_value' }

TWO_FACTOR_ENCRYPTION_KEY

It seems that we no longer need this key, and 2FA can use the rails 7 active record encryption keys generated. At least from this comment and this comment.
Not clear though, since this comment seems to be referring to "some_secret_key" again.

@gazayas
Copy link
Contributor

gazayas commented Nov 15, 2022

@bhumi1102 Awesome report, I agree we need to do the following things that you mentioned:

  1. Add the otp_secret column to the User model via a migration.
  2. Add the private method legacy_otp_secret to the User model.
  3. Set up Rails 7 encryption keys.

I think that would be enough to open a pull request!

As far as the encryption key itself, if we're exclusively enabling 2FA for Rails 7 I think we can safely replace ENV["TWO_FACTOR_ENCRYPTION_KEY"] with Rails.application.credentials.active_record_encryption.primary_key and just fall back on the legacy_otp_secret method for users that enabled 2FA with the older three columns (encrypted_otp_secret_iv, etc.). I would usually open a PR at this point and then ask Andrew what direction he wants to go in if I'm unsure, but I'll leave that to your judgment!

@andrewculver
Copy link
Contributor Author

andrewculver commented Nov 15, 2022

Hey, just a heads up that we need to maintain Rails 6 compatibility for this at the moment. 😬 Not saying we can't use Active Record encryption, just need to also support not having it as well. 😐

@bhumi1102
Copy link
Contributor

bhumi1102 commented Nov 16, 2022

👍 will do. For the latest version of the 2FA gem we do have to use AR encryption as they have deprecated attr_encrypted gem used previously. But we'll can do a migration strategy that allows existing data and Rails 6 to keep working.

5.0.0
Breaking Changes

  • attr_encrypted has been deprecated in favor of native Rails attribute encryption. See UPGRADING.md for details on how to migrate your records. You must use or build a migration strategy (see examples in UPGRADING.md) to use existing data!
  • Rails 7 is now required.

@bhumi1102
Copy link
Contributor

Back from vacation. Got 2FA working with devise_two_factor, Submitted PR #519

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants