DuendeSoftware / Support

Support for Duende Software products
20 stars 0 forks source link

Signing key rotation does not consider actual key lifetime #1234

Closed bertsch-ronja-office closed 2 months ago

bertsch-ronja-office commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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

bertsch-ronja-office commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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

bertsch-ronja-office commented 3 months ago

@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 commented 3 months ago

@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 commented 2 months ago

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

bertsch-ronja-office commented 2 months ago

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 commented 2 months ago

@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 https://github.com/DuendeSoftware/IdentityServer/issues/1571 to discuss the issue.

RolandGuijt commented 2 months ago

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