delight-im / PHP-Auth

Authentication for PHP. Simple, lightweight and secure.
MIT License
1.08k stars 234 forks source link

2FA authentication #222

Open nssnl opened 4 years ago

nssnl commented 4 years ago

I'm working on implementing 2FA. I read the previous posts, like #34 and #133.

I want to offer several ways, including SMS and time based OTP's (e.g. through Google Autenticator / Authy, etc). Along with entering the 2FA code, the user can opt to "Trust the device". In that case a cookie will be saved with a unique identifier that is also saved in the database.

This commit is useful, but I want to be more flexible in terms of error reporting - as only TRUE or FALSE is returned. Also if a user is logged in while in the OTP auhentication process, it is easier to retrieve the mobile phone number of a user in case of resending SMS - see below.

To this end, I was thinking of logging the user in, and then checking in an additional user table whether 2FA is verified through the "Trust the device"-cookie and/or current session. If not verified, a 2FA screen is shown with the different options: time based OTP and fallback SMS. User can opt for either one. If the user opts for SMS, for example, and the SMS is not received within 30sec, the user is able to request for resending the SMS. If the user would not be logged in (i.e. if the commit is used), this would only be possible with a new login request, as the mobile number is not known.

To verify the 2FA I was thinking of the following approach:

Both ways ensure that a user is considered to be 2FA-verified. If a user enters an incorrect code X times, $auth->logOutEverywhere(); is triggered.

What do you think of the above, does it make sense?

ocram commented 4 years ago

Thanks for sharing your thoughts on this!

It seems one of the main points of your proposal is that you want to let users sign in immediately, and only then check if 2FA is required (and provided).

I’m not really sure about that, because it could be more error-prone. When it comes to authentication and security, you don’t want mistakes to be easy to make and likely to happen. You want to fail safely or “closed”. With your implementation, it seems you would only have to forget your additional checks once to have a severe bug, because users appear authenticated while another step may be necessary. Apart from that, checking Auth#isLoggedIn as a single source of truth would not be sufficient anymore. You would have to check two things – always. Is that a wrong impression?

If all this is only because you need the phone number before the user is signed in, I think we could make this work in a different way.

For example, with the onBeforeSuccess callback from that commit, after an otherwise correct and complete login, you get the user ID and can use it to check in your database if 2FA is required. If it is, you return false and some more work is required, otherwise you return true and let it succeed. If you do this via AJAX/XHR, the user experience is quite good and you can also let the user request a new SMS again. Without AJAX/XHR, which is the more common case for the login (although the implementation can be changed, of course), it’s a worse experience.

Another thing we could do as an alternative is the following:

  1. Define fields in the database and add accompanying methods that let you mark users as either 2FA-required or just “1FA”
  2. Allow for tokens to be generated that can be used to sign in, where a token can either replace the first factor (password) only (for 2FA) or both factors (e.g. for “remember me”, which we already have)
  3. If a user tries to sign in but they are marked as 2FA-required, that process fails with an exception but that exception includes both the user ID and a token which you can use to sign in later if the second factor is completed, with a validity of 5 minutes
  4. Now you can perform your checks for the second factor in any way you like, and when you’re ready, just use the token to complete the sign in
  5. When it comes to “trusting the device”, we already have the “remember me” feature, which really trusts the device and keeps the authentication active for a longer time, but perhaps you’d also want a way to just keep the second factor completed and only ask for the first factor (password) later again

What do you think? Would that solve your problems with 2FA in better ways?

nssnl commented 4 years ago

Thanks for your response and thoughts!

You would have to check two things – always. Is that a wrong impression?

Yes that's correct. You're absolutely right that it's more error-prone. In my specific use case this a manageable risk as there is one "controller"-file where all the pages, get/post requests, etc are routed through. From the perspective of the development of the library it may not be a sustainable general solution indeed.

If all this is only because you need the phone number before the user is signed in, I think we could make this work in a different way.

For example, with the onBeforeSuccess callback from that commit, after an otherwise correct and complete login, you get the user ID and can use it to check in your database if 2FA is required. If it is, you return false and some more work is required, otherwise you return true and let it succeed. If you do this via AJAX/XHR, the user experience is quite good and you can also let the user request a new SMS again. Without AJAX/XHR, which is the more common case for the login (although the implementation can be changed, of course), it’s a worse experience.

I'm using AJAX/XHR for the login form (with jQuery). What I initially did was hiding the 2FA input field, and show this "div" when the onBeforeSuccess failed. Making the SMS request work along with the other options felt a bit like making too many work-arounds. I also want to offer email verifications (in certain cases; if enabled on the account) and time-based OTP's. But I guess it would't be that bad anyway; the following would be a possibility:

With the above, you would need to send the username/password for each action (rate limiting might be triggered earlier than normale use case). From a user experience perspective, it seems the nicest way if the username/password are hidden once the return array is received - since it is certain that the credentials are correct.

If this way seems right, I'm struggling a bit how to return this array with additional data, since the onBeforeSuccess needs to return false. I guess this should be returned in the exception, but I'm not sure how to do that in the most proper way.

Regarding the "trust device". I indeed want to keep the second factor completed for a long time, like 4-6 months, while the "remember me" is typically set for a short period. But this should be easy to check as part of the onBeforeSuccess part, returning true if the cookie is present with a valid 2FA token (tight to the user ID with a validity date).

Another thing we could do as an alternative is the following:

  1. Define fields in the database and add accompanying methods that let you mark users as either 2FA-required or just “1FA”
  2. Allow for tokens to be generated that can be used to sign in, where a token can either replace the first factor (password) only (for 2FA) or both factors (e.g. for “remember me”, which we already have)
  3. If a user tries to sign in but they are marked as 2FA-required, that process fails with an exception but that exception includes both the user ID and a token which you can use to sign in later if the second factor is completed, with a validity of 5 minutes
  4. Now you can perform your checks for the second factor in any way you like, and when you’re ready, just use the token to complete the sign in
  5. When it comes to “trusting the device”, we already have the “remember me” feature, which really trusts the device and keeps the authentication active for a longer time, but perhaps you’d also want a way to just keep the second factor completed and only ask for the first factor (password) later again

What do you think? Would that solve your problems with 2FA in better ways?

The above is certainly a nice elegant solution. Perhaps for the future this would be a nice addition to the library.

ocram commented 4 years ago

Thanks for your further feedback!

Maybe we should indeed consider the solution suggested above for the future, and make some quick changes for now that let’s us use 2FA with the existing callback function.

If your main problem is how to pass around the identifiers and the name of the verification method for 2FA, the solution below might be for you. You are right in that the callback should not be used to pass around those pieces of data. Instead, the callback is only expected to return a boolean value to indicate if 2FA is required (false) or not (true).

You could try changing the definition of AttemptCancelledException in src/Exceptions.php as follows:

class AttemptCancelledException extends AuthException {

    protected $tfaMethodName;
    protected $tfaEmailAddress;
    protected $tfaPhoneNumber;

    public function __construct($tfaMethodName, $tfaEmailAddress = null, $tfaPhoneNumber = null, $message = '', $code = 0, \Exception $previous = null) {
        $this->tfaMethodName = $tfaMethodName;
        $this->tfaEmailAddress = $tfaEmailAddress;
        $this->tfaPhoneNumber = $tfaPhoneNumber;

        parent::__construct($message, $code, $previous);
    }

    public function getTfaMethodName() {
        return $this->tfaMethodName;
    }

    public function getTfaEmailAddress() {
        return $this->tfaEmailAddress;
    }

    public function getTfaPhoneNumber() {
        return $this->tfaPhoneNumber;
    }

}

That would enable you to use the exception instance to pass around the data you need.

Thus, you could then replace

throw new AttemptCancelledException();

in src/Auth.php with

throw new AttemptCancelledException('sms', null, '0123456789');
// or
// throw new AttemptCancelledException('totp', null, null);
// or
// ...

What do you think?

nssnl commented 4 years ago

Thanks again for the detailed response - as always, very much appreciated.

This seems indeed the best approach, but I would have to make direct changes into the library right (which I prefer not to)? Anyway, I'm going to further research it and work on it. Thanks

ocram commented 4 years ago

You’re right, but I’m afraid it won’t work without either making some (temporary) changes to the library or waiting for the integration here.

As long as you’re making only a few changes to the library and documenting them (!) in a precise way, I think that can be done.

Would love to hear your feedback on the modified exception class and its usage. If that works and simplifies your implementation, we could at least merge that modified exception class back into the main library here very quickly.

nssnl commented 4 years ago

Thanks. I'll keep you updated once I have made the changes.

ahanley commented 2 years ago

Hi ... i know this post is old but i was curious if there were any traction to get this functionality added to the core Auth library. 2FA seems like the norm now for authentication these days!
Thanks for the great library btw much appreciated!

ocram commented 2 years ago

@ahanley Thanks! 2FA is the standard today for high-profile and high-value services/accounts and for services with hundreds of millions of users. Apart from that, a number of services offer 2FA, but it’s not really everywhere yet. Anyway, it’s certainly a best practice and there is no reason why one should not offer this, if possible. As for this library right here, unfortunately, it’s not really clear yet what would be the best way to integrate it.