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

[Bug] Enable token decryption certificates rotation #905

Closed
jmprieur opened this issue Jan 22, 2021 · 4 comments
Closed

[Bug] Enable token decryption certificates rotation #905

jmprieur opened this issue Jan 22, 2021 · 4 comments
Assignees
Labels
bug Something isn't working fixed P1
Milestone

Comments

@jmprieur
Copy link
Collaborator

jmprieur commented Jan 22, 2021

Which version of Microsoft Identity Web are you using?
1.5.1

Where is the issue?

  • Web API
    • [x ] Protected web APIs (validating tokens)

Is this a new or an existing app?
New or existing

Repro
Today, Microsoft.Identity.Web allows the developers to provide a collection of token decryption certificates (to be able to rotate certificates), but only the fist one is used. See this code

Expected behavior
Developers need to be able to rotate their certificates, and therefore all the certs provided in the TokenDecryptionCertificates property

Actual behavior
Only the first certificate is loaded and used for the token decryption

Possible solution
Use TokenValidationParameters.TokenDecryptionKeys instead of TokenValidationParameters.TokenDecryptionKey. We'll also need to have a new method DefaultCertificateLoader.LoadAllCertificates in addition to the existing method DefaultCertificateLoader.LoadFirstCertificate, with the same signature, but returning a collection of X509Certificate2.

Note we don't want to remove the existing method (LoadFirstCertificate) as it will still be used for client certificates, for which the rotation is helped by the use of sendX5c.

@jmprieur jmprieur added bug Something isn't working P1 labels Jan 22, 2021
@jmprieur jmprieur added this to the 1.6.0 milestone Jan 22, 2021
@brentschmaltz
Copy link
Member

brentschmaltz commented Jan 22, 2021

@jmprieur A note about specifying certificates.
Some users have seen issues when specifying the certificate.
A more performant pattern would be to obtain the RSA object from the certificate once on startup and pass an RSASecurityKey (s) instantiated with the RSA object. Our libraries will not dispose of the RSA as they did not create it.

@jennyf19
Copy link
Collaborator

@jmprieur there was fix for the issue i hit w/this is in IdentityModel.* 6.8.0, however, Microsoft.AspNetCore.Authentication.* is currently on 6.7.1 of IdentityModel.*, and updating to 6.7.1 does not support net472 or netcoreapp3.1.

so, marking as blocked, we can discuss how to proceed.

@jmprieur
Copy link
Collaborator Author

Fixed by #909

@jennyf19
Copy link
Collaborator

Included in 1.6.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed P1
Projects
None yet
Development

No branches or pull requests

3 participants