Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Have GetSigningCredentialsAsync refresh its key earlier #359

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Have GetSigningCredentialsAsync refresh its key earlier #359

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e9a1b89
Have GetSigningCredentialsAsync refresh its key earlier
j3parker Feb 7, 2024
d66888d
Add a grace period
j3parker Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions src/D2L.Security.OAuth2/Keys/KeyManagementService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,20 @@ public sealed partial class KeyManagementService : IKeyManagementService, IPriva

private D2LSecurityToken m_current = null;

// This controls how long the background refresh is going to wait for a
// new key to be generated
private static readonly TimeSpan BackgroundRefreshDelay = TimeSpan.FromMinutes( 1 );

// This controls how frequently background refresh will retry if it
// doesn't find a key
private static readonly TimeSpan BackgroundRefreshRetryDelay = TimeSpan.FromMinutes( 1 );

// Wait for the delay and some number of retries before making GetSigningCredentials
// do a foreground Refresh
private static readonly TimeSpan GetSigningCredentialsRefreshGracePeriod
= BackgroundRefreshDelay
+ BackgroundRefreshRetryDelay + BackgroundRefreshRetryDelay;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't do 2* 😒


internal KeyManagementService(
IPublicKeyDataProvider publicKeys,
IPrivateKeyDataProvider privateKeys,
Expand Down Expand Up @@ -51,7 +65,9 @@ async Task<D2LSecurityToken> IPrivateKeyProvider.GetSigningCredentialsAsync() {

var now = m_clock.UtcNow;

if ( current == null || current.ValidTo <= now ) {
if ( current == null
|| ExpectedTimeOfNewUsableKey( current ) + GetSigningCredentialsRefreshGracePeriod < now
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

) {
// Slow path: RefreshKeyAsync() wasn't called on boot and/or it
// isn't being called in a background job.
await RefreshKeyAsync( now )
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand All @@ -67,6 +83,13 @@ await RefreshKeyAsync( now )
return current.Ref();
}

private DateTimeOffset ExpectedTimeOfNewUsableKey( D2LSecurityToken current )
// A new key will get generated some time before the current key
// expires, but will only become usable some time after that.
=> current.ValidTo
- m_config.KeyRotationBuffer
+ m_config.KeyTimeUntilUse;

[GenerateSync]
async Task<TimeSpan> IKeyManagementService.RefreshKeyAsync() {
var now = m_clock.UtcNow;
Expand All @@ -81,20 +104,16 @@ await RefreshKeyAsync( now )
return TimeSpan.FromSeconds( 10 );
}

// A new key will get generated some time before the current key
// expires, but will only become usable some time after that.
var expectedTimeOfNewUsableKey = current.ValidTo
- m_config.KeyRotationBuffer
+ m_config.KeyTimeUntilUse;
var expectedTimeOfNewUsableKey = ExpectedTimeOfNewUsableKey( current );

if( now > expectedTimeOfNewUsableKey ) {
// If we would have expected a new key by now, retry again in a
// bit. This code branch supports configuration changes mostly.
return TimeSpan.FromMinutes( 1 );
return BackgroundRefreshRetryDelay;
} else {
// Otherwise use that but with a little buffer for key
// generation time/imprecisely scheduled cron jobs.
return expectedTimeOfNewUsableKey.AddMinutes( 1 ) - now;
return expectedTimeOfNewUsableKey + BackgroundRefreshDelay - now;
}
}

Expand Down
Loading