in memory cache verify password implementation

[eregnier]

As bcrypt algorithm computations are expensives and long, I implemented a simple cache system allowing to bypass token validation on each requests when the same validation is already in cache.

The result is a combination of the token and the user passwords. Documentation in the code describes more in detail the implementation.

It's usage is optionnal when the USE_VERIFY_PASSWORD_CACHE is set.

I would like some maintainer of the repository validate the code before I go further by adding options in documentation and implementing unit tests. I would be pleased to do this if this current commit is accepted.

[eregnier]

It look that the CI errors does not come from my PR

[jwag956]

Couple things: 1) why not use a standard cache package such as: https://cachetools.readthedocs.io/en/latest/ 2) while the TTL is settable - 1 day seems like a really bad idea - it would seem that mostly, API requests come in rapid succession - so that is the important thing to fix - so a TTL of say 5 minutes would probably suffice. 3) I assume that you feel bcrypt is slow and you have the C version (passlib)? 4) could you make sure there is an exposed method to clear the entire cache?

[eregnier]

Hi jwag956, Thank you for your advices.

1. I did not want to add more dependencies to the project when required code is about 20 loc or so (but I remain open to do it if needed).

2. I will publish a revision where default ttl is 5 minutes. As there is an option to set the custom desired TTL there is no problems for me. What's more you're right, on most api clients, a user session is not as long as 5 minutes, so this default TTL will be enough.

3. Yes I assume (by observations) that bcrypt is slow with the version that flask-security brings/uses (maybe this is flask-login that fetches it) . I don't know exactly which version I have, if this is implemented with python or C, but I guess the flask layers for security uses the C version as it surely more efficient. What is sure is that whatever the future hashing algorithm library is used, this cache system will make it a lot faster for all n+1 calls within the TTL duration.

I read (not deeply) around the web, that the slow computation of bcrypt is something good. Maybe it is to prevent brute force or another reason I don't know yet, but in our case, the check needs to be preformed once only (every TTL) as each next call is only an exact recomputation of what just have been computed. Let me know if I am wrong in any way.

4. Same a point 2. I will publish a revision including such method.

[eregnier]

I will try to do an implementation using LRU cache, I am interested in writing less code for the same thing :)

[jwag956]

Thanks - I did some tests as well as yes- bcrypt/verify_password is really slow. The only place we need to worry about brute force is for actual authentication i.e. when the user has supplied a plaintext password. For normal APIs where a token is passed looking at https://pythonhosted.org/itsdangerous/ I think that is pretty immune to brute-force attacks (assuming you have a good SECRET_KEY)

I did observe that newer versions of itsdangerous provide for signing of the tokens - much like oauth2 - which could mean we just have to verify correct-signed token rather than token contents - which would be fast. But that is for another day.

As with anything security related - always good to get a few sets of eyes on the changes.

[eregnier]

I just tried to do it using https://github.com/arnov/lru-ttl/blob/master/lruttl/lru.py. This library just not work (or I have a problem)

The main problem to deal with in this problem is to clear cache while flask-security has no schedule system. The implementation I done takes care of cleaning values in cache each ttl duration (on a call to the cache system -> when auth stack is called).

As there is no specific daemon setup / thread doing the clean, just feeding a cache , even with cache library requires some clean at one moment. There is no magic and LRUCache or cachetools will need some code to do the clean. So I think there is not too much code in my implementation in the end. Maybe am I missing something about caching ?

[eregnier]

So, what about merging ?

[jwag956]

I was thinking that you would use cachetools:TTLCache as described here: https://cachetools.readthedocs.io/en/latest/

[jwag956]

Just to be clear - I am not trying to cause a huge amount more work, and I am not currently a maintainer/approver - just someone trying to help. I certainly wouldn't suggest you adding 100 lines of code in order to not write 40 lines of cache code - however in general, I usually encourage other well supported packages rather than roll-my-own since they will have thought about many of the edge cases (size, performance) that I might not.

I do think what you are trying to do is useful and clearly this has been an issue for many folks.

[eregnier]

I appreciate your help and your advices jwag956 :)

[eregnier]

I refactored the cache class code using cachetools. This is a cool library I didn't know. It works like charm now and has very fewer code. Thank you jwag956 !

[tyacbovi]

Hi, I have a small suggestion, why not also cache failed attempts? maybe on a separate cache, so the "good" users could still get verified quicker in case of a lot of bad attempts

[eregnier]

@tyacbovi I am not sure tu understand your proposal. If a user check attempt fails, so it is not authenticated because of a wrong token given in the query headers. Caching such result seems useless to me and can only lead to fill memory with a cache instance that holds meaningless information. Am I missing something about your proposal ?

[tyacbovi]

If that "bad" user will keep on attempting to gain access, it will waste resources and time by running verify_hash many times so if that result was cached it could be partially mitigated

[eregnier]

I think all is ok now.

[eregnier]

@tyacbovi Ok , I understand your point now, but I still think this is not usefull to do this as the primary goal of this implementation is to make faster subsequent calls to authenticated flask routes when the first one is ok and set the cache accordingly.

Functionally, a user that attempts a bad authentication call is not expected and looping over a bad authentication that lead to 401 response is not important to me to be fast as the logic of authentication is to try to send other credentials when one fail if the user really wants to be authenticated.

If a user loops authentication it means for me that the authentication process client side is wrong or deliberately trying many combination for a bruteforce attack or so. And in this case, I DO want the check passord checking to be slow in order to prevent the attacker to perform a lot of queries quickly. Moreover, by bruteforcing it won't be faster using cache but it will only fill the cache with a lot of (in my point of vue) useless information. I also think that handling a bruteforce attack may not be done at application level, or at least this cache mechanisme is not the appropriate pattern for this.

Is there any case I miss here ?

