Lock users out temporarily after too many OTPs by zachmargolis · Pull Request #1464 · 18F/identity-idp

zachmargolis · 2017-05-31T19:17:18Z

Why:
Use the database rather than rack-attack

monfresh · 2017-05-31T19:56:16Z

app/controllers/users/two_factor_authentication_controller.rb

Call this method inside handle_valid_otp_delivery_preference since it only applies to OTP, and not TOTP? A global before action means this logic will get exercised when it doesn't need to. It only needs to be exercised when handle_valid_otp_delivery_preference is called.

That's a great point! Thanks

monfresh · 2017-05-31T20:03:29Z

app/controllers/users/two_factor_authentication_controller.rb

I'm not sure we want to treat this as a MULTI_FACTOR_AUTH_MAX_ATTEMPTS event. I think we need a separate event. Also, we were discussing not locking the user out, and displaying a message such as "Didn't receive your OTP? Please wait a minute before requesting a new one."

Yeah you're right, it woudl be confusing with the same lockout event, I'll make another.

I don't recall the discussion about not locking the user out, but happy to change that behavior if we want

monfresh · 2017-05-31T20:30:39Z

Thanks for taking a first stab at this. Some notes:

We want the rate limit to be applied at the phone number level to prevent someone from creating multiple accounts that use the same phone number, and logging into each account, thereby triggering an OTP to the phone number from each account.
The logic doesn't seem quite right. The rate limit is meant to be timeboxed. If the rate limit is 3 tries within a findtimeof 5 minutes, if I make an OTP request at 12:01, and then wait until 12:06 to make the second request, my count should be reset back to 1. Instead, my count is set to 2, and my otp_first_sent_at is still set to 12:01.

monfresh · 2017-05-31T20:35:27Z

Given the complexity of the logic, I propose that we move it to a separate class.

zachmargolis · 2017-05-31T20:36:54Z

Good idea! Will move it to an OtpLimiter class

monfresh · 2017-06-01T19:38:29Z

app/controllers/concerns/two_factor_authenticatable.rb

This reset_attempt_count_if_user_no_longer_locked_out method only gets called after a user submits an OTP. It's a separate event than the one we are dealing with here. We need a new method in the OtpLimiter class that resets otp_send_count and otp_first_sent_at when the findtime has expired. Every time a user is about to be sent an OTP, the questions we need to ask are:

Has it been more than findtime minutes since the user last received an OTP?

YES: set otp_send_count to 1 and otp_first_sent_at to Time.now. Incidentally, this should probably be called otp_last_sent_at.

NO: Is otp_send_count >= maxretry?

YES: Prevent the user from receiving OTPs for bantime minutes

NO: Increment otp_send_count

Ah, yes. Thanks for pointing this out!

I will rename the column and try to implement this, thanks for the details 😁

Ok I gave this a shot & rebased

monfresh · 2017-06-19T18:00:21Z

I pushed a new commit with specs that define the desired behavior. Our goal is to make the tests pass. I can work on this.

monfresh · 2017-06-20T00:25:39Z

I pushed a commit to make one of the tests pass. One more to go.

monfresh · 2017-06-22T02:44:18Z

I added a new commit that should make all tests pass now. Let me know what you think of this approach. I know there are CC issues. I'll fix them later. There's also an issue that I'll need to write a test for because it's not tested: once a user gets locked out due to sending too many OTPs, when they log back in, the countdown timer is based on the max attempts lockout, which is 5 minutes, which might not necessarily be the same as the bantime for sending too many OTPs. We'll probably need to start tracking the cause of the lock out and display the appropriate content.

zachmargolis

New changes make sense to me!

zachmargolis · 2017-06-22T17:01:56Z

db/migrate/20170621202836_create_otp_requests_tracker.rb

Can we add t.timestamps and an index on updated_at? That will let us have a periodic job to delete old records so the table doesn't grow too much

zachmargolis · 2017-06-22T17:02:40Z

spec/features/two_factor_authentication/sign_in_spec.rb

nit: double it

monfresh · 2017-06-23T20:24:45Z

@zachmargolis I addressed your feedback and made the changes to use a single lockout period for both max OTP attempts and requests. I think this PR is in good enough shape for now. Wanna take one last look, please?

zachmargolis

LGTM! I have some small cleanup suggestions but nothing big

zachmargolis · 2017-06-26T14:03:52Z

app/decorators/user_decorator.rb

This is identitcal to lockout_time_remaining so I think we can remove it now

Nice catch!

zachmargolis · 2017-06-26T14:11:41Z

app/services/otp_rate_limiter.rb

So I think (like you mentioned) there is a small opportunity for a race condition here.

I think that if possible we should take advantage of Rail's first_or_create (as well as a top-level retry), so something like:

def find_or_create_with_phone(phone) return if phone.blank? # maybe just raise an error here? begin OtpRequestsTracker.where(phone_fingerprint: Pii::Fingerprinter.fingerprint(phone)). first_or_create(otp_send_count: 0, otp_last_sent_at: Time.zone.now) rescue ActiveRecord::RecordNotUnique retry # possibly use with_retries gem or add additional logic to only retry once end end

Good idea. Thanks for the suggestion!

Weird. I pushed a 6th commit several minutes ago, but I don't see it on GitHub yet.

zachmargolis · 2017-06-26T18:20:15Z

app/models/otp_requests_tracker.rb

We need to raise when we're not retrying otherwise this will just return nil and we'll get a NoMethodError rather than a duplicate key error

so like

rescue ActiveRecord::RecordNotUnique retry unless ... raise end

Sweet! Thanks!!

zachmargolis

LGTM!

monfresh · 2017-06-26T19:14:43Z

What do you think about the Code Climate similar code issues? Are they worth fixing now, or can we ignore them?

zachmargolis · 2017-06-26T19:41:29Z

This PR has dragged out so long I think it would be worth ignoring the duplicate code, I can follow up with a PR to generate an encrypted setter for one-way hashes the way we do for others

monfresh

LGTM

monfresh · 2017-06-26T19:57:45Z

Sounds good. I told CC to approve this PR. Wanna squash and merge?

zachmargolis · 2017-06-26T19:58:05Z

ok! I'll rebase this PR into two commits (one for my changes, one for yours) and then merge this

**Why**: Use the database rather than rack-attack

**Why**: - Since we don't enforce uniqueness of phone number, a malicious user could create multiple accounts with the same number and request OTPs from each account. We want to keep track of the request count based on the phone number used and then lock out all users who have that phone number. - To keep things simple, we use the same lockout period for both max OTP attempts and max OTP requests

zachmargolis requested review from amoose and monfresh May 31, 2017 19:17

monfresh reviewed May 31, 2017

View reviewed changes

monfresh reviewed Jun 1, 2017

View reviewed changes

zachmargolis force-pushed the margolis-otp-lockout branch from 9cca387 to dfbe091 Compare June 5, 2017 20:47

zachmargolis added the status - work in progress label Jun 5, 2017

zachmargolis force-pushed the margolis-otp-lockout branch from 168a2a0 to fb12051 Compare June 7, 2017 16:59

zachmargolis added status - ready for review and removed status - work in progress labels Jun 14, 2017

monfresh added status - work in progress and removed status - ready for review labels Jun 19, 2017

monfresh force-pushed the margolis-otp-lockout branch from 386163e to fd1dfab Compare June 22, 2017 02:56

zachmargolis commented Jun 22, 2017

View reviewed changes

monfresh added status - ready for review and removed status - work in progress labels Jun 23, 2017

monfresh force-pushed the margolis-otp-lockout branch 2 times, most recently from 19ba64a to d97e4f0 Compare June 24, 2017 02:58

zachmargolis commented Jun 26, 2017

View reviewed changes

monfresh force-pushed the margolis-otp-lockout branch from d97e4f0 to f2682b6 Compare June 26, 2017 16:33

zachmargolis commented Jun 26, 2017

View reviewed changes

monfresh force-pushed the margolis-otp-lockout branch from 0880ef5 to f65cadd Compare June 26, 2017 18:39

zachmargolis commented Jun 26, 2017

View reviewed changes

monfresh approved these changes Jun 26, 2017

View reviewed changes

zachmargolis and others added 2 commits June 26, 2017 15:59

Lock users out temporarily after too many OTPs

e30d157

**Why**: Use the database rather than rack-attack

zachmargolis force-pushed the margolis-otp-lockout branch from f65cadd to af3f5c7 Compare June 26, 2017 20:00

zachmargolis merged commit b8f61a9 into master Jun 26, 2017

zachmargolis deleted the margolis-otp-lockout branch June 26, 2017 20:32

zachmargolis mentioned this pull request Jun 26, 2017

Remove OtpRequestsTracker#encrypted_phone #1505

Merged

Conversation

zachmargolis commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented May 31, 2017

Uh oh!

monfresh commented May 31, 2017

Uh oh!

zachmargolis commented May 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented Jun 19, 2017

Uh oh!

monfresh commented Jun 20, 2017

Uh oh!

monfresh commented Jun 22, 2017

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monfresh commented Jun 23, 2017

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

monfresh commented Jun 26, 2017

Uh oh!

zachmargolis commented Jun 26, 2017

Uh oh!

monfresh left a comment

Choose a reason for hiding this comment

Uh oh!

monfresh commented Jun 26, 2017

Uh oh!

zachmargolis commented Jun 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone