Fix AadIssuerValidator's handling of trailing forward slashes #2361
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
halter73
force-pushed
the
halter73/51005
branch
from
f6f5ae5
to
1217e24
Compare
brentschmaltz
approved these changes
Oct 16, 2023
11 tasks
mkArtakMSFT
added a commit
to dotnet/aspnetcore
that referenced
this pull request
Oct 18, 2023
…kages to the latest patch release (7.0.3 & 2.15.2) (#51430) # Update the Microsoft.IdentityModel.* and Microsoft.Identity.Web.* packages to the latest patch release (7.0.3 & 2.15.2) Update the reference to the Microsoft.IdentityModel.* and Microsoft.Identity.Web.* packages so that we don't regress AAD authentication scenarios for web apps. ## Description We've hit an issue with AAD authentication in ASP.NET Core web apps, which was resulting in errors during login. This was due to an issue in the IdentityModel package, for which @halter73 has proposed a fix: AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet#2361 The Identity team has approved the fix and has released a new package to NuGet so that we can update our dependency and **avoid the regression in 8.0**. Please note, that this change will have to include the soure-build change as well: dotnet/source-build-externals#228 Fixes #51005 ## Customer Impact Customers who will try to use AAD authentication for their ASP.NET Core web applications in 8.0 will fail to login. ## Regression? - [x] Yes - [ ] No This was technically an existing bug, which was already in the IdentityModel package, however only after a recent change #49542 the issue has surfaced impacting 8.0 apps. ## Risk - [ ] High - [ ] Medium - [x] Low From our point of view this is a dependency update. And the dependency has taken only a targeted fix to avoid the bug, going through all the necessary validation on the AAD side. ## Verification - [x] Manual (required) - [ ] Automated ## Packaging changes reviewed? - [ ] Yes - [ ] No - [x] N/A ---- ## When servicing release/2.1 - [ ] Make necessary changes in eng/PatchConfig.props --------- Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com> Co-authored-by: Stephen Halter <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes errors like the following reported by multiple customers at dotnet/aspnetcore#51005 when they tried to upgrade their app using
AddMicrosoftIdentityWebAppto .NET 8.While
https://login.microsoftonline.com/{TenantId}/v2.0/.well-known/openid-configuration, responds with"issuer": "https://login.microsoftonline.com/{TenantId}/v2.0"and issues JWTs withissclaims containing no trailing slashes, if you use a custom B2C authority likehttps://{TenantName}.b2clogin.com/{TenantName}.onmicrosoft.com/B2C_1_SignUpSignIn/v2.0/.well-known/openid-configurationyou will get"issuer": "https://TenantName}.b2clogin.com/{TenantId}/v2.0/"responses andissclaims with trailing slashes.This is causing upgrades of Azure B2C customers to .NET 8 to fail because
AadIssuerValidator.GetEffectiveConfigurationManager()does not account for trailing slashes. This causes it to query V1 metadata for a V2 token, which causes a mismatch because the V1 metadata does not include a trailing forward slash for the issuer while the V2 token does include a trailing forward slash.While it looks like this bug has been around for a while, I think the reason customers haven't seen this until upgrading to .NET 8 is because the
OpenIdConnectHandlerhas stopped settingTokenValidationParameters.ValidIssuers. It is now expected expected settingTokenValidationParameters.ConfigurationMageris sufficient as of dotnet/aspnetcore#49542.In the particular case of people using
AadIssuerValidator, this PR is sufficient to fix regression becauseAadIssuerValidator.Validatefalls back to querying theConfigurationManageritself rather than completely rely onOpenIdConnectHandler.HandleRemoteAuthenticateAsync()having done that already.However, I am concerned that there could be other custom
TokenHanderorTokenValidationParameters.IssuerValidatorcallbacks that read fromTokenValidationParameters.ValidIssuersother thanAadIssuerValidator, and that don't have the logic to query theConfigurationManageritself since it was never before necessary.I'm wondering if we shouldn't also just update the
else if (_configuration != null)to beif (_configuration != null)in OpenIdConnectHandler.ValidateTokenUsingHandlerAsync, JwtBearerHandler.SetupTokenValidationParametersAsync and WsFederationHandler.SetupTokenValidationParametersAsync to be extra safe. I assume this would have perf implications though.@jennyf19 @keegan-caruso @brentschmaltz @eerhardt @mkArtak