[tyacbovi]

No, I pretty much agree with your points and completely agree that bruteforce protection is not supposed to be done in the application level, thanks for the detailed response. Can not wait for this to be merged and released :)

[eregnier]

Hey maintainers, the feature is ok now, but CI crashes because of some other issues. It would be cool to merge it when possible :+1:

[jwag956]

Need some unit tests... please! you don't need to test if cachetools works - but that in fact you properly call it to initialize and properly handle positive and negative cases... mocks probably needed!

[eregnier]

Ok, I did unit tests few days ago, but test folder is gitignored, So I missed to add my test file to the commit. This is now done.

[jwag956]

Being overly pedantic here - but could you add a couple tests that verify the code in core.py::_request_loader that verifies if new config variable that appropriate calls to new caching module are made, etc? Thanks!

[eregnier]

I lack time these days. Next week I should be able to do this :) .

[jwag956]

Thanks.

I have a question for you - why are you not using Flask Sessions (which are built on Werkzueg sessions) - which are secure cookies. If you do - there is no extra bcrypt step - that only happens when using token authentication. Could you explain your basic architecture a bit?

[eregnier]

Hey @jwag956 !

I will try a short answer as I am not sure to understand all of your question. Why not using Flask Sessions : Why not. I did not thought about this with my implementation. I just wanted something simple that works at first glance, then I met you and now the work is way better :) . (and I don't know much about pros and cons of Flask Sessions instead of my solution)

However, I am not sure to understand why / how Werkzeug sessions would improve my commit. More precisely, I don't mind what "there is no extra bcrypt step" means in this context.

The architecture I implemented was first to solve the issue of long time computation of password verification for flask routes that are protected with the flask-security token validation system. I just tried to do something quickly and acceptable enought as I have not much time these days, I (really) want my work to be merged asap, and the project looks not much active. So this is not very engaging to possibly spend a lot of time for some lost code.

Now, you takes care of the quality of my work, this is cool as I am rarely (qualitatively) mentored, so beside trying to make my work merged, I feel some sort of community entertainement ;) I share you this point of view because the way I contribute to open source project is often like I descibed above. This is time consuming, and in case my work looks interesting the community expects very soon a sort of very (very) high quality for the code I share.

This is a good thing for projects I think, but this is really hard for me and surely for many other that want to contribute and may feel not strong enought (this is a feeling I have).

Well, enough philosophy for now. Please keep in mind, my initial goal was simply to fix a performance issue with the simplest code I could produce (with my current competences).

Finally, this was not a quick answer.

[eregnier]

And for the extra tests that would cover the whole feature, I will try to find some time as soon as possible to enhence my commit.

[jwag956]

Thanks for your comments. I agree that with all the configuration variables and possible ways to use flask-security there isn't a lot of documentation giving 'options'.

Ok - let me see if I can describe sessions (which are an integral part of Flask and the underlying Werkzeug). If you are accessing your application from a UI then the best and most secure thing is to use cookies which define a session. From what I can tell - this is the default and you should see a cookie being returned when you log in through flask-security - that cookie is called 'session' by default. Your browser of course will send that cookie on every request to your backend, and if you guard your API endpoints with either @login_required or @auth_required('token', 'session') then Flask will take the cookie and valid the session and no expensive passwd hashing will be done. Thus if you use sessions (which I do in my app) then there is no performance issue and so your commit while useful may not actually be needed by you!

This is much more secure than having your UI store the authentication_token and send it on every ajax request.

Now - your situation might be completely different and this might not apply - that's why I was asking the question.

I am not trying to dissuade you from continuing your work on the cache - just wanting to clarify your use case.

[eregnier]

Ok, you're right again. The fact is that I started develop an app using jwt for auth layer so I met the issue I try to solve there. I think for now I don't really want to reimplement the authentication layer of my whole application for now and as it has no critical security requirements. I think I just wanted to test using jwt system for this project and I guess this is not the best choice. So I don't really know what should be done.

• My commit works

• I have merly not the time to reimplement the auth layer for my app at the moment

• My commit still enhence authentication performance for flask-security for the jwt part at least.

I guess if my commit is not merged, I will use my fork for my current project for now, and think different for the next one.

Please don't be upset, all what you told me already made me grown :)

Edit : And oh yes, I may have forgot to (re)mention that my initial intention was to make it simpler to use flask-security with a SPA and where using jwt makes it a lot simpler and faster to implement both client and serverside security layer using flask-security (on my point of view), and now even more with my sub project https://github.com/eregnier/Flask_AuthOOB that is just born, maybe ready to die following your advices but almost ready to use anyway.

The initial issue I met is that I am bored to always reimplement auth mechanism for my apps using flask.

[jwag956]

So to be clear - today, Flask-Security has a real shortcoming when using tokens for auth due to the performance issue. A fix there is critical since there are many real use cases for token auth. While I think there are potentially other ways to fix this - a cache seems fine and I would be happy to merge it into my fork. For that to happen I would like to see (as I stated before) a few more unit tests for the code you added in _request_loader.

As for the larger conversation - that's just exchanging ideas and concepts to help build better applications. I too started an SPA using tokens - I then realized that flask by default is already sending a cookie and that I didn't need to do ANYTHING in my code (I was grabbing the token, storing it in web browser etc). While we all have different levels of risk/security - it's also useful I think to help steer people to current best practices (another doc I hope to write).

[eregnier]

Missing unit test are comming. I am really busy these days.

[eregnier]

Hey @jwag956 , I did the required tests. Does it looks good to you ? I hope this is fine now :) .

[jwag956]

I have cherry-picked your PR into my fork - and made a few minor cosmetic changes. All tests pass - please look at: https://github.com/jwag956/flask-security/pull/80

and I will merge it into my fork.