feat(auth): cache Argon2 hash calculations - #793
Conversation
rhafer
left a comment
rhafer
left a comment
There was a problem hiding this comment.
Thanks! This should help a lot to improve the auth performance for apptokens.
Unless I am overlooking something I don't think the use of hmac instead of plain sha256 provides any benefits. But please correct me if I am wrong here.
Find my other comments inline.
| generator: generator, | ||
| uTimeUpdateInterval: uTimeUpdateInterval, | ||
| authCache: expirable.NewLRU[string, *apppb.AppPassword](0, nil, defaultCacheTTL), | ||
| authCacheSecret: []byte(sharedconf.GetJWTSecret("")), |
There was a problem hiding this comment.
Why are we picking the JWTSecret here as the key for hmac? If we really need to use hmac at all (I have doubts, see below), I'd suggest to just generate a random key at initialization, after all this is just an in memory cache.
| } | ||
|
|
||
| func (m *manager) createAuthCacheKey(userID, secret string) string { | ||
| mac := hmac.New(sha256.New, m.authCacheSecret) |
There was a problem hiding this comment.
What is the benefit of using hmac here? The cache is in-memory only. If an attacker is able to obtain a memory dump, that dump not only contains the cache but almost certainly also the secret. Or am I missing something?
There was a problem hiding this comment.
I was thinking initially just about sha256, the reason for hmac was just to have a more complex encoding. But I believe you are right here, are we ok to switch to sha256 or you have a better suggestion what can we use here?
|
|
||
| cached := proto.Clone(pw).(*apppb.AppPassword) | ||
| cached.Password = id | ||
| m.authCache.Add(cacheKey, cached) |
There was a problem hiding this comment.
This executes inside the store.Update callback, before the metadata update is confirmed. If the subsequent upload fails or retries after an etag conflict caused by another replica revoking the token, GetAppPassword can return NotFound while the stale credential remains usable from cache for up to 60 seconds.
It's better to move the cache insertion outside the callback and perform it only after store.Update succeeded.
| return hex.EncodeToString(mac.Sum(nil)) | ||
| } | ||
|
|
||
| func (m *manager) removeFromAuthCache(userID, secretOrId string) { |
There was a problem hiding this comment.
IMO it would help to have some comment here about what this is doing and why secretOrId can be a secret or id. It's a bit confusing.
|
|
||
| func (m *manager) createAuthCacheKey(userID, secret string) string { | ||
| mac := hmac.New(sha256.New, m.authCacheSecret) | ||
| _, _ = mac.Write([]byte(userID + ":" + secret)) |
There was a problem hiding this comment.
BTW, I'd suggest to not use : as the delimiter here but something unambigous e.g. a 0 byte:
_, _ = x.Write([]byte(userID))
_, _ = x.Write([]byte{0})
_, _ = x.Write([]byte(secret))
rhafer
left a comment
rhafer
left a comment
There was a problem hiding this comment.
Looks good to me, apart from the type which make the linter fail in CI.
2 participants
Introduced cache for in app tokens, this will prevent regeneration of argon2 token on every auth verification which is time and resource consuming
closes #opencloud-eu/opencloud#3157