fix raft tls key rotation panic when rotation time in past #15156
Conversation
| // NewTicker to panic when used with time.Until, prevent this by | ||
| // pushing out rotation time into very near future | ||
| if next.Before(now) { | ||
| return now.Add(1 * time.Minute) |
There was a problem hiding this comment.
I think this is fine, though I think it's not quite what the behaviour was before, right? Prior to the breaking change, I assume a negative next would be treated as "immediately", not "now+1m"?
There was a problem hiding this comment.
That's true but with NewTicker I think all you can really do is try to be as close to "immediately" as possible. The reason for adding 1m was just to try to add enough buffer to prevent the negative NewTicker issue when using time.Until. It needs to be some time in the future in other words.
There was a problem hiding this comment.
I reduced the padding to be 10 seconds. I'll admit that the 1 minute was a bit aggressive.
vault/raft.go
Outdated
| @@ -509,7 +524,7 @@ func (c *Core) raftTLSRotatePhased(ctx context.Context, logger hclog.Logger, raf | |||
| continue | |||
| } | |||
|
|
|||
| nextRotationTime = next | |||
| nextRotationTime = getNextRotationTime(next) | |||
There was a problem hiding this comment.
Can we move the ticker.Reset to here, as well as eliminate the backoff variable and just put all the time calculation logic here? Feels like it would be safer and clearer.
There was a problem hiding this comment.
Is your thought to put all of this logic into the current getNextRotationTime (maybe with a different name)? That might be tricky as we have to worry about rotation time for the initialization of the ticker, the reset, and the handling of the error returned by the rotateKeyring in the case of the backoff.
There was a problem hiding this comment.
No, I propose to leave getNextRotationTime alone, but get rid of the backoff bool, the continue, and just do all the calculations for nextRotationTime here in this case.
There was a problem hiding this comment.
That way we can have more confidence that the padding we add to nextRotationTime is big enough (and possibly can be smaller than 1m), because we'll update the ticker with it immediately.
There was a problem hiding this comment.
Ah sorry I misunderstood, moved things around a bit.
| @@ -0,0 +1,3 @@ | |||
| ```release-note:bug | |||
| rafft: fix Raft TLS key rotation panic that occurs if active key is more than 24 hours old | |||
Raft TLS key rotation will panic if the active key's creation time is more than 24 hours the past. Calculating the duration using
time.Untilwill produce a negative value which causes time.NewTicker` to panic. This PR introduces a fix for this issue by creating a ticker with a duration equal to 1 minute to effectively cause rotation to occur immediately.Fixes: #15147