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

[Feature] TOTP 2FA support #2195

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

Conversation

jozip
Copy link

@jozip jozip commented Dec 11, 2022

Synopsis

This PR implements rudimentary support for Time-Based One-Time Password (TOTP) Two-Factor Authentication (2FA) that is compatible with apps such as Google Authenticator, FreeOTP and KeePass.

Rationale

Using a second factor for login greatly improves security. HOTP and TOTP are common and well-known schemes, where the latter is more widely used.

Overview

The user can enable two-factor authentication by clicking a button under the 2FA-heading in the account settings page.
This generates a TOTP-token and presents a QR-code that the user can scan using their preferred OTP-app. The setup is enabled once the user inputs a correct TOTP-code within the valid time-frame into.

The user can then use two-factor authentication when logging in using a password or via email.

Disabling 2FA is simply done by clicking the corresponding button in the account settings page.

Implementation details

I've tried to be as unintrusive as I can regarding the UX and the authentication flow, since I'm unfamiliar with the prior design considerations and I want to minimize the risk of messing up such a critical part of the system. Therefore some things may appear a bit more roundabout than they necessarily should've been.

The data related to the 2FA-logic resides in the participants table, since it needs to be accessible regardless of authentication method, it's not terribly secret (in that it's just a random nonce and it leaking mean little outside of this context) and I couldn't figure out if user_secrets really was a suitable place.

The core logic resides in the Participants-model with a few incisions in the authentication logic. I opted to add a couple of simplates to make enabling 2FA a bit easier, rather than trying to jack into www/%username/settings/edit.spt.

Adding an additional, optional field for the password login was easy enough, since it's a form that is POST:ed. The email login flow is via a GET, which prompted the creation of the odd javascript snippet in order to feed it an additional querystring parameter for the TOTP-code. (Again, I didn't want to up-heave the current logic with my limited knowledge.)

Dependencies

The following new dependencies have been added:

  • pyotp==2.7.0
  • PyQRCode==1.2.1
  • pypng==0.20220715.0

A cursory glance of each library's source doesn't raise any red flags. pip-audit doesn't report any known vulnerabilities either.

Remaining work

  • Integration tests
  • Improved look and feel
  • Improved failure feedback in simplates
  • Cache some of the calls in Participant
  • Implement support for recovery codes

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are accessibility issues in these changes.

www/%username/settings/enable_2fa.spt Outdated Show resolved Hide resolved
@Changaco
Copy link
Member

Changaco commented Dec 12, 2022

@jozip Welcome, and thanks for stepping up. Here is my initial feedback on the code you've written so far:

  • The TOTP verifications must all be rate-limited to a few attempts per account per day. With the current implementation an attacker could try a significant percentage of the possible codes, especially since each request tests 11 different codes (the current one, the previous 5 and the next 5).
  • The js/log-in-link-is-valid.js file should be dropped. JavaScript should used be as little as possible.
  • The VALID_WINDOW_2FA constant should be renamed to TOTP_TOLERANCE_PERIODS and a comment should be added to clarify that it isn't a number of seconds.
  • The TOTP token should be moved to the user_secrets table, using -1 as the id.
  • handle_2fa.spt and enable_2fa.spt should be merged and renamed to totp.spt.
  • The new Participant methods and the FailedToVerifyOTP exception class should be dropped. All that logic should be moved to the simplate, and the tests should be modified to make requests to the simplate.
  • 2FA and TOTP should be (briefly) explained to the user. There are still plenty of people who don't understand these things.
  • An alt="QR code" attribute should be added to the <img> tag flagged by AccessLint.
  • An <a> tag pointing to the same URI as the QR code should be added. Users should be encouraged to click on it directly if they have a TOTP app on the machine they're viewing the page on.

@Changaco
Copy link
Member

I found a small problem in pyotp and the Ruby library it was forked from. The fixes are in pyauth/pyotp#148 and mdp/rotp#119.

This branch should be updated to use the 2.8.0 version of pyotp.

Copy link

@accesslint accesslint bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏 You fixed the issue(s)! Great work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants