Skip to content

AddIdentityServerJwt might accidentally use the wrong issuer URL. #28880

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

Open
SebastianStehle opened this issue Dec 28, 2020 · 8 comments
Open
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Milestone

Comments

@SebastianStehle
Copy link

Describe the bug

When using AddIdentityServerJwt the IssuerURL seems to be derived from the actual request. The option management hooks into the events and enriches the options with the host name from the request:

if (options.TokenValidationParameters.ValidIssuer == null || options.TokenValidationParameters.IssuerSigningKey == null)

When the first request that comes in is not the public host name, the options is enriched with the wrong host and all subsequent calls to authorize fail because the issuer URL does not match.

Consider a scenario where you have a health check running. The health service uses the internal IP (e.g. in kubernetes) to call the health endpoint and the issuer URL is configured with the IP address.

To Reproduce

  1. Create a new SPA sample (dotnet new react -au Individual)
  2. Login
  3. Restart the server
  4. Go to https://127.0.0.1:5001 before you go to any other URL
  5. Go to https://localhost:5001 and press fetch data

The result in Chrome:

Bearer error="invalid_token", error_description="The issuer 'https://localhost:5001' is invalid"

Further technical details

  • ASP.NET Core version 5
@mkArtakMSFT mkArtakMSFT added area-identity Includes: Identity and providers investigate labels Dec 28, 2020
@mkArtakMSFT
Copy link
Contributor

Thanks for contacting us, @SebastianStehle.
Due to the holiday season please expect some delays in our responses.
@HaoK can you please investigate this when you're back? Thanks!

@ghost
Copy link

ghost commented Dec 28, 2020

Thanks for contacting us.
We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@blowdart
Copy link
Contributor

This is by design and matches what identity server itself does.

If you have multiple host names you must select an canonical one by setting the Authority on the actual underlying identity server options.

            services.AddIdentityServer(options => options.IssuerUri = "https://localhost")
                .AddApiAuthorization<ApplicationUser, ApplicationDbContext>();

The JWT pieces should then use the identity server metadata to resolve the issuer.

However what I'm seeing at that point is that the URL in the template for the metadata is wrong, and doesn't have the port number. As I'm not a react person, I can't help further, but it feels more like a template issue that identity itself.

@SebastianStehle
Copy link
Author

SebastianStehle commented Dec 29, 2020

Thank your @blowdart for your answer. I also solved the problem with IssuerUri before writing this bug report.

I think it is a very weird behavior. It first happened to me, when I had no health check at all, so I was not using a second host by purpose. So you actually make your server unusable by accident and it took my days to figure out what the problem is. If you think the behavior is okay there should be at least a warning. e.g. a Middleware that writes all the hosts to a hashset and logs a warning if 2 or more hosts are received.

I had a similar problem a while ago in another project: https://github.com/squidex/squidex. A java client of a customer was using a library that always added the port number to the host header. So instead of adding host: cloud.squidex.io this client has added host: cloud.squidex.io:443. Then the customer also got an issuer URL error and I had to integrate a workaround for this customer: https://github.com/Squidex/squidex/blob/master/backend/src/Squidex.Web/Pipeline/CleanupHostMiddleware.cs. In this scenario here one customer could accidentally make the whole application unusable.

If I check my logs I see a lot of requests to paths like /setup.php or /install.php and so on. Websites seem to be under constant attack these days. And with this behavior some attacker could bring your system down (or make it unusable) by pure luck.

@blowdart
Copy link
Contributor

To be fair this isn't us, this is Identity Server's behaviour, we're just not overriding it. Whilst I can't speak for Dom & Brock, when we make decisions like this, it's because customers want an f5 experience, where you can run a project immediately without having to think about configuration.

As for bringing hosts down, I'd say maybe, but again it's something to take up with the identity server folks. We tend to lean towards saying not to listen on * and not to bind to IP addresses, but rather host names, and once you're thinking about host names, you tend to start thinking about configuration of everything more. Hopefully :)

@SebastianStehle
Copy link
Author

SebastianStehle commented Dec 29, 2020

I don't see that this is a identity server behavior. The problem is the "caching" and that the options property is changed. If you would always use the HttpContext.GetIdentityServerIssuerUri() for each request, if not specified otherwise, the problem would not exist. I had a look to the source code and I actually do not see that much identity server code at all. You just use identity server to copy over some values like the signing key and the issuer URL, but that's it.

You need to hook into this place:

var issuers = new[] { _configuration.Issuer };
and then just change the cloned parameter.

I totally agree to the host name thing, but the question is: Who handles that. There are load balancers, requests and stuff like kubernetes wo make requests and manipulate the request and especially if your software is hosted in so many crazy ways you do not have full control over everything.

@HaoK HaoK added the ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. label Jan 21, 2021
@ghost ghost added the Status: Resolved label Jan 21, 2021
@HaoK HaoK closed this as completed Jan 21, 2021
@SebastianStehle
Copy link
Author

I still don't understand how a behavior that can bring your system down without any warning can be by design.

@ghost ghost locked as resolved and limited conversation to collaborators Feb 20, 2021
@HaoK HaoK reopened this Sep 10, 2021
@HaoK HaoK modified the milestones: Next sprint planning, 6.0.0 Sep 10, 2021
@adityamandaleeka adityamandaleeka modified the milestones: 6.0.0, Backlog Sep 16, 2021
@HaoK
Copy link
Member

HaoK commented Sep 17, 2021

Doc/release note issue filed to track this for 6.0 #36676

@HaoK HaoK removed their assignment Sep 17, 2021
@HaoK HaoK added area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer and removed investigate ✔️ Resolution: By Design Resolved because the behavior in this issue is the intended design. Status: Resolved labels Sep 17, 2021
@danmoseley danmoseley removed the area-identity Includes: Identity and providers label Feb 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer
Projects
None yet
Development

No branches or pull requests

6 participants