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

Optional 'remember MFA' for browser #203

Merged
merged 5 commits into from
May 24, 2020

Conversation

SibrenVasse
Copy link
Contributor

1. Use MFA cookie to whitelist browser for MFA for 30 days.
In this PR only for TOTP. Maybe also add this for FIDO?
2. Prevent OTP replay attacks by invalidating last token
See security checklist on https://pyotp.readthedocs.io/en/latest/

Deny replay attacks by rejecting one-time passwords that have been used by the client
(this requires storing the most recently authenticated timestamp, OTP,
or hash of the OTP in your database, and rejecting the OTP when a match is seen)

3. Add autofocus to email field on login screen
4. Remove cookies from browser on logout
5. Fix flash() calls in mfa.py

@SibrenVasse
Copy link
Contributor Author

Also a thing from the checklist is rate limiting the authentication endpoints.
This can be done very easily with https://flask-limiter.readthedocs.io/en/stable/#
What do you think?

Copy link
Contributor

@nguyenkims nguyenkims left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I tested it locally (on localhost) but somehow the mfa cookie doesn' seem to be saved and the feature doesn't work correctly: it always ask me to enter the OTP code even if I have ticked "Remember this browser for 30 days".

It's also good to support FIDO as well. There's a pending PR on FIDO that would be merged soon, maybe you can base this PR on this one to avoid conflicts.

I left some comments on the PR.

app/auth/views/logout.py Show resolved Hide resolved
app/auth/views/mfa.py Show resolved Hide resolved
app/auth/views/mfa.py Outdated Show resolved Hide resolved
app/auth/views/mfa.py Outdated Show resolved Hide resolved
app/auth/views/mfa.py Show resolved Hide resolved
app/auth/views/mfa.py Show resolved Hide resolved
app/models.py Outdated Show resolved Hide resolved
app/models.py Outdated Show resolved Hide resolved
@SibrenVasse
Copy link
Contributor Author

Are you sure you're testing it right? If you use the logout button, the 'mfa' cookie is also deleted. (A user action logout should in remove all user cookies)
Easiest way to test is to manually remove the 'slapp' cookie in the browser dev tools and login again.

@nguyenkims
Copy link
Contributor

Are you sure you're testing it right? If you use the logout button, the 'mfa' cookie is also deleted. (A user action logout should in remove all user cookies)
Easiest way to test is to manually remove the 'slapp' cookie in the browser dev tools and login again.

Here's how I tested the feature locally:

  • setup TOTP
  • log out, login again, make sure to tick the "remember for 30 days"
  • logout, login again: I'm still asked for TOTP token. When "remember for 30 days" is ticked, I should not be asked again right?

@SibrenVasse
Copy link
Contributor Author

Yeah so in logout.py:L13 the mfa cookie is deleted. So if you use the 'logout' button to exit the session, it won't work. Only if the session expires (i.e. the cookie slapp is no longer valid) and the user then goes to login, it does work.

So in short:

  1. Setup TOPT
  2. log out, login again, make sure to tick the "remember for 30 days"
  3. close browser
  4. login

Or we can just remove logout.py:L13 and it will always work.

@nguyenkims
Copy link
Contributor

@SibrenVasse I see! I also understand now why you need to use the cookie directly. The PR is good for me, I will merge it when we merge the multiple FIDO keys PR.

@SibrenVasse
Copy link
Contributor Author

I'll also add the feature to the FIDO path.
Shall I take a look into the rate limiting?

@nguyenkims
Copy link
Contributor

I'll also add the feature to the FIDO path.
Shall I take a look into the rate limiting?

Great, thanks! I think the rate-limiting can be in a separate PR.

@SibrenVasse
Copy link
Contributor Author

I'll also add the feature to the FIDO path.
Shall I take a look into the rate limiting?

Great, thanks! I think the rate-limiting can be in a separate PR.

Yeah I'll do that.

@nguyenkims nguyenkims merged commit 93b7ff3 into simple-login:master May 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants