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

Multi-factor login #236

Open
dantownsend opened this issue Oct 3, 2022 · 5 comments
Open

Multi-factor login #236

dantownsend opened this issue Oct 3, 2022 · 5 comments
Labels
enhancement New feature or request proposal - input needed An idea for a new feature which requires input

Comments

@dantownsend
Copy link
Member

dantownsend commented Oct 3, 2022

BaseUser has an email field, so we could integrate multi-factor login.

When a user attempts to login, after verifying the username and password, we email them a code which they need to enter into the UI.

We would need a table for storing the codes - we could create a new Piccolo app called something like multi_factor.

We would have a base class called something like MultiFactorProvider, with a send_code method. Out of the box we can have a subclass for sending email over SMTP.

from piccolo_admin.multi_factor import SMTPProvider

SMTP_CREDENTIALS = {
    ...
}

app = create_admin(tables=[Movie, Director], multi_factor_provider=SMTPProvider(**SMTP_CREDENTIALS))

There are already some security measures on the login page, like rate limiting, but having multi-factor would be a great step forward.

Email seems like the best route forward. With authenticator apps, we would need to build additional UI for displaying QR codes etc. With SMS, there's no standardised API that I'm aware of for sending them, like with SMTP, so would be more work.

This is a large feature, but would be pretty awesome.

Example UI / workflow:

multi_factor

@dantownsend dantownsend added enhancement New feature or request proposal - input needed An idea for a new feature which requires input labels Oct 3, 2022
@dantownsend dantownsend added this to To do in Enhancements via automation Oct 3, 2022
@sinisaos
Copy link
Member

sinisaos commented Oct 4, 2022

@dantownsend It's a nice idea for a regular website, but for the admin interface I don't think it's necessary because it's too much work, and the Piccolo Admin has already limited access. When a user create an account admin argument is False by default and only the superuser can change it for users he trusts, so I don't think additional authorization is needed for those users (other users without admin=True cannot access the admin anyway, and only superuser can change data thanks to superuser_validators). That's just my opinion, but it won't hurt if you implement that feature.

@dantownsend
Copy link
Member Author

dantownsend commented Oct 4, 2022

@sinisaos It's mostly a defence-in-depth in case someone brute forces the username and password on the login page. We use rate limiting, so it would take a long time, but it's possible if someone had a really obvious username and password (like admin/admin), or their credentials had been leaked somewhere.

It would be a nice feature - I would definitely use it. But it is a fair bit of work. I'll leave this issue here for now, and might pick it up in the future.

@sinisaos
Copy link
Member

sinisaos commented Oct 4, 2022

@dantownsend Good point. I didn't think about the superuser himself and his potential mistakes. In that case, multi-factor authentication makes perfect sense. It will be a great feature when it's done.

@Skelmis
Copy link
Contributor

Skelmis commented Nov 11, 2023

Here's my two cents on MFA from both a user view and security view.
Note MFA is amazing from a security viewpoint and is recommended for basically all use-cases haha.

Support multiple forms of MFA

As a user when I think of the ability to sign up with MFA, I tend to think of TOTP codes or Yubikeys. It would be nice to provide an end user the ability to sign up using their preferred MFA method. Some examples are listed below:

  • Using the BaseUser's existing email field
  • Using TOTP codes via an app such as Google Authenticator
  • Using SMS
  • Hardware keys such as yubikeys

From a security aspect, each of these approaches come at different security risks. For example, it's a lot easier to conduct phishing on TOTP codes versus something like hardware keys which are currently considered unphishable.

From a developers point of view as well, if you added support for things such as hardware keys or TOTP then it would be a lot easier to run a local development environment. I'm just thinking that if you only support email, then in order to test locally you'd always need an email backend which isn't always trivial to acquire.

Allow for multiple forms of MFA at once

Often sites only allow an end user to set up one form of MFA at once which isn't great from both a usability aspect and security aspect. For example, what happens when you lose your only form of MFA?

All I have to say here is I'd recommend that this is made as something like an M2M and allow users to setup as many forms of MFA as they wish. For example I personally want to sign up using TOTP and hardware based keys. I know others who use a primary hardware key but also sign up a second one for if they lose the first.

Ensure codes expire and are one time usage

From a security aspect, ensuring any email or SMS codes meet the following criteria is something worth seriously considering:

  • Codes should feature a 'time to live' after which an end user must request a new one
  • Codes should only be able to be used once

Other forms of MFA should also meet these expectations, but RFC compliant libraries in theory already handle that for you. A lot don't so it'd be worth checking.

Ensure MFA checks count towards lockouts

As far as I am aware Piccolo does not currently feature an account lockout policy. If that changes in the future however, it should be noted that MFA failures should count towards a form of lockout. If it does not, then MFA codes could possibly be brute forced for example.

Ensure MFA modifications are treated as privileged actions

Prior clarification, a privileged action is something which in my eyes modifies an account / access to an account and should require higher verification to undertake. An example of a privileged action within the current code base is changing your password via the admin panel. This change requires you to submit your current password in order to set a new one.

When implementing similar functionality for adding/changing/deleting a users MFA options it should also enter this higher privileged mode.

As an example, the following is a real world attack which may be undertaken if any authenticated session can simply modify MFA sessions.
A malicious user could find and abuse some form of stored cross site scripting vulnerability which when viewed executes arbitrary javascript from the context of a users session. If a privileged mode is not enforced, this javascript could for example delete a users current MFA. After this occurs an attacker would then only need to bruteforce the password in order to gain account takeover.


Lastly, I know that adding support for these points would involve more effort then email alone however from a security aspect I feel they are definitely worth it. If I end up sometime in the future I may do some work towards this point also, but I'll use this issue as a discussion area.

@dantownsend
Copy link
Member Author

@Skelmis I didn't adequately thank you for this - it's super helpful, thanks!

I'm going to take a serious look at implementing MFA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request proposal - input needed An idea for a new feature which requires input
Projects
Development

No branches or pull requests

3 participants