Disallow authentication code once verified about devise-two-factor HOT 9 CLOSED

Thanks for pointing this out! I'll fix it when I have a chance.

from devise-two-factor.

Submitted to ROTP gem as mdp/rotp#44 as it's their domain to comply with HOTP and TOTP specs as defined in the RFCs.

from devise-two-factor.

I'm not sure the ROTP library is a good candidate. You have a good point that it is supposed to follow RFC (and at least document clearly where it does not). However, since you have to store somewhere the last accepted code or timestamp, ROTP would need to implement access layer to a database or other storage. That seems out of scope for such simple (purely algorithm) library.

from devise-two-factor.

@vilda I'm inclined to agree with you and the ROTP maintainer at this point. I'd hope that he'll provide extendable hooks and clear documentation in future work.

I'll probably dabble in implementing a solution in your library and submitting a PR, but I have little (read: zero) experience in working with Rails gems.

from devise-two-factor.

Would an approach like this work?

1. Add a column consumed_otp to the devise two-factor model table.
2. Server, upon supplied a valid OTP, hashes a concat of the current timestep and valid OTP and compares it to the value stored in consumed_otp.
3. If there is no match, permit login and update consumed_otp value for the row. If there is a match, deny login as it's a "burned" OTP code for that time step.

I feel like it's acceptable because:

1. The concatenation of the time step ensures the burned code applies only for that given time step
2. There's no master list of N-many "burned codes" since we only need to know the one code issued for a given timestep.
3. A valid OTP is inherently unique to a given user, therefore the resulting hash is unique to a given user.

from devise-two-factor.

I don't think the hashing step is necessary. Storing the timestep of the most recently successful authentication should be sufficient -- we don't care what the OTP was, we just care that a given OTP is only used once, and since each timestep has only one OTP, we can avoid needing to store the OTP entirely.

Other than that, your approach looks fine to me.

It's also important to ensure that we work with the timestep used to generate the OTP -- not the timestep associated with the request. Because of the drift period, these may not be the same.

from devise-two-factor.

I'm happy with your suggestion.

But with your approach, if you want to be anal about it, a user could theoretically login (thereby burning that timestep) disable & renable OTP, log out and try to login with a new OTP for that same timestep and getting denied, despite providing a unique OTP. Now with the default ROTP timestep interval being 30 seconds, and tinfoil's gem offering no option to override it, this becomes a very unpractical & narrow issue.

from devise-two-factor.

Ha, I hadn't thought of that!

That does sound like a valid bug, though it's enough of an edge case that I think I'd prefer sticking with the simpler implementation. If anyone raises their pitchforks in the future because of it, you can point them in my direction :P

from devise-two-factor.

Fine by me 😉

from devise-two-factor.