[WIP] #996 Add two-factor authentication#4373
Conversation
|
Thanks for the PR! I think we'll probably want to hold off on merging this until we have the rest of #996 implemented as well. |
|
Printscreen of the verification page http://prntscr.com/kc4x8e |
|
@asfaltboy Could you please edit the title of the PR? This one is a little bit confusing.. Maybe to |
|
Good point @Sparkycz done that! |
|
I think we should add one column more into the table AccountUser to indicate that the user has enabled two-factor auth. Something like.. However, i'm still working on the login presenters.. |
|
@Sparkycz I'd like to make a number of clean-ups, commit squashes, etc, which basically involves rewriting branch history. Please notify me when it's an appropriate moment to do so, so that it won't conflict with your work on presenters. |
63f1781 to
255f9a3
Compare
|
Updated the branch as follows:
@nlhkabu suggested teaming up with others involved in #996 to avoid putting effort into duplicate artifacts. |
|
@Sparkycz please address unittest & linting issues. |
|
Could someone add the EuroPython label on this? :) |
|
Those who had already applied DB migrations, before pulling the updated branch, make sure to downgrade first: |
|
FWIW, we do |
dstufft
left a comment
There was a problem hiding this comment.
Make sure that Not only do we guard the web based login, but we also need to figure out a strategy for requiring 2fa through basic auth as well. The easiest is probably going to be that whenever a user has 2fa, they have to send a password like <password>:<otp>. Alternatively we could accept a header like Warehouse-OTP, but that would require changes in twine.
| consider using os.urandom(); this, however, is likely to cause | ||
| compatibility issues unless base32 vocabulary is used. | ||
| """ | ||
| return pyotp.random_base32(length=32) |
There was a problem hiding this comment.
Looking at this function, it's not actually being base32 encoded, it's just generating a 16 length random string which just happens to also be valid base32. If this just needs a be a base32 encoded string, then I'd prefer to do something like:
import base64
import os
def generate_totp_secret():
return base64.b32encode(os.urandom(32)).decode("ascii")This matches how we've done things in warehouse/utils/crypto.py.
| """ | ||
| Verifies a given TOTP-secret and value. | ||
|
|
||
| The *valid_window* argument value is intentionally chosen to be non-zero |
There was a problem hiding this comment.
Can we add a note here as to what exactly the valid window translate into? Is it an extra second? 6?, 30?
255f9a3 to
657c865
Compare
The presenter does not do anything. Fixes: pypa#996
657c865 to
ac18ee7
Compare
|
I amended fix of the routes test into the commit with template. However there is still output from Meanwhile i'm finishing first version of the presenters (1-2 unit-tests left) despite We'll have to discuss the behavior of sessions etc.. |
|
i guess you'll have a lot of requests for changes but somewhere we've gotta start ;-) todo:
|
Fixes: pypa#996
787d835 to
b87ab39
Compare
|
I've restarted work on this in master...trailofbits:tob/996-add-2fa-support. Will open a PR shortly so that others can review progress in real time. |
|
Closing in favor of #5567, but thank you for your contribution! |
EuroPython 2018 collaborators working on #996