DuendeSoftware / IdentityServer

The most flexible and standards-compliant OpenID Connect and OAuth 2.x framework for ASP.NET Core
https://duendesoftware.com/products/identityserver
Other
1.47k stars 344 forks source link

Authorization Code flow barfs with null pointer exception #473

Closed mmacagno closed 2 years ago

mmacagno commented 3 years ago

Which version of Duende IdentityServer are you using? 5.2.3

Which version of .NET are you using? .NET 5.0

Describe the bug Aspnet MVC website (running on DOTNETCORE 3.1) authenticating against identity provider via authorization code. Identity provider just migrated to Duende. Aspnet MVC website using https://github.com/IdentityModel/IdentityModel 4.2

When connecting to the aspnet MVC website (unauthenticated) I get redirected to the authorization endpoint:

https://{endpoint}/connect/authorize?client_id={client_id}&redirect_uri={redirectURI}&x-client-SKU=ID_NETSTANDARD2_0&x-client-ver=5.5.0.0

The identity server barfs with this exception: Object reference not set to an instance of an object.

To Reproduce

It repros with all my authorization code clients. Not sure how to specify repro steps for you sorry.

Expected behavior

login page displayed. This flow is properly working with identity server, and stopped working after migrating to Duende. I followed the migration steps, which are straightforward, not sure what I may have messed up.

Log output/exception with stacktrace

{
"type": "https://tools.ietf.org/html/rfc7231#section-6.6.1",
"title": "Object reference not set to an instance of an object.",
"status": 500,
"detail": "Object reference not set to an instance of an object.:    at Duende.IdentityServer.Extensions.StringExtensions.AddQueryString(String url, String query) in /_/src/IdentityServer/Extensions/StringsExtensions.cs:line 196\r\n   at Duende.IdentityServer.Extensions.StringExtensions.AddQueryString(String url, String name, String value) in /_/src/IdentityServer/Extensions/StringsExtensions.cs:line 211\r\n   at Duende.IdentityServer.Endpoints.Results.LoginPageResult.ExecuteAsync(HttpContext context) in /_/src/IdentityServer/Endpoints/Results/LoginPageResult.cs:line 77\r\n   at Duende.IdentityServer.Hosting.IdentityServerMiddleware.Invoke(HttpContext context, IEndpointRouter router, IUserSession session, IEventService events, IIssuerNameService issuerNameService, IBackChannelLogoutService backChannelLogoutService) in /_/src/IdentityServer/Hosting/IdentityServerMiddleware.cs:line 103\r\n   at Duende.IdentityServer.Hosting.IdentityServerMiddleware.Invoke(HttpContext context, IEndpointRouter router, IUserSession session, IEventService events, IIssuerNameService issuerNameService, IBackChannelLogoutService backChannelLogoutService) in /_/src/IdentityServer/Hosting/IdentityServerMiddleware.cs:line 103\r\n   at Duende.IdentityServer.Hosting.MutualTlsEndpointMiddleware.Invoke(HttpContext context, IAuthenticationSchemeProvider schemes) in /_/src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs:line 95\r\n   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)\r\n   at Duende.IdentityServer.Hosting.DynamicProviders.DynamicSchemeAuthenticationMiddleware.Invoke(HttpContext context) in /_/src/IdentityServer/Hosting/DynamicProviders/DynamicSchemes/DynamicSchemeAuthenticationMiddleware.cs:line 48\r\n   at Duende.IdentityServer.Hosting.BaseUrlMiddleware.Invoke(HttpContext context) in /_/src/IdentityServer/Hosting/BaseUrlMiddleware.cs:line 32\r\n   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)\r\n   at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.<Invoke>g__Awaited|6_0(ExceptionHandlerMiddleware middleware, HttpContext context, Task task)",
"traceId": "00-580edba24fed2141ba9027b94c9a00f7-11604a00ae72514a-00"
}

Additional context

The line on which it barfs is here. how can url be null being an extension method?

image

mmacagno commented 3 years ago

Going one step further, the url is prepared in LoginPageResult.cs:

image

Even if IServerUrls >> _urls was null, returnUrl should not be null. I don't understand how and why this is breaking.

Any clue?

brockallen commented 3 years ago

Hi, thanks for the report. We'll investigate.

brockallen commented 3 years ago

In your text above you show the authorize request as:

https://{endpoint}/connect/authorize?client_id={client_id}&redirect_uri=&x-client-SKU=ID_NETSTANDARD2_0&x-client-ver=5.5.0.0

I notice the redirect_uri is empty -- is that the case, or did you just edit it out for this issue?

Are you setting any of the values on the UserInteraction IdentityServer options (e.g. LoginUrl, or LogoutUrl)?

Are you setting the BasePath on the IServerUrls or calling SetIdentityServerBasePath anywhere?

brockallen commented 3 years ago

Also, if you can get the full stack trace that will help. Thx

mmacagno commented 3 years ago

It was in the initial report, anyway, here it is again:

Duende.IdentityServer.Extensions.StringExtensions.AddQueryString(String url, String query) in //src/IdentityServer/Extensions/StringsExtensions.cs:line 196\r\n at Duende.IdentityServer.Extensions.StringExtensions.AddQueryString(String url, String name, String value) in //src/IdentityServer/Extensions/StringsExtensions.cs:line 211\r\n at Duende.IdentityServer.Endpoints.Results.LoginPageResult.ExecuteAsync(HttpContext context) in //src/IdentityServer/Endpoints/Results/LoginPageResult.cs:line 77\r\n at Duende.IdentityServer.Hosting.IdentityServerMiddleware.Invoke(HttpContext context, IEndpointRouter router, IUserSession session, IEventService events, IIssuerNameService issuerNameService, IBackChannelLogoutService backChannelLogoutService) in //src/IdentityServer/Hosting/IdentityServerMiddleware.cs:line 103\r\n at Duende.IdentityServer.Hosting.IdentityServerMiddleware.Invoke(HttpContext context, IEndpointRouter router, IUserSession session, IEventService events, IIssuerNameService issuerNameService, IBackChannelLogoutService backChannelLogoutService) in //src/IdentityServer/Hosting/IdentityServerMiddleware.cs:line 103\r\n at Duende.IdentityServer.Hosting.MutualTlsEndpointMiddleware.Invoke(HttpContext context, IAuthenticationSchemeProvider schemes) in //src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs:line 95\r\n at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)\r\n at Duende.IdentityServer.Hosting.DynamicProviders.DynamicSchemeAuthenticationMiddleware.Invoke(HttpContext context) in //src/IdentityServer/Hosting/DynamicProviders/DynamicSchemes/DynamicSchemeAuthenticationMiddleware.cs:line 48\r\n at Duende.IdentityServer.Hosting.BaseUrlMiddleware.Invoke(HttpContext context) in //src/IdentityServer/Hosting/BaseUrlMiddleware.cs:line 32\r\n at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)\r\n at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.g__Awaited|6_0(ExceptionHandlerMiddleware middleware, HttpContext context, Task task)"

mmacagno commented 3 years ago

In your text above you show the authorize request as:

https://{endpoint}/connect/authorize?client_id={client_id}&redirect_uri=&x-client-SKU=ID_NETSTANDARD2_0&x-client-ver=5.5.0.0

I notice the redirect_uri is empty -- is that the case, or did you just edit it out for this issue?

Sorry I used angular parenthesis and the markup ate them. The URL has the redirectUri populated.

https://{endpoint}/connect/authorize?client_id={client_id}&redirect_uri={redirectURI}&x-client-SKU=ID_NETSTANDARD2_0&x-client-ver=5.5.0.0

Are you setting any of the values on the UserInteraction IdentityServer options (e.g. LoginUrl, or LogoutUrl)?

No. Also, if it can help, this is the Client definition (edited for confidentiality), as it is working with IS4:

new Client
                {
                    ClientId = "MotusDashboard",
                    ClientName = "Motus Dashboard",
                    AllowedGrantTypes = GrantTypes.Code,
                    RequirePkce = true,
                    AllowOfflineAccess = true,
                    ClientSecrets = { new Secret("*******".Sha256()) },
                    RedirectUris = new string[] { "https://{fqdn}/signin-oidc", "https://staging-{fqdn}/signin-oidc", "https://test-{fqdn}/signin-oidc", "https://localhost:44359/signin-oidc", "https://local.sensoria.io:44359/signin-oidc" },  
                    PostLogoutRedirectUris = new string[] { "https://{fqdn}", "https://staging-{fqdn}", "https://test-{fqdn}", "https://localhost:44359", "https://local.sensoria.io:44359" },
                    FrontChannelLogoutUri = $"https://{fqdn}/signout-oidc",
                    AllowedScopes = new List<string>
                    {
                        IdentityServerConstants.StandardScopes.OpenId,
                        IdentityServerConstants.StandardScopes.Profile,
                        IdentityServerConstants.StandardScopes.Email,
                        "role",  // role claim for each user.
                        "customRole1",
                        "customRole2",
                    },
                    RefreshTokenUsage = TokenUsage.ReUse,  // Not using OneTime.
                }

Are you setting the BasePath on the IServerUrls or calling SetIdentityServerBasePath anywhere?

No

mmacagno commented 3 years ago

One oddity that might be a red herring but...

I tried to inject IServerUrls in a TestController to inspect it at runtime and it breaks the build. Am I missing some references / nuget packages?

image

image

thanks mac

mmacagno commented 3 years ago

I was looking at the code in the wrong branch (6.0). In 5.2.3 there is no IServerUrls. The crash is happening at line 77 here:

image

mmacagno commented 3 years ago

So I injected the options in a TestController, and indeed LoginUrl is null.

image

So I went back and added these two values to the options and it now works.

image

I did not do that in IS4, I think it had a default. Is that no longer the case in Duende.IdentityServer?

I'm leaving to you to decide if this is a bug or by design and close accordingly. Thanks for the insightful questions, you put me on the right track!

mac

mmacagno commented 3 years ago

@brockallen I found another hurdle on the interactive login right after solving this. I will open a separate issue or discussion topic, but wanted to ask: are there any additional "no-longer" default settings to take care of?

brockallen commented 3 years ago

For the LoginUrl, are you setting the LoginPath on the CookieAuthenticationOptions anywhere?

mmacagno commented 3 years ago

For the LoginUrl, are you setting the LoginPath on the CookieAuthenticationOptions anywhere?

No, the only place where that path is set is here (Startup.cs, before IdentityServer is added to the services):

            services.AddRazorPages(options =>
            {
                options.Conventions.AddPageRoute("/Identity/Account/Login", "/");
            });
brockallen commented 3 years ago

If this used to work, then I have a feeling it's somewhat related to our other discussion about what's the default authentication scheme. Internally we'd set the LoginUrl for you based on the default auth cookie scheme. But since that's now Bearer, there is no longer a LoginPath to use.

brockallen commented 2 years ago

Any update on this @mmacagno, or can we close this issue?

mmacagno commented 2 years ago

I haven't got around to investigate this yet, it's still an issue. Please leave it open a little longer. Will give an update ASAP.

mmacagno commented 2 years ago

Related conversation https://github.com/DuendeSoftware/IdentityServer/discussions/483 Finally resolved. Closing.

MonstraG commented 1 year ago

Just had this happen to me (stacktrace is basically the same, but I'm on 6.2.0 (also tried 6.1.7))).

Was following quickstart, so AddRazorPages looks like this:

        // uncomment if you want to add a UI
        builder.Services.AddRazorPages();

Link for related conversation gives 404 for me.

Adding options.Conventions.AddPageRoute("/Identity/Account/Login", "/"); did not help.