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

Override BaseUri #1675

Closed
1 task done
SebastianStehle opened this issue Feb 14, 2023 · 16 comments
Closed
1 task done

Override BaseUri #1675

SebastianStehle opened this issue Feb 14, 2023 · 16 comments

Comments

@SebastianStehle
Copy link

Confirm you've already contributed to this project or that you sponsor it

  • I confirm I'm a sponsor or a contributor

Version

4.x

Question

Hi,

I always struggle with the several layers of proxies that some of my customers have. Very often, the actual public host name gets lost somewhere. This is an issue if you create absolute URLs.

Therefore I have a configuration value for the public URL. In AttachEndpoints you calculate the URIs based on the host URL if the configured URLs are not absolute.

Would it be possible to introduce an option to override the base uri? I know that I could override AttachEndpoints method, but I would prefer a more lightweight option.

@kevinchalet
Copy link
Member

Hi,

Would it be possible to introduce an option to override the base uri? I know that I could override AttachEndpoints method, but I would prefer a more lightweight option.

BaseUri was added in 4.0, as part of #1613:

// OpenIddict supports both absolute and relative URIs for all its endpoints, but only absolute
// URIs can be properly canonicalized by the BCL System.Uri class (e.g './path/../' is normalized
// to './' once the URI is fully constructed). At this stage of the request processing, rejecting
// requests that lack the host information (e.g because HTTP/1.0 was used and no Host header was
// sent by the HTTP client) is not desirable as it would affect all requests, including requests
// that are not meant to be handled by OpenIddict itself. To avoid that, a fake host is temporarily
// used to build an absolute base URI and a request URI that will be used to determine whether the
// received request matches one of the URIs assigned to an OpenIddict endpoint. If the request
// is later handled by OpenIddict, an additional check will be made to require the Host header.
(context.BaseUri, context.RequestUri) = request.Host switch
{
{ HasValue: true } host => (
BaseUri: new Uri(request.Scheme + Uri.SchemeDelimiter + host + request.PathBase, UriKind.Absolute),
RequestUri: new Uri(request.GetEncodedUrl(), UriKind.Absolute)),
{ HasValue: false } => (
BaseUri: new UriBuilder
{
Scheme = request.Scheme,
Path = request.PathBase.ToUriComponent()
}.Uri,
RequestUri: new UriBuilder
{
Scheme = request.Scheme,
Path = (request.PathBase + request.Path).ToUriComponent(),
Query = request.QueryString.ToUriComponent()
}.Uri)
};

That said, you shouldn't use it: an incorrect inferred value is the sign ASP.NET Core itself didn't receive the correct information, which doesn't only affect OpenIddict but any other middleware that relies on HttpRequest.Host. Instead, you should consider relying on the ASP.NET Core forwarded headers middleware or setting HttpRequest.Host yourself: https://learn.microsoft.com/en-us/aspnet/core/host-and-deploy/proxy-load-balancer?view=aspnetcore-7.0

@SebastianStehle
Copy link
Author

Yes, I know that it would be better if all proxies would work correctly, but it is not the case. Therefore I would just like to enforce to use a specific host name when absolute URLs are generated.

I went through this issue with the X-Forwarded-Host header at least 50 times now with my customers / users now.

@kevinchalet
Copy link
Member

Yes, I know that it would be better if all proxies would work correctly, but it is not the case. Therefore I would just like to enforce to use a specific host name when absolute URLs are generated.

If the forwarded headers approach is not working, consider just adding a middleware that sets HttpRequest.Host: it's better than setting the OpenIddict BaseUri yourself.

@SebastianStehle
Copy link
Author

I think you misunderstand me.

I would like to differentiate between:

  • Routing: Which endpoint should be called, depending on the current request that might be made to an internal host name or just to localhost, depending on the current setting.
  • Url Generation: The process of creating URLs for client that usually sit in front of the reverse proxy or load balancer and have no idea about the internal structure.

So I would like to have an option for a "PublicUri" that just works like this: dev...SebastianStehle:openiddict-core:patch-1

@kevinchalet
Copy link
Member

Well, I've never seen a scenario where having a different host value for the two things would make sense, and it's certainly not a scenario ASP.NET Core itself supports (there's a single HttpRequest.Host that serves for both routing - see HostMatcherPolicy for a concrete example - and URL generation).

I still think setting HttpRequest.Host does what you want but if it doesn't, you can still create a custom handler that overrides context.BaseUri with the value you want. All this stuff is already terribly complicated when you add proxies in the mix: another option would just make things even more confusing 😄

@SebastianStehle
Copy link
Author

I was thinking about this as well, but it has many other implications that I cannot predict. Perhaps I will just write a custom AttachEndpoints handler.

@kevinchalet
Copy link
Member

You can, but it will only solve part of the problem: all the other ASP.NET Core components that rely on HttpRequest.Host to generate absolute URLs won't behave correctly and will return non-public/non-routable hosts (even AuthenticationHandler uses it: https://github.com/dotnet/aspnetcore/blob/7d29bfae60a8f109da85cf927066e73e461a1593/src/Security/Authentication/Core/src/AuthenticationHandler.cs#L100)

Here's another related thread that may interest you: #1610.

@SebastianStehle
Copy link
Author

I totally agree that they should just configure their reverse proxies correctly ... but this is the reality I live in.

I will think about it.

@SebastianStehle
Copy link
Author

Do you know if something has changed with .NET 7? I have not changed anything, but it seems that a wrong host name was not a problem for .NET 6 (I am still running on 3.X). I am not entirely sure about that.

@kevinchalet
Copy link
Member

kevinchalet commented Feb 15, 2023

Do you know if something has changed with .NET 7?

Nope. To my knowledge, there are only two points that can affect OpenIddict:

  • The fact the minimal web host now calls app.UseAuthentication() under the hood if you don't call it. Existing apps are not impacted, but new apps using the new templates have to explicitly call app.UseAuthentication() if they need CORS support, as app.UseCors() must appear before app.UseAuthentication() for CORS policies to include the OpenIddict endpoints.

  • ASP.NET Core 7.0's default authentication handler fallback breaks OpenIddict dotnet/aspnetcore#44661, but OpenIddict 4.x includes a built-in workaround to avoid it.

Note: you probably already know it, but 3.x is no longer supported.

@kevinchalet
Copy link
Member

Do you still need help, @SebastianStehle? 😃

@SebastianStehle
Copy link
Author

No, it was indeed a problem with the host header. they have fucked up something in their configuration.

@kevinchalet
Copy link
Member

Great to hear! 👍🏻

@pksorensen
Copy link

Hi Kevin, know this is old, but i just moved apps to container apps and they have their proxy. One of the things i needed to fix was the problem that the openid-configuration endpoint is adding http:// urls (since its hosted on http in the containers internally and ssl offload happens at the proxy). I did set the issuer to the public endpoint (https). Started looking for "baseuri" as to get the keys endpoint to show https (otherwise clients that call it gets no signing tokens found because it does not allow by default to load a http endpoint).

Reading this thread i did however come to the conclussion that i should not set it myself, however i am 99% sure that the forwarded headers work and that the host should have been updated using that and discovery document should be okay, hoever it is not. Trying to validate if its indead a host header thing not propergating.

@kevinchalet
Copy link
Member

Hi @pksorensen,

Please open a new ticket (note: you need to sponsor the project to get support. More information here: https://github.com/sponsors/kevinchalet).

Cheers.

@pksorensen
Copy link

No worries - just wanted to leave the information to the next guy coming here. Turns out my problem was indeed the ForwardedHeadersOptions - where the host was considered a unknown host. I had only cleared proxies, but forgot to clear know networks. So anyone running into wrong urls in discovery document, validate that your setup is really correct. Enable debug/tracing to see the warnings from forwarded headers.

Unknown proxy: [::1]:57639

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants