⚡️(api) improve performance of HTTP basic auth

[wilbrdt]

Purpose

HTTP Basic auth implementation in Ralph is using the secure and standard bcrypt algorithm to hash/salt passwords before storing them.
But this implementation comes with a performance cost, as each request has an overhead of around 200ms.

Proposal

By LRU caching the HTTP auth credentials, we should greatly decrease the overhead of the vast majority of requests.

• add LRU caching
• test with a timing middleware on endpoint /whoami

INFO:     Application startup complete.
INFO:ralph.api:TIMING: Wall:  209.6ms | CPU:  209.8ms | app.ralph.api.whoami (HTTP Basic Auth overhead)
INFO:ralph.api:TIMING: Wall:  210.0ms | CPU:  210.2ms | app.ralph.api.whoami
INFO:     172.26.0.1:35240 - "GET /whoami HTTP/1.1" 200 OK
INFO:ralph.api:TIMING: Wall:    0.5ms | CPU:    0.5ms | app.ralph.api.whoami (HTTP Basic Auth overhead)
INFO:ralph.api:TIMING: Wall:    0.7ms | CPU:    0.7ms | app.ralph.api.whoami
INFO:     172.26.0.1:43642 - "GET /whoami HTTP/1.1" 200 OK
INFO:ralph.api:TIMING: Wall:    0.4ms | CPU:    0.4ms | app.ralph.api.whoami (HTTP Basic Auth overhead)
INFO:ralph.api:TIMING: Wall:    0.5ms | CPU:    0.6ms | app.ralph.api.whoami
INFO:     172.26.0.1:43644 - "GET /whoami HTTP/1.1" 200 OK
INFO:ralph.api:TIMING: Wall:    0.5ms | CPU:    0.4ms | app.ralph.api.whoami (HTTP Basic Auth overhead)
INFO:ralph.api:TIMING: Wall:    0.7ms | CPU:    0.6ms | app.ralph.api.whoami
INFO:     172.26.0.1:43658 - "GET /whoami HTTP/1.1" 200 OK

• test on near-real conditions with LBT
  

[jmaupetit]

That's wonderful and promising, wow!

Maybe you should also add python tests ensuring we hit the cache on subsequent requests?

[Leobouloc]

Seems good to me (I agree with comments by @jmaupetit concerning the default values). One suggestion:

Maybe only cache results with valid credentials to avoid the following situation:

a user (probably an admin in this situation) tries to connect with his credentials, then realizes they are not valid and proceeds to create an account with these credentials. The user would be locked out of the account for the next hour (probably without knowing why) as the credentials were cached as invalid.

This would not lead to a timing exploit or decrease performance for users.

[wilbrdt]

Maybe only cache results with valid credentials to avoid the following situation:

a user (probably an admin in this situation) tries to connect with his credentials, then realizes they are not valid and proceeds to create an account with these credentials. The user would be locked out of the account for the next hour (probably without knowing why) as the credentials were cached as invalid.

This would not lead to a timing exploit or decrease performance for users.

For now, we are caching what's in the auth.json file, so adding/removing a user is only taken into account after a server reboot.
Moreover, if someone tries to connect with invalid credentials, an exception is raised and nothing is returned, so I don't think the invalid credentials are cached (I might be wrong though).

[wilbrdt]

Maybe you should also add python tests ensuring we hit the cache on subsequent requests?

We would either need to do a timing test, or to activate with info=True instrumentation that gives us access to cache_info(), but that would induce a performance penalty for each requests.

[SergioSim]

We would either need to do a timing test, or to activate with info=True instrumentation that gives us access to cache_info(), but that would induce a performance penalty for each requests.

Maybe we could directly call the authenticated_user method in a unit test (without starting the server) to check that a new entry is added to its cache property?

[Leobouloc]

Maybe only cache results with valid credentials to avoid the following situation:

a user (probably an admin in this situation) tries to connect with his credentials, then realizes they are not valid and proceeds to create an account with these credentials. The user would be locked out of the account for the next hour (probably without knowing why) as the credentials were cached as invalid.

This would not lead to a timing exploit or decrease performance for users.

For now, we are caching what's in the auth.json file, so adding/removing a user is only taken into account after a server reboot.

Good point concerning get_stored_credentials being cached. Is this necessary ? As long as we are using a file to store Basic Auth information, I assume we are dealing with a low number of users, so maybe we could do without, this would allow to create new users without rebooting the server.

Moreover, if someone tries to connect with invalid credentials, an exception is raised and nothing is returned, so I don't think the invalid credentials are cached (I might be wrong though).

My bad, I thought cachetools was also caching Exceptions 🙃

[wilbrdt]

Good point concerning get_stored_credentials being cached. Is this necessary ? As long as we are using a file to store Basic Auth information, I assume we are dealing with a low number of users, so maybe we could do without, this would allow to create new users without rebooting the server.

@Leobouloc We could probably remove get_stored_credentials from being cached so we can cache users without rebooting indeed.
It would be better to do that in another PR, as we would need to check if opening/closing the file for each request is fast enough, or if we would need to cache the file based on its last modified time?

[SergioSim]

This looks awesome! 👍

[Leobouloc]

Good point concerning get_stored_credentials being cached. Is this necessary ? As long as we are using a file to store Basic Auth information, I assume we are dealing with a low number of users, so maybe we could do without, this would allow to create new users without rebooting the server.

@Leobouloc We could probably remove get_stored_credentials from being cached so we can cache users without rebooting indeed. It would be better to do that in another PR, as we would need to check if opening/closing the file for each request is fast enough, or if we would need to cache the file based on its last modified time?

My guess is that as long, as we are using a single file (auth.json) to store user credentials, we are dealing with a limited amount of users, for which credentials would all fit in the cache. This means reading the file would only be done in the following cases: 1/ a new user was added 2/ a user provided wrong credentials . Both cases would be pretty infrequent, and therefore would not affect performance in a significant manner.

[Leobouloc]

Seems good to me. I left a few small suggestions.