dev-xo / remix-auth-totp

A Time-Based One-Time Password (TOTP) Authentication Strategy for Remix-Auth.
https://totp.fly.dev
MIT License
418 stars 28 forks source link

[ Request ] Allow the flow to continue when emails are invalid. #36

Closed alindsay55661 closed 11 months ago

alindsay55661 commented 11 months ago

This is essentially a security feature where I don't want to reveal that the email is, in fact, invalid. In my case, I am currently checking the database prior to sending out the auth codes and I am doing so discreetly without informing the end user. However, I'd like to check the database prior to storing the auth codes as well so as to keep junk requests out of my system. There is no self sign-up in my flow so I'm not creating users via this process. By the time a user is authenticating, they have already been invited by an administrator and their email is in the system.

Because of this, attempting to get an auth code for a given email potentially reveals that the email is or is not in the system (I'm currently hiding this by pretending all emails are "valid") and populates the database with auth codes no matter what, even if emails are never sent. I'd like to discreetly stop the auth code population in the same way I'm stopping the email.

As validateEmail seems to be for throwing errors, exposing email to sendTOTP would satisfy my requirements as I could perform the db check before storing the auth code and continue the flow normally. Thanks for your consideration.

dev-xo commented 11 months ago

As this seems to be a more specific case than the norm, or the usual authentication flow of a Time-Based One-Time Password, it would be great if you could come up with an implementation for your current issue @alindsay55661.

I'm quite busy nowadays, although I will look into your request and will consider a few things. In any case, if you bring up a solution that does not alter the main strategy and also solves your issue, that could potentially be integrated into the library.

Also, @mw10013, any thoughts on this?

mw10013 commented 11 months ago

@alindsay55661: I'm having trouble fully understanding the scenario and ask for a breakdown of the following,

Here's how I might approach it with my vague understanding of your scenario,

The white-listing of emails would be done in a layer above remix-auth-totp

alindsay55661 commented 11 months ago

The white-listing of emails would be done in a layer above remix-auth-totp

@mw10013 this can work to move the logic completely outside of remix-auth-totp. It will be extra work, however, to maintain two flows, one that appears to function and one that actually functions. The primary need here is not to reveal anything about existing users but to always have the appearance of codes being sent successfully.

Back to a native solution, I can achieve my goal if email is added to StoreTOTPOptions interface as an optional field similar to expiresAt. This gives me access to the email of the requestor at the time the storage request is made, which is all I need. Additionally, it allows for improved auditing of totps in the database for those that want it, for example to monitor which emails are requesting codes and how often.

If you're not opposed, I can open a PR for this. It will keep me from having to implement two separate UI flows. Let me know.

alindsay55661 commented 11 months ago
  • What is in the database already.

The user accounts / emails as you correctly assumed.

  • What is the behavior when an unauthenticated visitor navigates to a protected page.

Redirect to /login -> Enter your email to login -> We've sent you a code, check your email and enter code here

  • What is the login page behavior when an email that is not white-listed is used.

From the user's perspective, identical to whitelisted emails. UX "behavior" is the same. Behind the scenes emails that aren't whitelisted never get sent a code. This is convenient and easy for me to manage, a single auth flow using remix-auth-totp apis. The only limitation I have currently is that I accumulate auth codes in the db for emails that aren't whitelisted. Not the end of the world but would be nice to avoid it.

dev-xo commented 11 months ago

Interesting. As far as I can say, lately I simply do not have much time to take care of the library as it deserves. However, what you are suggesting seems more like an enhancement than a drawback, and could be useful for people with the same issue.

If the implementation does not require significant changes, and does not affect the current refactors @mw10013 is doing https://github.com/dev-xo/remix-auth-totp/pull/37, feel free to open a PR. I will take the time to review it, and I would also appreciate some help/review from @mw10013 if he has the time.

mw10013 commented 11 months ago

I'm still having trouble piecing together your scenario and seem to have a disconnect. Let's walk through the scenario and you can point out where my disconnects are.

At this point the visitor is still unauthenticated from the perspective of remix-auth-totp and will remain so forever since the TOTP code is gone with the email that was never sent out. Any page protected by remix-auth-totp will redirect the visitor to login.

My disconnect is how you expect to fake out remix-auth-totp into believing a visitor is authenticated simply by not storing the TOTP and sending out the TOTP email. The only way remix-auth-totp will authenticate a visitor is by presenting the correct code and I don't see anywhere in your scenario where that would happen.

What am I overlooking or misunderstanding?

mw10013 commented 11 months ago

Advise not creating a PR against storeTOTP() since the API may change due to #37. Let's see if we can sort this out here and maybe I can roll any API changes stemming from this into #37.

alindsay55661 commented 11 months ago

At this point the visitor is still unauthenticated from the perspective of remix-auth-totp and will remain so forever since the TOTP code is gone with the email that was never sent out. Any page protected by remix-auth-totp will redirect the visitor to login.

Correct. In my case all the logic (email + code handling) is in the same route like you have in the example on the README: https://github.com/dev-xo/remix-auth-totp?tab=readme-ov-file#logintsx. So, when a user tries to access a protected page, they are redirected to /login and see the auth code verification form since that is where they left off. They have links to request a new code or use a different email which will reset remix-auth-totp and start the flow again. Either way, you are correct that the user is not authenticated. This is the desired behavior.

My disconnect is how you expect to fake out remix-auth-totp into believing a visitor is authenticated simply by not storing the TOTP and sending out the TOTP email. The only way remix-auth-totp will authenticate a visitor is by presenting the correct code and I don't see anywhere in your scenario where that would happen.

I'm not seeking to authenticate anyone outside of remix-auth-totp. The person I am "faking out" is the real user who is trying to authenticate with an invalid email address. Perhaps "fake out" is a confusing term here. I am merely hiding the fact that their email doesn't exist in the system. So, by presenting the same UI that valid users get, the one that says "check your email for the code...", it communicates to them that the auth process is working normally, which is what I want to communicate. Some companies use different wording such as "if you have an email on file, the code was sent there" which still protects data privacy but also divulges that the auth processes works conditionally. It is a matter of policy how much any company wants to disclose during the authentication process. I prefer to use the same experience for all authentication attempts. Good actors reach out to support if they have a legitimate issue and bad actors recognize they can't read from the database by prompting the auth system. Either way, remix-auth-totp is doing its job correctly, blocking all users that have not legitimately authenticated.

This is working really well already in terms of the desired UX. The only byproduct is that I can't prevent auth codes from being injected into the database for non-whitelisted emails because once remix-auth-totp receives an email address, it invokes storeTOTP without providing the email address to that method so I can't check it against the whitelist and prevent the database insert. It will always store the totp. Technically a bad actor can flood the database with totp records since that insert is 100% open to the public. If I do the whitelist check in validateEmail I am disclosing to the end user that this particular email is not in the system because they are never advanced to the code verification stage. On the other hand, when remix-auth-totp calls sendTOTP just after storeTOTP, it provides the email for obvious reasons. So this is where I am doing the whitelist check and preventing the email from going out. Again, not a huge deal really, but would be nice if I could perform that whitelist check in the prior step (storeTOTP) so I don't even push those codes into the database at all.

In summary:

  1. I want identical user-facing UX for both whitelisted and non-whitelisted email addresses. ("go check your email")
  2. I want to prevent emails from going out to non-whitelisted email addresses (done)
  3. I want to prevent totps from being added to the database for non-whitelisted emails (would be very easy if email is provided to storeTOTP.

I hope this makes more sense, it is a security / privacy situation aimed at the bad actors so not your typical happy path request 😃

mw10013 commented 11 months ago

Interesting. I think I'm understanding more.

At this point, you want the bad actor to believe the email is ok from the application's perspective. Does the following tip off the bad actor that the email really is invalid,

In other words, the bad actor would still be able to probe whether an email was valid in the application or not. What are your thoughts about that since isn't that what you want to prevent.

As an aside, wondering what your plans are around purging expired TOTP's from the database. They will accumulate every time a valid email requests a code.

alindsay55661 commented 11 months ago

Does the following tip off the bad actor that the email really is invalid,

  • The fact that the bad actor doesn't receive an email with the code.

Yes, a bad actor can conclude that any emails that don't receive codes are not in the system. If these emails are owned by the bad actor this doesn't matter, the bad actor already knows those emails aren't in the system. If the bad actor has gained access to others' email then there are bigger problems. Not only will they be able to conclude that such emails are not in the system, they will gain system access from any emails that are in the system.

  • If the bad actor submits any code on the verify page, remix-auth-totp reports a TOTP not found in the database error.

Fortunately remix-auth-totp allows custom error messages. In my case the error messages are set to an identical error: "Sorry, we could not authenticate this code, it may be expired or mistyped. [Request a new code] [Use a different email] [Contact support]..." etc. A generic message doesn't mean the bad actor won't correctly infer that the code (and hence, the email) isn't in the db so the protection here isn't perfect. Even if the error messages were all set to "code is expired" a bad actor could still make a correct inference by testing the timing of repeated attempts.

As an aside, wondering what your plans are around purging expired TOTP's from the database. They will accumulate every time a valid email requests a code.

I've been putting off this decision, lol. I am considering adding a purge call to a regularly accessed api, maybe one that doesn't need the highest latency. If the purge is fast enough it could potentially be done at the same time totps are inserted. That way auth code generation and clean up happen together and the table stays lean. Otherwise I'll set up scheduled tasks but I don't currently have infrastructure in place for that so have been avoiding. You mention this as "an aside" but it's related since my ask here is about managing the totps table more than anything else. remix-auth-totp is flexible enough already to address the privacy issues I mentioned.

mw10013 commented 11 months ago

Thanks for the additional color. I wonder if simply filtering emails on SendOTP() and focusing on purging expired TOTP's would suffice.

remix-auth-totp is designed for unreliable email (eg. can simply request a new code) so its behavior doesn't change if emails don't go through.

PR #37 refactors the api to help applications simplify their implementations for purging expired TOTP's. When remix-auth-totp wants the application to store or update the TOTP, it will also pass along expiresAt so it can be persisted in the database along with the TOTP data.

Put an index on the expiresAt column and you may be able to get away with purging expired TOTP's whenever a TOTP is stored.

A key-value store with expiration is an attractive option for storing TOTP's since no scheduled task would be needed for purging. remix-auth-totp supports the Cloudflare runtime making it easy to leverage Cloudflare KV.

Please do let us know what purging strategy you end up using and how well it's works for you. We continue to be on the look out for ways remix-auth-totp can help simplify purging implementations for applications.

alindsay55661 commented 11 months ago

I wonder if simply filtering emails on SendTOTP() and focusing on purging expired TOTP's would suffice.

Yes, it would. The ask on this issue is merely an optimization, it's not required for me to accomplish my goals.

Please do let us know what purging strategy you end up using and how well it works for you.

I'm on Planetscale so won't be leveraging KV auto expiration features. I think I will start off testing purges during creation and go from there.

dev-xo commented 11 months ago

All right, I've been trying to keep up with the conversation as much as I could, although for sure I'm missing some pieces, as I wasn't the one discussing it.

Thank you both for taking the time and for providing great arguments based on it. At this point, as I said to @mw10013, I've got the feeling that you guys understand the Strategy and the library better than me, and that's good. At the end of the day, this is an open-source library, and although I'm the one who makes the final decisions, most of the time surrounding members are the ones who pushes the library to some new levels.

I'm not sure if you want to discuss anything else, or if we could close this discussion here for now. In any case, let me know, and feel free to open any new issues if you encounter them, @alindsay55661!