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

Filtering expired X.509 certificates #1727

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

glucaci
Copy link

@glucaci glucaci commented Jan 21, 2025

What issue does this PR address?
This PR addresses a potential issue where an expired X.509 certificate could remain in use if it was generated with a different configuration and landed in the database. This change provides an additional safeguard to ensure no expired certificates are used as signing keys. The FilterExpiredKeys method now checks the NotAfter date of X509KeyContainer keys, and unit tests have been updated to validate this scenario.

@josephdecock
Copy link
Member

josephdecock commented Jan 24, 2025

Thanks for the Pull Request @glucaci.

We ignore certification expiration because the spec isn’t specific about what to do, client libraries in the .NET ecosystem ignore it, and attempting to respect it would add significant complexity.

The JWK spec doesn’t specify the semantics of the expiration of a certificate. Keys in the key set that don’t use certificates don’t even have an expiration at all. While the spec does say that certificate data must match the other members of the JWK, that only applies to things like the intended usage and algorithm of the key.

Historically, we haven't run into problems validating expired x.509 wrapped keys in the ASP.NET Core authentication handlers for OAuth and OIDC. So, while the intended semantic in the spec might be ambiguous, the client libraries that most of our users use seem to just ignore it.

If we wanted to try to respect the expiration, it's not clear what the right thing to do really is. The rules for when we rotate our keys are fairly complex, and to get into a situation where there is an expired cert holding the key that is being used, the user would have to change their configuration over time. If expiration means that keys are to be discarded immediately, that causes breakage for clients and apis caching that old key. If instead it would mean mean that the expired key should be rotated, that could still surprise some users and there may be cases where users change their config in an unexpected way. We don't have any history associated with the configuration, so we would have to infer the intention of the user based entirely on what certificate expirations are available and what the current config is. And we could even have a mixture of keys wrapped in x509 certs and keys that are not wrapped, which would behave differently. All of that sounds really difficult to reason about, vs the relative simplicity of just not using the expiration in the certificate at all.

Sorry about the wall of text, but that's my current thoughts about signing keys wrapped in X.509 certs and why IdentityServer behaves as it does today. I'm quite curious about your use case. Is the expired certificate causing an issue for you? Is there some client library that enforces that?

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

Successfully merging this pull request may close these issues.

2 participants