Ensure TOTP codes cannot be reused. #2908
Conversation
| err = b.usedCodes.Add(usedName, nil, time.Duration( | ||
| int64(time.Second)* | ||
| int64(key.Period)* | ||
| int64((2+key.Skew)))) |
There was a problem hiding this comment.
Shouldn't we add the skew and not multiple it?
There was a problem hiding this comment.
How about (time.Second * key.Period) + 2 * key.Skew? Multiplying the skew but adding the multiplied number.
There was a problem hiding this comment.
Skew isn't seconds, it's periods. So that will give you 2 extra nanoseconds.
There was a problem hiding this comment.
Then the equation should be time.Second * (key.Period + 2 * key.Skew). Otherwise, the value will bump up multifold.
There was a problem hiding this comment.
key.Period is not a duration, it's an integer number of seconds. If the period is 30 seconds and the skew is 1 period, that equation will give you time.Second * 32. What we want is time.Second * 90.
There was a problem hiding this comment.
Ah, okay. Skew was something different in my mind. Looks good.
builtin/logical/totp/backend.go
Outdated
| @@ -25,11 +27,15 @@ func Backend(conf *logical.BackendConfig) *backend { | |||
| Secrets: []*framework.Secret{}, | |||
| } | |||
|
|
|||
| b.usedCodes = cache.New(0, 1*time.Second) | |||
There was a problem hiding this comment.
Can we add comments here about the expiration and cleanup durations?
There was a problem hiding this comment.
I also wonder if we can afford to wait a little longer to look for expired items instead of doing it every second.
There was a problem hiding this comment.
Sure, I can make it be longer.
Fixes #2775