Authentication caching for passwords
ClosedPublic
Actions

Authored by mollekopf on Mon, Jan 27, 9:35 AM.

Details

Reviewers

machniak

Group Reviewers

Restricted Project

Commits

rK31bfd99f93c4: Authentication caching for passwords

Summary

This gets the time for authentication from 160ms to 0.5ms.

If somebody would get a hold of the cacheId,
a dictionary attack on the password becomes feasible (because the hash function is fast).
We only store this value on the internal redis instance, so the cacheId
could only be accessed if there already is breach, and the alternative would be to lower the bcrypt rounds to 6, which results
in 4ms, but that affects data at rest.

In the current implementation we incur one slow request every 60s,
and thus would also discover e.g. a password change after 60s.

Diff Detail

Repository

rK kolab

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mollekopf requested review of this revision.Mon, Jan 27, 9:35 AM

mollekopf created this revision.

Harbormaster completed remote builds in B52634: Diff 14596.Mon, Jan 27, 9:35 AM

mollekopf added a reviewer: Restricted Project.Mon, Jan 27, 9:36 AM

When $verifyMFA=true you still call hash() and store it in the cache. When using $verifyMFA=false that hash would be used. If you add to this not using $clientIP it looks to me like a potential geo-lock bypass.

BTW, the name of this argument probably should be just $with_checks as it enables both MFA and location check.

src/tests/Feature/UserTest.php
1674	The reference sign for $user is not needed.
1677	Please, remove print()s.

This revision now requires changes to proceed.Mon, Jan 27, 10:27 AM

Addressed comments

Harbormaster completed remote builds in B52636: Diff 14598.Mon, Jan 27, 10:50 AM

In D5090#64308, @machniak wrote:

When $verifyMFA=true you still call hash() and store it in the cache. When using $verifyMFA=false that hash would be used. If you add to this not using $clientIP it looks to me like a potential geo-lock bypass.

I don't think it's a bypass because you can't choose from the client side whether or not to apply mfa/geo-lock in checks. But we don't have geo-lock in checks for e.g. plain imap at the moment, that's correct.
I always inserted the cache entry because username + password are verified, therefore we cache that. The fact that we don't have a geo-lock in check implemented is orthogonal to this (we're not caching geo-lock in, we're caching username + password)
In any case, I now changed this to only add the cache entry in the $withChecks case, to make this more explicit.