DuendeSoftware / Support

Support for Duende Software products
21 stars 0 forks source link

Automatic key management doesn't work as expected #764

Closed ivan-sedlak-visma closed 1 year ago

ivan-sedlak-visma commented 1 year ago

Which version of Duende IdentityServer are you using? 6.3.3

Which version of .NET are you using? .NET 6

Describe the bug We enabled the automatic key management and initially, it worked as expected, generating new signing keys in db which are visible in the disco doc. We still keep the old key registered manually. In the meantime, we disabled some of the algorithms and left only the RSA256 (X509). The other autogenerated signing keys disappeared from disco doc, all is good thus far. But the issue is, seems like new keys are not generated as the existing one (autogenerated) already expired. We still sign tokens with our manual key but we can't disable it because new keys are not being announced.

To Reproduce

Expected behavior

josephdecock commented 1 year ago

@ivan-sedlak-visma, thanks for this bug report. We'll investigate and update here soon.

josephdecock commented 1 year ago

@ivan-sedlak-visma - so far I've not been able to reproduce this issue in my environment. Can you go into more detail about the values you're using for all the relevant configurations? For anything that you're changing, before and after values would be helpful too.

ivan-sedlak-visma commented 1 year ago

Thanks @josephdecock. If the feature is up and running properly on your side it might be something we do with injecting a signing certificate and a validation certificate the manual way or something with our db store. When I get the time I will go in-depth and report back with more info.

ivan-sedlak-visma commented 1 year ago

@josephdecock Here are more details.

I've noticed that when I run IdentityServer in localhost, with a fresh db instance (no cert data), it generates the RS256 certificate as specified by current configuration (same key management config in all envs):

        options.KeyManagement.Enabled = true;
        options.KeyManagement.RotationInterval = TimeSpan.FromDays(90);
        options.KeyManagement.PropagationTime = TimeSpan.FromDays(14);
        options.KeyManagement.RetentionDuration = TimeSpan.FromDays(7);
        options.KeyManagement.DeleteRetiredKeys = true;
        options.KeyManagement.SigningAlgorithms = new[]
        {
            new SigningAlgorithmOptions(SecurityAlgorithms.RsaSha256) { UseX509Certificate = true }
        };

However, in staging and production environments, where db is not fresh (has data) it still keeps outdated certificate. I enabled the Verbose logging and this is what I got:

Getting all the keys.
Cache miss when loading all keys.
Loading keys from store.
Loaded keys from store: "88CF32355BAE0F4E0DD9E73BFA6E232E,3030AAC5B919B4567A0BCC47C5074B83,5B49719B0EF3414A640251AAEAAC236F"
Remaining keys after filter: "88CF32355BAE0F4E0DD9E73BFA6E232E,3030AAC5B919B4567A0BCC47C5074B83,5B49719B0EF3414A640251AAEAAC236F"
Keys with allowed alg from store: "88CF32355BAE0F4E0DD9E73BFA6E232E"
Keys successfully returned from store.
Caching keys with KeyCacheDuration for 1.00:00:00
Looking for active signing keys.
Looking for an active signing key for alg "RS256".
Checking if key with kid "88CF32355BAE0F4E0DD9E73BFA6E232E" is active (respecting activation delay).
Key with kid "88CF32355BAE0F4E0DD9E73BFA6E232E" is active.
Active signing key found (respecting the activation delay) with kid: "88CF32355BAE0F4E0DD9E73BFA6E232E".
Found active signing key for alg "RS256" with kid "88CF32355BAE0F4E0DD9E73BFA6E232E".
Checking if key with kid "88CF32355BAE0F4E0DD9E73BFA6E232E" is active (respecting activation delay).
Key with kid "88CF32355BAE0F4E0DD9E73BFA6E232E" is active.
Active signing key found (respecting the activation delay) with kid: "88CF32355BAE0F4E0DD9E73BFA6E232E".
Key rotation not required for alg "RS256"; New key expected to be created in 11.20:59:48
Getting the current key.
Cache hit when loading all keys.
Looking for active signing keys.
Looking for an active signing key for alg "RS256".
Checking if key with kid "88CF32355BAE0F4E0DD9E73BFA6E232E" is active (respecting activation delay).
Key with kid "88CF32355BAE0F4E0DD9E73BFA6E232E" is active.
Active signing key found (respecting the activation delay) with kid: "88CF32355BAE0F4E0DD9E73BFA6E232E".
Found active signing key for alg "RS256" with kid "88CF32355BAE0F4E0DD9E73BFA6E232E".
Checking if key with kid "88CF32355BAE0F4E0DD9E73BFA6E232E" is active (respecting activation delay).
Key with kid "88CF32355BAE0F4E0DD9E73BFA6E232E" is active.
Active signing key found (respecting the activation delay) with kid: "88CF32355BAE0F4E0DD9E73BFA6E232E".
Key rotation not required for alg "RS256"; New key expected to be created in 11.20:59:48

Below is the jwks endpoint result. The second key is registered manually, not a part of the key management. The key with id "88CF32355BAE0F4E0DD9E73BFA6E232E" expired on 24th of June 2023. No other keys are registered.

{
  "keys": [
    {
      "kty": "RSA",
      "use": "sig",
      "kid": "88CF32355BAE0F4E0DD9E73BFA6E232E",
      "x5t": "vj9T2ohOOVkNBXUhdsziVWRnKHI",
      "e": "AQAB",
      "n": "ssWj...YvJEw",
      "x5c": [
        "MIIDDD...eDCQ=="
      ],
      "alg": "RS256"
    },
    {
      "kty": "RSA",
      "use": "sig",
      "kid": "AA9FD220B3D04F4F5E1B6344A47EA9CB2C379E57RS256",
      "x5t": "qp_SILPQT09eG2NEpH6pyyw3nlc",
      "e": "AQAB",
      "n": "oUAB...u-2Q",
      "x5c": [
        "MIIDmD...0TtE3dR"
      ],
      "alg": "RS256"
    }
  ]
}

More cert details:

Valid From: 18 May 2023, 8:59 a.m.
Valid To: 24 Jun 2023, 8:59 a.m.

Previous key management config:

        options.KeyManagement.Enabled = true;
        options.KeyManagement.RotationInterval = TimeSpan.FromDays(30);
        options.KeyManagement.PropagationTime = TimeSpan.FromDays(7);
        options.KeyManagement.RetentionDuration = TimeSpan.FromDays(7);
        options.KeyManagement.DeleteRetiredKeys = true;
        options.KeyManagement.SigningAlgorithms = new[]
        {
            new SigningAlgorithmOptions(SecurityAlgorithms.RsaSha256) { UseX509Certificate = true },
            new SigningAlgorithmOptions(SecurityAlgorithms.RsaSsaPssSha256),
            new SigningAlgorithmOptions(SecurityAlgorithms.EcdsaSha256)
        };

Please let me know if you need more details. Thank you.

ivan-sedlak-visma commented 1 year ago

I think I have found the bug in KeyManager.cs

It happens because the rotation interval was changed to a higher value and the diff is bigger than the propagation interval. In our particular case, the issue should fix itself in approximately 12 days and as long as we don't change the key management config anymore it should keep working as expected from that point on. I guess the proper fix would be to keep the config alongside the generated certificate and check against those values instead of the current ones. I am not sure that is the best strategy so I don't want to create any PRs at this point in time to suggest a bugfix.

Here is the code snippet to reproduce:

        DateTime created = DateTime.Parse("2023-05-18");
        TimeSpan interval = TimeSpan.FromDays(90);
        TimeSpan propagation = TimeSpan.FromDays(14);
        DateTime now = DateTime.Parse("2023-07-21");

        TimeSpan age = now.Subtract(created);
        TimeSpan diff = interval.Subtract(age);
        bool needed = (diff <= propagation);

        Console.WriteLine($"Age: {age}");
        Console.WriteLine($"Diff: {diff}");
        Console.WriteLine($"Needed? {needed}");

        if (!needed) {
          Console.WriteLine("Key rotation not required for alg {0}; New key expected to be created in {1}", "ALG", diff.Subtract(propagation));
        } else {
          Console.WriteLine("Key rotation required now for alg {0}.", "ALG");
        }

Result:

Age: 64.00:00:00
Diff: 26.00:00:00
Needed? False
Key rotation not required for alg ALG; New key expected to be created in 12.00:00:00
josephdecock commented 1 year ago

Signing keys never really expire. They can be stored in an x.509 certificate and that certificate might have an expiration date on it, but IdentityServer isn't validating any of the x.509 certificate details. It's just using the certificate as a container for the key material, and will happily use a certificate regardless of expiration. This is a common point of confusion, and I can see why folks would think "oh, my certificate is about to expire, I have a problem". But as I say, that's not how IdentityServer uses certificates.

Given this, do you still think there is a bug?

ivan-sedlak-visma commented 1 year ago

Signing keys never really expire. They can be stored in an x.509 certificate and that certificate might have an expiration date on it, but IdentityServer isn't validating any of the x.509 certificate details. It's just using the certificate as a container for the key material, and will happily use a certificate regardless of expiration. This is a common point of confusion, and I can see why folks would think "oh, my certificate is about to expire, I have a problem". But as I say, that's not how IdentityServer uses certificates.

Given this, do you still think there is a bug?

In that context, the behavior seems valid, but then why bother with creating a certificate with precise expiration dates? Using an expired certificate might trigger some clients to reject it? Idk seems a bit backward to me but if you think this is expected behavior please feel free to close the issue. Thank you.

josephdecock commented 1 year ago

In that context, the behavior seems valid, but then why bother with creating a certificate with precise expiration dates?

There's not a lot of benefit to creating x509 certificates at all. From a security point of view in OIDC, there's no benefit at all, but saml requires it, and sometimes corporate IT policies will require that as well.

Using an expired certificate might trigger some clients to reject it?

We haven't run into this in practice. Clients in general trust the public key material because they authenticate the server using TLS (they got it over https, so it has to be legit). If you run into a client that is rejecting these certs, we'd be interested to hear about it, and could consider adding additional checks to the rotation based on the expiration of existing certs. In an unusual circumstance like that, there's always the option of manual key management as well. For now, I'll close this issue. Thanks for all your thoughts and feedback!