Closed
Bug 666237
Opened 13 years ago
Closed 12 years ago
cache authentication for n seconds
Categories
(Cloud Services :: Server: Core, defect)
Tracking
(Not tracked)
RESOLVED
WONTFIX
People
(Reporter: tarek, Assigned: ygjb)
References
Details
(Whiteboard: [needs loadtest][qa+])
Attachments
(1 file)
3.69 KB,
patch
|
Details | Diff | Splinter Review |
In order to reduce the ldap load, we're adding a cache for the authentication that will last for 60 seconds. This is dramatically reduce the number of calls made on the ldap server, in particular on the initial sync. Also, a password change API will purge it. => needs a security audit
Reporter | ||
Comment 2•13 years ago
|
||
So the plan is to store a hash of (user name + password) into memcache as a key, and its id as the value. the authentication function in our ldap backend becomes roughly: def authenticate(user, pass): id = memcache,get(hash(user+pass)) if id is found: return id id = get_id(user+pass) # that authenticates in ldap. it's the original code memcache.set(hash(user+pass), id, time=60 seconds) return id Specifics: - the cache is shared by all webheads (because of the round robin load balancing) so probably membased - the cache is purged when the password is changed via an API - the hash is a sha1 (seems to be a good compromise speed/security) Assumptions: The benefit expected is to switch to memcached for most authentication calls and thus avoiding ldap round trips and overload as it's easier for our infra to scale memcached (and memcached roundtrip are faster and won't require an auth) -- but we need to load test this !
Updated•13 years ago
|
Assignee: nobody → yboily
Assignee | ||
Comment 3•13 years ago
|
||
The biggest concern I would have here is ensuring that there is no simple mechanism to determine the usernames and passwords. Using a basic hashing algorithm is a good start, but I would suggest using an HMAC using a key that is shared across all of the webheads. def authenticate(user, pass): hash = hmac.new(secretserverkey, user+pass, hashlib.sha1).hexdigest() id = memcache.get(hash) if id is found: return id id = get_id(user+pass) # that authenticates in ldap. it's the original code memcache.set(hash, id, time=60 seconds) return id
Comment 4•13 years ago
|
||
If we're going to 60-second timeouts, purging the memcache on a password change is probably not worth the effort. As long as we hash the originating IP, they'll simply have a false positive from their IP for 60 seconds. This would actually be kind of compelling with a 25-hour timeout, but then we would need to work out a purging process.
Assignee | ||
Comment 5•13 years ago
|
||
Has this been implemented? Is it still being planned?
Reporter | ||
Comment 6•13 years ago
|
||
This is an experimental implementation to try it out on our loadtest environment. It will cache the auth using a local memcached at 127.0.0.1:11211 We should see a better qps with this patch, since it'll skip some ldap hits as long as our load tests are reusing the same user ids of course. If it's the case, I'll take the patch to phase 2 (tests + configurable + skippable)
Attachment #548720 -
Flags: feedback?(petef)
Reporter | ||
Comment 7•13 years ago
|
||
Pete, can we work on this this week ? the patch is getting old and deprecated a bit
Reporter | ||
Updated•13 years ago
|
Whiteboard: [pending secreview] → [pending secreview][needs loadtest]
Comment 8•13 years ago
|
||
Since we now have support for repoze.who in server-core from Bug 692709, this looks like an ideal pieces of functionality to factor out into a stand-alone plugin. I've taken the ideas from Tarek's code and made it into a "memcached" plugin here: https://github.com/mozilla-services/repoze.who.plugins.memcached If this seems like a good way to go, I can work up a configuration file that will load this for testing purposes.
Updated•12 years ago
|
Whiteboard: [pending secreview][needs loadtest] → [pending secreview][needs loadtest][qa+]
Comment 9•12 years ago
|
||
we probably don't need this, since browserid support is coming, right?
Comment 10•12 years ago
|
||
Agree with pete. Good idea at the time, supplanted by other forces.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Updated•12 years ago
|
Keywords: sec-review-needed
Updated•12 years ago
|
Keywords: sec-review-needed
Whiteboard: [pending secreview][needs loadtest][qa+] → [needs loadtest][qa+]
Reporter | ||
Updated•6 years ago
|
Attachment #548720 -
Flags: feedback?(petef)
You need to log in
before you can comment on or make changes to this bug.
Description
•