Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign up2FA (TOTP) support #5567
2FA (TOTP) support #5567
Conversation
fc478c7
to
543376a
This comment has been minimized.
This comment has been minimized.
|
@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:
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. |
| @@ -80,6 +81,9 @@ class User(SitemapMixin, db.Model): | |||
| nullable=True, | |||
| ) | |||
|
|
|||
| totp_secret = Column(Binary(length=20), nullable=True) | |||
| totp_provisioned = Column(Boolean, nullable=False, server_default=sql.false()) | |||
This comment has been minimized.
This comment has been minimized.
woodruffw
Mar 21, 2019
Author
Collaborator
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_secretas a cookie or hidden form parameter inmanage.account.totp-secret, but this would allow the user/page javascript to modify the TOTP secret and potentially compromise it. - We could regenerate
totp_secreteach time the user refreshesmanage.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)?
This comment has been minimized.
This comment has been minimized.
ewdurbin
Mar 21, 2019
Member
@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.
This comment has been minimized.
This comment has been minimized.
offlinemark
Mar 27, 2019
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.
|
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. |
This comment has been minimized.
This comment has been minimized.
Yeah, I think so. Looks like the current |
| totp_uri = self.user_service.totp_provisioning_uri(self.request.user.id) | ||
|
|
||
| if form.validate(): | ||
| totp_value = form.totp_value.data.encode("ascii") |
This comment has been minimized.
This comment has been minimized.
offlinemark
Mar 22, 2019
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?
This comment has been minimized.
This comment has been minimized.
woodruffw
Mar 22, 2019
Author
Collaborator
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}/.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
offlinemark
commented
Mar 28, 2019
|
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:
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. |
This comment has been minimized.
This comment has been minimized.
|
Thanks @Mossberg! I'm going to start merging your changes in https://github.com/trailofbits/warehouse/tree/tob/otp-model-refactor into this branch |
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
Summarizing the discussion @ewdurbin and I had about requiring 2FA to disable 2FA:
|
This comment has been minimized.
This comment has been minimized.
|
Replaced the dual state ( |
This comment has been minimized.
This comment has been minimized.
|
It looks like it's better to have Google Play Store links not include a language query param. |
This comment has been minimized.
This comment has been minimized.
|
Thanks @webknjaz! |
87dcb16
to
8bb88ed
|
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). |
This comment has been minimized.
This comment has been minimized.
|
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 :) |
This comment has been minimized.
This comment has been minimized.
|
Yay! Thanks to everyone who's worked on this! |
This comment has been minimized.
This comment has been minimized.
|
#5795 is the successor to this PR. |
| <p>If you receive an error message saying that "This password appears in a breach or has been compromised and cannot be used", you should change it all other places that you use it as soon as possible.</p> | ||
| <p>If you have received this error while attempting to log in or upload to PyPI, then your password has been reset and you cannot log in to PyPI until you <a href="{{ request.route_url('accounts.request-password-reset') }}">reset your password</a>.</p> | ||
|
|
||
| <h3 id="2fa">{{ twoFA() }}</h3> |
This comment has been minimized.
This comment has been minimized.
merwok
Jul 23, 2019
Contributor
Unless HTML5 changed this, ID values should not start with a number.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
merwok
Jul 23, 2019
Contributor
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».



woodruffw commentedMar 13, 2019
See #4373
Issue #996
CC: @nlhkabu