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] Specifying multiple decryption certificates #1243

Closed
6 tasks
husseinkorly opened this issue Jun 6, 2021 · 16 comments
Closed
6 tasks

[Bug] Specifying multiple decryption certificates #1243

husseinkorly opened this issue Jun 6, 2021 · 16 comments
Assignees
Labels
bug Something isn't working fixed regression regression between Microsoft Identity Web versions Spec'd
Milestone

Comments

@husseinkorly
Copy link

husseinkorly commented Jun 6, 2021

Which version of Microsoft Identity Web are you using?
1.12.0

Where is the issue?

  • Web app
    • Sign-in users
    • Sign-in users and call web APIs
  • Web API
    • [ x] Protected web APIs (validating tokens)
    • Protected web APIs (validating scopes)
    • Protected web APIs call downstream web APIs
  • Token cache serialization
    • [ x] In-memory caches
    • Session caches
    • Distributed caches
  • Other (please describe)

Is this a new or an existing app?
The app is in production and I have upgraded to a new version of Microsoft Identity Web.

Repro
Configuring multiple certificates for decryption and only one of them is valid and it can be used.

"TokenDecryptionCertificates": [
      {
        "SourceType": "StoreWithThumbprint",
        "CertificateStorePath": "LocalMachine/My",
        "CertificateThumbprint": "962D129A...D18EFEB6961684"
      },
      {
        "SourceType": "StoreWithThumbprint",
        "CertificateStorePath": "LocalMachine/My",
        "CertificateThumbprint": "962D129A...D18EFEB6960000"
      }
    ]

Expected behavior
A valid certificate (from list of certificates) will be used to decrypt incoming tokens

Actual behavior
Fails to decrypt a token if any of the provided certificate is invalid

Possible solution
Try to decrypt the token with any valid certificate provided.

Additional context / logs / screenshots
n/a

@jmprieur jmprieur added bug Something isn't working P1 labels Jun 7, 2021
@jmprieur jmprieur added this to the 1.13.0 milestone Jun 7, 2021
@jmprieur jmprieur self-assigned this Jun 7, 2021
@jmprieur
Copy link
Collaborator

jmprieur commented Jun 7, 2021

@husseinkorly are you saying this is a regression?

@jmprieur jmprieur added the regression regression between Microsoft Identity Web versions label Jun 7, 2021
@jmprieur
Copy link
Collaborator

jmprieur commented Jun 7, 2021

I think I understand what happens.
Now, when the certificate has expired we set it to null in the CertificateDescription (this was done when implementing the certificate rotation). But Microsoft.IdentityModel does not accept a null certificate.

We should only do the yield:

when certDescription.Certificate is not null

@jmprieur jmprieur assigned jennyf19 and unassigned jmprieur Jun 7, 2021
@husseinkorly
Copy link
Author

husseinkorly commented Jun 7, 2021

@jmprieur I never tested this functionality before until we're getting close to renewing the decryption cert for one of our services. I was hopping to add the new cert as another decryption cert and switch it in the portal (first-party).

@jmprieur jmprieur added the Spec'd label Jun 7, 2021
@jmprieur
Copy link
Collaborator

jmprieur commented Jun 7, 2021

@jennyf19 is working on a fix.
Meanwhile, I think that version 1.9.2 should work fine in your case.

@husseinkorly
Copy link
Author

Thank you. I tried using version 1.9.2 but the API still failing to decrypt the token.

@jmprieur
Copy link
Collaborator

jmprieur commented Jun 7, 2021

Can you tell us more about the exception? https://github.com/AzureAD/microsoft-identity-web/wiki/Logging

@husseinkorly
Copy link
Author

husseinkorly commented Jun 7, 2021

I pulled the new changes in master and I still see the same issue. I am using 2 decryption certificates. One of them is a valid certificate (used to encrypt) and another one that still valid but not used to encrypt the token. I see the API still returning 401 and I don't see any exceptions being logged. I think it's related to this issue with identityModel package that doesn't accept a list of decryption keys.

@jennyf19
Copy link
Collaborator

jennyf19 commented Jun 7, 2021

@husseinkorly they both have to be decryption certs, not encryption certs. See this page -> "usage": "Encrypt"

if you're using a client credential certificate, like instead of a secret, a cert, it can go in the configuration.

@husseinkorly
Copy link
Author

husseinkorly commented Jun 7, 2021

Yes. I am only interested in the decryption cert I have both in the configuration like this for example:

"TokenDecryptionCertificates": [
      {
        "SourceType": "StoreWithThumbprint",
        "CertificateStorePath": "LocalMachine/My",
        "CertificateThumbprint": "962D129A...D18EFEB6961684"
      },
      {
        "SourceType": "StoreWithThumbprint",
        "CertificateStorePath": "LocalMachine/My",
        "CertificateThumbprint": "962D129A...D18EFEB6960000"
      }
    ]

One of them is a valid decryption cert (where I have the .pfx installed in my local machine) and it can decrypt the token just fine, but when I add another cert (I also have the .pfx installed locally) to the list that's just any other cert the API start returning 401.

@jennyf19
Copy link
Collaborator

jennyf19 commented Jun 7, 2021

@husseinkorly do both certs work independently? like one at a time, do they both work?

@husseinkorly
Copy link
Author

Only one cert can be configured in AAD, but I was assuming if we can have 2 certs and it will use the valid one. This can be useful to rotate the decryption certs without any downtime. We can add the new cert to the configuration, replace it in AAD, and remove the the old one from configuration.

@jmprieur
Copy link
Collaborator

jmprieur commented Jun 8, 2021

@husseinkorly : provided they are both decrypt certificates, this should work.
@brentschmaltz @GeoK @mafurman : we are using TokenValidationParameters.TokenDecryptionKeys. Shouldn't it work?

@GeoK
Copy link
Member

GeoK commented Jun 8, 2021

@jmprieur - Yes, this should work in IdentityModel >=6.8.0.
The library will try to decrypt a token using all decryption keys set on TokenValidationParameters.

@jennyf19
Copy link
Collaborator

Fixed in 1.13 release.

but seems to be an issue still with other components. @jmprieur

@jennyf19 jennyf19 removed the P1 label Jun 11, 2021
@jennyf19 jennyf19 modified the milestones: 1.13.0, 1.14 Jun 11, 2021
@jennyf19
Copy link
Collaborator

Included in 1.14 release

@husseinkorly
Copy link
Author

husseinkorly commented Jan 4, 2022

@jennyf19 @jmprieur
I am not sure how this was validated, but I just got a chance to test it because we have a decryption cert expiring in few weeks. I configured the function with a second decryption cert, and it looks like the library only try the first certificate to decrypt the cert! it is not using the second cert if the first one fails.

image

We have other services where we pass multiple certificates, and identity model will try to all the available keys to decrypt the cert

image

I am using version 1.14.0

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 regression regression between Microsoft Identity Web versions Spec'd
Projects
None yet
Development

No branches or pull requests

4 participants