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

2FA: "trust this device for 30 days" option #5867

Closed
brainwane opened this issue May 16, 2019 · 5 comments · Fixed by #14194
Closed

2FA: "trust this device for 30 days" option #5867

brainwane opened this issue May 16, 2019 · 5 comments · Fixed by #14194
Labels
2FA feature request help needed We'd love volunteers to advise on or help fix/implement this. UX/UI design, user experience, user interface

Comments

@brainwane
Copy link
Contributor

What's the problem this feature will solve?
The multi-factor auth makes logging in a little less convenient, and the fact of a previous login to a user's account from the same device makes it more likely that the user auth is legit.

Describe the solution you'd like
Much as other 2FA implementations do, we could add a "trust this device for 30 days" (or some similar period) checkbox during the 2FA check (during login).

Additional context
Hat-tip to @ericholscher for asking for this ("don't bother me on this computer for 30 days") during the PyCon NA 2019 sprints.

@brainwane brainwane added feature request needs UX/UI review needs discussion a product management/policy issue maintainers and users should discuss labels May 16, 2019
@ericholscher
Copy link
Contributor

Thanks for writing this up @brainwane! 💯 🎉

@nlhkabu nlhkabu added UX/UI design, user experience, user interface and removed UX/UI design, user experience, user interface needs UX/UI review labels May 20, 2019
@trishankatdatadog
Copy link
Contributor

@brainwane This is a great idea, I don't see any issue from a security point of view, many companies do it (including Amazon), so the major security issues will really be on the server-side, on how we handle sessions in Warehouse, but that's it!

@brainwane brainwane added help needed We'd love volunteers to advise on or help fix/implement this. and removed needs discussion a product management/policy issue maintainers and users should discuss labels Jun 8, 2019
@di
Copy link
Member

di commented Jul 9, 2020

Some additional discussion is in #8033

@callmecampos
Copy link
Contributor

There are a few ways to ensure persistence of session information, and they mostly depend on whether we care about the longevity of these sessions enough that we need to store them outside Redis.

If we don't care too much about the longevity of these sessions, all we need to do is extend the lifetime of the session cookie if the user logs in and selects the "Keep me signed in" checkbox (which can just be added to the login.html template). Currently session information is stored in Redis for max 12 hour sessions (see warehouse/sessions.py).

If we want to ensure data persistence no matter what, it seems we'd have to use Postgres, since I don't think we can (or even want to) configure Redis to have data persistence. One way would be to just store relevant session information in a Postgres database in addition to Redis, and if Redis doesn't have the session information (whether Redis lost the data or if the session actually expired), we can double check in Postgres. However, this seems redundant and I imagine there's a better way to correlate session information with Redis with information in Postgres.

@ewdurbin wondering if you have an opinion on how to best go about this?

@ristomcgehee
Copy link
Contributor

I think I'd like to work on this. Here's the approach I would take. Feedback is welcome.

I would say that it would not be worth doing device fingerprinting. If an attacker can steal files off your machine, they can likely simply submit malicious requests from your device. Fingerprinting would protect against an attacker accessing a backup of your machine, and it would also prevent an attacker from stealing the session cookie and selling it to another party. It appears that GitHub does not fingerprint a device in order to allow a session cookie. I took the cookies from a Linux Chrome browser, entered them into a Windows Edge browser on a different physical machine, and GitHub logged me in. It's possible that GitHub is looking at the IP address to determine whether to honor the session cookie, but that seems unlikely since IP addresses can change for many reasons. Maybe down the road, it would be worth adding device fingerprinting to PyPI, but I don't think the security benefit outweighs the effort at this time.

In terms of implementation details, I would store saved devices in Postgres. When the user enters the 2nd authentication factor and selects "Remember this device for 30 days", a row will be entered in a new table (I'll call it user_devices for now). This table will store the userid, the creation date, and a device id, which will be a randomly generated string. The device id will be set as a cookie on the request.

When a user logs in, after they enter their password, if the request has a device id cookie, the user_devices table will be queried to see if there is a device that matches that device id and user id and is less than 30 days old. If so, the user will be logged in; if not, they will be prompted for 2FA.

A user would still have to login every 12 hours (or if they explicitly logged out), but they would only have to enter their password, and not the 2nd factor.

We could create a scheduled task to remove expired rows from user_devices, but that doesn't have to be done right away.

A potential future enhancement could be to also store the browser's user agent string and IP address and display in the web UI the current saved devices. The user would have a button to revoke devices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2FA feature request help needed We'd love volunteers to advise on or help fix/implement this. UX/UI design, user experience, user interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants