Have GetSigningCredentialsAsync refresh its key earlier #359
Conversation
Only refreshing after the key is expired will cause us to sign tokens that might not validate for their entire lifetime. The spirit was to have GetSigningCredentialsAsync call RefreshAsync even if there was no background job calling it -- so this change makes it call it at the same cadence/time we expect a background service to. This hasn't been a problem in production for various reasons but it was for a specific use-case in the dev environment, but regardless its a bug.
| // do a foreground Refresh | ||
| private static readonly TimeSpan GetSigningCredentialsRefreshGracePeriod | ||
| = BackgroundRefreshDelay | ||
| + BackgroundRefreshRetryDelay + BackgroundRefreshRetryDelay; |
| @@ -51,7 +65,9 @@ OAuth2Configuration config | |||
|
|
|||
| var now = m_clock.UtcNow; | |||
|
|
|||
| if ( current == null || current.ValidTo <= now ) { | |||
| if ( current == null | |||
| || ExpectedTimeOfNewUsableKey( current ) + GetSigningCredentialsRefreshGracePeriod < now | |||
There was a problem hiding this comment.
The implication here is that once this period is passed every call to GetSigningCredentials is going to fight to update that key.
That basically matches how it worked before the background refresh anyway. The only things without background refresh for us are Lambda functions though, which are single threaded anyway.
| if ( current == null || current.ValidTo <= now ) { | ||
| if ( current == null | ||
| || ExpectedTimeOfNewUsableKey( current ) + GetSigningCredentialsRefreshGracePeriod < now | ||
| ) { | ||
| // Slow path: RefreshKeyAsync() wasn't called on boot and/or it | ||
| // isn't being called in a background job. | ||
| await RefreshKeyAsync( now ) |
There was a problem hiding this comment.
i thought about listening to the response from RefreshKeyAsync here, but the approach I went with was "stateless" and doesn't require having previously called RefreshKeyAsync 🤷 the logic is all in this file and shared so it doesn't seem too bad to me?
There was a problem hiding this comment.
If key rotation fails for 3 minutes I think the results are pretty bad here? Taking a look now at whether dev key rotation ever gets delayed...
There was a problem hiding this comment.
dev dynamo metrics show writes every 3 hours in the first 5 minutes of the hour steady for the past two months. I think we're still in pretty rough shape if cert generation gets delayed, but hopefully that won't happen too often.
Only refreshing after the key is expired will cause us to sign tokens that might not validate for their entire lifetime.
The spirit was to have GetSigningCredentialsAsync call RefreshAsync even if there was no background job calling it -- so this change makes it call it at the same cadence/time we expect a background service to.
This hasn't been a problem in production for various reasons but it was for a specific use-case in the dev environment, but regardless its a bug.