dotnet / blazor-samples

Creative Commons Attribution 4.0 International
722 stars 616 forks source link

8.0 BlazorWebAppOidc IDX21323: RequireNonce is 'False' #173

Closed travaille-dev closed 8 months ago

travaille-dev commented 9 months ago

Describe the issue When running this blazor sample initially the authentication works well, but if you idle on the page (I'm assuming until the auth token needs to be refreshed) then you get this nonce error.

Since the error is pointing to the CookieOidcRefresher file I was wondering if I can get some direction on how to implement a fix for this.

To Reproduce

  1. Set up the sample project with proper Microsoft Entra
  2. Run the project
  3. Authenticate to the weather page
  4. Wait for the token to need to be refreshed
  5. Open the counter page
  6. See error

Expected behavior I expect for the token to be refreshed in the background if it has expired.

Screenshots

Screenshot 2024-01-23 at 3 41 07 PM

Additional context Instead of setting global render mode, I've implemented a per component render mode. However, I don't think that's part of the problem here.

.NET Version Output of dotnet --info. .NET SDK: Version: 8.0.200-preview.23624.5 Commit: 8065b9770c Workload version: 8.0.200-manifests.ba313bcd

Runtime Environment: OS Name: Windows OS Version: 10.0.19045 OS Platform: Windows RID: win-x64 Base Path: C:\Program Files\dotnet\sdk\8.0.200-preview.23624.5\

.NET workloads installed: Workload version: 8.0.200-manifests.ba313bcd [aspire] Installation Source: SDK 8.0.200-preview.23624, VS 17.9.34511.98 Manifest Version: 8.0.0-preview.2.23619.3/8.0.100 Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.sdk.aspire\8.0.0-preview.2.23619.3\WorkloadManifest.json Install Type: Msi

[wasm-tools] Installation Source: VS 17.8.34511.84, VS 17.9.34511.98 Manifest Version: 8.0.1/8.0.100 Manifest Path: C:\Program Files\dotnet\sdk-manifests\8.0.100\microsoft.net.workload.mono.toolchain.current\8.0.1\WorkloadManifest.json Install Type: Msi

Host: Version: 8.0.1 Architecture: x64 Commit: bf5e279d92

.NET SDKs installed: 6.0.418 [C:\Program Files\dotnet\sdk] 7.0.102 [C:\Program Files\dotnet\sdk] 8.0.101 [C:\Program Files\dotnet\sdk] 8.0.200-preview.23624.5 [C:\Program Files\dotnet\sdk]

.NET runtimes installed: Microsoft.AspNetCore.App 6.0.26 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 7.0.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 7.0.15 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 8.0.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.AspNetCore.App 8.0.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App] Microsoft.NETCore.App 6.0.26 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 7.0.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 7.0.15 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 8.0.0 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.NETCore.App 8.0.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App] Microsoft.WindowsDesktop.App 6.0.26 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 7.0.2 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 7.0.15 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 8.0.0 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App] Microsoft.WindowsDesktop.App 8.0.1 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Other architectures found: x86 [C:\Program Files (x86)\dotnet] registered at [HKLM\SOFTWARE\dotnet\Setup\InstalledVersions\x86\InstallLocation]


Issue Details

Do not edit this section. It is required for issue processing.

guardrex commented 9 months ago

Hello @travaille-dev ... Please open this for the product unit at ...

https://github.com/dotnet/aspnetcore/issues

Please add ...

cc: @guardrex https://github.com/dotnet/blazor-samples/issues/173

... to the bottom of your opening comment to cross-link the issues. We'll leave this one open to work on the sample.

guardrex commented 9 months ago

Just out of curiosity, could you try enabling the nonce to see what happens?

At https://github.com/dotnet/blazor-samples/blob/main/8.0/BlazorWebAppOidc/BlazorWebAppOidc/CookieOidcRefresher.cs#L21, try setting it to true ...

RequireNonce = true,

However, I'm not quite sure what the last part of Stephen's comment means ...

Refresh requests do not use the nonce parameter. Otherwise, we'd use oidcOptions.ProtocolValidator.

Perhaps, it just means that when set to true the default oidcOptions.ProtocolValidator works (i.e., no further action is necessary in a base use case).

travaille-dev commented 9 months ago

Almost same thing.

Screenshot 2024-01-24 at 9 43 42 AM

When looking online I get some reference to rechallenge the authentication. However, the code samples reference unrelated packages. https://stackoverflow.com/a/54020017 https://learn.microsoft.com/en-us/answers/questions/1255930/idx21323-requirenonce-is-true-openidconnectprotoco

guardrex commented 9 months ago

Ok ... thanks for trying that just to see what would happen.

I'll be 👂 on the PU issue to see what they say. Hopefully, they'll have a fix for both of us ... to get your app running and for the publication of the companion article that I'm writing for the sample apps at https://github.com/dotnet/AspNetCore.Docs/pull/31555.

guardrex commented 9 months ago

Note that Stephen Halter is OOF until February 1. If they think he should work that issue because he's the one who built this sample app, then it will need to wait until he gets back on the 1st.

travaille-dev commented 9 months ago

Thanks for your help! I don't mind pivoting to utilize the oidc example without aspire + yarp as I know aspire is still early access.

guardrex commented 9 months ago

There probably will be some additional updates, too. It turns out that if the user is on an authenticated page and logs out that it then redirects back to same page, which triggers another sign-in redirect. I hacked up an approach to deal with it using reflection, but it's probably not elegant (I'll note it on the issue and ping you over there). Another concern is that when the user is returned to app after log in that the Weather component can try to render server-side and :boom: when the IWeatherForecaster service isn't available there. I'll see if I can fix that tomorrow. Anyway, these sorts of things will be worked out on the PR. This particular PU issue can just focus on the nonce situation.

Krangth commented 9 months ago

I am having both the problems described here. Nonce and IWeatherForecaster. You can reproduce the IWeatherForecaster by bookmarking it and clicking the bookmark.

guardrex commented 9 months ago

@Krangth This must be dealt with by the product unit on the issue that @travaille-dev opened on their repo. They won't look at this one. This is only to track the fix in the sample app, whatever it turns out to be.

Can you thumb UP 👍 the product unit issue's opening comment?

https://github.com/dotnet/aspnetcore/issues/53585

Halter will be back from OOF today. Idk how long he will need to dig out of email, etc. He'll be working the PU issue soon tho. I suspect that if he doesn't work on it today/tomorrow that he'll get to it next week.

LapinskasL commented 8 months ago

@guardrex I don't mean to go off-topic in this thread but I am confused about this sample app and I don't know where to post this.

With this issue present in the sample app, I can't use and trust its code in a Blazor 8.0 application that I want to deploy to production. And I'd argue this isn't a minor bug. I also encountered another issue, the details of which escape me at the moment. I understand the responsibility to secure my app and what I deploy is on me. However, in comparison to implementing auth in an MVC app, in Blazor it's far more complicated the moment we use InteractiveAuto mode. In MVC I can trust the methods provided by Microsoft without much question even though they are black boxes to me.

Since authentication is so critical to implement correctly and this sample app from Microsoft has a bug in it, I'm even more reluctant to implement a solution proposed by an authority outside of Microsoft. Figuring out the details of auth code independently also seems time-consuming (which I know is my problem) and also risky (also my problem).

So what is the purpose of this sample-app? Is this something that is still in-progress and developers shouldn't trust the code yet and use it without understanding it fully? Does Microsoft not have palatable production-ready way yet for developers to add auth? If the answer is yes to these questions, that's totally fine. I know auth is complicated. But coming from my great familiarity of implementing the simple and working auth code for the well-established MVC framework, I don't have the appropriate reference point/context to interpret what's going on here for this new framework release.

guardrex commented 8 months ago

This (and a few other security-related 8.0 issues) must be addressed by the product unit. My next contact with management is on Friday, and I'll be providing a list of everything that requires attention.

For this particular issue, keep en 👁️ on that PU issue that was opened :point_up:, and you can 👍 the issue's opening comment if you wish.

There are several items to address ...

I'll go over these with them on Friday to see if we can get them resolved ASAP 🏃‍♂️.

They can all be tracked via the 8.0 tracking issue at https://github.com/dotnet/AspNetCore.Docs/issues/28161.

guardrex commented 8 months ago

I did just notice that they marked that for .NET 9 ... i.e., a November resolution. Idk why it has the priority that it does. Perhaps, it isn't a quick fix.

LapinskasL commented 8 months ago

Thanks, guardrex. This is a bottleneck for me because the apps we work on require authentication and authorization. We've been using MVC and ajax to create highly interactive apps and Blazor server+client would make the whole process easier. Considering Microsoft has expressed we shouldn't be writing custom [Authorize] attributes, I take it also that we shouldn't mess around with custom code for auth in general.

I appreciate your responses. I'm hoping fixes won't take until November.

guardrex commented 8 months ago

I'm sending an email to management now to try and unblock a lot of security issues and PRs. I hope within a week or two that they will all be resolved. WRT this pesky nonce situation :smiling_imp:, I'll at least remark on it here if I hear back verbally on it and nothing is added on the PU issue about it.

guardrex commented 8 months ago

Actually ... some good news on the nonce item 😈 ... they scheduled it for Preview 3, so that should be around April to hear what the fix will be. I assume that we'll try to present a workaround for 8.0 apps after I find out what they figured out to do about it. Therefore, I'm going to list it in my email but note to them that I understand it will be looked at reasonably soon.

captnord commented 8 months ago

I first want to say thank you @guardrex and your team for providing the Blazor Sample projects. I was wracking my brain trying to get a .NET 8 Blazor Web App to work gracefully with the Entra ID authentication flow and couldn't have done it without you and your team's work on putting the the samples out there.

Secondly, I have run into this snag as well which manifests as our site (deployed to an on prem server but using hosted Azure Entra ID for auth) being unavailable if left idle past the token refresh period (1 hour?). The end user sees an HTTP 500 error and we see the detailed exception that @travaille-dev posted in the submission (IDX21323).

In digging into the CookieOidcRefresher class provided in the 8.0/BlazorWebAppOidc project, I see that when the OpenIdConnectProtocolValidator is constructed it is done so with the property RequireNonce being set to false. When the method in OpenIdConnectProtocolValidator is called to validate the refresh response (starting at line 80 of the CookieOidcRefresher class) it fails on a null nonce value. It appears that the OpenIdConnectProtocolValidator is not honoring the RequireNonce = false setting.

OpenIdConnectProtocolValidator.ValidateTokenResponse() calls ValidateNonce() which checks the RequireNonce property and also checks the Nonce value of the ValidationContext being passed in as well as the ValidatedIdToken.Payload.Nonce value of the ValidationContext being passed in. What's more, once the Nonce values are validated there is a further check to see if it is expired. ValidateNonce() compares the timestamp of the ValidatedIdToken.Payload.Nonce value to the current UTC DateTime and if the nonce timestamp plus the NonceLifetime (set on the OpenIdConnectProtocolValidator) is less than UTC Now it throws OpenIdConnectProtocolInvalidNonceException (IDX21324).

TL;DR Even if RequireNonce is set to false a Nonce value still needs to be passed to the OpenIdConnectProtocolValidator and the Validator's NonceLifetime needs to be accounted for.

My proposed workaround for this is to set the Nonce value when creating the OpenIdConnectProtocolValidationContext model that is passed into OpenIdConnectProtocolValidator.ValidateTokenResponse. If it is set to the validated token's Payload.Nonce value then it should pass all of the checks in the RequireNonce method of OpenIdConnectProtocolValidator.

var validatedIdToken = JwtSecurityTokenConverter.Convert(validationResult.SecurityToken as JsonWebToken);

_oidcTokenValidator.ValidateTokenResponse(new()
{
    ProtocolMessage = message,
    Nonce = validatedIdToken.Payload.Nonce,
    ClientId = oidcOptions.ClientId,
    ValidatedIdToken = validatedIdToken,
});

Additionally, when constructing the OpenIdConnectProtocolValidator the NonceLifetime property should be set to an extended TimeSpan (I am setting it to 365 days):

private readonly OpenIdConnectProtocolValidator _oidcTokenValidator = new()
{
    // Refresh requests do not use the nonce parameter. Otherwise, we'd use oidcOptions.ProtocolValidator.
    RequireNonce = false,
    NonceLifetime = TimeSpan.FromDays(365)
};

The security implication for this is that we are essentially bypassing the Nonce check in the token refresh validation. I don't believe this is making CookieOidcRefresher any less secure than it already is since the RequireNonce property was already being set to false in order, presumably, to bypass the nonce check anyway. I am by no means an expert in any of this so please take my proposed workaround with a large grain of salt and I would appreciate those with more knowledge on the subject to offer their guidance if possible.

guardrex commented 8 months ago

thank you @guardrex and your team

I'm glad you find the sample(s) helpful. Stephen Halter and Jeremy Likness are due full credit for the sample apps. I'm mostly just handling writing up the companion articles, one of which we have for WASM+Identity and one that's under construction on https://github.com/dotnet/AspNetCore.Docs/pull/31555.

I recommend that you copy all of those remarks into a comment on the PU's issue. That's where they'll probably work it. I'm going to keep this issue open in case they resolve and close the PU issue before I/we get the sample app updated over here.

Place a copy of that here ...

https://github.com/dotnet/aspnetcore/issues/53585

guardrex commented 8 months ago

Just checking back in here ... the PU still has it scheduled for Preview 3, which will be for April's release. I'll continue to keep an 👁️ on the PU issue.

guardrex commented 8 months ago

I just want to note here that one source of this is ...

Stale cookies are the devils playground! 😈😆

I just hit this while working another issue. I already have guidance in the article on using an InPrivate/incognito browser for testing, but I'm going to strengthen the remarks on doing that tomorrow when I issue a new PR to address some updates to the sample app that just merged over here.

guardrex commented 8 months ago

My bad on one point ..... the guidance is in the WASM-focused articles at this time.

Tomorrow, I'll surface that guidance in the BWA+OIDC article. For now, you can see it here ...

https://learn.microsoft.com/en-us/aspnet/core/blazor/security/webassembly/standalone-with-identity?view=aspnetcore-8.0#cookies-and-site-data

guardrex commented 8 months ago

I'm placing the troubleshooting guidance (updated from what the WASM articles show) on https://github.com/dotnet/AspNetCore.Docs/pull/31937.

I'm going to let this nonce situation play out on the PU issue that was opened. They've marked it for Preview 3 investigation. If we need to make changes to the sample app, I'll either re-open this or open a new issue.

Anyone finding this issue should make sure that they're looking at the current (latest) version of the sample app and update their app(s) to match the latest sample/article because multiple updates have been made. There will be one more set of article updates soon ... perhaps even by tomorrow (Friday, 3/1).

Everyone PLEASE use an InPrivate/incognito browser for ALL testing ... no matter how insignificant the changes to the app, configuration, or test user account(s). Stale cookies have been horrible TIME SINKS 😈😡 as the dev spends hours or even days trying to figure out what's wrong with an app. It turns out in some cases that it's just stale cookies that are breaking the authn/z.