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.

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

Signing key rotation does not consider actual key lifetime #1234

Closed
bertsch-ronja-office opened this issue Apr 26, 2024 · 11 comments
Closed
Assignees

Comments

@bertsch-ronja-office
Copy link

bertsch-ronja-office commented Apr 26, 2024

Which version of Duende IdentityServer are you using?
6.2.3

Which version of .NET are you using?
7.0

Describe the bug

When increasing the RotationInterval for signing keys as described here new signing keys are not created anymore until the new RotationInterval passed. This leads to invalid keys at the endpoint for some time (~ NewRotationInternal - OldRotationInterval)

To Reproduce

  1. Set the RotationInterval to 1 hour.
  2. Set the RotationInterval to 12 hours.

Expected behavior

After the old key expired (latest one hour after deployment of new RotationInterval), new ones should be created.

Log output/exception with stacktrace

There is no exception within the application, but it can be discovered that they keys at the endpoint will be invalid after the OldRotationInterval passed.

@RolandGuijt
Copy link

When IdentityServer creates keys it doesn't store them with a date-time on which they should retire. Instead when determining if a key should be rotated it calculates its age (it does know when it is created) and checks it against the current RotationInterval setting.
It also checks if there are younger keys and if there are it will use the younger key as the basis for the calculation.
So to find out what's wrong we need to do some more digging. When you say "invalid keys at the endpoint" do you mean the discovery endpoint?

@RolandGuijt
Copy link

@bertsch-ronja-office Did this help you out? If so I'd like to close.

@bertsch-ronja-office
Copy link
Author

@RolandGuijt Exactly, I was referring to the discovery endpoint. We felt that the new rotation interval was used for calculating the next rotation time (the RotationInterval was increased and the keys were not rotated, hence expired).

@RolandGuijt
Copy link

As I explained the key doesn't have an embedded or stored rotation time. When it's time for a key rotation check IdentityServer simply uses the current RotationInterval that is set to see if a rotation is necessary.
So there should be no "gap" between the old and new setting.

So knowing this: is there still a problem?

@RolandGuijt
Copy link

@bertsch-ronja-office All good? If so I'd like to close.

@bertsch-ronja-office
Copy link
Author

@RolandGuijt I understand your explanation, but it does not fit what happened to us. We increased the rotation time and afterwards the Identity Server did not rotate the signing keys. And indeed we have the same assumption as you have:

IdentityServer simply uses the current RotationInterval that is set to see if a rotation is necessary

Current RotationInterval from my understanding is the new (increased) one which led IdentityServer to not rotate the key.

@AndersAbel
Copy link
Member

@bertsch-ronja-office I'm not quite following how the scenario you describe is not behaving as expected.

If I got this right you started with a setup where RotationInterval was set to 1 hour. You had an active key (let's call it A). After one hour new key B was created. Then you shut down, changed the RotationInterval to 12 hours and restarted.

At this point key B will be used until it is 12 hours old. There are some more details of when key creation happens, there is also a PropagationTime involved in the calculation. The default PropagationTime is 14 days. The PropagationTime cannot be smaller than the RotationInterval. What did you set RotationInterval to in your example?

I also don't quite understand what you mean with "invalid keys at the endpoint". There is normally no lifetime/expiry time exposed in the keys at the jwks endpoint. Do you have UseX509Certificate set to true?

@RolandGuijt
Copy link

@bertsch-ronja-office Did you manage to resolve this? If so I'd like to close the issue.

@bertsch-ronja-office
Copy link
Author

bertsch-ronja-office commented Jun 3, 2024

Hi @RolandGuijt ,

we have the UseX509Certificate set to true.

Before the change we had:

options.KeyManagement.PropagationTime = TimeSpan.FromMinutes(10);
options.KeyManagement.RotationInterval = TimeSpan.FromHours(1);
options.KeyManagement.RetentionDuration = TimeSpan.FromMinutes(10);

The change included:

options.KeyManagement.PropagationTime = TimeSpan.FromMinutes(120);
options.KeyManagement.RotationInterval = TimeSpan.FromHours(12);
options.KeyManagement.RetentionDuration = TimeSpan.FromMinutes(120);

Is the issue that the PropagationTime is smaller than the RotationInterval?

@AndersAbel
Copy link
Member

@bertsch-ronja-office Thank you for the additional details. The PropagationTime must be smaller than the RotationInterval.

The UseX509Certificate flag is rarely used, but that explains the behaviour. Without that flag, there is no notion of an expiry time for a key. The accepted keys are simply those that are published by IdentityServer at a specific point in time. When the UseX509Certificate flag is set to true, we do use the configured expiry-time to set the validity time of the certificate. Knowing that you are using certificates, your initial statement/question makes sense.

I think that this is an oversight on our end.

I've added DuendeSoftware/IdentityServer#1571 to discuss the issue.

@RolandGuijt
Copy link

Closing this issue. Please track it using the mentioned IdentityServer issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants