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

Update the Microsoft.IdentityModel.* and Microsoft.Identity.Web.* packages to the latest patch release (7.0.3 & 2.15.2) #51430

Merged
merged 4 commits into from
Oct 18, 2023

Conversation

mkArtakMSFT
Copy link
Member

@mkArtakMSFT mkArtakMSFT commented Oct 17, 2023

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?

  • 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
  • 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

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

When servicing release/2.1

  • Make necessary changes in eng/PatchConfig.props

We've made a fix to the IdentityModel validation logic, which has introduced a regression in .NET 8 AAD authentication scenarios. The AAD team has approved the fix and have release a new package to NuGet. This change updates our reference to the patched version, so that we avoid the regression impacting 8.0 apps.
@mkArtakMSFT mkArtakMSFT added Servicing-consider Shiproom approval is required for the issue area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer labels Oct 17, 2023
@mkArtakMSFT mkArtakMSFT requested a review from halter73 October 17, 2023 04:17
@mkArtakMSFT mkArtakMSFT requested review from wtgodbe and a team as code owners October 17, 2023 04:17
@ghost
Copy link

ghost commented Oct 17, 2023

Hi @mkArtakMSFT. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Oct 17, 2023
martincostello added a commit to aspnet-contrib/AspNet.Security.OpenId.Providers that referenced this pull request Oct 17, 2023
Bump Microsoft.IdentityModel.Protocols to 7.0.3 as there's been some bugs in v7 that .NET 8 is picking up e.g. dotnet/aspnetcore#51430.
martincostello added a commit to aspnet-contrib/AspNet.Security.OAuth.Providers that referenced this pull request Oct 17, 2023
Bump Microsoft.IdentityModel.Protocols to 7.0.3 as there's been some bugs in v7 that .NET 8 is picking up e.g. dotnet/aspnetcore#51430.
eng/Versions.props Show resolved Hide resolved
@mkArtakMSFT
Copy link
Member Author

@halter73 I've now included the MIcrosoft.Identity.Web.* packages too and this should be ready now.

@mkArtakMSFT mkArtakMSFT requested a review from halter73 October 18, 2023 01:23
@mkArtakMSFT mkArtakMSFT changed the title Update the IdentityModel version to 7.0.3 Update the Microsoft.IdentityModel.* and Microsoft.Identity.Web.* packages to the latest patch release (7.0.3 & 2.15.2) Oct 18, 2023
@mkArtakMSFT mkArtakMSFT added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Oct 18, 2023
@ghost
Copy link

ghost commented Oct 18, 2023

Hi @mkArtakMSFT. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@mkArtakMSFT
Copy link
Member Author

mkArtakMSFT commented Oct 18, 2023

I believe the CI failures will resolve after dotnet/source-build-externals#228 is merged. Marking this with NO MERGE label, waiting for that PR to merge first.

@mkArtakMSFT mkArtakMSFT added the * NO MERGE * Do not merge this PR as long as this label is present. label Oct 18, 2023
<MicrosoftIdentityWebGraphServiceClientVersion>2.13.0</MicrosoftIdentityWebGraphServiceClientVersion>
<MicrosoftIdentityWebUIVersion>2.13.0</MicrosoftIdentityWebUIVersion>
<MicrosoftIdentityWebDownstreamApiVersion>2.13.0</MicrosoftIdentityWebDownstreamApiVersion>
<MicrosoftIdentityWebVersion>2.15.2</MicrosoftIdentityWebVersion>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still looks like this has a 6.33.0 rather than a 7.0.3 dependency on Microsoft.IdentityModel.Validators for everything but the net8.0 TFM.

https://www.nuget.org/packages/Microsoft.Identity.Web/2.15.2#dependencies-body-tab

This is probably okay given we only really need Microsoft.IdentityModel.Validators 7.0.3 for .NET 8 as the old OIDC handler avoids the issue, but it is odd to leave it at 6.33.0 for all the other TFMs. Is 7.0.3 known to be broken on the non-net8.0 TFMs? @jennyf19

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I has 7.0.3 for the .NET 8.0 target framework, and 6.33.0 for the other target framework, @halter73 ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I has 7.0.3 for the .NET 8.0 target framework, and 6.33.0 for the other target framework

I noticed that. That's why I mentioned it was "for everything but the net8.0 TFM."

It's not a huge problem. It probably even makes sense for Microsft.AspNetCore.* and Microsoft.Extensions.* where using 7.x packages on necoreapp3.1 to avoid issues related to breaking changes in the ASP.NET Core shared runtime. For example Microsoft.AspNetCore.Authentication.JwtBearer 3.1.x only directly targets necoreapp3.1 and isn't tested on newer runtimes.

However, I don't think it makes sense for any of the other packages. You ship Microsoft.IdentityModel.Validators yourself, it has no direct or indirect Microsft.AspNetCore.* or Microsoft.Extensions.* dependency I see, and it directly targets net8.0, net6.0, netstardard2.0, etc.. Why wouldn't you use the latest Microsoft.IdentityModel.Validators as a dependency in the latest Microsoft.Identity.Web net6.0 TFM? The latest Microsoft.IdentityModel.Validators also directly targets net6.0!

…nals build 20231018.1

Microsoft.SourceBuild.Intermediate.source-build-externals
 From Version 8.0.0-alpha.1.23502.1 -> To Version 8.0.0-alpha.1.23518.1
@eerhardt
Copy link
Member

@halter73 @mkArtakMSFT - The source build leg should now be fixed in this PR. I've updated to include the changes from dotnet/source-build-externals#229.

@halter73
Copy link
Member

@dotnet/aspnet-build @dotnet/dnceng I posted about this on our internal "ASP.NET Build" teams chat, but I'll ask here too for wider visibility.

We are having an issue updating our project templates to reference the latest version of Microsoft.Identity.Web 2.15.2 because it depends on Microsoft.Identity.Web.TokenAcquisition 2.15.2 which in turn depends on RC2 packages that we ship like Microsoft.AspNetCore.Authentication.OpenIdConnect (>= 8.0.0-rc.2.23480.2).

The problem is that our template tests see our Microsoft.AspNetCore.Authentication.OpenIdConnect 8.0.0-ci artifacts as a forced downgrade and fails our template tests with a build error like:

/datadisks/disk1/work/BCE909F9/w/B40D0985/e/Templates/BaseFolder/AspNet.vxrmciskazme/AspNet.vxrmciskazme.csproj : error NU1605: Warning As Error: Detected package downgrade: Microsoft.AspNetCore.Authentication.JwtBearer from 8.0.0-rc.2.23480.2 to 8.0.0-ci. Reference the package directly from the project to select a different version.
/datadisks/disk1/work/BCE909F9/w/B40D0985/e/Templates/BaseFolder/AspNet.vxrmciskazme/AspNet.vxrmciskazme.csproj : error NU1605:  AspNet.vxrmciskazme -> Microsoft.Identity.Web 2.15.2 -> Microsoft.Identity.Web.TokenAcquisition 2.15.2 -> Microsoft.AspNetCore.Authentication.JwtBearer (>= 8.0.0-rc.2.23480.2)
/datadisks/disk1/work/BCE909F9/w/B40D0985/e/Templates/BaseFolder/AspNet.vxrmciskazme/AspNet.vxrmciskazme.csproj : error NU1605:  AspNet.vxrmciskazme -> Microsoft.AspNetCore.Authentication.JwtBearer (>= 8.0.0-ci)

https://dev.azure.com/dnceng-public/public/_build/results?buildId=442668&view=ms.vss-test-web.build-test-results-tab

Have we ever referenced an external package in our project templates that reference a prerelease package we built? I assume not, or else we would have run into this issue before. What should we do to fix this?

@wtgodbe
Copy link
Member

wtgodbe commented Oct 18, 2023

It's probably fine to include NU1605 in NoWarn for those tests - In the bits we actually ship, our templates will depend on a newer Microsoft.AspNetCore.Authentication.JwtBearer than the transitive dependency, so it shouldn't cause any real issues. At least some of our templates already NoWarn the reference to JwtBearer:

<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="${MicrosoftAspNetCoreAuthenticationJwtBearerVersion}" Condition=" '$(IndividualB2CAuth)' == 'True' OR '$(OrganizationalAuth)' == 'True'" NoWarn="NU1605" />

- This is already there in RazorPagesWeb-CSharp.csproj.in and StarterWeb-CSharp.csproj.in
@halter73
Copy link
Member

Thanks for the suggestion. It looks like we already had been using NoWarn="NU1605" for these exact dependencies in RazorPagesWeb-CSharp.csproj.in and StarterWeb-CSharp.csproj.in but not WebApi-CSharp.csproj.in.

@mkArtakMSFT mkArtakMSFT removed the * NO MERGE * Do not merge this PR as long as this label is present. label Oct 18, 2023
@mkArtakMSFT mkArtakMSFT merged commit 198857c into release/8.0 Oct 18, 2023
26 checks passed
@mkArtakMSFT mkArtakMSFT deleted the mkArtakMSFT-patch-1 branch October 18, 2023 20:24
@ghost ghost added this to the 8.0.0 milestone Oct 18, 2023
martincostello added a commit to aspnet-contrib/AspNet.Security.OpenId.Providers that referenced this pull request Nov 14, 2023
- Update to the .NET 8 SDK and target `net8.0`.
- Add a README file to the NuGet packages as recommended by the .NET 8 SDK.
- Remove now-obsolete use of `ISystemClock`.
- Fix code analysis warning by using `TryGetValue()`.
- Use static `ArgumentException` methods.
- Use `AuthenticationFailureException` where relevant.
- Use primary constructors in a few places where relevant.
- Apply some IDE suggestions to use newer C# syntax.
- Fix typo.
- Update to the latest release of arcade for .NET 8.
- Update NuGet packages to their latest versions.
- Remove System.Text.Json as an explicit package reference.
- Bump Microsoft.IdentityModel.Protocols to 7.0.3 as there's been some bugs in v7 that .NET 8 is picking up e.g. dotnet/aspnetcore#51430.
- Fix AoT warning.
- Simplify member name.
- Add support for native AoT for the libraries.
martincostello added a commit to aspnet-contrib/AspNet.Security.OAuth.Providers that referenced this pull request Nov 14, 2023
- Update to ASP.NET Core 8.
- Add a README file to the NuGet packages as recommended by the .NET 8 SDK.
- Remove obsolete TODO.
- Remove usage of `ISystemClock` and use `TimeProvider` instead.
- Suppress `CA1861` in tests.
- Apply some style suggestions from Visual Studio.
- Use `AuthenticationFailureException` where relevant.
- Use `ArgumentException.ThrowIfNullOrWhiteSpace()` where relevant.
- Specify value with `ArgumentOutOfRangeException` where relevant.
- Suppress `CA1863` warnings to use `CompositeFormat`.
- Use `JsonDocument.ParseAsync()` where relevant.
- Remove `ISystemClock`.
- Suppress `CA1863`.
- Remove obsolete members for the EVE Online provider.
- Use primary constructors where relevant.
- Use collection literals where relevant.
- Add helper method to reduce duplicative tests.
- Apply some IDE suggestions.
- Remove redundant using statements.
- Fix some typos.
- Hide shared files from the Visual Studio project explorer.
- Use collection literals for string arrays.
- Update to the latest release of arcade for .NET 8.
- Update NuGet packages to their latest versions.
- Fix obsolete warnings.
- Fix StyleCop warning.
- Fix tests after update to Wilson 7.0.0.
- Bump Microsoft.IdentityModel.Protocols to 7.0.3 as there's been some bugs in v7 that .NET 8 is picking up e.g. dotnet/aspnetcore#51430.
- Add support for native AoT for the libraries.
- Trim off all the fractions of a second before comparing.
- Remove redundant casts.
- Fix query string parameters being duplicated if `AuthorizationEndpoint` contains any user-added query string parameters.
- Use unified BattleNet server
@jaliyaudagedara
Copy link

Hi Guys,

Unfortunately, I am experiencing the issue: 51005 after updating to .NET 8.

Bearer was not authenticated. Failure message: IDX40001: Issuer: 'https://something.b2clogin.com/<tenantId>/v2.0/', does not match any of the valid issuers provided for this application.

I am using Microsoft.Identity.Web: 2.15.5

services.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
    .AddMicrosoftIdentityWebApi(Configuration, "AzureAdB2C");

I could get around by allowing all the issuers, but that's not good.

services.Configure<JwtBearerOptions>(JwtBearerDefaults.AuthenticationScheme, options =>
{
    options.TokenValidationParameters.IssuerValidator = 
        (string issuer, SecurityToken securityToken, TokenValidationParameters validationParameters) => issuer;
});

@ghost
Copy link

ghost commented Nov 22, 2023

Hi @jaliyaudagedara. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants