2FA (TOTP) support

[woodruffw]

See #4373
Issue #996

CC: @nlhkabu

[woodruffw]

@ewdurbin @di @brainwane I think we're ready to do an initial review here!

We should be at 100% coverage, and the basic flow for TOTP-based 2FA is present: users can add and remove a TOTP secret from their account, provision a device, and are prompted to enter a TOTP code upon login. Recovery codes haven't been added yet; my current plan is to add them in a separate PR, but I'm open to changes to that.

Here are some things I'd like to also handle in this PR:

• QR code generation. Right now only a provisioning URL is shown; I was going to take a look at @mschwager's work in Added multi-factor authentication for account login #4618 and adapt it this implementation.
• Integrating @nlhkabu's proposed authentication UI in 2FA User Interfaces (authentication workflow) #5587 and additional management UI (no issue yet).

This changeset is getting pretty big, though, so I can look at splitting it apart to make reviewing easier. Let me know what you think.

[woodruffw]

NTS: I don't really like this double state here (totp_secret holds the user's TOTP secret key, while totp_provisioned is only set to true once the user verifies that they've added their device correctly by entering a code), but I'm not sure about better ways to do it:

• We could pass totp_secret as a cookie or hidden form parameter in manage.account.totp-secret, but this would allow the user/page javascript to modify the TOTP secret and potentially compromise it.
• We could regenerate totp_secret each time the user refreshes manage.account.totp-secret, but this produces a really bad UX: the user will have to scan a new QR code each time they fail to validate their device.

Is there another place we can sequester the TOTP secret state without actually updating the user model? Alternatively, is there a way to ensure cookie integrity (i.e., that the user/page hasn't modified the cookie we sent)?

[ewdurbin]

@woodruffw you could use this utility module to create a signed payload if we choose to go the cookie route.

Alternatively Redis is readily available for stashing this state.

[offlinemark]

Summarizing a discussion @woodruffw and I had last week:

While having double state enables a user to potentially "get stuck" in the intermediate state of having a secret, but not being provisioned, any alternative where a secret key is generated and displayed, but not fully persisted could result in a bad UX scenario where a user has broken Pypi entries in their 2FA app.

For example, consider the case where:

• we generate and display a secret
• the user adds it to their device, but, for whatever reason, does not provision
• we expire that secret after some amount of time (since we do not fully persist)
• the user attempts to enable 2FA again
• we generate and display a new secret
• the user adds it to their device

The user will then have two Pypi entries in their device, of which only one is correct and usable.

Depending on our expectations for the type of user, this might be a rare case, but in order to fully ensure good UX related to their 2FA app, we probably want to go with updating the user model when we generate the secret. Especially given that there are no real security drawbacks of having this intermediate state, it seems like a totally adequate solution to me. The only thing is that future development needs to be aware of the double totp state.

[ewdurbin]

I am curious if there is a case to be made for separating this from the User model except by relation.

While supporting a single TOTP is logical, if support for U2F or WebAuthN is added there is good reason to have multiple devices associated.

This would make the model a bit more straightforward and avoid hammering the User model unnecessarily.

[woodruffw]

I am curious if there is a case to be made for separating this from the User model except by relation.

Yeah, I think so. Looks like the current email relation will be a good reference for this 😄

[offlinemark]

Just wondering why the ascii codec is explicitly used here rather than the default utf-8? (I'm assuming form.totp_value.data is a str). Does form.validate() guarantee that the data here will be ascii?

[woodruffw]

No particular reason! encode() with the default UTF-8 encoding would suffice. I'll make this change tomorrow.

form.validate() does indeed ensure that the data is ASCII, by virtue of only accepting /[0-9]{6}/.

[woodruffw]

Alright! We've got QR generation:

The provisioning URI is also placed in the <canvas>'s aria-label attribute, for accessibility purposes.

[offlinemark]

Hi all, after getting CI passing yesterday, I began designing a solution for one of the outstanding features to be implemented: password reverification before TOTP can be disabled. The idea is that when a user disables TOTP, they are decreasing their security, so we want to ensure they are actually the owner of the account before doing this. The initial solution I've come up with involves the following flow:

• User clicks the "disable" button, submits the form on the account settings page
• That submits to /manage/account/totp-provision, which does the username check (current behavior)
• User is redirected to normal login flow. They enter their username and password and reauthenticate.
• (Optional/undecided) They are redirected to TOTP reauthentication. They enter a valid TOTP code.*
• After valid reauthentication, server generates a signed "reauthentication token" with the user id and timestamp, sends it to the client. Server stores no state.
• User is then redirected to an endpoint that actually handles the TOTP disabling. Endpoint verifies there is a valid reauthentication token, the user id within matches the current session, and the timestamp is not too old. If those pass, it then finally disables TOTP for the user id.
• User is redirected to the account management page.

feedback welcome!

*the question of whether to require 2FA during the process of disabling 2FA is an open one, and is in many ways a UX question. If a user is disabling 2FA because they lost their device, we may not want to require a code. Requiring this really only seems necessary if we anticipate a threat model where an adversary has access to an active session and knows the user's password.

[woodruffw]

Thanks @Mossberg! I'm going to start merging your changes in https://github.com/trailofbits/warehouse/tree/tob/otp-model-refactor into this branch 😄

[woodruffw]

NTS: 3ca936d will probably break a few of the services unit tests due to changes in the way we access the TOTP secret and provisioning status.

[woodruffw]

Summarizing the discussion @ewdurbin and I had about requiring 2FA to disable 2FA:

• We don't want the user to use the second factor that they're trying to remove, since it's common enough for people to lose TOTP applications and hardware tokens. We'd actually be decreasing the security of an account by refusing to let a user remove a second factor that they know is lost/compromised.

• If a user has multiple second factors, we could choose one of the ones not selected for removal for confirmation. This has a (slight) security benefit, in that it requires the user to prove that they have the authority to log in fully before they can strictly decrease the security of the account. A quick survey of websites shows that most don't do this, since they treat a fully authenticated user as knowing what's best for their 2FA settings.

• Regardless of the above, we should ensure that the user must affirmatively request that a second factor be removed. GitHub's current MFA UI/UX, while otherwise excellent, is a perfect example of how we don't want to do this: they allow the user to click a single red trashcan button to remove a second factor.

[woodruffw]

Replaced the dual state (totp_secret + totp_provisioned) with a combination of a short-expiry signed cookie and just a totp_secret column. Extant unit tests should be passing (and should be simplified in many places), although I'll need to add a few more to handle new cases.

[woodruffw]

Ping @ewdurbin @di @dstufft -- I think this is ready for your review!

[webknjaz]

It looks like it's better to have Google Play Store links not include a language query param.

[woodruffw]

Thanks @webknjaz!

[di]

This LGTM. I've outlined a timeline for merging/releasing this here: #5661 (comment). Barring any additional requested changes, I'll plan on merging this on or before this Friday (5/3).

[nlhkabu]

Looks good to me - thank you for your work on this @woodruffw. There are a couple of UI changes I'd like to propose, but I think it will be much, much easier to make a separate PR for these later :)

[pradyunsg]

Yay! Thanks to everyone who's worked on this!

[woodruffw]

#5795 is the successor to this PR.

[merwok]

Unless HTML5 changed this, ID values should not start with a number.

[brainwane]

Yup, fixed in #6110.

[merwok]

Ha nice! Of course, I didn’t find a clear spec before I posted my message, and soon after I posted I found out that requirements were relaxed to «at least 1 char, no whitespace».