Add a 2FA Validity Window #401

TillerBurr · 2020-10-12T21:22:01Z

It is sometimes preferable to have two factor authentication with a grace period where one does not have to revalidate the second factor. This pull request adds such a functionality by setting a cookie with a signed token confirming that the user has been authenticated within a given time frame and does not need to go through the process again. This is an OWASP recommendation for improving the overall UX (see https://cheatsheetseries.owasp.org/cheatsheets/Multifactor_Authentication_Cheat_Sheet.html)

Added 3 configuration options SECURITY_TWO_FACTOR_ALWAYS_VALIDATE, SECURITY_TWO_FACTOR_VALIDITY_SALT and SECURITY_TWO_FACTOR_LOGIN_VALIDITY. In the event of a compromise, the cookies can be invalidated by resetting a user's fs_uniquifier.

Also, tests were failing due to Black reformatting them so all files were ran through Black as well.

jwag956 · 2020-10-12T22:05:40Z

I will look at the content closely - but many of the changes were due to formatting - that shouldn't be since it always passes 'black' - so somehow there must be some configuration difference.

Could you try to figure out why? otherwise when I get it - it will reformat again...

That will reduce the PR by a bunch.....

TillerBurr · 2020-10-12T22:16:34Z

I'm currently changing a couple of things. I didn't realize some of the tests could potentially fail if two functions were called within the same second.

I don't know what the issue is. I am currently using version 20.8b1, so I don't know if they changed something between versions or not. I'll look at see if I can find what the issue is. It's odd that it's changed, since I run pre-commit run --all-files.

jwag956 · 2020-10-12T22:20:12Z

Odd - I just use pre-commit and it picks up 'stable' which is 20.8b1... The most strange thing is that is is removing a space after """?

Do you have something else in your precommit file?

TillerBurr · 2020-10-13T02:26:33Z

I think it has something to do with the way that black is formatting docstrings, via psf/black#1053.

There are several issues complaining about the way it handles docstrings since the first 20 version.

Even now, I see that it failed the black check, even though they pass locally and in the Travis ci that's built in my fork.

Here is my fork's Travis build before I changed the branch. https://travis-ci.com/github/baurt/flask-security-1/builds

Even here is shows that the latest master commit fails,which shouldn't be possible, so I think it has to be something with black.

jwag956 · 2020-10-13T15:21:10Z

Odd - but why are we getting different versions? There hasn't been a black release since august - and you can see in the travis logs that I have built master etc many times since then...
One thing I noticed - but it appears that using pre-commit -we can't see which version and from where it is picking up black - it isn't actually pip installed from what I can tell.

Ok - I removed my black cache and changed my pre-commit file to specify 20.8b1 and now I see it re-formatting. So possibly this has something to do with pre-commit caching its environment - more digging required.

TillerBurr · 2020-10-13T15:35:33Z

What I don't really understand is that it's not even returning the same number of modified files. The most recent build only would have changed 8 files, but I initially changed around 25.

I wonder if it's using some sort of cached version? I know that pre-commit builds an environment for each hook and runs the hook from that environment.
The thing that is driving me crazy is that this is the results from pre-commit run --all-files. This is the same commit that most recently failed on travis.

jwag956 · 2020-10-13T15:37:11Z

It is definitely caching things - black has a cache and so does pre-commit.
I will post a PR that attempts to fix this (by running pre-commit autoupdate prior to running pre-commit).

TillerBurr · 2020-10-13T15:48:45Z

I'll keep an eye out for that and then rebase this pr on the new version.

…fter a successful validation

jwag956 · 2020-10-13T17:50:42Z

I am going to be traveling for the next week - so review comments will likely be delayed..

Thanks for doing this.

TillerBurr · 2020-10-13T18:18:57Z

No worries. Should be easier to view now, since I rebased and squashed some commits.

Safe travels!

jwag956

unified_signin also calls tf_login - so it needs the same new logic...

I made a few comments about the remember token/cookie w.r.t. JSON. Should we support non-form/JSON front ends to be able to save the token (i.e. return the token on successfull login) and they can return it on next login attempt? or say that this feature relies on cookies?
Not sure myself which way to go.

I haven't looked carefully at the default cookie options - but this one should be http-only, probably secure, and maybe same-site?

docs/api.rst

docs/two_factor_configurations.rst

flask_security/twofactor.py

tests/test_two_factor.py

TillerBurr · 2020-10-15T21:47:03Z

Good point on the cookie options. I'll make those configurable. I can definitely see it being a pain for development if secure was always on. I can work on creating a way to pass the token via json. I'd have to think how to implement that first.

TillerBurr · 2020-10-16T02:39:57Z

unified_signin also calls tf_login - so it needs the same new logic...

I made a few comments about the remember token/cookie w.r.t. JSON. Should we support non-form/JSON front ends to be able to save the token (i.e. return the token on successfull login) and they can return it on next login attempt? or say that this feature relies on cookies?
Not sure myself which way to go.

I haven't looked carefully at the default cookie options - but this one should be http-only, probably secure, and maybe same-site?

I'll have to take a look at the unified signin. I haven't looked much into it, so I may need a little help with tests. I can create the logic, but the tests might evade me.

…Remove signal for emitting token value. If the tokens dont match, the verification wouldnt work

TillerBurr · 2020-10-16T20:00:12Z

I think I got everything. I figured out the unified signin stuff. That is impressive work.

docs/configuration.rst

flask_security/core.py

jwag956

This is looking really good - thanks for all the work.

As I noted - I think the cookie should default to httponly - since there is no reason for scripts to be able to see it.

Second - and hopefully last - please update the openapi.yml to document the new JSON return token. There are basic instructions for how to do that in CONTRIBUTING.rst.

Thanks!

TillerBurr · 2020-10-20T15:38:13Z

I'm not sure if I updated the openapi.yaml correctly. Let me know if I need to change anything.

jwag956 · 2020-10-21T01:41:19Z

Thanks for all the hard work!

TillerBurr · 2020-10-21T18:31:42Z

My pleasure. Thank you for maintaining this package.

TillerBurr force-pushed the tf_validity branch from 9f3e9f6 to 0841103 Compare October 13, 2020 17:46

Add a validity window for 2FA that allows a user to skip validation a…

0841103

…fter a successful validation

TillerBurr changed the title ~~Add a 2FA Validity Window/Blacken Files~~ Add a 2FA Validity Window Oct 13, 2020

jwag956 requested changes Oct 15, 2020

View reviewed changes

Allow JSON response to get and validate the tokens

1c93a26

Add the Validity Window cookie/token to the unified signin endpoint. …

4d11fa3

…Remove signal for emitting token value. If the tokens dont match, the verification wouldnt work

jwag956 reviewed Oct 18, 2020

View reviewed changes

docs/configuration.rst Outdated Show resolved Hide resolved

Change the cookie config to a dict instead of separate variables

e469174

jwag956 reviewed Oct 20, 2020

View reviewed changes

docs/configuration.rst Outdated Show resolved Hide resolved

jwag956 reviewed Oct 20, 2020

View reviewed changes

flask_security/core.py Outdated Show resolved Hide resolved

jwag956 requested changes Oct 20, 2020

View reviewed changes

change httponly to default to true. update openapi.yaml

8cf1ac2

jwag956 approved these changes Oct 21, 2020

View reviewed changes

jwag956 merged commit a3ee327 into pallets-eco:master Oct 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a 2FA Validity Window #401

Add a 2FA Validity Window #401

TillerBurr commented Oct 12, 2020

jwag956 commented Oct 12, 2020

TillerBurr commented Oct 12, 2020

jwag956 commented Oct 12, 2020 •

edited

Loading

TillerBurr commented Oct 13, 2020 •

edited

Loading

jwag956 commented Oct 13, 2020 •

edited

Loading

TillerBurr commented Oct 13, 2020

jwag956 commented Oct 13, 2020

TillerBurr commented Oct 13, 2020

jwag956 commented Oct 13, 2020

TillerBurr commented Oct 13, 2020

jwag956 left a comment

TillerBurr commented Oct 15, 2020

TillerBurr commented Oct 16, 2020

TillerBurr commented Oct 16, 2020

jwag956 left a comment

TillerBurr commented Oct 20, 2020

jwag956 commented Oct 21, 2020

TillerBurr commented Oct 21, 2020

Add a 2FA Validity Window #401

Add a 2FA Validity Window #401

Conversation

TillerBurr commented Oct 12, 2020

jwag956 commented Oct 12, 2020

TillerBurr commented Oct 12, 2020

jwag956 commented Oct 12, 2020 • edited Loading

TillerBurr commented Oct 13, 2020 • edited Loading

jwag956 commented Oct 13, 2020 • edited Loading

TillerBurr commented Oct 13, 2020

jwag956 commented Oct 13, 2020

TillerBurr commented Oct 13, 2020

jwag956 commented Oct 13, 2020

TillerBurr commented Oct 13, 2020

jwag956 left a comment

Choose a reason for hiding this comment

TillerBurr commented Oct 15, 2020

TillerBurr commented Oct 16, 2020

TillerBurr commented Oct 16, 2020

jwag956 left a comment

Choose a reason for hiding this comment

TillerBurr commented Oct 20, 2020

jwag956 commented Oct 21, 2020

TillerBurr commented Oct 21, 2020

jwag956 commented Oct 12, 2020 •

edited

Loading

TillerBurr commented Oct 13, 2020 •

edited

Loading

jwag956 commented Oct 13, 2020 •

edited

Loading