AzureAD / azure-activedirectory-identitymodel-extensions-for-dotnet

IdentityModel extensions for .Net
MIT License
1.06k stars 401 forks source link

[Bug] Metadata refresh not working when metadata is changed, since version 8.0.2 #3008

Open shaheer-k opened 1 week ago

shaheer-k commented 1 week ago

Which version of Microsoft.IdentityModel are you using? Microsoft.IdentityModel.Protocols.OpenIdConnect 8.0.2

Where is the issue?

We have upgraded to version 8.0.2 from 8.0.1. After this upgrade, the metadata refresh not seems to be working as expected.

We have added a test to ensure that the metadata refresh is working when there is change in the metadata.

var token = tokenHandler.ValidateTokneAsync(.....)
// added test code to refresh the metadata, and then call the token validation again as below:
token = tokenHandler.ValidateTokneAsync(.....)
// Here it is failing with invalid token signature

Expected behavior Token is validated succesfully.

Actual behavior Token validation fails with SecurityTokenInvalidSignatureException exception.

The above test is working with Microsoft.IdentityModel.Protocols.OpenIdConnect 8.0.1. But it is failng with version 8.0.2 onwards.

Possible solution Please check if this issue is related to the change #2780

pmaytak commented 1 week ago

Does this repro with the latest 8.2.0?

keegan-caruso commented 1 week ago

The metadata refresh was moved to a background thread after the initial request, it should refresh, but in the timespan of a unit test, the task to update the metadata may not have completed.

shaheer-k commented 1 week ago

@pmaytak yes its happening with version 8.0.2 onwards, also in latest version 8.2.0.

@keegan-caruso

In version 8.0.1, when generating a new bearer token with keys not present in the current in-memory configuration, the metadata was automatically refreshed upon encountering an initial SecurityTokenInvalidSignatureException, allowing the token to successfully validate automatically (due to the retry in JwtSecurityTokenHandler.cs). In this scenario, line 898 was executed, resulting in successful token validation. See line 898 in JwtSecurityTokenHandler.cs.

The unit test pseudo-code sequence is as follows:

Generate metadata
Generate new access token
Validate access token
Generate metadata again
Generate new access token (with new key)
Validate access token

Note: In the unit test, metadata is read from a local JSON file.

However, in version 8.0.2 (and the latest versions), if we generate a bearer token with new keys not found in the current in-memory configuration, token validation fails without attempting a retry with the updated metadata. In this case, line 911 is executed, and the SecurityTokenInvalidSignatureExceptionis returned to the caller. See line 911 in JwtSecurityTokenHandler.cs.

We have also tried adding a delay in the unit test and retrying the token validation after regenerating the metadata, but all validation attempts continue to fail.

Due to this behavior, we are unable to proceed with the package update to the latest version in Production. Could you please advise if any modifications are required in our token validation process or if adjustments to the unit test code are necessary?

Notes: The project includes the following package references:

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="8.0.0" />
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="8.0.2" />
    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="8.0.2" />
    <PackageReference Include="Microsoft.Extensions.Options" Version="8.0.2" />
    <PackageReference Include="Microsoft.IdentityModel.Abstractions" Version="8.2.0" />
    <PackageReference Include="Microsoft.IdentityModel.Protocols.OpenIdConnect" Version="8.2.0" />
    <PackageReference Include="BouncyCastle.Cryptography" Version="2.4.0" />
  </ItemGroup>
shaheer-k commented 1 week ago

Please let me know if any additional information is required to understand the problem.

keegan-caruso commented 1 week ago

A minimal repro of code would help us a lot. thanks @shaheer-k

jennyf19 commented 15 hours ago

@shaheer-k any update on a repro?

brentschmaltz commented 9 hours ago

@shaheer-k I understand your issue in reviewing the code. We moved GetConfigurationAsync() onto a background task as users were complaining that we were blocking when the interval was small. These users were updating metadata on a 15 minute interval. Every 15 minutes the lock would get in the way.

However, upon reflection, we should think about RequestRefresh() as blocking, as the logic will not work as before.

Note: you should make sure all the version of IdentityModel are the same.